fix(generator): preserve legacy null-chain observation semantics - #63
Conversation
Superseded - regressions found in deeper property chains, see follow-up review.
glennawatson
left a comment
There was a problem hiding this comment.
Thanks for chasing this down Dan - the diagnosis is right, ReturnObservable(default(T)) on a null intermediate really does diverge from ReactiveUI's expression-chain semantics, and NullParentObservationBehavior is the right shape for the fix.
We hit a regression when testing it against deeper chains though, so a couple of things need fixing before this can land - details inline.
|
@danwalmsley tagging for the notification. Just some regressions I found testing the branch. Thanks for the PR looks good other than that. |
|
@glennawatson thank you for the speedy review, I have applied your suggested fixes and added tests. let me know what you think :D |
|
Looks good @danwalmsley -- I'll do a release after the CI goes green (I'll merge in the other outstanding PRs as much as possible too first). |
@glennawatson I came across this behavioural difference when trying to upgrade some applications. Performance improvements are incredible :D but this is blocking the upgrade.
What kind of change does this PR introduce?
Bug fix - generated observation code no longer diverges from the runtime expression-chain semantics when an intermediate object in a property path is null.
What is the new behavior?
WhenAnyValueand the other generated observation APIs suppress emission while an intermediate parent is null, and reattach when it becomes non-null again:EmptyObservable<T>is used while an intermediate parent is null.Switchsubscription stays alive, so observation reattaches once the parent is set.ReturnObservable<T>(default(T))is preserved for inline binding consumers, which need the default notification to clear targets and unregister handlers.NullParentObservationBehaviorenum.A legitimate null or default leaf is unaffected: if
Childexists andChild.Nameis null, that null is still emitted.What is the current behavior?
WhenAnyValueis documented as a drop-in ReactiveUI compatibility shim, but generated deep-property observations substitutedefault(T)whenever an intermediate object is null. Given:the generated implementation emits
new ReturnObservable<T>(default(T))whileChildis null. An isolated comparison over an identical transition trace produced:Alice, Bob, CharlieRuntimeObservationFallbackAlice, Bob, Charlienull, Alice, Bob, null, CharlieThose synthetic values are observable behavior changes: reference-type consumers can receive an unexpected null and throw, and value-type consumers can receive a synthetic zero or false.
What might this PR break?
This is an intentional behavioural change for generated observation:
default(T)while an intermediate was null will now receive nothing until the chain is repaired. This is the ReactiveUI-compatible behavior.Bind,BindOneWay,BindTwoWay,OneWayBind,BindCommand,BindInteraction) are deliberately unchanged and still receive the default, so targets are cleared and handlers unregistered.Checklist
mainbranchAdditional information
Regression coverage added:
WhenAnyValuedirectly withRuntimeObservationFallback.WhenAnyValueover an identical transition trace.WhenChangednull-intermediate expectation.BindInteractioncleanup behavior.Validation: