-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Annotate more of CoreLib for trimming #38265
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
// See the LICENSE file in the project root for more information. | ||
|
||
using System.Diagnostics; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Diagnostics.Tracing; | ||
using System.Reflection; | ||
using System.Threading; | ||
|
@@ -92,6 +93,8 @@ internal static bool TrackAsyncMethodCompletion | |
/// <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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we close #32921 with this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup |
||
Justification = "It's okay if unused fields disappear from debug views")] | ||
internal static string GetAsyncStateMachineDescription(IAsyncStateMachine stateMachine) | ||
{ | ||
Debug.Assert(stateMachine != null); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,6 +72,7 @@ public override IList<CustomAttributeData> GetCustomAttributesData() | |
|
||
// GetDefaultMembers | ||
// This will return a MemberInfo that has been marked with the [DefaultMemberAttribute] | ||
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why All? The doc says it can be only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the attribute have any effect here when the type is sealed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The attribute is on the "this" - which is a special case - it annotates the type this RuntimeType represents. So for example calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure but as this is C# code there won't be any call (IL reference) to this method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that the same annotation is on Other than the above rule, this also means that when analyzing the method body of this method the linker can make assumptions about the |
||
public override MemberInfo[] GetDefaultMembers() | ||
{ | ||
// See if we have cached the default member name | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -339,6 +339,7 @@ public ConstructorInfo? TypeInitializer | |
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties)] | ||
public abstract PropertyInfo[] GetProperties(BindingFlags bindingAttr); | ||
|
||
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why All (that includes NestedTypes as well) ? The doc says it can be only |
||
public virtual MemberInfo[] GetDefaultMembers() => throw NotImplemented.ByDesign; | ||
|
||
public virtual RuntimeTypeHandle TypeHandle => throw new NotSupportedException(); | ||
|
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.
Why are
IL2006
andIL2026
bothUnrecognizedReflectionPattern
? Shouldn't different warning numbers have different titles/names?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.
@mateoatr to comment on the second parameter values...
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.
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.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.
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.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.
Renamed to RequiresUnreferencedCode.