search

hazzik / DelegateDecompiler

A library which is able to decompile a delegate or a method body to its lambda representation
MIT License
522 stars 62 forks source link

Bug: Cannot Decompile IQueryable<T> after using constrained generic when TInterface has multiple implementations #199

Closed cmeyertons closed 1 year ago

cmeyertons commented 1 year ago

It seems like there is a bug when there is an interface property involved -- when there are multiple implementations of the interface, the decompiler generates a ternary of each available implementation type.

However, in the initial decompile, we have a IQueryable (not TInterface), so we shouldn't have to do any kind of ternary logic at all.

Please see https://github.com/hazzik/DelegateDecompiler/pull/198 for the repro case.

Update: PR now has a working solution that appears to pass all tests, but it might not be fully comprehensive for what we are doing.

If I detect that a cast is occurring from the T from Queryable<T> and is the expressions parameter, we inform the method body decompiler the type we want to use is T, not a ternary of all of the possible implementations.

hazzik commented 1 year ago

That is interesting. There is a similar test to one you've submitted:

https://github.com/hazzik/DelegateDecompiler/blob/bf1fffc907a8e346852580107547aaaf4dc5eb4c/src/DelegateDecompiler.EntityFramework.Tests/TestGroup50Types/Test01Strings.cs#L73-L93

Only difference is in where T definition. One we have with class, the submitted is without class. Adding class to your test fixes it. And removing breaks our test.

cmeyertons commented 1 year ago

The class constraint fixing it is quite bizarre! Your solution is more elegant than mine - I just wasn’t sure I could apply it as globally as you did and tried to leverage the knowledge of the caller.

Looks great to me. What is the turnaround time on getting a fix published? On Monday, we will test adding the class constraint as a workaround.

hazzik commented 1 year ago

What is the turnaround time on getting a fix published?

I was going to review one other PR, and then wanted to publish a new version. It is already past my deadline as I was going to do that on the past weekend (it's Monday here already).

hazzik commented 1 year ago

I just wasn’t sure I could apply it as globally as you did

When there is an upcasting (cast to a parent type) - it should be safe to remove. I was worried about explicit interfaces, but tested them locally - works as expected.

cmeyertons commented 1 year ago

Sounds great! Thank you very much for the quick resolution on this one!