Mark most specific static DIM for types marked RelevantToVariantCasting #97487
Conversation
ghost
added
the
linkable-framework
dotnet-issue-labeler
bot
added
the
area-Tools-ILLink
|
Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar Issue DetailsWhen a type is marked as relevant to variant casting we should consider all it's interfaces and base types also relevant to variant casting. This causes these lines to mark overrides of static interfaces whose types are relevant to variant casting, so we mark all static default interface methods. This causes a little bit of overmarking (IMiddle in the tests could be removed).
|
jtschuster
changed the title
| var overridingMethods = Annotations.GetOverrides (method); | ||
| if (overridingMethods is not null) { | ||
| foreach (var ov in overridingMethods ?? []) { | ||
| if (IsInterfaceImplementationMethodNeededByTypeDueToInterface (ov, ov.Override.DeclaringType)) |
There was a problem hiding this comment.
I'm not sure using ov.Override.DeclaringType as the type with interfaceimpl is correct. I'm worried this could miss a case where we added an interface method due to IgnoreScope, and the type with interfaceimpl was passed as a generic arg, but the override declaring type is an interface that provides the default impl (but wasn't passed as a generic arg).
There was a problem hiding this comment.
Shouldn't that situation be caught above when we loop over the default implementations of each interface method?
There was a problem hiding this comment.
I suppose that's true. It still worries me that the override declaring type isn't the same as the type with interfaceimpl that IsInterfaceImplementationMethodNeededByTypeDueToInterface expects. But I can't think of any problems with it. Maybe add a comment here in case we need to look at this again?
There was a problem hiding this comment.
I thought about this a little more, and it's another one of those "It works all the time, but not because the logic is correct". The DIM logic above handles static interface methods since they cannot be provided by a base type, only a DIM, but If a base method provides an instance method, we'll pass the wrong type in, but if the type implementing the interface is instantiated, the type providing the method for the interface will also be marked as instantiated.
There was a problem hiding this comment.
Looks like this fix is going to be more involved. I'll make an issue and make a separate PR.
There was a problem hiding this comment.
The DIM logic above handles static interface methods since they cannot be provided by a base type
I think static interface methods can be provided by a base type: https://sharplab.io/#v2:EYLgtghglgdgNAFxAJwK7wCYgNQB8ACATAIwCwAUBQBICmANgA43IB0AIgPYA8bAfABQBKANwUKRAAS1GzCQG8KEpRPwBmFcQBsKgCwTOXACoDBEgO4ALZjQmGJICQEl5i5W8MsAskNHk3AXwpAynJYBGYAMwgAYxtnBT9lfC0JCGAAZwRkGIRdCW8RILFySQBhF0SlNQ1tfD0Circ3ZIBOfgAiWgBPdsLKiWDg8UJ9ewlSuCd5YKA==
There was a problem hiding this comment.
Uh oh, I really thought static interface methods couldn't be provided by the base, and I think that assumption might be made in the linker. I'll set up some test cases and see what's broken.
There was a problem hiding this comment.
Ah, the compiler injects a static interface method on D that call's C's method, so I think the assumption is valid. If the implementation on D is removed, ilasm can compile the IL, but it fails at runtime with a TypeLoadException:
Unhandled exception. System.TypeLoadException: Virtual static method 'M' is not implemented on type 'D' from assembly '_, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
There was a problem hiding this comment.
Oh, good to know. Thanks for checking!
| MarkMethod (ov.Override, new DependencyInfo (DependencyKind.Override, ov.Base), ScopeStack.CurrentScope.Origin); | ||
| } | ||
| } | ||
| var overridingMethods = Annotations.GetOverrides (method); |
There was a problem hiding this comment.
I wonder if this might be causing the perf regression dotnet/sdk#40060, since it looks like we're newly walking the overrides of a virtual method every time we get here.
Fixes #96855
Fixes #96854
Previously, we weren't handling static DIMs to ensure that DIMs that provided an implementation of an interface method for an inheriting type would be kept.
This method gets rid of
_interfaceOverridesand uses_virtual_methodswith TypeMapInfo to find all interface method / implementation pairs.This PR adds static method handling to the
ProcessDefaultImplementationmethod where it previously only handled instance interface methods. It assumes all static interface methods will be needed if the type implementing the interfaceIsRelevantToVariantCasting.The DIM cache also is updated to include the method that provides the implementation for a type.