Skip to content

Forwarding proceeds through apply to reach default rule - #760

Merged
eb8680 merged 2 commits into
masterfrom
jf-fwd-apply
Sep 4, 2026
Merged

Forwarding proceeds through apply to reach default rule#760
eb8680 merged 2 commits into
masterfrom
jf-fwd-apply

Conversation

@jfeser

@jfeser jfeser commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Closes #759

This changes the behavior of fwd so that __apply__ is called instead of reaching the default rule when forwarding from the last handler in the chain. This gives __apply__ handlers a chance to see the operation. The operation default rule is now called from within the default rule of __apply__.

This behavior does not apply to the __apply__ operations themselves. They are now instances of ApplyOperation to facilitate making this distinction.

@jfeser
jfeser requested a review from eb8680 August 31, 2026 20:25

@eb8680 eb8680 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure all the special-casing around ApplyOperation is right. Why do we need that type at all? Why shouldn't behavior be uniform with the sole exception of the base Operation.__apply__?

@jfeser

jfeser commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

A version that only special cases Operation.__apply__ can work as well, but it involves more calls to __apply__. For example, the call sequence in test_fwd_through_apply_operation_subtypes would become ['exact', 'derived apply', 'apply', 'base apply', 'apply', 'apply', 'default'].

@eb8680

eb8680 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

That test seems especially suspect. Why should calling fwd in a handler of DerivedOperation.__apply__ jump directly up the DerivedOperation MRO to a handler of BaseOperation.__apply__, as opposed to going to Operation.__apply__'s rule for DerivedOperation.__apply__? Won't this interfere with the way bound-method operations currently resolve behavior by going instance -> class in their default rule and using super() to traverse their owners' MROs? I think we need a diagram or something that clarifies the interactions between the different resolution mechanisms once and for all before we change the status quo behavior.

@jfeser

jfeser commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

If we run the same test on master, but remove the handler for f, we get calls = ['derived apply', 'base apply', 'apply', 'default']. This direct forwarding behavior is the current semantics, not a new change.

@eb8680
eb8680 merged commit 57312bb into master Sep 4, 2026
35 checks passed
@eb8680
eb8680 deleted the jf-fwd-apply branch September 4, 2026 14:08
@eb8680

eb8680 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

I merged this because of the downstream dependency but I would like to resolve the correctness question #783 at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Apply handlers should be possible targets of fwd

2 participants