Skip to content

Update DynamicMethod to use module instead of type - #6

Closed
carlosxjose wants to merge 2 commits into
geomatics-io:masterfrom
carlosxjose:master
Closed

Update DynamicMethod to use module instead of type#6
carlosxjose wants to merge 2 commits into
geomatics-io:masterfrom
carlosxjose:master

Conversation

@carlosxjose

Copy link
Copy Markdown

Replaced typeof(T) with typeof(T).Module as the fourth parameter in the DynamicMethod constructor within BuildGetPropertyMethod. This change ensures the dynamic method is associated with the module of the type T, improving compatibility and addressing potential issues with dynamic method creation.

Replaced `typeof(T)` with `typeof(T).Module` as the fourth parameter
in the `DynamicMethod` constructor within `BuildGetPropertyMethod`.
This change ensures the dynamic method is associated with the module
of the type `T`, improving compatibility and addressing potential
issues with dynamic method creation.
@carlosxjose

Copy link
Copy Markdown
Author

This fix #5

Changed IL code generation in AggregateBindingListView.cs to use OpCodes.Callvirt instead of OpCodes.Call for property getter calls, ensuring correct virtual dispatch.
@christianarielli

Copy link
Copy Markdown
Member

Thanks for digging into this and for the PR — the typeof(T).Module insight
is exactly the right fix for the root cause, and it pointed us in the right
direction. I'm going to close this without merging, though, because it turned
out to be a partial fix, and we've since landed a more complete one that
supersedes it.

Why this alone wasn't enough

BuildGetPropertyMethod is only one of four places in
AggregateBindingListView.SortComparer that build a DynamicMethod using
typeof(T) as the owner:

  • BuildGetPropertyMethod (reference-type properties implementing
    IComparable, e.g. string) — the one this PR fixes
  • BuildValueTypeComparison (value-type properties, e.g. int, DateTime)
  • BuildRefTypeComparison (currently dead code, but same pattern)
  • BuildNullableComparison (Nullable<T> properties)

When T is an interface (e.g. BindingListView<IModelView>), all four throw
Invalid type owner for DynamicMethod for exactly the reason you found —
this PR just happened to only touch the one exercised by string-typed
properties. That matches what showed up later in the issue #5 thread:
string columns started sorting fine after this change was applied locally,
but int/DateTime columns kept throwing the same error, because
BuildValueTypeComparison still used typeof(T) as the owner.

There was also a second, independent bug in BuildValueTypeComparison /
BuildRefTypeComparison: the emitted IL cast the second compared value to
IComparable instead of the first (the Castclass was emitted after both
operands were already pushed), and used Call instead of Callvirt to
invoke IComparable.CompareTo. The desktop .NET Framework JIT tolerated
both of those, but CoreCLR doesn't — it's a separate reason sorting could
fail under .NET Core/5+/8+ even for a concrete (non-interface) T.

What we did instead

  • Applied the typeof(T)typeof(T).Module fix to all four builder
    methods, not just BuildGetPropertyMethod.
  • Changed every generated getter/CompareTo call from Call to Callvirt
    (interface methods have no method body of their own, so a non-virtual
    Call is invalid IL for them).
  • Fixed the Castclass ordering bug in BuildValueTypeComparison /
    BuildRefTypeComparison described above.

See b9c2fad and 0917592 for the actual changes, and tests/Issue5 /
tests/Issue5Core for repro projects covering both the interface-T case
from your PR and the value-type sorting case, on both net48 and net8.0.

Thanks again for tracking this down and posting the fix — it saved a lot of
diagnosis time even though we ended up generalizing it further.

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.

2 participants