Annotate required APIs with DynamicallyAccessedMemberTypes.Interfaces #52461

vitek-karas · 2021-05-07T19:38:12Z

After the basic annotation of Type.GetInterface and Type.GetInterfaces this propagates this annotation to all places which require it. It also resolves warnings in all the places where this shows up.

For the most part this is just blindly propagating the annotations.

Most of the suppressions rely on the fact that is IFoo is referenced (and thus the Type instance of it exists at runtime), trimmer guarantees that it will keep it on all types which implement that interface. And also on additional fact that if IFoo<> is preserved, then all instantiations of it are also preserved. This is true for linker, but not necessarily true for AOT - that is we can do better if we don't have to make that assumption. For now I rely on that assumption.

After the basic annotation of Type.GetInterface and Type.GetInterfaces this propagates this annotation to all places which require it. It also resolves warnings in all the places where this shows up.

ghost · 2021-05-07T19:38:19Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-05-07T19:38:21Z

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

After the basic annotation of Type.GetInterface and Type.GetInterfaces this propagates this annotation to all places which require it. It also resolves warnings in all the places where this shows up.

For the most part this is just blindly propagating the annotations.

Most of the suppressions rely on the fact that is IFoo is referenced (and thus the Type instance of it exists at runtime), trimmer guarantees that it will keep it on all types which implement that interface. And also on additional fact that if IFoo<> is preserved, then all instantiations of it are also preserved. This is true for linker, but not necessarily true for AOT - that is we can do better if we don't have to make that assumption. For now I rely on that assumption.

Author:	vitek-karas
Assignees:	vitek-karas
Labels:	`linkable-framework`
Milestone:	6.0.0

MichalStrehovsky · 2021-05-10T09:15:58Z

src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs

                    return list.ToArray();
                }

+                [UnconditionalSuppressMessage ("ReflectionAnalysis", "IL2075:UnrecognizedReflectionPattern",


I've submitted #52535 to see whether we can just delete the problematic code instead.

I think this would only do something if we allowed instance fields on interfaces, but that smells a lot like multiple-inheritance problems, so I don't think we're ever going to allow that.

Thanks a lot for testing this.

src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs

src/libraries/System.Private.Xml/src/System/Xml/Serialization/CodeGenerator.cs

eerhardt · 2021-05-10T15:53:30Z

src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs

+                    "The trimmer will keep the interface and thus all of its implementations in that case. " +
+                    "The call to GetInterfaces may return less results in trimmed apps, but it will " +
+                    "include the interfaces this method looks for if they should be there.")]
+            static Type[] GetInterfacesOnType (Type t)


Suggested change

static Type[] GetInterfacesOnType (Type t)

static Type[] GetInterfacesOnType(Type t)

I've been officially tainted by the linker repo coding style 😄

We need to make this part of the process after working in the linker repo:

eerhardt · 2021-05-10T17:24:07Z

src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureType.cs

+        [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]
        public sealed override Type[] FindInterfaces(TypeFilter filter, object? filterCriteria) => throw new NotSupportedException(SR.NotSupported_SignatureType);
+
        public sealed override InterfaceMapping GetInterfaceMap(Type interfaceType) => throw new NotSupportedException(SR.NotSupported_SignatureType);


What about GetInterfaceMap? Does this need any annotations?

I don't think we need to. The GetInterfaceMap returns the per-method details for the specified interface type - so the interface type is guaranteed to exist. Maybe we need to annotate it with PublicMethods? @MichalStrehovsky would you know?

Maybe we need to annotate it with PublicMethods

Does PublicMethods include explicitly implemented methods?

Or do you mean PublicMethods on the interface type?

Might have to be all the methods on the interface: #46912 (comment)

Yeah - we will need to make it all methods... although I will have to play with it - if it has different semantics than GetMethod regarding inheritance... oh well.

src/libraries/System.Reflection.TypeExtensions/ref/System.Reflection.TypeExtensions.cs

eerhardt · 2021-05-11T15:17:42Z

    public virtual System.Collections.Generic.IEnumerable<System.Type> ImplementedInterfaces { get { throw null; } }

What about TypeInfo.ImplementedInterfaces? Shouldn't that be annotated?

Looks like it was just missed in the ref file. The src file has it annotated.

Refers to: src/libraries/System.Runtime/ref/System.Runtime.cs:9079 in 702bed0. [](commit_id = 702bed0, deletion_comment = False)

eerhardt

LGTM. I had just one more comment on TypeInfo.ImplementedInterfaces ref file. Other than that, looks good.

The Interfaces annotation is transitive (by definition), so once it applies to a Type it also applies to all the interfaces implemented by that type.

vitek-karas · 2021-05-11T21:37:53Z

With the last change I annotated the return value of GetInterface with Interfaces as well.
The Interfaces annotation is transitive - it guarantees that on a type A all of the interfaces that type implements (as in, can be casted to) will be preserved. That means it's recursively marking all interfaces - and thus if I pick IA which is implemented by A, all of the interfaces on IA are already marked as well.

eerhardt

Annotate required APIs with DynamicallyAccessedMemberTypes.Interfaces

df9b104

After the basic annotation of Type.GetInterface and Type.GetInterfaces this propagates this annotation to all places which require it. It also resolves warnings in all the places where this shows up.

vitek-karas added the linkable-framework Issues associated with delivering a linker friendly framework label May 7, 2021

vitek-karas added this to the 6.0.0 milestone May 7, 2021

vitek-karas requested review from MichalStrehovsky, eerhardt and joperezr May 7, 2021 19:38

vitek-karas self-assigned this May 7, 2021

vitek-karas requested review from eiriktsarpalis, jozkee, layomia, marek-safar and steveharter as code owners May 7, 2021 19:38

ghost added area-System.Reflection new-api-needs-documentation labels May 7, 2021

Fix build

b0eda4c

MichalStrehovsky reviewed May 10, 2021

View reviewed changes

vitek-karas added 2 commits May 10, 2021 03:01

Merge remote-tracking branch 'origin/main' into GetInterfacesAnnotation

fece35b

Merge with main and fix warnings in OOB packages

3c62154

eerhardt reviewed May 10, 2021

View reviewed changes

src/libraries/System.Reflection.TypeExtensions/ref/System.Reflection.TypeExtensions.cs Show resolved Hide resolved

vitek-karas added 2 commits May 10, 2021 11:34

PR feedback

748a107

Fix ref assemblies

702bed0

eerhardt approved these changes May 11, 2021

View reviewed changes

eerhardt mentioned this pull request May 11, 2021

Type.GetInterfaceMap is not annotated for trimming #46912

Closed

vitek-karas added 2 commits May 11, 2021 11:59

Fix ref assemblies

379f848

Annotate return value of GetInterface with Interfaces.

1b7bf61

The Interfaces annotation is transitive (by definition), so once it applies to a Type it also applies to all the interfaces implemented by that type.

vitek-karas mentioned this pull request May 11, 2021

DynamicallyAccessedMembers should transitively apply to GetInterfaces() dotnet/linker#1731

Closed

Fix build issues on Linux

90c6fdf

eerhardt approved these changes May 12, 2021

View reviewed changes

vitek-karas merged commit e43bd13 into dotnet:main May 12, 2021

vitek-karas deleted the GetInterfacesAnnotation branch May 12, 2021 21:20

ghost locked as resolved and limited conversation to collaborators Jun 11, 2021

	static Type[] GetInterfacesOnType (Type t)
	static Type[] GetInterfacesOnType(Type t)

Annotate required APIs with DynamicallyAccessedMemberTypes.Interfaces #52461

Annotate required APIs with DynamicallyAccessedMemberTypes.Interfaces #52461

Uh oh!

Conversation

vitek-karas commented May 7, 2021

Uh oh!

ghost commented May 7, 2021

Uh oh!

ghost commented May 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eerhardt commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

vitek-karas commented May 11, 2021

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eerhardt commented May 11, 2021 •

edited

Loading