Annotate more of CoreLib for trimming #38265

MichalStrehovsky · 2020-06-23T09:41:52Z

Yet another batch of trimming annotations.

Resolves #32921.

Yet another batch of trimming annotations.

marek-safar · 2020-06-23T12:49:45Z

src/libraries/System.Private.CoreLib/src/System/RuntimeType.cs


        // GetDefaultMembers
        // This will return a MemberInfo that has been marked with the [DefaultMemberAttribute]
+        [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]


Why All? The doc says it can be only constructor, method, property, or field and I think they have to be public

The doc says it can be only constructor, method, property, or field and I think they have to be public

Couldn't find that mention in the doc - the impl also works for events and nested types. It's a janky API.

It was forced by the annotation on GetMember. I've fixed that up.

marek-safar · 2020-06-23T12:52:05Z

src/libraries/System.Private.CoreLib/src/System/RuntimeType.cs


        // GetDefaultMembers
        // This will return a MemberInfo that has been marked with the [DefaultMemberAttribute]
+        [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]


Does the attribute have any effect here when the type is sealed?

The attribute is on the "this" - which is a special case - it annotates the type this RuntimeType represents. So for example calling typeof<MyType>.GetDefaultMembers() would end up (due to this attribute) marking the entire MyType.

Sure but as this is C# code there won't be any call (IL reference) to this method

Note that the same annotation is on Type.GetDefaultMembers() - the current data flow implementation in the linker assumes that the entire inheritance hierarchy for a given method has the same annotations (We're missing the validation in the linker for now dotnet/linker#1028).

Other than the above rule, this also means that when analyzing the method body of this method the linker can make assumptions about the this and won't generate warnings...

marek-safar · 2020-06-23T12:53:02Z

src/libraries/System.Private.CoreLib/src/System/Type.cs

        [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties)]
        public abstract PropertyInfo[] GetProperties(BindingFlags bindingAttr);

+        [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]


Why All (that includes NestedTypes as well) ? The doc says it can be only constructor, method, property, or field and I think they have to be public

vitek-karas

Other than the comment about All it looks good.

eerhardt · 2020-06-23T17:28:14Z

...braries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncMethodBuilderCore.cs

        /// <summary>Gets a description of the state of the state machine object, suitable for debug purposes.</summary>
        /// <param name="stateMachine">The state machine object.</param>
        /// <returns>A description of the state machine.</returns>
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2006:UnrecognizedReflectionPattern",


Can we close #32921 with this?

eerhardt · 2020-06-23T17:33:01Z

src/libraries/System.Private.CoreLib/src/System/Activator.RuntimeType.cs


        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2006:UnrecognizedReflectionPattern",
            Justification = "Implementation detail of Activator that linker intrinsically recognizes")]
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",


Why are IL2006 and IL2026 both UnrecognizedReflectionPattern ? Shouldn't different warning numbers have different titles/names?

@mateoatr to comment on the second parameter values...

Anything that is put after the colon in the CheckId parameter is used only for documentation purposes (this is, the warning suppression mechanism completely ignores what the user puts after the colon, it only cares for the four numbers of the warning id).
We sometimes put a subcategory on the warnings to further categorize them, although this specific warning does not have the UnrecognizedReflectionPattern subcategory, I don't know if we should add it.

It is super confusing as a reader to see 2 warning numbers with the same string. What is the title of IL2026? RequiresUnreferencedCode? We should update it here so this doesn't look like duplicated attributes.

Renamed to RequiresUnreferencedCode.

eerhardt

Just a couple clarifying questions. LGTM

marek-safar · 2020-06-24T08:44:25Z

src/libraries/System.Private.CoreLib/src/System/Type.cs

+            | DynamicallyAccessedMemberTypes.PublicFields
+            | DynamicallyAccessedMemberTypes.PublicNestedTypes)]
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2006:UnrecognizedReflectionPattern",
+            Justification = "Linker doesn't recongnize GetMember(BindingFlags.Public) but this is what the body is doing")]


nit: typo in recongnize

vitek-karas · 2020-06-24T09:21:46Z

src/libraries/System.Private.CoreLib/src/System/RuntimeType.cs

+            | DynamicallyAccessedMemberTypes.PublicEvents
+            | DynamicallyAccessedMemberTypes.PublicProperties
+            | DynamicallyAccessedMemberTypes.PublicConstructors
+            | DynamicallyAccessedMemberTypes.PublicFields


PublicFields is there twice

vitek-karas · 2020-06-24T09:22:08Z

src/libraries/System.Private.CoreLib/src/System/Type.cs

+            | DynamicallyAccessedMemberTypes.PublicEvents
+            | DynamicallyAccessedMemberTypes.PublicProperties
+            | DynamicallyAccessedMemberTypes.PublicConstructors
+            | DynamicallyAccessedMemberTypes.PublicFields


Same as above - duplicate PublicFields

And also below some more...

Sigh. Fixed.

I hope the one user of this useless API is going to appreciate the size savings from the restriction on .All because we just spent quite a bit of Microsoft money on this.

Maybe this is an indicator we are missing DynamicallyAccessedMemberTypes.AllPublic enum value

Maybe this is an indicator we are missing DynamicallyAccessedMemberTypes.AllPublic enum value

We can consider that - would that annotation be useful outside these two rarely used APIs?

MichalStrehovsky added 2 commits June 23, 2020 11:34

Annotate more of CoreLib for trimming

38c36b3

Yet another batch of trimming annotations.

Update ref

16cf511

MichalStrehovsky requested review from eerhardt and vitek-karas June 23, 2020 09:41

MichalStrehovsky requested review from EgorBo, akoeplinger, marek-safar, steveisok and vargaz as code owners June 23, 2020 09:41

Dotnet-GitSync-Bot added the area-crossgen2-coreclr label Jun 23, 2020

marek-safar reviewed Jun 23, 2020

View reviewed changes

vitek-karas approved these changes Jun 23, 2020

View reviewed changes

jaredpar mentioned this pull request Jun 23, 2020

PayloadGroup0 is timing out #38284

Closed

eerhardt reviewed Jun 23, 2020

View reviewed changes

eerhardt approved these changes Jun 23, 2020

View reviewed changes

MichalStrehovsky added 2 commits June 24, 2020 10:34

Review feedback

43a4679

Review feedback

ef26ac8

marek-safar reviewed Jun 24, 2020

View reviewed changes

marek-safar approved these changes Jun 24, 2020

View reviewed changes

vitek-karas approved these changes Jun 24, 2020

View reviewed changes

F

43c79cb

MichalStrehovsky merged commit 188a3a8 into dotnet:master Jun 24, 2020

MichalStrehovsky deleted the annotmore branch June 24, 2020 11:13

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Annotate more of CoreLib for trimming #38265

Annotate more of CoreLib for trimming #38265

Uh oh!

Conversation

MichalStrehovsky commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

vitek-karas left a comment

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!

eerhardt left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MichalStrehovsky commented Jun 23, 2020 •

edited

Loading