refactor(pumpkin-solver): `PropagatorConstructor::create` now returns event registrations by maartenflippo · Pull Request #456 · ConSol-Lab/Pumpkin

maartenflippo · 2026-05-27T12:31:25Z

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 · 2026-05-28T06:30:24Z

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 · 2026-05-28T09:41:22Z

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 · 2026-05-29T06:22:28Z

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!

ImkoMarijnissen · 2026-06-24T08:08:32Z

+            self.notification_engine
+                .watch_all(domain_id, events, propagator_var);


Think this could also be called register or register_events

ImkoMarijnissen · 2026-06-24T08:09:30Z

+struct NotificationEngineWatchers<'a> {
+    propagator_id: PropagatorId,
+    notificaton_engine: &'a mut NotificationEngine,
+}


Docs; also the naming could be improved, there are no watcher here, it is just the notification engine

ImkoMarijnissen · 2026-06-24T08:10:55Z

+    fn create(
+        self,
+        context: PropagatorConstructorContext,
+    ) -> (EventsToRegister, Self::PropagatorImpl);


Could we make this a separate struct, I am personally not a big fan of returning tuples since it is difficult to keep track of what is related to what (especially if we want to return more from the constructor, such as consistency checkers).

ImkoMarijnissen · 2026-06-24T08:11:02Z

@@ -48,7 +45,10 @@ pub trait PropagatorConstructor {
    fn add_inference_checkers(&self, _checkers: InferenceCheckers<'_>) {}

    /// Create the propagator instance from `Self`.


Docs should be updated

ImkoMarijnissen · 2026-06-24T08:12:59Z

+use crate::propagation::LocalId;
+use crate::variables::DomainId;
+
+/// Anything that can subscribe to domain events.


An example would be nice

ImkoMarijnissen · 2026-06-24T08:13:27Z

+
+/// Anything that can subscribe to domain events.
+pub trait EventTarget {
+    /// Add a registration of self for the given domain events with a local id.


What does it mean to add this "registration"?

ImkoMarijnissen · 2026-06-24T08:14:28Z

+
+/// Contains all the events and domains that a propagator needs to be enqueued for.
+#[derive(Clone, Debug)]
+pub struct EventsToRegister(Vec<(DomainId, EnumSet<DomainEvent>, LocalId)>);


Same comment here about structs

ImkoMarijnissen · 2026-06-24T08:46:02Z

+        let mut registration = EventsToRegister::builder();
        for (idx, var) in array.iter().enumerate() {
-            context.register(var.clone(), DomainEvents::BOUNDS, LocalId::from(idx as u32));
+            registration = registration.add(var, DomainEvents::BOUNDS, LocalId::from(idx as u32));


Could be nice to implement an Into which allows you to provide an iterator?

maartenflippo added 3 commits May 27, 2026 22:11

PropagatatorConstructor::create now returns an EventRegistration

39f97fd

Remove registration from PropagatorConstructorContext

4dd4222

Do the registration in the state

27ed751

maartenflippo requested review from EmirDe and ImkoMarijnissen May 27, 2026 12:31

maartenflippo added 2 commits May 27, 2026 22:46

Remove Watchers::watch_all in favor of event target

c5290b1

Implement the new interface in the rest of pumpkin

b456892

maartenflippo marked this pull request as ready for review May 27, 2026 13:27

Also update the proof processor

b95ad41

This was referenced May 27, 2026

feat(pumpkin-solver): Implement consistency checker infrastructure #449

Draft

refactor(pumpkin-solver): Force checkers to be registered in propagator constructor #457

Open

EmirDe reviewed May 28, 2026

View reviewed changes

Rename 'EventRegistration' to 'EventsToRegister'

6839f51

Clarify documentation

9a7d21b

EmirDe approved these changes May 29, 2026

View reviewed changes

ImkoMarijnissen approved these changes Jun 24, 2026

View reviewed changes

		self.notification_engine
		.watch_all(domain_id, events, propagator_var);

		@@ -48,7 +45,10 @@ pub trait PropagatorConstructor {
		fn add_inference_checkers(&self, _checkers: InferenceCheckers<'_>) {}

		/// Create the propagator instance from `Self`.

Uh oh!

Conversation

maartenflippo commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this adds

What this does not do

Feedback

Uh oh!

EmirDe commented May 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maartenflippo commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EmirDe commented May 29, 2026

Uh oh!

EmirDe left a comment

Choose a reason for hiding this comment

Uh oh!

ImkoMarijnissen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maartenflippo commented May 27, 2026 •

edited

Loading

maartenflippo commented May 28, 2026 •

edited

Loading