Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix computed abstract property decompilation #180

Merged
merged 10 commits into from
Dec 14, 2021
Merged

Fix computed abstract property decompilation #180

merged 10 commits into from
Dec 14, 2021

Conversation

DennisInSky
Copy link
Contributor

@DennisInSky DennisInSky commented Dec 2, 2021

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

Fix abstract computed property decompilation in case of there is an intermediate generic abstract class
@DennisInSky DennisInSky changed the title Fix abstract computed property decompilation Fix computed abstract property decompilation Dec 2, 2021
@hazzik
Copy link
Owner

hazzik commented Dec 2, 2021

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

@DennisInSky
Copy link
Contributor Author

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
Copy link
Owner

hazzik commented Dec 7, 2021

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
Copy link
Owner

hazzik commented Dec 7, 2021

@DennisInSky please check if that solution works for you.

@DennisInSky
Copy link
Contributor Author

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
                    }
                }
            }
        }
  1. 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
Copy link
Owner

hazzik commented Dec 7, 2021

  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
Copy link
Contributor Author

DennisInSky commented Dec 9, 2021

  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
Copy link
Owner

hazzik commented Dec 9, 2021

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

@DennisInSky
Copy link
Contributor Author

@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
Copy link
Owner

hazzik commented Dec 12, 2021

Cool thanks for confirming.

@hazzik hazzik merged commit bfeddbd into hazzik:develop Dec 14, 2021
@hazzik
Copy link
Owner

hazzik commented Dec 14, 2021

Released in 0.30.0

@DennisInSky DennisInSky deleted the bugfix/hierarchyWithGenerics branch December 14, 2021 15:55
billybraga pushed a commit to billybraga/DelegateDecompiler that referenced this pull request Nov 3, 2023
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.

Abstract property in hierachy with intermediate generic classes decompiled as default
2 participants