refactor(pumpkin-solver): PropagatorConstructor::create now returns event registrations

[maartenflippo]

We want to make it more explicit that variables should be registered in the propagator constructor. To do so, the signature of PropagatorConstructor::create is updated to not only return a propagator implementation, but also a bag of events with local IDs for which the propagator should be registered.

Implemented as a result of the discussion in #449.

What this adds

• EventTarget: Anything that can be registered for domain events. Implemented for all variables, and required for any implementation of IntegerVariable. This is a separate trait instead of a method on IntegerVariable to try to move away from the monolithic interface that IntegerVariable is becoming.
• EventDispatcher: Anything that an EventTarget can be registered with. Implemented for a wrapper of NotificationEngine as well as the new EventRegistration type.
• EventRegistration: The bag of domain events that is returned by PropagatorConstructor::create alongside the propagator. The implementation tries to be explicit about when an empty registration is expected. To do so, we introduced the EventRegistrationBuilder abstraction.

What this does not do

• Port the unwatching and backtrack event registration to the new EventTarget trait. In the future it is likely we will want to do this.
• Handle registration of predicates, which should also eventually happen through the EventTarget and EventRegistration.

Feedback

The names of the new types added here may not be very clear. Please consider whether you have better ideas, or whether you like the names as they are now.

[EmirDe]

I have done a partial review, e.g., I did not assess the "what is does not do". Added some comments to the code, here are a few more:

About naming.

• EventDispatcher -> EventNotifier? We use the term "notify" for propagators and engine, so it might make sense to use that word here too.
• EventRegistration -> perhaps EventsToRegister is a good name, this would be more explicit so I like it. EventRegistrations could be another alternative. The current version ("EventRegistration") sounds a bit like a function or otherwise something actively doing something.

About the trait EventTarget: I am not against having the trait, but I am wondering. You said that you would like to keep this as a separate trait as opposed to adding to IntegerVariable. But it seems to me that this trait will only be used with IntegerVariables, and it is required to implement it, so why not keep it as part of IntegerVariable, it does not seem to be that problematic. It also helps with naming. But fine by me in both cases.

Other minor comments:

I suppose we would like to have the event registration be an enum with types "EventsToRegister" and "Empty". But this might be a bit cumbersome for something used in only a few lines of code, so I am fine with the current design with the builder!

Both EventTarget and Dispatcher have the same function name "register" but they do slightly different things. Not sure if this is a problem or not, but just mentioning it, since in the code it would look like calling the same function (but it is also obviously different, the name is simply the same).

[maartenflippo]

EventDispatcher -> EventNotifier? We use the term "notify" for propagators and engine, so it might make sense to use that word here too.

Of these two I prefer EventDispatcher as events are dispatched but propagators are notified. So then PropagatorNotifier would be slightly better, but I still prefer EventDispatcher over that within this module.

About the trait EventTarget: I am not against having the trait, but I am wondering. You said that you would like to keep this as a separate trait as opposed to adding to IntegerVariable. But it seems to me that this trait will only be used with IntegerVariables, and it is required to implement it, so why not keep it as part of IntegerVariable, it does not seem to be that problematic. It also helps with naming. But fine by me in both cases.

I have developed a preference for smaller traits. They make it easier to write test dummies in the future. If we want to write unit tests for this part, we do not need to create dummy implementation for the entire IntegerVariable trait.

I suppose we would like to have the event registration be an enum with types "EventsToRegister" and "Empty". But this might be a bit cumbersome for something used in only a few lines of code, so I am fine with the current design with the builder!

I also prefer the current builder pattern with an assertion over the enum.

Both EventTarget and Dispatcher have the same function name "register" but they do slightly different things. Not sure if this is a problem or not, but just mentioning it, since in the code it would look like calling the same function (but it is also obviously different, the name is simply the same).

I don't think they do different things. EventTarget::register maps the events to eventually put them into Dispatcher::register. So In this case I think the names being the same is okay.

---

I have made changes for and responded to the other comments @EmirDe

[EmirDe]

Of these two I prefer EventDispatcher as events are dispatched but propagators are notified. So then PropagatorNotifier would be slightly better, but I still prefer EventDispatcher over that within this module.

You are right that EventNotifier might not be ideal, because it seems like it notifies events. I like PropagatorNotifier, or even more PropagatorEventNotifier, because it is more explicit in what it does and uses the term 'notify'. But if you still prefer EventDispatcher then let us go with that, it is not that important in the end.

I have developed a preference for smaller traits. They make it easier to write test dummies in the future. If we want to write unit tests for this part, we do not need to create dummy implementation for the entire IntegerVariable trait.

I thought about this since, and now I am convincing having this as a trait is a fantastic idea, much better than adding it to IntegerVariable. In general, smaller traits are great, even if only one struct uses the trait. It makes it clearer for functions which part of the interface is relevant. We should use smaller traits more in Pumpkin!

I don't think they do different things. EventTarget::register maps the events to eventually put them into Dispatcher::register. So In this case I think the names being the same is okay.

One registers itself, and the other registers another entity. Not entirely the same but similar. In any case this is probably not so important so we could leave the names and if later it becomes an issue we can change.

Overall this looks good to me. The only issue is with the things this PR does not do. It seems okay with me to proceed without those points, but please see what to do with those, e.g., open issues for later?

[EmirDe]

See my comments for potential changes, but overall seems good for merging

[ImkoMarijnissen]

Some small points I think could be addressed but other than that it looks good to me!