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

Fix computed abstract property decompilation #180

Closed DennisInSky closed 2 years ago

DennisInSky commented 2 years ago

Fix comuted abstract property decompilation in case of there is an intermediate generic abstract class Fixes #179

hazzik commented 2 years ago

Hi, this just hides the symptoms unfortunately. You can check that by adding another property to Fish<> which has own implementation.

DennisInSky commented 2 years ago

Hi, this just hides the symptoms unfortunately. You can check that by adding another property to Fish<> which has own implementation.

Hi, not quite sure that I follow what you mean by "hides the symptoms". Do you mean that if there is intermediate implmentation of Fish<>.Species the result won't be as expected?

hazzik commented 2 years ago

Do you mean that if there is intermediate implmentation of Fish<>.Species the result won't be as expected?

Yes. If Fish<>.Species has an implementation whole thing fails.

hazzik commented 2 years ago

@DennisInSky please check if that solution works for you.

DennisInSky commented 2 years ago

Do you mean that if there is intermediate implmentation of Fish<>.Species the result won't be as expected?

Yes. If Fish<>.Species has an implementation whole thing fails.

Hi @hazzik thank you very much for the commits. There are a couple of things to mention:

  1. With the new code, the generated expression looks quite complex. There are redundant conditions
        .If (
            $p .Is DelegateDecompiler.EntityFramework.Tests.EfItems.Abstracts.WhiteShark
        ) {
            "Carcharodon carcharias"
        } .Else {
            .If (
                $p .Is DelegateDecompiler.EntityFramework.Tests.EfItems.Abstracts.AtlanticCod
            ) {
                "Gadus morhua"
            } .Else {
                .If (
                    $p .Is DelegateDecompiler.EntityFramework.Tests.EfItems.Abstracts.WhiteShark
                ) {
                    "Carcharodon carcharias"
                } .Else {
                    .If (
                        $p .Is DelegateDecompiler.EntityFramework.Tests.EfItems.Abstracts.AtlanticCod
                    ) {
                        "Gadus morhua"
                    } .Else {
                        null
                    }
                }
            }
        }
  2. I have added yet another test for an abstract property implemented in generic class. The generated expression looks like this which definitely won't and it does not (the test fails) work with EF Core.
        .If (
            $p .Is DelegateDecompiler.EntityFramework.Tests.EfItems.Abstracts.Fish`1[System.String]
        ) {
            "Fish"
        } .Else {
            .If (
                $p .Is DelegateDecompiler.EntityFramework.Tests.EfItems.Abstracts.Fish`1[System.Int32]
            ) {
                "Fish"
            } .Else {
                null
            }
        }

    To be more precise it works till the moment a condition applied to it. I am not sure if your changes were supposed to cover this case as well. To my mind, in order to make such cases working, traversing should go down till the leaves even though implemtation belongs to an intermediate class. It is the only safe option with EF as I might not report about all intermediate classes from hierarchy to EF, i.e. even with non-generic classes checking for some type which EF is not aware about as of an entity won't work as it does not parse class hierarchy. Please let me know what you think.

hazzik commented 2 years ago
  1. With the new code, the generated expression looks quite complex. There are redundant conditions

Fixed, thanks.

  1. <...> The generated expression looks like this which definitely won't and it does not (the test fails) work with EF Core.

The expression generated is correct. It does not work, because you've skipped Fish<> in your initial mapping (You were having .HasBaseType<Fish>() instead of a generic base type.)

DennisInSky commented 2 years ago
  1. With the new code, the generated expression looks quite complex. There are redundant conditions

Fixed, thanks.

Thanks, it looks good now.

  1. <...> The generated expression looks like this which definitely won't and it does not (the test fails) work with EF Core.

The expression generated is correct. It does not work, because you've skipped Fish<> in your initial mapping (You were having .HasBaseType<Fish>() instead of a generic base type.)

I thought that it might not be always the case when the entire CLR hierarchy mapped to EF hierarchy.

Thank you very much for your effort for fixing the issue. Any plans to release a version with this fix?

hazzik commented 2 years ago

@DennisInSky can you try if it works in your case? If so I'll merge and release ASAP.

DennisInSky commented 2 years ago

@DennisInSky can you try if it works in your case? If so I'll merge and release ASAP.

Hi @hazzik . I checked my case with the new code wired up and everything worked out as a charm. Thanks once again.

hazzik commented 2 years ago

Cool thanks for confirming.

hazzik commented 2 years ago

Released in 0.30.0