Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 5.6k
ILLink analyzer: add warnings for DAM on type#105994
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
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
ffeb629
Don't store diagnostics in DiagnosticContext
sbomer d42e9b7
Avoid DiagnosticContext in ReflectionAccessAnalyzer
sbomer 27465ea
Add analyzer warnings for DAM on type
sbomer 26aaa17
Turn on more tests for analyzer
sbomer 373168e
Merge remote-tracking branch 'origin/main' into damOnType
sbomer 5c8e0f0
PR feedback
sbomer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
2 changes: 1 addition & 1 deletion
2 src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs
This file contains hidden or 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
3 changes: 0 additions & 3 deletions
3 src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs
This file contains hidden or 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
8 changes: 7 additions & 1 deletion
8 src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs
This file contains hidden or 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
54 changes: 54 additions & 0 deletions
54 src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersTypeHierarchy.cs
This file contains hidden or 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,54 @@ | ||
| // 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.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis; | ||
| using ILLink.Shared; | ||
| using ILLink.Shared.TrimAnalysis; | ||
| using ILLink.RoslynAnalyzer.TrimAnalysis; | ||
| namespace ILLink.RoslynAnalyzer | ||
| { | ||
| sealed class DynamicallyAccessedMembersTypeHierarchy | ||
| { | ||
| public static void ApplyDynamicallyAccessedMembersToTypeHierarchy (Location typeLocation, INamedTypeSymbol type, Action<Diagnostic> reportDiagnostic) | ||
| { | ||
| var annotation = FlowAnnotations.GetTypeAnnotation (type); | ||
| // We need to apply annotations to this type, and its base/interface types (recursively) | ||
| // But the annotations on base/interfaces may already be applied so we don't need to apply those | ||
| // again (and should avoid doing so as it would produce extra warnings). | ||
| var reflectionAccessAnalyzer = new ReflectionAccessAnalyzer (reportDiagnostic, type); | ||
| if (type.BaseType is INamedTypeSymbol baseType) { | ||
| var baseAnnotation = FlowAnnotations.GetTypeAnnotation (baseType); | ||
| var annotationToApplyToBase = Annotations.GetMissingMemberTypes (annotation, baseAnnotation); | ||
| // Apply any annotations that didn't exist on the base type to the base type. | ||
| // This may produce redundant warnings when the annotation is DAMT.All or DAMT.PublicConstructors and the base already has a | ||
| // subset of those annotations. | ||
| reflectionAccessAnalyzer.GetReflectionAccessDiagnostics (typeLocation, baseType, annotationToApplyToBase, declaredOnly: false); | ||
| } | ||
| // Most of the DynamicallyAccessedMemberTypes don't select members on interfaces. We only need to apply | ||
| // annotations to interfaces separately if dealing with DAMT.All or DAMT.Interfaces. | ||
| if (annotation.HasFlag (DynamicallyAccessedMemberTypes.Interfaces)) | ||
| { | ||
| var annotationToApplyToInterfaces = annotation == DynamicallyAccessedMemberTypes.All ? annotation : DynamicallyAccessedMemberTypes.Interfaces; | ||
| foreach (var iface in type.AllInterfaces) { | ||
| if (FlowAnnotations.GetTypeAnnotation (iface).HasFlag (annotationToApplyToInterfaces)) | ||
| continue; | ||
| // Apply All or Interfaces to the interface type. | ||
| // DAMT.All may produce redundant warnings from implementing types, when the interface type already had some annotations. | ||
| reflectionAccessAnalyzer.GetReflectionAccessDiagnostics (typeLocation, iface, annotationToApplyToInterfaces, declaredOnly: false); | ||
| } | ||
| } | ||
| // The annotations this type inherited from its base types or interfaces should not produce | ||
| // warnings on the respective base/interface members, since those are already covered by applying | ||
| // the annotations to those types. So we only need to handle the members directly declared on this type. | ||
| reflectionAccessAnalyzer.GetReflectionAccessDiagnostics (typeLocation, type, annotation, declaredOnly: true); | ||
| } | ||
| } | ||
| } | ||
91 changes: 91 additions & 0 deletions
91 src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FlowAnnotations.cs
This file contains hidden or 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 |
|---|---|---|
| @@ -35,6 +35,97 @@ public static bool RequiresDataFlowAnalysis (IMethodSymbol method) | ||
| return false; | ||
| } | ||
| internal static bool ShouldWarnWhenAccessedForReflection (ISymbol symbol) => | ||
| symbol switch { | ||
| IMethodSymbol method => ShouldWarnWhenAccessedForReflection (method), | ||
| IFieldSymbol field => ShouldWarnWhenAccessedForReflection (field), | ||
| _ => false | ||
| }; | ||
| static bool ShouldWarnWhenAccessedForReflection (IMethodSymbol method) | ||
| { | ||
| bool? hasParameterAnnotation = null; | ||
| if (GetMethodReturnValueAnnotation (method) == DynamicallyAccessedMemberTypes.None) { | ||
| if (!HasParameterAnnotation (method)) | ||
| return false; | ||
| hasParameterAnnotation = true; | ||
| } | ||
| // If the method only has annotation on the return value and it's not virtual avoid warning. | ||
| // Return value annotations are "consumed" by the caller of a method, and as such there is nothing | ||
| // wrong calling these dynamically. The only problem can happen if something overrides a virtual | ||
| // method with annotated return value at runtime - in this case the trimmer can't validate | ||
| // that the method will return only types which fulfill the annotation's requirements. | ||
| // For example: | ||
| // class BaseWithAnnotation | ||
| // { | ||
| // [return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields)] | ||
| // public abstract Type GetTypeWithFields(); | ||
| // } | ||
| // | ||
| // class UsingTheBase | ||
| // { | ||
| // public void PrintFields(Base base) | ||
| // { | ||
| // // No warning here - GetTypeWithFields is correctly annotated to allow GetFields on the return value. | ||
| // Console.WriteLine(string.Join(" ", base.GetTypeWithFields().GetFields().Select(f => f.Name))); | ||
| // } | ||
| // } | ||
| // | ||
| // If at runtime (through ref emit) something generates code like this: | ||
| // class DerivedAtRuntimeFromBase | ||
| // { | ||
| // // No point in adding annotation on the return value - nothing will look at it anyway | ||
| // // Trimming will not see this code, so there are no checks | ||
| // public override Type GetTypeWithFields() { return typeof(TestType); } | ||
| // } | ||
jtschuster marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| // | ||
| // If TestType from above is trimmed, it may not have all its fields, and there would be no warnings generated. | ||
| // But there has to be code like this somewhere in the app, in order to generate the override: | ||
| // class RuntimeTypeGenerator | ||
| // { | ||
| // public MethodInfo GetBaseMethod() | ||
| // { | ||
| // // This must warn - that the GetTypeWithFields has annotation on the return value | ||
| // return typeof(BaseWithAnnotation).GetMethod("GetTypeWithFields"); | ||
| // } | ||
| // } | ||
| return method.IsVirtual || method.IsOverride || (hasParameterAnnotation ?? HasParameterAnnotation (method)); | ||
| static bool HasParameterAnnotation (IMethodSymbol method) { | ||
| foreach (var param in method.GetParameters ()) { | ||
| if (GetMethodParameterAnnotation (param) != DynamicallyAccessedMemberTypes.None) | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
| } | ||
| static bool ShouldWarnWhenAccessedForReflection (IFieldSymbol field) | ||
| { | ||
| return field.GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None; | ||
| } | ||
| internal static DynamicallyAccessedMemberTypes GetTypeAnnotations (INamedTypeSymbol type) | ||
| { | ||
| DynamicallyAccessedMemberTypes typeAnnotation = type.GetDynamicallyAccessedMemberTypes (); | ||
| // Also inherit annotation from bases | ||
| INamedTypeSymbol? baseType = type.BaseType; | ||
| while (baseType is not null) { | ||
| typeAnnotation |= baseType.GetDynamicallyAccessedMemberTypes (); | ||
| baseType = baseType.BaseType; | ||
| } | ||
| // And inherit them from interfaces | ||
| foreach (INamedTypeSymbol interfaceType in type.AllInterfaces) { | ||
| typeAnnotation |= interfaceType.GetDynamicallyAccessedMemberTypes (); | ||
| } | ||
| return typeAnnotation; | ||
| } | ||
| internal static DynamicallyAccessedMemberTypes GetMethodParameterAnnotation (ParameterProxy param) | ||
| { | ||
| IMethodSymbol method = param.Method.Method; | ||
2 changes: 1 addition & 1 deletion
2 src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/GenericArgumentDataFlow.cs
This file contains hidden or 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
2 changes: 1 addition & 1 deletion
2 src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs
This file contains hidden or 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
62 changes: 60 additions & 2 deletions
62 src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/ReflectionAccessAnalyzer.cs
This file contains hidden or 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
2 changes: 1 addition & 1 deletion
2 src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisAssignmentPattern.cs
This file contains hidden or 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
2 changes: 1 addition & 1 deletion
2 ...ools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisReflectionAccessPattern.cs
This file contains hidden or 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 hidden or 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
18 changes: 14 additions & 4 deletions
18 src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/ReflectionTests.cs
This file contains hidden or 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
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.