Don't remove MethodImpl if overridden method is not in a link assembly #2771
Conversation
Before removing overrides, we should check if the overridden method is in an ignore scope.
|
Definitely think we need a test here. @sbomer any idea on what might be happening in the runtime repo that isn't the same in our testing system? |
|
We should be able to repro it in the test infra, sounds like @jtschuster was able to get a repro. |
|
One nit: when you check it in, I'd update the commit title to not mention |
jtschuster
changed the title
|
Ah, so the thing I was missing here was
which turns the S.P.C reference to "skip" |
|
About how long does it normally take this to flow to |
|
Looks like dotnet/runtime#68650 has it and I just need to wait on that to merge. |
|
Typically less then a day... if everything is green - which it hasn't been (as we improve the analyzer we find either issues in the analyzer or in the setup in runtime repo) |
… assembly (dotnet#2771)" This reverts commit 4461068.
* Revert "Trim static interfaces (#2741)" This reverts commit a073a68. * Revert "Don't remove MethodImpl if overridden method is not in a link assembly (#2771)" This reverts commit 4461068. * Revert "Fix NullReferenceException when sweeping unused static interface (#2783)" This reverts commit 00e9a15.
dotnet/linker#2771) Before removing overrides, we should check if the overridden method is in an ignore scope. Commit migrated from dotnet/linker@4461068
* Revert "Trim static interfaces (dotnet/linker#2741)" This reverts commit dotnet/linker@a073a68. * Revert "Don't remove MethodImpl if overridden method is not in a link assembly (dotnet/linker#2771)" This reverts commit dotnet/linker@4461068. * Revert "Fix NullReferenceException when sweeping unused static interface (dotnet/linker#2783)" This reverts commit dotnet/linker@00e9a15. Commit migrated from dotnet/linker@eb6144b
Before removing overrides, we should check if the overridden method is
in an ignore scope.
In the linker we consider types and methods in a
skipassembly as "removed", so when the linker would check if an overridden method is removed, it would see that it is considered removed, even though it should be considered kept. The workaround is to use the IgnoreScope helper to determine if the interface method is in alinkassembly and removed, and only remove the MethodImpl if both are true.Now, whether
ShouldRemoveshould return true or false for things in a skip assembly, I'm not sure. It depends on whether it does / should mean "Actively removed from the assembly" or "not in the final output".This PR fixes the issues found in dotnet/runtime#68612.