Skip to content

Commit

Permalink
Make ReflectionAccessAnalyzer logic more similar to ILLink/ILC (#105956)
Browse files Browse the repository at this point in the history 
ReflectionAccessAnalyzer is analogous to ILLink/ILC's
ReflectionMarker, and will be responsible for producing warnings
about DynamicallyAccessedMembers on types when we fix
#102002. These warnings
will not all have the same location since they can be reported on
different members of the type.

To support this, this is changing ReflectionAccessAnalyzer to
create a DiagnosticContext as needed when reporting a warning,
instead of taking it as an argument, which would require all
warnings to share the same location (since DiagnosticContext
holds a Location). This is similar to how ReflectionMarker takes
a MessageOrigin instead of DiagnosticContext.
  • Loading branch information
@sbomer
sbomer authored Aug 6, 2024
1 parent fdb7415 commit d5af5bd
Show file tree
Hide file tree
Showing 15 changed files with 134 additions and 154 deletions.
11 changes: 3 additions & 8 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,7 @@ public override void Initialize (AnalysisContext context)
foreach (var operationBlock in context.OperationBlocks) {
TrimDataFlowAnalysis trimDataFlowAnalysis = new (context, dataFlowAnalyzerContext, operationBlock);
trimDataFlowAnalysis.InterproceduralAnalyze ();
foreach (var diagnostic in trimDataFlowAnalysis.CollectDiagnostics ())
context.ReportDiagnostic (diagnostic);
trimDataFlowAnalysis.ReportDiagnostics (context.ReportDiagnostic);
}
});

Expand All @@ -117,16 +116,12 @@ public override void Initialize (AnalysisContext context)
return;

var location = GetPrimaryLocation (type.Locations);
DiagnosticContext diagnosticContext = new (location);

if (type.BaseType is INamedTypeSymbol baseType)
GenericArgumentDataFlow.ProcessGenericArgumentDataFlow (diagnosticContext, baseType);
GenericArgumentDataFlow.ProcessGenericArgumentDataFlow (location, baseType, context.ReportDiagnostic);

foreach (var interfaceType in type.Interfaces)
GenericArgumentDataFlow.ProcessGenericArgumentDataFlow (diagnosticContext, interfaceType);

foreach (var diagnostic in diagnosticContext.Diagnostics)
context.ReportDiagnostic (diagnostic);
GenericArgumentDataFlow.ProcessGenericArgumentDataFlow (location, interfaceType, context.ReportDiagnostic);
}, SymbolKind.NamedType);
context.RegisterSymbolAction (context => {
VerifyMemberOnlyApplyToTypesOrStrings (context, context.Symbol);
Expand Down
19 changes: 9 additions & 10 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/DiagnosticContext.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,36 +12,35 @@ namespace ILLink.Shared.TrimAnalysis
{
public readonly partial struct DiagnosticContext
{
public List<Diagnostic> Diagnostics { get; } = new ();
public readonly Location Location { get; }

readonly Location? Location { get; init; }
readonly Action<Diagnostic>? _reportDiagnostic;

public DiagnosticContext (Location location)
public DiagnosticContext (Location location, Action<Diagnostic>? reportDiagnostic)
{
Location = location;
_reportDiagnostic = reportDiagnostic;
}

public static DiagnosticContext CreateDisabled () => new () { Location = null };

public Diagnostic CreateDiagnostic (DiagnosticId id, params string[] args)
private Diagnostic CreateDiagnostic (DiagnosticId id, params string[] args)
{
return Diagnostic.Create (DiagnosticDescriptors.GetDiagnosticDescriptor (id), Location, args);
}

public partial void AddDiagnostic (DiagnosticId id, params string[] args)
{
if (Location == null)
if (_reportDiagnostic is null)
return;

Diagnostics.Add (CreateDiagnostic (id, args));
_reportDiagnostic (CreateDiagnostic (id, args));
}

public partial void AddDiagnostic (DiagnosticId id, ValueWithDynamicallyAccessedMembers actualValue, ValueWithDynamicallyAccessedMembers expectedAnnotationsValue, params string[] args)
{
if (Location == null)
if (_reportDiagnostic is null)
return;

Diagnostics.Add (CreateDiagnostic (id, actualValue, expectedAnnotationsValue, args));
_reportDiagnostic (CreateDiagnostic (id, actualValue, expectedAnnotationsValue, args));
}

private Diagnostic CreateDiagnostic (DiagnosticId id, ValueWithDynamicallyAccessedMembers actualValue, ValueWithDynamicallyAccessedMembers expectedAnnotationsValue, params string[] args)
Expand Down
13 changes: 6 additions & 7 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FeatureCheckReturnValuePattern.cs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// 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.Collections.Generic;
using ILLink.Shared;
using ILLink.Shared.DataFlow;
Expand Down Expand Up @@ -29,22 +30,22 @@ public FeatureCheckReturnValuePattern (
OwningSymbol = owningSymbol;
}

public IEnumerable<Diagnostic> CollectDiagnostics (DataFlowAnalyzerContext context)
public void ReportDiagnostics (DataFlowAnalyzerContext context, Action<Diagnostic> reportDiagnostic)
{
var diagnosticContext = new DiagnosticContext (Operation.Syntax.GetLocation ());
var diagnosticContext = new DiagnosticContext (Operation.Syntax.GetLocation (), reportDiagnostic);
// For now, feature check validation is enabled only when trim analysis is enabled.
if (!context.EnableTrimAnalyzer)
return diagnosticContext.Diagnostics;
return;

if (!OwningSymbol.IsStatic || OwningSymbol.Type.SpecialType != SpecialType.System_Boolean || OwningSymbol.SetMethod != null) {
// Warn about invalid feature checks (non-static or non-bool properties or properties with setter)
diagnosticContext.AddDiagnostic (
DiagnosticId.InvalidFeatureGuard);
return diagnosticContext.Diagnostics;
return;
}

if (ReturnValue == FeatureChecksValue.All)
return diagnosticContext.Diagnostics;
return;

ValueSet<string> returnValueFeatures = ReturnValue.EnabledFeatures;
// For any analyzer-supported feature that this property is declared to guard,
Expand All @@ -63,8 +64,6 @@ public IEnumerable<Diagnostic> CollectDiagnostics (DataFlowAnalyzerContext conte
}
}
}

return diagnosticContext.Diagnostics;
}
}
}
31 changes: 15 additions & 16 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/GenericArgumentDataFlow.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,55 +2,54 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using Microsoft.CodeAnalysis;
using ILLink.Shared.DataFlow;
using ILLink.Shared.TrimAnalysis;
using ILLink.Shared.TypeSystemProxy;

namespace ILLink.RoslynAnalyzer.TrimAnalysis
{
internal static class GenericArgumentDataFlow
{
public static void ProcessGenericArgumentDataFlow (DiagnosticContext diagnosticContext, INamedTypeSymbol type)
public static void ProcessGenericArgumentDataFlow (Location location, INamedTypeSymbol type, Action<Diagnostic> reportDiagnostic)
{
ProcessGenericArgumentDataFlow (diagnosticContext, type.TypeArguments, type.TypeParameters);
ProcessGenericArgumentDataFlow (location, type.TypeArguments, type.TypeParameters, reportDiagnostic);
}

public static void ProcessGenericArgumentDataFlow (DiagnosticContext diagnosticContext, IMethodSymbol method)
public static void ProcessGenericArgumentDataFlow (Location location, IMethodSymbol method, Action<Diagnostic> reportDiagnostic)
{
ProcessGenericArgumentDataFlow (diagnosticContext, method.TypeArguments, method.TypeParameters);
ProcessGenericArgumentDataFlow (location, method.TypeArguments, method.TypeParameters, reportDiagnostic);

ProcessGenericArgumentDataFlow (diagnosticContext, method.ContainingType);
ProcessGenericArgumentDataFlow (location, method.ContainingType, reportDiagnostic);
}

public static void ProcessGenericArgumentDataFlow (DiagnosticContext diagnosticContext, IFieldSymbol field)
public static void ProcessGenericArgumentDataFlow (Location location, IFieldSymbol field, Action<Diagnostic> reportDiagnostic)
{
ProcessGenericArgumentDataFlow (diagnosticContext, field.ContainingType);
ProcessGenericArgumentDataFlow (location, field.ContainingType, reportDiagnostic);
}

static void ProcessGenericArgumentDataFlow (
DiagnosticContext diagnosticContext,
Location location,
ImmutableArray<ITypeSymbol> typeArguments,
ImmutableArray<ITypeParameterSymbol> typeParameters)
ImmutableArray<ITypeParameterSymbol> typeParameters,
Action<Diagnostic> reportDiagnostic)
{
var diagnosticContext = new DiagnosticContext (location, reportDiagnostic);
for (int i = 0; i < typeArguments.Length; i++) {
var typeArgument = typeArguments[i];
// Apply annotations to the generic argument
var genericParameterValue = new GenericParameterValue (typeParameters[i]);
if (genericParameterValue.DynamicallyAccessedMemberTypes != DynamicallyAccessedMemberTypes.None) {
SingleValue genericArgumentValue = SingleValueExtensions.FromTypeSymbol (typeArgument)!;
var requireDynamicallyAccessedMembersAction = new RequireDynamicallyAccessedMembersAction (diagnosticContext, default (ReflectionAccessAnalyzer));
var reflectionAccessAnalyzer = new ReflectionAccessAnalyzer (reportDiagnostic);
var requireDynamicallyAccessedMembersAction = new RequireDynamicallyAccessedMembersAction (diagnosticContext, reflectionAccessAnalyzer);
requireDynamicallyAccessedMembersAction.Invoke (genericArgumentValue, genericParameterValue);
}

// Recursively process generic argument data flow on the generic argument if it itself is generic
if (typeArgument is INamedTypeSymbol namedTypeArgument && namedTypeArgument.IsGenericType) {
ProcessGenericArgumentDataFlow (diagnosticContext, namedTypeArgument);
}
if (typeArgument is INamedTypeSymbol namedTypeArgument && namedTypeArgument.IsGenericType)
ProcessGenericArgumentDataFlow (location, namedTypeArgument, reportDiagnostic);
}
}

Expand Down
28 changes: 15 additions & 13 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// 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.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
Expand All @@ -27,18 +28,19 @@ internal partial struct HandleCallAction
ValueSetLattice<SingleValue> _multiValueLattice;

public HandleCallAction (
in DiagnosticContext diagnosticContext,
Location location,
ISymbol owningSymbol,
IOperation operation,
ValueSetLattice<SingleValue> multiValueLattice)
ValueSetLattice<SingleValue> multiValueLattice,
Action<Diagnostic>? reportDiagnostic)
{
_owningSymbol = owningSymbol;
_operation = operation;
_isNewObj = operation.Kind == OperationKind.ObjectCreation;
_diagnosticContext = diagnosticContext;
_diagnosticContext = new DiagnosticContext (location, reportDiagnostic);
_annotations = FlowAnnotations.Instance;
_reflectionAccessAnalyzer = default;
_requireDynamicallyAccessedMembersAction = new (diagnosticContext, _reflectionAccessAnalyzer);
_reflectionAccessAnalyzer = new (reportDiagnostic);
_requireDynamicallyAccessedMembersAction = new (_diagnosticContext, _reflectionAccessAnalyzer);
_multiValueLattice = multiValueLattice;
}

Expand Down Expand Up @@ -201,25 +203,25 @@ private partial bool TryResolveTypeNameForCreateInstanceAndMark (in MethodProxy
}

private partial void MarkStaticConstructor (TypeProxy type)
=> _reflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForConstructorsOnType (_diagnosticContext, type.Type, BindingFlags.Static, parameterCount: 0);
=> _reflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForConstructorsOnType (_diagnosticContext.Location, type.Type, BindingFlags.Static, parameterCount: 0);

private partial void MarkEventsOnTypeHierarchy (TypeProxy type, string name, BindingFlags? bindingFlags)
=> _reflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForEventsOnTypeHierarchy (_diagnosticContext, type.Type, name, bindingFlags);
=> _reflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForEventsOnTypeHierarchy (_diagnosticContext.Location, type.Type, name, bindingFlags);

private partial void MarkFieldsOnTypeHierarchy (TypeProxy type, string name, BindingFlags? bindingFlags)
=> _reflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForFieldsOnTypeHierarchy (_diagnosticContext, type.Type, name, bindingFlags);
=> _reflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForFieldsOnTypeHierarchy (_diagnosticContext.Location, type.Type, name, bindingFlags);

private partial void MarkPropertiesOnTypeHierarchy (TypeProxy type, string name, BindingFlags? bindingFlags)
=> _reflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForPropertiesOnTypeHierarchy (_diagnosticContext, type.Type, name, bindingFlags);
=> _reflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForPropertiesOnTypeHierarchy (_diagnosticContext.Location, type.Type, name, bindingFlags);

private partial void MarkPublicParameterlessConstructorOnType (TypeProxy type)
=> _reflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForPublicParameterlessConstructor (_diagnosticContext, type.Type);
=> _reflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForPublicParameterlessConstructor (_diagnosticContext.Location, type.Type);

private partial void MarkConstructorsOnType (TypeProxy type, BindingFlags? bindingFlags, int? parameterCount)
=> _reflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForConstructorsOnType (_diagnosticContext, type.Type, bindingFlags, parameterCount);
=> _reflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForConstructorsOnType (_diagnosticContext.Location, type.Type, bindingFlags, parameterCount);

private partial void MarkMethod (MethodProxy method)
=> ReflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForMethod (_diagnosticContext, method.Method);
=> _reflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForMethod (_diagnosticContext.Location, method.Method);

// TODO: Does the analyzer need to do something here?
private partial void MarkType (TypeProxy type) { }
Expand All @@ -229,7 +231,7 @@ private partial bool MarkAssociatedProperty (MethodProxy method)
if (method.Method.MethodKind == MethodKind.PropertyGet || method.Method.MethodKind == MethodKind.PropertySet) {
var property = (IPropertySymbol) method.Method.AssociatedSymbol!;
Debug.Assert (property != null);
ReflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForProperty (_diagnosticContext, property!);
_reflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForProperty (_diagnosticContext.Location, property!);
return true;
}

Expand Down
Loading

0 comments on commit d5af5bd

Please sign in to comment.