Skip to content

Don't warn for reflection access to compiler-generated code #104757

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.

Sign up for GitHub

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

Merged
sbomer merged 3 commits into from
Jul 16, 2024
Merged

Don't warn for reflection access to compiler-generated code #104757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
01fe5d8
Don't warn for reflection access to compiler-generated code
sbomer Jul 11, 2024
95e2d4d
Remove unnecessary suppressions
sbomer Jul 11, 2024
c6d61af
Merge remote-tracking branch 'origin/main' into removeWarningsForGene…
sbomer Jul 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions eng/testing/tests.wasm.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@
<PropertyGroup Condition="'$(EnableAggressiveTrimming)' == 'true'">
<!-- suppress warnings as these are tests, and not expected to be trim-safe -->
<SuppressTrimAnalysisWarnings>true</SuppressTrimAnalysisWarnings>
<!-- This warning code isn't yet included in SuppressTrimAnalysisWarnings -->
<NoWarn>$(NoWarn);IL2118</NoWarn>
<!-- IL2121: Unnecessary UnconditionalSuppressMessage attribute -->
<NoWarn>$(NoWarn);IL2121</NoWarn>
</PropertyGroup>
Expand Down
3 changes: 0 additions & 3 deletions ...System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,9 +648,6 @@ public FilterAndTransform(string filterAndPayloadSpec, int startIdx, int endIdx,

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "DiagnosticSource.Write is marked with RequiresUnreferencedCode.")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2119",
Justification = "DAM on EventSource references this compiler-generated local function which calls a " +
"method that requires unreferenced code. EventSource will not access this local function.")]
void OnEventWritten(KeyValuePair<string, object?> evnt)
{
// The filter given to the DiagnosticSource may not work if users don't is 'IsEnabled' as expected.
Expand Down
3 changes: 0 additions & 3 deletions src/libraries/System.Diagnostics.Tracing/tests/TrimmingTests/EventSourceManifestTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,6 @@ void EventSourceTest_Method_4(){}
int EventSourceTest_Method_7() => 5;
}

[UnconditionalSuppressMessage ("ReflectionAnalysis", "IL2118",
Justification = "DAM on EventSource.GenerateManifest references compiler-generated local function GetTrimSafeTraceLoggingEventTypes " +
"which calls a constructor that requires unreferenced code. EventSource will not access this local function.")]
public static int Main()
{
string manifest = EventSource.GenerateManifest(typeof(EventSourceTest), null);
Expand Down
3 changes: 0 additions & 3 deletions src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2160,9 +2160,6 @@ private unsafe void WriteEventString(string msgString)

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "The call to TraceLoggingEventTypes with the below parameter values are trim safe")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2119",
Justification = "DAM on EventSource references this compiler-generated local function which calls a " +
"constructor that requires unreferenced code. EventSource will not access this local function.")]
static TraceLoggingEventTypes GetTrimSafeTraceLoggingEventTypes() =>
new TraceLoggingEventTypes(EventName, EventTags.None, new Type[] { typeof(string) });

Expand Down
3 changes: 0 additions & 3 deletions src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Type/TypeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,6 @@ public static IEnumerable<object[]> FindMembers_TestData()

[Theory]
[MemberData(nameof(FindMembers_TestData))]
[UnconditionalSuppressMessage ("ReflectionAnalysis", "IL2118",
Justification = "DAM on FindMembers references compiler-generated members which use reflection. " +
"These members are not accessed by the test.")]
public void FindMembers_Invoke_ReturnsExpected(MemberTypes memberType, BindingFlags bindingAttr, MemberFilter filter, object filterCriteria, int expectedLength)
{
Assert.Equal(expectedLength, typeof(TypeTests).FindMembers(memberType, bindingAttr, filter, filterCriteria).Length);
Expand Down
6 changes: 3 additions & 3 deletions src/tools/illink/src/ILLink.Shared/DiagnosticId.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,9 @@ public enum DiagnosticId
DynamicallyAccessedMembersOnTypeReferencesMemberOnBaseWithDynamicallyAccessedMembers = 2115,
RequiresUnreferencedCodeOnStaticConstructor = 2116,
MethodsAreAssociatedWithUserMethod = 2117,
CompilerGeneratedMemberAccessedViaReflection = 2118,
DynamicallyAccessedMembersOnTypeReferencesCompilerGeneratedMember = 2119,
DynamicallyAccessedMembersOnTypeReferencesCompilerGeneratedMemberOnBase = 2120,
_unused_CompilerGeneratedMemberAccessedViaReflection = 2118,
_unused_DynamicallyAccessedMembersOnTypeReferencesCompilerGeneratedMember = 2119,
_unused_DynamicallyAccessedMembersOnTypeReferencesCompilerGeneratedMemberOnBase = 2120,
Comment on lines -185 to +187
Copy link
Member

@MichalStrehovsky MichalStrehovsky Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we also doc that people shouldn't expect this warning anymore? We could even go as far as saying this is safe to suppress since it's likely this generates more false positives than point out real issues.

Copy link
Member Author

@sbomer sbomer Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we didn't doc these warnings yet so I think we can just leave them undocumented.

RedundantSuppression = 2121,

// Single-file diagnostic ids.
Expand Down
64 changes: 2 additions & 62 deletions src/tools/illink/src/linker/Linker.Steps/MarkStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1620,20 +1620,6 @@ void ReportWarningsForReflectionAccess (in MessageOrigin origin, MethodDefinitio
break;
}
}

// Warn on reflection access to compiler-generated methods, if the method isn't already unsafe to access via reflection
// due to annotations. For the annotation-based warnings, we skip virtual overrides since those will produce warnings on
// the base, but for unannotated compiler-generated methods this is not the case, so we must produce these warnings even
// for virtual overrides. This ensures that we include the unannotated MoveNext state machine method. Lambdas and local
// functions should never be virtual overrides in the first place.
bool isCoveredByAnnotations = isReflectionAccessCoveredByRUC || isReflectionAccessCoveredByDAM;
switch (dependencyKind) {
case DependencyKind.AccessedViaReflection:
case DependencyKind.DynamicallyAccessedMember:
if (ShouldWarnForReflectionAccessToCompilerGeneratedCode (method, isCoveredByAnnotations))
Context.LogWarning (origin, DiagnosticId.CompilerGeneratedMemberAccessedViaReflection, method.GetDisplayName ());
break;
}
}

void ReportWarningsForTypeHierarchyReflectionAccess (IMemberDefinition member, MessageOrigin origin)
Expand Down Expand Up @@ -1676,23 +1662,6 @@ static bool IsDeclaredWithinType (IMemberDefinition member, TypeDefinition type)
var id = reportOnMember ? DiagnosticId.DynamicallyAccessedMembersOnTypeReferencesMemberWithDynamicallyAccessedMembers : DiagnosticId.DynamicallyAccessedMembersOnTypeReferencesMemberOnBaseWithDynamicallyAccessedMembers;
Context.LogWarning (origin, id, type.GetDisplayName (), ((MemberReference) member).GetDisplayName ());
}

// Warn on reflection access to compiler-generated methods, if the method isn't already unsafe to access via reflection
// due to annotations. For the annotation-based warnings, we skip virtual overrides since those will produce warnings on
// the base, but for unannotated compiler-generated methods this is not the case, so we must produce these warnings even
// for virtual overrides. This ensures that we include the unannotated MoveNext state machine method. Lambdas and local
// functions should never be virtual overrides in the first place.
bool isCoveredByAnnotations = isReflectionAccessCoveredByRUC || isReflectionAccessCoveredByDAM;
if (member is MethodDefinition method && ShouldWarnForReflectionAccessToCompilerGeneratedCode (method, isCoveredByAnnotations)) {
var id = reportOnMember ? DiagnosticId.DynamicallyAccessedMembersOnTypeReferencesCompilerGeneratedMember : DiagnosticId.DynamicallyAccessedMembersOnTypeReferencesCompilerGeneratedMemberOnBase;
Context.LogWarning (origin, id, type.GetDisplayName (), method.GetDisplayName ());
}

// Warn on reflection access to compiler-generated fields.
if (member is FieldDefinition field && ShouldWarnForReflectionAccessToCompilerGeneratedCode (field, isCoveredByAnnotations)) {
var id = reportOnMember ? DiagnosticId.DynamicallyAccessedMembersOnTypeReferencesCompilerGeneratedMember : DiagnosticId.DynamicallyAccessedMembersOnTypeReferencesCompilerGeneratedMemberOnBase;
Context.LogWarning (origin, id, type.GetDisplayName (), field.GetDisplayName ());
}
}

void MarkField (FieldDefinition field, in DependencyInfo reason, in MessageOrigin origin)
Expand Down Expand Up @@ -1756,24 +1725,6 @@ void MarkField (FieldDefinition field, in DependencyInfo reason, in MessageOrigi
}
}

bool ShouldWarnForReflectionAccessToCompilerGeneratedCode (FieldDefinition field, bool isCoveredByAnnotations)
{
// No need to warn if it's already covered by the Requires attribute or explicit annotations on the field.
if (isCoveredByAnnotations)
return false;

if (!CompilerGeneratedState.IsNestedFunctionOrStateMachineMember (field))
return false;

// Only warn for types which are interesting for dataflow. Note that this does
// not include integer types, even though we track integers in the dataflow analysis.
// Technically we should also warn for integer types, but this leads to more warnings
// for example about the compiler-generated "state" field for state machine methods.
// This should be ok because in most cases the state machine types will also have other
// hoisted locals that produce warnings anyway when accessed via reflection.
return Annotations.FlowAnnotations.IsTypeInterestingForDataflow (field.FieldType);
}

void ProcessAnalysisAnnotationsForField (FieldDefinition field, DependencyKind dependencyKind, in MessageOrigin origin)
{
switch (dependencyKind) {
Expand All @@ -1794,32 +1745,21 @@ void ProcessAnalysisAnnotationsForField (FieldDefinition field, DependencyKind d
if (Annotations.ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode (origin.Provider, out _))
return;

bool isReflectionAccessCoveredByRUC;
if (isReflectionAccessCoveredByRUC = Annotations.ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode (field, out RequiresUnreferencedCodeAttribute? requiresUnreferencedCodeAttribute))
if (Annotations.ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode (field, out RequiresUnreferencedCodeAttribute? requiresUnreferencedCodeAttribute))
ReportRequiresUnreferencedCode (field.GetDisplayName (), requiresUnreferencedCodeAttribute!, new DiagnosticContext (origin, diagnosticsEnabled: true, Context));

bool isReflectionAccessCoveredByDAM = false;
switch (dependencyKind) {
case DependencyKind.AccessedViaReflection:
case DependencyKind.DynamicDependency:
case DependencyKind.DynamicallyAccessedMember:
case DependencyKind.InteropMethodDependency:
case DependencyKind.Ldtoken:
case DependencyKind.UnsafeAccessorTarget:
if (isReflectionAccessCoveredByDAM = Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (field))
if (Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (field))
Context.LogWarning (origin, DiagnosticId.DynamicallyAccessedMembersFieldAccessedViaReflection, field.GetDisplayName ());

break;
}

switch (dependencyKind) {
case DependencyKind.AccessedViaReflection:
case DependencyKind.DynamicallyAccessedMember:
bool isCoveredByAnnotations = isReflectionAccessCoveredByRUC || isReflectionAccessCoveredByDAM;
if (ShouldWarnForReflectionAccessToCompilerGeneratedCode (field, isCoveredByAnnotations))
Context.LogWarning (origin, DiagnosticId.CompilerGeneratedMemberAccessedViaReflection, field.GetDisplayName ());
break;
}
}

/// <summary>
Expand Down
4 changes: 0 additions & 4 deletions ...ols/illink/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,8 +333,6 @@ static void Ldvirtftn ()
[ExpectedWarning ("IL2111", nameof (MethodWithSingleAnnotatedParameter))]
[ExpectedWarning ("IL2111", nameof (IWithAnnotatedMethod.AnnotatedMethod))]
[ExpectedWarning ("IL2111", nameof (IWithAnnotatedMethod.AnnotatedMethod))]
[UnexpectedWarning ("IL2118", nameof (LdftnOnLambdaTriggersLamdaAnalysis), Tool.Trimmer, "https://github.com/dotnet/runtime/issues/85042")]
[UnexpectedWarning ("IL2118", nameof (LdftnOnLocalMethodTriggersLocalMethodAnalysis), Tool.Trimmer, "https://github.com/dotnet/runtime/issues/85042")]
static void DynamicallyAccessedMembersAll1 ()
{
typeof (AnnotatedMethodParameters).RequiresAll ();
Expand All @@ -347,8 +345,6 @@ static void DynamicallyAccessedMembersAll1 ()
[ExpectedWarning ("IL2111", nameof (MethodWithSingleAnnotatedParameter))]
[ExpectedWarning ("IL2111", nameof (IWithAnnotatedMethod.AnnotatedMethod))]
[ExpectedWarning ("IL2111", nameof (IWithAnnotatedMethod.AnnotatedMethod))]
[UnexpectedWarning ("IL2118", nameof (LdftnOnLambdaTriggersLamdaAnalysis), Tool.Trimmer, "https://github.com/dotnet/runtime/issues/85042")]
[UnexpectedWarning ("IL2118", nameof (LdftnOnLocalMethodTriggersLocalMethodAnalysis), Tool.Trimmer, "https://github.com/dotnet/runtime/issues/85042")]
static void DynamicallyAccessedMembersAll2 ()
{
typeof (AnnotatedMethodParameters).RequiresAll ();
Expand Down
Loading