Skip to content

Add analyzer for detecting misplaced attributes on delegates #35779

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 6 commits into from
Sep 1, 2021
Merged

Add analyzer for detecting misplaced attributes on delegates #35779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8330349
Add analyzer for detecting misplaced attributes on delegates
captainsafia Aug 21, 2021
15a1dc7
Address feedback from peer review
captainsafia Aug 27, 2021
f86beeb
Update IsInValidNamespace check
captainsafia Aug 30, 2021
b65f1a8
Avoid using ToString in namespace comparison
captainsafia Aug 31, 2021
a0dab23
Update src/Framework/Analyzer/src/DelegateEndpoints/DetectMisplacedLa…
captainsafia Aug 31, 2021
8fc6816
Update src/Framework/Analyzer/src/DelegateEndpoints/DetectMisplacedLa…
captainsafia Aug 31, 2021
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: 2 additions & 0 deletions src/Framework/Analyzer/src/DelegateEndpoints/DelegateEndpointAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public partial class DelegateEndpointAnalyzer : DiagnosticAnalyzer
{
DiagnosticDescriptors.DoNotUseModelBindingAttributesOnDelegateEndpointParameters,
DiagnosticDescriptors.DoNotReturnActionResultsFromMapActions,
DiagnosticDescriptors.DetectMisplacedLambdaAttribute
});

public override void Initialize(AnalysisContext context)
Expand Down Expand Up @@ -54,6 +55,7 @@ public override void Initialize(AnalysisContext context)
var lambda = ((IAnonymousFunctionOperation)delegateCreation.Target);
DisallowMvcBindArgumentsOnParameters(in operationAnalysisContext, wellKnownTypes, invocation, lambda.Symbol);
DisallowReturningActionResultFromMapMethods(in operationAnalysisContext, wellKnownTypes, invocation, lambda);
DetectMisplacedLambdaAttribute(operationAnalysisContext, invocation, lambda);
}
else if (delegateCreation.Target.Kind == OperationKind.MethodReference)
{
Expand Down
86 changes: 86 additions & 0 deletions src/Framework/Analyzer/src/DelegateEndpoints/DetectMisplacedLambdaAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;

namespace Microsoft.AspNetCore.Analyzers.DelegateEndpoints;

public partial class DelegateEndpointAnalyzer : DiagnosticAnalyzer
{
private static void DetectMisplacedLambdaAttribute(
in OperationAnalysisContext context,
IInvocationOperation invocation,
IAnonymousFunctionOperation lambda)
{
// This analyzer will only process invocations that are immediate children of the
// AnonymousFunctionOperation provided as the delegate endpoint. We'll support checking
// for misplaced attributes in `() => Hello()` and `() => { return Hello(); }` but not in:
// () => {
// Hello();
// return "foo";
// }
InvocationExpressionSyntax? targetInvocation = null;

// () => Hello() has a single child which is a BlockOperation so we check to see if
// expression associated with that operation is an invocation expression
if (lambda.Children.FirstOrDefault().Syntax is InvocationExpressionSyntax innerInvocation)
{
targetInvocation = innerInvocation;
}

if (lambda.Children.FirstOrDefault().Children.FirstOrDefault() is IReturnOperation returnOperation
&& returnOperation.ReturnedValue is IInvocationOperation returnedInvocation)
{
targetInvocation = (InvocationExpressionSyntax)returnedInvocation.Syntax;
}

if (targetInvocation is null)
{
return;
}

var methodOperation = invocation.SemanticModel.GetSymbolInfo(targetInvocation);
var methodSymbol = methodOperation.Symbol ?? methodOperation.CandidateSymbols.FirstOrDefault();

// If no method definition was found for the lambda, then abort.
if (methodSymbol is null)
{
return;
}

var attributes = methodSymbol.GetAttributes();
var location = lambda.Syntax.GetLocation();

foreach (var attribute in attributes)
{
if (IsInValidNamespace(attribute.AttributeClass?.ContainingNamespace))
{
context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.DetectMisplacedLambdaAttribute,
location,
attribute.AttributeClass?.Name,
methodSymbol.Name));
}
}

bool IsInValidNamespace(INamespaceSymbol? @namespace)
{
if (@namespace != null && [email protected])
{
// Check for Microsoft.AspNetCore in the ContainingNamespaces for this type
if (@namespace.Name.Equals("AspNetCore", System.StringComparison.Ordinal) && @namespace.ContainingNamespace.Name.Equals("Microsoft", System.StringComparison.Ordinal))
{
return true;
}

return IsInValidNamespace(@namespace.ContainingNamespace);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

}

return false;
}
}
}
9 changes: 9 additions & 0 deletions src/Framework/Analyzer/src/DelegateEndpoints/DiagnosticDescriptors.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,14 @@ internal static class DiagnosticDescriptors
DiagnosticSeverity.Warning,
isEnabledByDefault: true,
helpLinkUri: "https://aka.ms/aspnet/analyzers");

internal static readonly DiagnosticDescriptor DetectMisplacedLambdaAttribute = new(
"ASP0005",
"Do not place attribute on route handlers",
"'{0}' should be placed on the endpoint delegate to be effective",
"Usage",
DiagnosticSeverity.Warning,
isEnabledByDefault: true,
helpLinkUri: "https://aka.ms/aspnet/analyzers");
}
}
240 changes: 240 additions & 0 deletions src/Framework/Analyzer/test/MinimalActions/DetectMisplacedLambdaAttributeTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,240 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Globalization;
using Microsoft.AspNetCore.Analyzer.Testing;
using Xunit;

namespace Microsoft.AspNetCore.Analyzers.DelegateEndpoints;

public partial class DetectMisplacedLambdaAttributeTest
{
private TestDiagnosticAnalyzerRunner Runner { get; } = new(new DelegateEndpointAnalyzer());

[Fact]
public async Task MinimalAction_WithCorrectlyPlacedAttribute_Works()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test with an invalid lambda or a invalid method reference? e.g.

app.MapGet(""/"", () => DoesNotExist());

One with a MethodReference:

app.MapGet(""/"", GetPersonId);

[Authorize]
public string GetPersonId() => string.Empty;

and with a reference to a different source:

// Source1 
app.MapGet(""/"", () => PersonHandlers.GetPersonId);

Where PersonHandlers.GetPersonId is in a separate source file / SyntaxTree

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, good idea. Here's the expected behavior for these:

  1. Invalid reference: No warning from us but user gets a not defined warning.
  2. Method reference: No warning from us since this warning only applies to lambdas.
  3. Reference to a method in a different class: Warning from us about misplaced attribute.

Where PersonHandlers.GetPersonId is in a separate source file / SyntaxTree

For this part, I couldn't find any examples in our codebase of the GetDiagnosticsAsync method processing sources from different source files so I put it in separate files. Is there a way to do that?

{
// Arrange
var source = @"
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Builder;
var app = WebApplication.Create();
app.MapGet(""/"", [Authorize] () => Hello());
void Hello() { }
";
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source);

// Assert
Assert.Empty(diagnostics);
}

[Fact]
public async Task MinimalAction_WithMisplacedAttribute_ProducesDiagnostics()
{
// Arrange
var source = TestSource.Read(@"
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Builder;
var app = WebApplication.Create();
app.MapGet(""/"", /*MM*/() => Hello());
[Authorize]
void Hello() { }
");
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);

// Assert
var diagnostic = Assert.Single(diagnostics);
Assert.Same(DiagnosticDescriptors.DetectMisplacedLambdaAttribute, diagnostic.Descriptor);
AnalyzerAssert.DiagnosticLocation(source.DefaultMarkerLocation, diagnostic.Location);
Assert.Equal("'AuthorizeAttribute' should be placed on the endpoint delegate to be effective", diagnostic.GetMessage(CultureInfo.InvariantCulture));
}

[Fact]
public async Task MinimalAction_WithMisplacedAttributeAndBlockSyntax_ProducesDiagnostics()
{
// Arrange
var source = TestSource.Read(@"
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Builder;
var app = WebApplication.Create();
app.MapGet(""/"", /*MM*/() => { return Hello(); });
Copy link
Contributor

Choose a reason for hiding this comment

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

This one's interesting - if the code looked like:

(int id) => if (id == 0) { return Results.NotFound(); } else { return Hello(); }

This would produce no diagnostics? Is that weird?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's a consequence of not doing the full search through the syntax tree that we were doing before (see #35779 (comment)).

Supporting this scenario invites the question of how "deep" in the syntax tree we want to look for returned invocations and support throwing a warning.

[Authorize]
string Hello() { return ""foo""; }
");
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);

// Assert
var diagnostic = Assert.Single(diagnostics);
Assert.Same(DiagnosticDescriptors.DetectMisplacedLambdaAttribute, diagnostic.Descriptor);
AnalyzerAssert.DiagnosticLocation(source.DefaultMarkerLocation, diagnostic.Location);
Assert.Equal("'AuthorizeAttribute' should be placed on the endpoint delegate to be effective", diagnostic.GetMessage(CultureInfo.InvariantCulture));
}

[Fact]
public async Task MinimalAction_WithMultipleMisplacedAttributes_ProducesDiagnostics()
{
// Arrange
var source = TestSource.Read(@"
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Mvc;
var app = WebApplication.Create();
app.MapGet(""/"", /*MM*/() => Hello());
[Authorize]
[Produces(""application/xml"")]
void Hello() { }
");
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);

// Assert
Assert.Collection(diagnostics,
diagnostic => {
Assert.Same(DiagnosticDescriptors.DetectMisplacedLambdaAttribute, diagnostic.Descriptor);
AnalyzerAssert.DiagnosticLocation(source.DefaultMarkerLocation, diagnostic.Location);
Assert.Equal("'AuthorizeAttribute' should be placed on the endpoint delegate to be effective", diagnostic.GetMessage(CultureInfo.InvariantCulture));
},
diagnostic => {
Assert.Same(DiagnosticDescriptors.DetectMisplacedLambdaAttribute, diagnostic.Descriptor);
AnalyzerAssert.DiagnosticLocation(source.DefaultMarkerLocation, diagnostic.Location);
Assert.Equal("'ProducesAttribute' should be placed on the endpoint delegate to be effective", diagnostic.GetMessage(CultureInfo.InvariantCulture));
}
);
}

[Fact]
public async Task MinimalAction_WithSingleMisplacedAttribute_ProducesDiagnostics()
{
// Arrange
var source = TestSource.Read(@"
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Mvc;
var app = WebApplication.Create();
app.MapGet(""/"", /*MM*/[Authorize]() => Hello());
[Produces(""application/xml"")]
void Hello() { }
");
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);

// Assert
Assert.Collection(diagnostics,
diagnostic => {
Assert.Same(DiagnosticDescriptors.DetectMisplacedLambdaAttribute, diagnostic.Descriptor);
AnalyzerAssert.DiagnosticLocation(source.DefaultMarkerLocation, diagnostic.Location);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we be producing the diagnostic on the attribute (rather than on the delegate)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, I think it makes more sense to put it on the delegate since that is where the user will take the action to resolve the issue (absent us providing a codefix).

Assert.Equal("'ProducesAttribute' should be placed on the endpoint delegate to be effective", diagnostic.GetMessage(CultureInfo.InvariantCulture));
}
);
}

[Fact]
public async Task MinimalAction_DoesNotWarnOnNonReturningMethods()
{
// Arrange
var source = @"
using System;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Builder;
var app = WebApplication.Create();
app.MapGet(""/"", /*MM*/() => {
Console.WriteLine(""foo"");
return ""foo"";
});";
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source);

// Assert
Assert.Empty(diagnostics);
}

[Fact]
public async Task MinimalAction_DoesNotWarnOrErrorOnNonExistantLambdas()
{
// Arrange
var source = @"
using System;
using Microsoft.AspNetCore.Builder;
var app = WebApplication.Create();
app.MapGet(""/"", () => ThereIsNoMethod());";
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source);

// Assert that there is a singal error but it is not ours (CS0103)
var diagnostic = Assert.Single(diagnostics);
Assert.Equal("CS0103", diagnostic.Descriptor.Id);
}

[Fact]
public async Task MinimalAction_WithMisplacedAttributeOnMethodGroup_DoesNotProduceDiagnostics()
{
// Arrange
var source = TestSource.Read(@"
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Builder;
var app = WebApplication.Create();
app.MapGet(""/"", Hello);
[Authorize]
void Hello() { }
");
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);

// Assert
Assert.Empty(diagnostics);
}

[Fact]
public async Task MinimalAction_WithMisplacedAttributeOnExternalReference_DoesNotProduceDiagnostics()
{
// Arrange
var source = TestSource.Read(@"
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Builder;
class Program {
public static void Main(string[] args)
{
var app = WebApplication.Create();
app.MapGet(""/"", /*MM*/() => Helpers.Hello());
}
}

public static class Helpers
{
[Authorize]
public static void Hello() { }
}
");
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);

// Assert
var diagnostic = Assert.Single(diagnostics);
Assert.Same(DiagnosticDescriptors.DetectMisplacedLambdaAttribute, diagnostic.Descriptor);
AnalyzerAssert.DiagnosticLocation(source.DefaultMarkerLocation, diagnostic.Location);
Assert.Equal("'AuthorizeAttribute' should be placed on the endpoint delegate to be effective", diagnostic.GetMessage(CultureInfo.InvariantCulture));
}

[Fact]
public async Task MinimalAction_OutOfScope_ProducesDiagnostics()
{
// Arrange
// WriteLine has nullability annotation attributes that should
// not warn
var source = TestSource.Read(@"
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Builder;
var app = WebApplication.Create();
app.MapGet(""/"", /*MM*/() => System.Console.WriteLine(""foo""));
");
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);

// Assert
Assert.Empty(diagnostics);
}
}