Skip to content

Compiled binding path for subclasses - support for mixed collection#382

Open
DanielTrommel wants to merge 4 commits into
w-ahmad:mainfrom
DanielTrommel:fix/compiled-binding-path-for-subclasses
Open

Compiled binding path for subclasses - support for mixed collection#382
DanielTrommel wants to merge 4 commits into
w-ahmad:mainfrom
DanielTrommel:fix/compiled-binding-path-for-subclasses

Conversation

@DanielTrommel

@DanielTrommel DanielTrommel commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

@w-ahmad , back again with an fix/improvement on the Func/BindingPath code.

Ref this PR, #178, in which we worked on generating a Func at runtime once, instead of repeated reflection to get a cell's value.
In the comments on that PR (#178 (comment)), I already mentioned that the current Func is built just using the first instance for which we happen do a GetPropertyValue(), which might cause trouble.

The specific problem: If the instances used for the rows in the table is a mixture of subclasses, and the (first) binding (step) is to an element on the shared parent class, the current code will throw an InvalidCastException when sorting.

The reason is that the first instance that is used to built the Func, dictates the explicit type cast that is done;
An example: suppose the type of the instance is MySubClass and for a column our BindingPath is "MyProperty", the current code in main generates this:

.Lambda #Lambda1<System.Func`2[System.Object,System.Object]>(System.Object $obj) {
    .If ((MySubClass)$obj != null) {
        ((MySubClass)$obj).MyProperty
    } .Else {
        null
    }
}

However, if MyProperty is actually defined on MySuperClass, that code should use a cast to that type instead:

.Lambda #Lambda1<System.Func`2[System.Object,System.Object]>(System.Object $obj) {
    .If ((MySuperClass)$obj != null) {
        ((MySuperClass)$obj).MyProperty
    } .Else {
        null
    }
}

This way, if the CollectionView contains a mixture of instance of different subclasses for MySuperClass, we won't get an InvalidCastException.

Furthermore:

  • The above example is only about the first segment in the Binding Path, but the subclassing can affect any segment of the binding path (different subclass instances, causing a potential InvalidCastException), but this PR handles this as well, and I added Tests for the case this subclassing happens further on in the BindingPath
  • While merging the latest main, I noticed you added a func for the Setter variant, so I also hardened your code so it will handle subclassing
  • Did some small renaming "PropertyPath" -> "BindingPath" (leftover from my initial code, which was not representative)
  • Then, your new Setter flow seemed to partially duplicate the Getter flow, and I figured that both flows are basically the same, except for the last segment of the BindingPath: For the Setter func, you do the set action. Hence, I refactored this, which resulted in a bit less code, and should make the difference between the two flows clearer.
    • This refactor gave rise to a subtle bug in the code (thanks to your test case!) which surfaced due to the above, different casting (before, the cast to the more specific subclass would `cover' this). See the last commit for the fix, and the commit description for more details.

The above points are in order and in separate commits.

… in the BindingPath string). Included test cases for collection of items with the same parent class, on which shared properties and indexers are defined.

Signed-off-by: Daniël Trommel <d.s.trommel@gmail.com>
…tial naming slip-up from when I first build it (focussed on properties)

Signed-off-by: Daniël Trommel <d.s.trommel@gmail.com>
…s the original Getter flow; when navigating to the final segment, the code is exactly the same. Only the final bit of code (get versus set) is different.

This refactor also exposed a subtle bug;  GetCompiledValueSetter_ShouldNotSet_ForOutOfBoundsNonGenericListIndex failed with ArgumentOutOfRangeException. This was a latent bug in the setter that the old over-specialization was masking:

 - For a property typed IList (non-generic ArrayList), the shared navigation correctly leaves current.Type as the interface IList (no over-specialization).
 - The setter's bounds-check did current.Type.GetProperty("Count"), but Count is declared on the base ICollection interface — and reflection doesn't surface inherited members on interface types. So countProperty was null, the bounds check was skipped, and the raw indexer assignment threw.
 - The old setter only "worked" by accident: it had converted current to the concrete ArrayList, where Count is found directly.
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.

1 participant