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

MarkExplicitInterfaceImplementation doesn't work for generic interfaces #1424

Open
MichalStrehovsky opened this issue Aug 10, 2020 · 2 comments

Comments

@MichalStrehovsky
Copy link
Member

https://github.com/mono/linker/blob/9e3191cad279e945a4e2c99340454959fec637b0/src/linker/Linker.Steps/MarkStep.cs#L2469-L2492

This code is trying to ensure that if an explicit interface implementation method was marked, the appropriate member of the interface list is also preserved.

The code falls victim to Cecil's behavior where the act of resolving a TypeRef/MethodRef strips away the genericness information.

As a result, for code like this:

class Foo : IFoo<object>, IFoo<string>
{
    int IFoo<object> Frob() => 0;
    int IFoo<string> Frob() => 0;
}

Will end up preserving the first IFoo entry in the interface list, irrespective of whether IFoo<object>.Frob or IFoo<string>.Frob was the one responsible for MarkExplicitInterfaceImplementation getting called.

I think this is low impact, since we can only get there if the explicit impl was rooted through ILLinkTrim, but ILLinkTrim was the motivating factor why MarkExplicitInterfaceImplementation was added in the first place, so YMMV.

Found while investigating #1421. This issue gives the other issue a more weird failure mode.

@am11
Copy link
Member

am11 commented Aug 10, 2020

The code falls victim to Cecil's behavior where the act of resolving a TypeRef/MethodRef strips away the genericness information.

Was this PR trying to address the same issue: jbevain/cecil#229?

@MichalStrehovsky
Copy link
Member Author

Was this PR trying to address the same issue: jbevain/cecil#229?

That looks like a different thing. Linker rolls its own override resolution logic and doesn't call into Cecil for this.

What I'm referring to is a fundamental design choice in Cecil and not a bugfix thing - it would break a lot of existing code based on Cecil. CCI has resolution logic that IMHO works better and doesn't come with this property/trap.

@marek-safar marek-safar added this to the .NET 6.0 milestone Nov 13, 2020
@marek-safar marek-safar added this to Priority 1 in .NET6 Prioritization Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

3 participants