Skip to content

Share intrinsic handling of GetMember and similar APIs #2639

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
merged 3 commits into from
Feb 28, 2022
Merged

Share intrinsic handling of GetMember and similar APIs #2639

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
b6a033b
Share intrinsic handling of GetMember and similar APIs
vitek-karas Feb 21, 2022
4380ba4
PR feedback - detect constants for all operations
vitek-karas Feb 24, 2022
1e989bf
Formatting
vitek-karas Feb 28, 2022
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
37 changes: 36 additions & 1 deletion src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using ILLink.RoslynAnalyzer;
using ILLink.RoslynAnalyzer.DataFlow;
using ILLink.RoslynAnalyzer.TrimAnalysis;
using ILLink.Shared.TypeSystemProxy;
using Microsoft.CodeAnalysis;
Expand All @@ -16,13 +19,15 @@ partial struct HandleCallAction

readonly ISymbol _owningSymbol;
readonly IOperation _operation;
readonly ReflectionAccessAnalyzer _reflectionAccessAnalyzer;

public HandleCallAction (in DiagnosticContext diagnosticContext, ISymbol owningSymbol, IOperation operation)
{
_owningSymbol = owningSymbol;
_operation = operation;
_diagnosticContext = diagnosticContext;
_requireDynamicallyAccessedMembersAction = new (diagnosticContext, new ReflectionAccessAnalyzer ());
_reflectionAccessAnalyzer = new ReflectionAccessAnalyzer ();
_requireDynamicallyAccessedMembersAction = new (diagnosticContext, _reflectionAccessAnalyzer);
}

// TODO: This is relatively expensive on the analyzer since it doesn't cache the annotation information
Expand All @@ -41,9 +46,39 @@ private partial GenericParameterValue GetGenericParameterValue (GenericParameter
private partial MethodThisParameterValue GetMethodThisParameterValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
=> new (method.Method, dynamicallyAccessedMemberTypes);

private partial MethodParameterValue GetMethodParameterValue (MethodProxy method, int parameterIndex, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
=> new (method.Method.Parameters[parameterIndex], dynamicallyAccessedMemberTypes);

private partial IEnumerable<SystemReflectionMethodBaseValue> GetMethodsOnTypeHierarchy (TypeProxy type, string name, BindingFlags? bindingFlags)
{
foreach (var method in type.Type.GetMethodsOnTypeHierarchy (m => m.Name == name, bindingFlags))
yield return new SystemReflectionMethodBaseValue (new MethodProxy (method));
Copy link
Member

@agocke agocke Feb 23, 2022

Choose a reason for hiding this comment

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

I think this is going to cause a quadratic number of calls in the depth of the tree. Since iterators are lazy, we'll walk all the way down the tree to fetch the first element, then walk n-1 levels to fetch the second, and so on.

Copy link
Member Author

@vitek-karas vitek-karas Feb 23, 2022

Choose a reason for hiding this comment

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

Sorry I don't understand. The GetMethodsOnTypeHierarchy helper we're calling here already uses iterator, so this is basically just a pass through...

I guess I could rewrite both to use ImmutableArray...

I must admit I don't understand what the problem is.

}

private partial IEnumerable<SystemTypeValue> GetNestedTypesOnType (TypeProxy type, string name, BindingFlags? bindingFlags)
{
foreach (var nestedType in type.Type.GetNestedTypesOnType (t => t.Name == name, bindingFlags))
yield return new SystemTypeValue (new TypeProxy (nestedType));
}

// TODO: Does the analyzer need to do something here?
private partial void MarkStaticConstructor (TypeProxy type) { }

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

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

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

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

// TODO: Does the analyzer need to do something here?
private partial void MarkType (TypeProxy type) { }

private partial string GetContainingSymbolDisplayName () => _operation.FindContainingSymbol (_owningSymbol).GetDisplayName ();
}
}
8 changes: 6 additions & 2 deletions src/ILLink.RoslynAnalyzer/TrimAnalysis/MethodParameterValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,15 @@ namespace ILLink.Shared.TrimAnalysis
{
partial record MethodParameterValue
{
public MethodParameterValue (IParameterSymbol parameterSymbol) => ParameterSymbol = parameterSymbol;
public MethodParameterValue (IParameterSymbol parameterSymbol)
: this (parameterSymbol, FlowAnnotations.GetMethodParameterAnnotation (parameterSymbol)) { }

public MethodParameterValue (IParameterSymbol parameterSymbol, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
=> (ParameterSymbol, DynamicallyAccessedMemberTypes) = (parameterSymbol, dynamicallyAccessedMemberTypes);

public readonly IParameterSymbol ParameterSymbol;

public override DynamicallyAccessedMemberTypes DynamicallyAccessedMemberTypes => FlowAnnotations.GetMethodParameterAnnotation (ParameterSymbol);
public override DynamicallyAccessedMemberTypes DynamicallyAccessedMemberTypes { get; }

public override IEnumerable<string> GetDiagnosticArgumentsForAnnotationMismatch ()
=> new string[] { ParameterSymbol.GetDisplayName (), ParameterSymbol.ContainingSymbol.GetDisplayName () };
Expand Down
33 changes: 26 additions & 7 deletions src/ILLink.RoslynAnalyzer/TrimAnalysis/ReflectionAccessAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using ILLink.RoslynAnalyzer.DataFlow;
using ILLink.Shared;
using ILLink.Shared.TrimAnalysis;
Expand All @@ -17,7 +18,7 @@ internal void GetReflectionAccessDiagnostics (in DiagnosticContext diagnosticCon
foreach (var member in typeSymbol.GetDynamicallyAccessedMembers (requiredMemberTypes, declaredOnly)) {
switch (member) {
case IMethodSymbol method:
GetDiagnosticsForMethod (diagnosticContext, method);
GetReflectionAccessDiagnosticsForMethod (diagnosticContext, method);
break;
case IFieldSymbol field:
GetDiagnosticsForField (diagnosticContext, field);
Expand All @@ -40,14 +41,32 @@ internal void GetReflectionAccessDiagnostics (in DiagnosticContext diagnosticCon
}
}

internal void GetReflectionAccessDiagnosticsForEventsOnTypeHierarchy (in DiagnosticContext diagnosticContext, ITypeSymbol typeSymbol, string name, BindingFlags? bindingFlags)
{
foreach (var @event in typeSymbol.GetEventsOnTypeHierarchy (e => e.Name == name, bindingFlags))
GetDiagnosticsForEvent (diagnosticContext, @event);
}

internal void GetReflectionAccessDiagnosticsForFieldsOnTypeHierarchy (in DiagnosticContext diagnosticContext, ITypeSymbol typeSymbol, string name, BindingFlags? bindingFlags)
{
foreach (var field in typeSymbol.GetFieldsOnTypeHierarchy (f => f.Name == name, bindingFlags))
GetDiagnosticsForField (diagnosticContext, field);
}

internal void GetReflectionAccessDiagnosticsForPropertiesOnTypeHierarchy (in DiagnosticContext diagnosticContext, ITypeSymbol typeSymbol, string name, BindingFlags? bindingFlags)
{
foreach (var prop in typeSymbol.GetPropertiesOnTypeHierarchy (p => p.Name == name, bindingFlags))
GetDiagnosticsForProperty (diagnosticContext, prop);
}

static void ReportRequiresUnreferencedCodeDiagnostic (in DiagnosticContext diagnosticContext, AttributeData requiresAttributeData, ISymbol member)
{
var message = RequiresUnreferencedCodeUtils.GetMessageFromAttribute (requiresAttributeData);
var url = RequiresAnalyzerBase.GetUrlFromAttribute (requiresAttributeData);
diagnosticContext.AddDiagnostic (DiagnosticId.RequiresUnreferencedCode, member.GetDisplayName (), message, url);
}

static void GetDiagnosticsForMethod (in DiagnosticContext diagnosticContext, IMethodSymbol methodSymbol)
internal static void GetReflectionAccessDiagnosticsForMethod (in DiagnosticContext diagnosticContext, IMethodSymbol methodSymbol)
{
if (methodSymbol.TryGetRequiresUnreferencedCodeAttribute (out var requiresAttributeData))
ReportRequiresUnreferencedCodeDiagnostic (diagnosticContext, requiresAttributeData, methodSymbol);
Expand All @@ -69,19 +88,19 @@ static void GetDiagnosticsForMethod (in DiagnosticContext diagnosticContext, IMe
static void GetDiagnosticsForProperty (in DiagnosticContext diagnosticContext, IPropertySymbol propertySymbol)
{
if (propertySymbol.SetMethod is not null)
GetDiagnosticsForMethod (diagnosticContext, propertySymbol.SetMethod);
GetReflectionAccessDiagnosticsForMethod (diagnosticContext, propertySymbol.SetMethod);
if (propertySymbol.GetMethod is not null)
GetDiagnosticsForMethod (diagnosticContext, propertySymbol.GetMethod);
GetReflectionAccessDiagnosticsForMethod (diagnosticContext, propertySymbol.GetMethod);
}

static void GetDiagnosticsForEvent (in DiagnosticContext diagnosticContext, IEventSymbol eventSymbol)
{
if (eventSymbol.AddMethod is not null)
GetDiagnosticsForMethod (diagnosticContext, eventSymbol.AddMethod);
GetReflectionAccessDiagnosticsForMethod (diagnosticContext, eventSymbol.AddMethod);
if (eventSymbol.RemoveMethod is not null)
GetDiagnosticsForMethod (diagnosticContext, eventSymbol.RemoveMethod);
GetReflectionAccessDiagnosticsForMethod (diagnosticContext, eventSymbol.RemoveMethod);
if (eventSymbol.RaiseMethod is not null)
GetDiagnosticsForMethod (diagnosticContext, eventSymbol.RaiseMethod);
GetReflectionAccessDiagnosticsForMethod (diagnosticContext, eventSymbol.RaiseMethod);
}

static void GetDiagnosticsForField (in DiagnosticContext diagnosticContext, IFieldSymbol fieldSymbol)
Expand Down
67 changes: 58 additions & 9 deletions src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,15 @@ public class TrimAnalysisVisitor : LocalDataFlowVisitor<MultiValue, ValueSetLatt
{
public readonly TrimAnalysisPatternStore TrimAnalysisPatterns;

readonly ValueSetLattice<SingleValue> _multiValueLattice;

public TrimAnalysisVisitor (
LocalStateLattice<MultiValue, ValueSetLattice<SingleValue>> lattice,
OperationBlockAnalysisContext context
) : base (lattice, context)
{
TrimAnalysisPatterns = new TrimAnalysisPatternStore (lattice.Lattice.ValueLattice);
_multiValueLattice = lattice.Lattice.ValueLattice;
TrimAnalysisPatterns = new TrimAnalysisPatternStore (_multiValueLattice);
}

// Override visitor methods to create tracked values when visiting operations
Expand All @@ -36,6 +39,26 @@ OperationBlockAnalysisContext context
// - 'this' parameter (for annotated methods)
// - field reference

public override MultiValue Visit (IOperation? operation, StateValue argument)
{
var returnValue = base.Visit (operation, argument);

// If the return value is empty (TopValue basically) and the Operation tree
// reports it as having a constant value, use that as it will automatically cover
// cases we don't need/want to handle.
if (operation != null && returnValue.IsEmpty () && operation.ConstantValue.HasValue) {
object? constantValue = operation.ConstantValue.Value;
if (constantValue == null)
return NullValue.Instance;
else if (operation.Type?.SpecialType == SpecialType.System_String && constantValue is string stringConstantValue)
return new KnownStringValue (stringConstantValue);
else if (operation.Type?.TypeKind == TypeKind.Enum && constantValue is int intConstantValue)
return new ConstIntValue (intConstantValue);
}

return returnValue;
}

public override MultiValue VisitConversion (IConversionOperation operation, StateValue state)
{
var value = base.VisitConversion (operation, state);
Expand Down Expand Up @@ -70,14 +93,15 @@ public override MultiValue VisitInstanceReference (IInstanceReferenceOperation i

public override MultiValue VisitFieldReference (IFieldReferenceOperation fieldRef, StateValue state)
{
if (!fieldRef.Field.Type.IsTypeInterestingForDataflow ())
return TopValue;
if (fieldRef.Field.Type.IsTypeInterestingForDataflow ()) {
var field = fieldRef.Field;
if (field.Name is "Empty" && field.ContainingType.HasName ("System.String"))
return new KnownStringValue (string.Empty);

var field = fieldRef.Field;
if (field.Name is "Empty" && field.ContainingType.HasName ("System.String"))
return new KnownStringValue (string.Empty);
return new FieldValue (fieldRef.Field);
}

return new FieldValue (fieldRef.Field);
return TopValue;
}

public override MultiValue VisitTypeOf (ITypeOfOperation typeOfOperation, StateValue state)
Expand All @@ -90,9 +114,34 @@ public override MultiValue VisitTypeOf (ITypeOfOperation typeOfOperation, StateV
return TopValue;
}

public override MultiValue VisitLiteral (ILiteralOperation literalOperation, StateValue state)
public override MultiValue VisitBinaryOperator (IBinaryOperation operation, StateValue argument)
Copy link
Member

@agocke agocke Feb 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I tried this locally and it looks like operation has a constant value for many of the tests. Is there a specific case I'm missing where it doesn't and we need to evaluate manually? And is it worth checking the ConstantValue anyway for the easy cases?

Copy link
Member Author

@vitek-karas vitek-karas Feb 23, 2022

Choose a reason for hiding this comment

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

You're right - I was looking at the ArgumentOperation which is the parent of this and that one doesn't have a constant value. I'll improve this to use the Roslyns const evaluator when available.

Copy link
Member

@agocke agocke Feb 23, 2022

Choose a reason for hiding this comment

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

You could maybe put it in the top-level Visit(IOperation) helper, unless there's some reason we would want to skip that.

Copy link
Member Author

@vitek-karas vitek-karas Feb 23, 2022

Choose a reason for hiding this comment

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

I'm kind of split on that - it's semi-dangerous. It would mean that the actual specific operation visitor method would not have final say in what the return value is. Maybe not a big deal... I'll probably do it.

Copy link
Member Author

@vitek-karas vitek-karas Feb 24, 2022

Choose a reason for hiding this comment

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

I went with that approach - Visit(IOperation) now handles constants (if the operation specific visit doesn't provide a value). It solves lot of easy cases and works generally for things like null and strings as well.
I kept the BinaryOperation visitor as there are cases where it still works (if one or both of the operands are local variables for example - that requires data flow to figure out values, so constant detection itself doesn't do it) and also as a sample what we would need to add for other operators if necessary.

{
return literalOperation.ConstantValue.Value == null ? NullValue.Instance : TopValue;
if (!operation.ConstantValue.HasValue && // Optimization - if there is already a constant value available, rely on the Visit(IOperation) instead
operation.OperatorKind == BinaryOperatorKind.Or &&
operation.OperatorMethod is null &&
(operation.Type?.TypeKind == TypeKind.Enum || operation.Type?.SpecialType == SpecialType.System_Int32)) {
MultiValue leftValue = Visit (operation.LeftOperand, argument);
MultiValue rightValue = Visit (operation.RightOperand, argument);

MultiValue result = TopValue;
foreach (var left in leftValue) {
if (left is UnknownValue)
result = _multiValueLattice.Meet (result, left);
else if (left is ConstIntValue leftConstInt) {
foreach (var right in rightValue) {
if (right is UnknownValue)
result = _multiValueLattice.Meet (result, right);
else if (right is ConstIntValue rightConstInt) {
result = _multiValueLattice.Meet (result, new ConstIntValue (leftConstInt.Value | rightConstInt.Value));
}
}
}
}

return result;
}

return base.VisitBinaryOperator (operation, argument);
}

// Override handlers for situations where annotated locations may be involved in reflection access:
Expand Down
Loading