Skip to content

Commit

Permalink
Warn on method access in Requires analyzers (#110086)
Browse files Browse the repository at this point in the history 
The issue with calling Methods with requires is the the method itself,
not the parameters. Warning on the entire invocation including the
parameters can be confusing. Instead, we should warn just on the method
access for Requires. DynamicallyAccessedMembers should still warn on the
invocation.

Also updates the Microsoft.Extensions.Configuration diagnostic suppressor to work suppress warnings on the method access, not only on method invocation warnings.
  • Loading branch information
@jtschuster
jtschuster authored Dec 31, 2024
1 parent 6045e29 commit 511d266
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 57 deletions.
8 changes: 6 additions & 2 deletions ...Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Suppressor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,16 @@ public override void ReportSuppressions(SuppressionAnalysisContext context)
? diagnostic.AdditionalLocations[0]
: diagnostic.Location;

// The trim analyzer changed from warning on the InvocationExpression to the MemberAccessExpression in https://github.com/dotnet/runtime/pull/110086
// In other words, the warning location went from from `{|Method1(arg1, arg2)|}` to `{|Method1|}(arg1, arg2)`
// To account for this, we need to check if the location is an InvocationExpression or a child of an InvocationExpression.
bool shouldSuppressDiagnostic =
location.SourceTree is SyntaxTree sourceTree &&
sourceTree.GetRoot().FindNode(location.SourceSpan) is SyntaxNode syntaxNode &&
BinderInvocation.IsCandidateSyntaxNode(syntaxNode) &&
(syntaxNode as InvocationExpressionSyntax ?? syntaxNode.Parent as InvocationExpressionSyntax) is InvocationExpressionSyntax invocation &&
BinderInvocation.IsCandidateSyntaxNode(invocation) &&
context.GetSemanticModel(sourceTree)
.GetOperation((InvocationExpressionSyntax)syntaxNode, context.CancellationToken) is IInvocationOperation operation &&
.GetOperation(invocation, context.CancellationToken) is IInvocationOperation operation &&
BinderInvocation.IsBindingOperation(operation);

if (shouldSuppressDiagnostic)
Expand Down
10 changes: 8 additions & 2 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisMethodCallPattern.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using ILLink.Shared.DataFlow;
using ILLink.Shared.TrimAnalysis;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using MultiValue = ILLink.Shared.DataFlow.ValueSet<ILLink.Shared.DataFlow.SingleValue>;

namespace ILLink.RoslynAnalyzer.TrimAnalysis
Expand Down Expand Up @@ -71,14 +72,19 @@ public TrimAnalysisMethodCallPattern Merge (

public void ReportDiagnostics (DataFlowAnalyzerContext context, Action<Diagnostic> reportDiagnostic)
{
var location = Operation.Syntax.GetLocation ();
Location location = Operation.Syntax.GetLocation ();
if (context.EnableTrimAnalyzer &&
!OwningSymbol.IsInRequiresUnreferencedCodeAttributeScope(out _) &&
!FeatureContext.IsEnabled (RequiresUnreferencedCodeAnalyzer.FullyQualifiedRequiresUnreferencedCodeAttribute))
{
TrimAnalysisVisitor.HandleCall(Operation, OwningSymbol, CalledMethod, Instance, Arguments, location, reportDiagnostic, default, out var _);
}

// For Requires, make the location the reference to the method, not the entire invocation.
// The parameters are not part of the issue, and including them in the location can be misleading.
location = Operation.Syntax switch {
InvocationExpressionSyntax invocationSyntax => invocationSyntax.Expression.GetLocation (),
_ => location
};
var diagnosticContext = new DiagnosticContext (location, reportDiagnostic);
foreach (var requiresAnalyzer in context.EnabledRequiresAnalyzers)
{
Expand Down
32 changes: 16 additions & 16 deletions src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresAssemblyFilesAnalyzerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ void M2()
""";
return VerifyRequiresAssemblyFilesAnalyzer (TestRequiresAssemblyFilesWithMessageAndUrl,
// (12,3): warning IL3002: Using member 'C.M1()' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app. https://helpurl
VerifyCS.Diagnostic (DiagnosticId.RequiresAssemblyFiles).WithSpan (12, 3, 12, 7).WithArguments ("C.M1()", "", " https://helpurl"));
VerifyCS.Diagnostic (DiagnosticId.RequiresAssemblyFiles).WithSpan (12, 3, 12, 5).WithArguments ("C.M1()", "", " https://helpurl"));
}

[Fact]
Expand Down Expand Up @@ -275,7 +275,7 @@ void M3()
""";
return VerifyRequiresAssemblyFilesAnalyzer (TestNoDiagnosticIsProducedIfCallerIsAnnotated,
// (7,3): warning IL3002: Using member 'C.M2()' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app. Warn from M2.
VerifyCS.Diagnostic (DiagnosticId.RequiresAssemblyFiles).WithSpan (7, 3, 7, 7).WithArguments ("C.M2()", " Warn from M2.", ""));
VerifyCS.Diagnostic (DiagnosticId.RequiresAssemblyFiles).WithSpan (7, 3, 7, 5).WithArguments ("C.M2()", " Warn from M2.", ""));
}

[Fact]
Expand Down Expand Up @@ -334,9 +334,9 @@ public void M()
""";
return VerifyRequiresAssemblyFilesAnalyzer (src,
// (7,7): warning IL3001: Assemblies embedded in a single-file app cannot have additional files in the manifest.
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyGetFilesInSingleFile).WithSpan (7, 7, 7, 35).WithArguments ("System.Reflection.Assembly.GetFile(String)"),
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyGetFilesInSingleFile).WithSpan (7, 7, 7, 16).WithArguments ("System.Reflection.Assembly.GetFile(String)"),
// (8,7): warning IL3001: Assemblies embedded in a single-file app cannot have additional files in the manifest.
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyGetFilesInSingleFile).WithSpan (8, 7, 8, 19).WithArguments ("System.Reflection.Assembly.GetFiles()")
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyGetFilesInSingleFile).WithSpan (8, 7, 8, 17).WithArguments ("System.Reflection.Assembly.GetFiles()")
);
}

Expand Down Expand Up @@ -388,7 +388,7 @@ public void M()
// (7,7): warning IL3000: 'System.Reflection.Assembly.Location' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'.
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (7, 7, 7, 17).WithArguments ("System.Reflection.Assembly.Location.get"),
// (8,7): warning IL3001: Assemblies embedded in a single-file app cannot have additional files in the manifest.
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyGetFilesInSingleFile).WithSpan (8, 7, 8, 19).WithArguments ("System.Reflection.Assembly.GetFiles()")
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyGetFilesInSingleFile).WithSpan (8, 7, 8, 17).WithArguments ("System.Reflection.Assembly.GetFiles()")
);
}

Expand Down Expand Up @@ -493,13 +493,13 @@ public class F
fixedSource: fixtest,
baselineExpected: new[] {
// /0/Test0.cs(6,14): warning IL3002: Using member 'C.M1()' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app. message.
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(6, 14, 6, 18).WithArguments("C.M1()", " message.", ""),
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(6, 14, 6, 16).WithArguments("C.M1()", " message.", ""),
// /0/Test0.cs(10,24): warning IL3002: Using member 'C.M1()' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app. message.
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(10, 24, 10, 30).WithArguments("C.M1()", " message.", ""),
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(10, 24, 10, 28).WithArguments("C.M1()", " message.", ""),
// /0/Test0.cs(13,25): warning IL3002: Using member 'C.M1()' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app. message.
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(13, 25, 13, 31).WithArguments("C.M1()", " message.", ""),
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(13, 25, 13, 29).WithArguments("C.M1()", " message.", ""),
// /0/Test0.cs(20,25): warning IL3002: Using member 'C.M1()' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app. message.
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(20, 25, 20, 31).WithArguments("C.M1()", " message.", "")
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(20, 25, 20, 29).WithArguments("C.M1()", " message.", "")
},
fixedExpected: Array.Empty<DiagnosticResult> ());
}
Expand Down Expand Up @@ -542,7 +542,7 @@ public void M2() {
// /0/Test0.cs(6,24): warning IL3000: 'System.Reflection.Assembly.Location' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'.
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (6, 24, 6, 41).WithArguments ("System.Reflection.Assembly.Location.get", "", ""),
// /0/Test0.cs(8,7): warning IL3001: 'System.Reflection.Assembly.GetFiles()' will throw for assemblies embedded in a single-file app
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyGetFilesInSingleFile).WithSpan (8, 7, 8, 26).WithArguments("System.Reflection.Assembly.GetFiles()", "", ""),
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyGetFilesInSingleFile).WithSpan (8, 7, 8, 24).WithArguments("System.Reflection.Assembly.GetFiles()", "", ""),
},
fixedExpected: Array.Empty<DiagnosticResult> ());
}
Expand Down Expand Up @@ -580,7 +580,7 @@ public class C
fixedSource: fix,
baselineExpected: new[] {
// /0/Test0.cs(9,12): warning IL3002: Using member 'C.M1()' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app. message.
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(9, 12, 9, 16).WithArguments("C.M1()", " message.", "")
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(9, 12, 9, 14).WithArguments("C.M1()", " message.", "")
},
fixedExpected: Array.Empty<DiagnosticResult> ());
}
Expand Down Expand Up @@ -627,9 +627,9 @@ private int M2 {
""";
var diag = new[] {
// /0/Test0.cs(12,16): warning IL3002: Using member 'C.M1()' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app. message.
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(12, 16, 12, 20).WithArguments("C.M1()", " message.", ""),
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(12, 16, 12, 18).WithArguments("C.M1()", " message.", ""),
// /0/Test0.cs(13,17): warning IL3002: Using member 'C.M1()' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app. message.
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(13, 17, 13, 21).WithArguments("C.M1()", " message.", "")
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(13, 17, 13, 19).WithArguments("C.M1()", " message.", "")
};
return VerifyRequiresAssemblyFilesCodeFix (src, fix, diag, Array.Empty<DiagnosticResult> ());
}
Expand Down Expand Up @@ -702,7 +702,7 @@ Action M2()
fixedSource: fix,
baselineExpected: new[] {
// /0/Test0.cs(11,22): warning IL3002: Using member 'C.M1()' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app. message.
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(11, 22, 11, 26).WithArguments("C.M1()", " message.", "")
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(11, 22, 11, 24).WithArguments("C.M1()", " message.", "")
},
fixedExpected: Array.Empty<DiagnosticResult> (),
numberOfIterations: 2);
Expand Down Expand Up @@ -741,7 +741,7 @@ public class C
fixedSource: fix,
baselineExpected: new[] {
// /0/Test0.cs(9,17): warning IL3002: Using member 'C.M1()' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app. message.
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(9, 17, 9, 21).WithArguments("C.M1()", " message.", "")
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(9, 17, 9, 19).WithArguments("C.M1()", " message.", "")
},
fixedExpected: Array.Empty<DiagnosticResult> ());
}
Expand Down Expand Up @@ -793,7 +793,7 @@ public event EventHandler E1
fixedSource: fix,
baselineExpected: new[] {
// /0/Test0.cs(13,12): warning IL3002: Using method 'C.M1()' which has 'RequiresAssemblyFilesAttribute' can break functionality when trimming application code. message.
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(13, 12, 13, 16).WithArguments("C.M1()", " message.", "")
VerifyCS.Diagnostic(DiagnosticId.RequiresAssemblyFiles).WithSpan(13, 12, 13, 14).WithArguments("C.M1()", " message.", "")
},
fixedExpected: Array.Empty<DiagnosticResult> ());
}
Expand Down
Loading

0 comments on commit 511d266

Please sign in to comment.