Make all of the trimmer aware of recursive interfaces #103317
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,8 +51,8 @@ void ValidateMethodRequiresUnreferencedCodeAreSame (OverrideInformation ov) | |
| nameof (RequiresUnreferencedCodeAttribute), | ||
| method.GetDisplayName (), | ||
| baseMethod.GetDisplayName ()); | ||
| IMemberDefinition origin = (ov.IsOverrideOfInterfaceMember && ov.RuntimeInterfaceImplementation.Implementor != method.DeclaringType) | ||
| ? ov.RuntimeInterfaceImplementation.Implementor | ||
| : method; | ||
| Context.LogWarning (origin, DiagnosticId.RequiresUnreferencedCodeAttributeMismatch, message); | ||
|
Comment on lines
+59
to
62
There was a problem hiding this comment. It looks like this is the reason for the class Base : IFoo
{
[Ruc("Message")]
[UnconditionalSuppressMessage("IL2046")]
void M() { } // Suppressed IL2046
}
class Derived : Base, IFoo // Another 2046 (unsuppressed)
{
}
interface IFoo
{
void M();
}Since There was a problem hiding this comment. I see, so it sounds like it's due to a combination of this PR and #104753. I think it's fine to produce another warning on the derived type in that case - it's similar to the compiler error behavior. I think we should just add the same suppressions to the derived types, citing dotnet/linker#1187, no need to block this PR on that issue. |
||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| // Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System.Collections.Immutable; | ||
| using System.Diagnostics; | ||
| using Mono.Cecil; | ||
|
|
||
| namespace Mono.Linker | ||
| { | ||
| internal sealed class InterfaceImplementationChain | ||
| { | ||
| /// <summary> | ||
| /// The type that has the InterfaceImplementation - either the <see cref="Implementor"/> or a base type of it. | ||
| /// </summary> | ||
| public TypeReference TypeWithInterfaceImplementation { get; } | ||
|
|
||
| /// <summary> | ||
| /// The path of .interfaceimpl on <see cref="RuntimeInterfaceImplementation.Implementor"/> or a base type that terminates with <see cref="RuntimeInterfaceImplementation.InflatedInterfaceType"/>. | ||
| /// </summary> | ||
| public ImmutableArray<InterfaceImplementation> InterfaceImplementations { get; } | ||
|
|
||
| public InterfaceImplementationChain (TypeReference typeWithInterfaceImplementation, ImmutableArray<InterfaceImplementation> interfaceImplementation) | ||
| { | ||
| TypeWithInterfaceImplementation = typeWithInterfaceImplementation; | ||
| InterfaceImplementations = interfaceImplementation; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns true if all the .interfaceImpls in the chain and the are marked. | ||
| /// </summary> | ||
| /// <param name="annotations"></param> | ||
| /// <returns></returns> | ||
| public bool IsMarked (AnnotationStore annotations, ITryResolveMetadata context) | ||
| { | ||
| var typeDef = context.TryResolve (TypeWithInterfaceImplementation); | ||
| // If we have the .interfaceImpls on this type, it must be resolvable | ||
| Debug.Assert (typeDef is not null); | ||
| if (!annotations.IsMarked (typeDef)) | ||
| return false; | ||
|
|
||
| foreach (var impl in InterfaceImplementations) { | ||
| if (!annotations.IsMarked (impl)) | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| // Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System; | ||
| using System.Diagnostics; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using Mono.Cecil; | ||
|
|
||
| namespace Mono.Linker | ||
| { | ||
|
|
@@ -14,28 +15,28 @@ public class OverrideInformation | |
|
|
||
| public MethodDefinition Override { get; } | ||
|
|
||
| internal RuntimeInterfaceImplementation? RuntimeInterfaceImplementation { get; } | ||
|
|
||
| internal OverrideInformation (MethodDefinition @base, MethodDefinition @override, RuntimeInterfaceImplementation? runtimeInterface = null) | ||
| { | ||
| Base = @base; | ||
| Override = @override; | ||
| RuntimeInterfaceImplementation = runtimeInterface; | ||
| // Ensure we have an interface implementation if the base method is from an interface and the override method is on a class | ||
| Debug.Assert (@base.DeclaringType.IsInterface && runtimeInterface != null | ||
| || [email protected] && runtimeInterface == null); | ||
| // Ensure the interfaceImplementor is for the interface we expect | ||
| Debug.Assert (@base.DeclaringType.IsInterface ? runtimeInterface!.InterfaceTypeDefinition == @base.DeclaringType : true); | ||
| } | ||
|
|
||
| public InterfaceImplementation? MatchingInterfaceImplementation | ||
| => RuntimeInterfaceImplementation?.InterfaceImplementationChains[0].InterfaceImplementations[0]; | ||
|
|
||
| public TypeDefinition? InterfaceType | ||
| => RuntimeInterfaceImplementation?.InterfaceTypeDefinition; | ||
|
|
||
| [MemberNotNullWhen (true, nameof (RuntimeInterfaceImplementation))] | ||
| public bool IsOverrideOfInterfaceMember | ||
| => RuntimeInterfaceImplementation != null; | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| // Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Collections.Immutable; | ||
| using System.Diagnostics; | ||
| using System.Linq; | ||
| using Mono.Cecil; | ||
|
|
||
| namespace Mono.Linker | ||
| { | ||
| /// <summary> | ||
| /// Represents an implementation of an interface on a type that may be directly on the type or on a base type or implemented interface. | ||
| /// This type is considered to implement the interface at runtime, even though the interface may not be directly on the type. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This type should be used for marking, but should NOT be used to check if a runtime interface implementation has been marked. | ||
| /// This type represents the most direct way an interface may be implemented, but it may be implemented in a less direct way that is not represented here. | ||
| /// You should check all possible implementation 'paths' to determine if an interface is implemented, for example <see cref="MarkStep.IsInterfaceImplementationMarkedRecursively"/>. | ||
| /// </remarks> | ||
| internal sealed class RuntimeInterfaceImplementation | ||
| { | ||
| /// <summary> | ||
| /// The type that implements <see cref="RuntimeInterfaceImplementation.InflatedInterfaceType"/>. | ||
| /// </summary> | ||
| public TypeDefinition Implementor { get; } | ||
|
|
||
| /// <summary> | ||
| /// All the chains of .interfaceImpls that cause <see cref="Implementor"/> to implement <see cref="InflatedInterfaceType"/> | ||
| /// </summary> | ||
| public ImmutableArray<InterfaceImplementationChain> InterfaceImplementationChains { get; } | ||
|
|
||
| /// <summary> | ||
| /// The inflated interface type reference that is implemented by <see cref="Implementor"/>. | ||
| /// </summary> | ||
| public TypeReference InflatedInterfaceType { get; } | ||
|
|
||
| /// <summary> | ||
| /// The <see cref="TypeDefinition"/> of <see cref="InflatedInterfaceType"/> | ||
| /// </summary> | ||
| public TypeDefinition? InterfaceTypeDefinition { get; } | ||
|
|
||
| public RuntimeInterfaceImplementation (TypeDefinition implementor, TypeReference interfaceType, TypeDefinition? interfaceTypeDefinition, IEnumerable<InterfaceImplementationChain> interfaceImplementations) | ||
| { | ||
| Implementor = implementor; | ||
| InterfaceImplementationChains = interfaceImplementations.ToImmutableArray (); | ||
| InflatedInterfaceType = interfaceType; | ||
| InterfaceTypeDefinition = interfaceTypeDefinition; | ||
| } | ||
|
|
||
| public bool IsAnyImplementationMarked (AnnotationStore annotations, ITryResolveMetadata context) | ||
| { | ||
| if (annotations.IsMarked (this)) | ||
| return true; | ||
| foreach (var chain in InterfaceImplementationChains) { | ||
| if (chain.IsMarked (annotations, context)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the linker recognizes interfaces on all base types and recursive interfaces, these methods are recognized as overrides of IReflect, which are marked with DAM. There are already suppressions for other missing DAM annotations, so I decided to just suppress the warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are warning due to interfaces declared on the base type (or recursively required by interfaces declared on the base type), right? For example:
I think this example should only warn on the Base virtual method. If Derived also had an interfaceimpl of I, then I would expect to see a warning on Derived.M as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes sense to me. But them should we warn if both have an interfaceimpl of I but Derived doesn't override M()? I lean towards yes in this case.
Also, in IL it is valid for an abstract Base to implement I but provide no methods. I assume we should still warn on Derived.M() in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - in general I think a good model to follow is the compiler error behavior when types don't implement interface methods due to mismatching signatures.
Your second example is interesting, thanks for pointing that out. I agree that one should warn on Derived.M, but it kind of breaks the way I've been thinking about this. I wanted the rule to be roughly:
"When a type requires an interface (via explicit or recursive .interfaceimpl), there should be a warning on any mismatching annotations for the method implementing a method of that interface - pointing to either a method on the type, or the type (if the implementing method isn't on the type itself)."
Now I think we need to add something like:
"When a type requires an interface via an explicit/recursive .interfaceimpl on an abstract (maybe recursive) base class, and none of the base classes provides an implementation of one of the interface methods...".
I think it would be fine to implement the simpler rule in this PR and make a separate change to deal with the abstract case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented the simple warning behavior and filed #106552