Skip to content

Evaluate value-changing conversions in property chains - #1194

Open
dwcullop wants to merge 1 commit into
mainfrom
u/dacullop/main/evaluate-value-changing-conversions
Open

dwcullop wants to merge 1 commit into
mainfrom
u/dacullop/main/evaluate-value-changing-conversions

Conversation

@dwcullop

Copy link
Copy Markdown
Member

Problem

Property path expressions passed to WhenPropertyChanged / WhenValueChanged may contain conversions. ExpressionBuilder.CreateInvoker treated every ExpressionType.Convert node as a no-op pass-through, returning the original chain target unchanged.

That is correct for reference casts and boxing/unboxing, where the target object is the same instance and the runtime resolves the cast anyway. It is wrong for value-changing conversions: numeric conversions ((decimal)price.Amount) and user-defined conversion operators produce a different value, and the next step in the property path must read from that converted value rather than from the original operand.

The result was that an expression such as price => ((decimal)price.Amount).Scale attempted to read decimal.Scale off the source double, surfacing an error instead of the observed value.

Fix

CreateInvoker now distinguishes the two cases:

  • Built-in reference casts and boxing/unboxing (no conversion method, and at least one side is a reference type) keep the existing pass-through.
  • Numeric and user-defined conversions compile a small invoker that unboxes the operand, applies the original conversion node, and re-boxes the converted result, so subsequent property access targets the converted value.

Conversions are also supported as the final step of a path, and reference/interface casts continue to follow object replacement in nested chains.

Tests

Adds WhenPropertyChangedBehaviorFixture.Conversions.cs covering:

  • numeric conversion before property access — initial value
  • numeric conversion before property access — subsequent changes, with and without notifyOnInitialValue
  • numeric conversion as the leaf of the path
  • reference cast before property access
  • interface cast before property access, including a replaced intermediate child

The shared fixture gains a seeded Randomizer (deterministic, seed written to test output) and a small ObservablePrice model to support these tests. Prior focused validation of these conversion tests passed.

Docs

Public XML documentation on WhenPropertyChanged / WhenValueChanged and the cache/list instruction files now state that property paths evaluate numeric conversions before subsequent property access.

Scope

Conversion handling only. No changes to property-subscription initialization lifetime behavior.

Fixes #1177

Property path expressions may contain conversions. Reference casts and boxing conversions leave the chain target usable as-is, but numeric and user-defined conversions produce a different value, and the next step in the path must read from that converted value.

Compile a conversion invoker for value-changing conversions so subsequent property access targets the converted result, and keep the pass-through for reference casts and boxing.

Fixes #1177
@dwcullop

Copy link
Copy Markdown
Member Author

P2: Converted property paths collide in the property-factory cache.

Locations: ExpressionBuilder.GetMembers, lines 15-21, and ToCacheKey, lines 134-147.

GetMembers() stops when it reaches a conversion. Consequently, ((decimal)x.First).Scale and ((decimal)x.Second).Scale generate the same key: the root type followed by Scale, without the property preceding the conversion. ObservablePropertyFactoryCache.GetFactory() then reuses the first expression's accessor and notification subscriptions for the second expression.

Reproduced with both expressions on the same model: the expected initial scales were 2 and 3, but both subscriptions reported 2. Changing Second to a value with scale 4 produced no update on its subscription. This is an existing cache-key weakness exposed by the newly supported conversion paths.

Suggested fix: build factory identity from the complete expression path, including each member's identity, conversion source/destination types, any user-defined conversion method, and the result type. SplitIntoSteps() already traverses the supported conversion nodes in this file, so it provides an existing traversal to reuse. Merely skipping conversion nodes in GetMembers() would still collide for different conversions of the same member. Add coverage for two different converted properties on one model and for different conversions of one property.

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.

[Bug]: Numeric conversions in observed property chains are discarded

1 participant