From 539e461cd10b480913032b2ae69ac8a3e584e07c Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Thu, 22 Sep 2022 20:47:45 +0800 Subject: [PATCH] Add RequestDelegate analyzer --- .../src/Analyzers/DiagnosticDescriptors.cs | 9 + .../Http/RequestDelegateReturnTypeAnalyzer.cs | 122 +++++++ .../RequestDelegateReturnTypeAnalyzerTests.cs | 319 ++++++++++++++++++ .../test/Verifiers/CSharpAnalyzerVerifier.cs | 1 + 4 files changed, 451 insertions(+) create mode 100644 src/Framework/AspNetCoreAnalyzers/src/Analyzers/Http/RequestDelegateReturnTypeAnalyzer.cs create mode 100644 src/Framework/AspNetCoreAnalyzers/test/Http/RequestDelegateReturnTypeAnalyzerTests.cs diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs index d1880674706e..58ee1ceea9d8 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs @@ -124,4 +124,13 @@ internal static class DiagnosticDescriptors DiagnosticSeverity.Info, isEnabledByDefault: true, helpLinkUri: "https://aka.ms/aspnet/analyzers"); + + internal static readonly DiagnosticDescriptor DoNotReturnValueFromRequestDelegate = new( + "ASP0016", + "Do not return a value from RequestDelegate", + "The method used to create a RequestDelegate returns Task<{0}>. RequestDelegate discards this value. If this isn't intended then don't return a value or change the method signature to not match RequestDelegate.", + "Usage", + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + helpLinkUri: "https://aka.ms/aspnet/analyzers"); } diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Http/RequestDelegateReturnTypeAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Http/RequestDelegateReturnTypeAnalyzer.cs new file mode 100644 index 000000000000..6e5c2132d0e1 --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Http/RequestDelegateReturnTypeAnalyzer.cs @@ -0,0 +1,122 @@ +// 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.Immutable; +using System.Diagnostics.CodeAnalysis; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +namespace Microsoft.AspNetCore.Analyzers.Http; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public partial class RequestDelegateReturnTypeAnalyzer : DiagnosticAnalyzer +{ + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(DiagnosticDescriptors.DoNotReturnValueFromRequestDelegate); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterCompilationStartAction(context => + { + var compilation = context.Compilation; + + if (!WellKnownTypes.TryCreate(compilation, out var wellKnownTypes)) + { + return; + } + context.RegisterOperationAction(context => + { + var methodReference = (IMethodReferenceOperation)context.Operation; + if (methodReference.Parent is { } parent && + parent.Kind == OperationKind.DelegateCreation && + SymbolEqualityComparer.Default.Equals(parent.Type, wellKnownTypes.RequestDelegate)) + { + // Inspect return type of method signature for Task. + var returnType = methodReference.Method.ReturnType; + + if (SymbolEqualityComparer.Default.Equals(returnType.OriginalDefinition, wellKnownTypes.TaskOfT)) + { + AddDiagnosticWarning(context, methodReference.Syntax.GetLocation(), returnType); + } + } + }, OperationKind.MethodReference); + context.RegisterOperationAction(context => + { + var anonymousFunction = (IAnonymousFunctionOperation)context.Operation; + if (anonymousFunction.Parent is { } parent && + parent.Kind == OperationKind.DelegateCreation && + SymbolEqualityComparer.Default.Equals(parent.Type, wellKnownTypes.RequestDelegate)) + { + // Inspect contents of anonymous function and search for return statements. + // Return statement of Task means a value was returned. + foreach (var item in anonymousFunction.Body.Descendants()) + { + if (item is IReturnOperation returnOperation && + returnOperation.ReturnedValue is { } returnedValue) + { + var resolvedOperation = WalkDownConversion(returnedValue); + var returnType = resolvedOperation.Type; + + if (SymbolEqualityComparer.Default.Equals(returnType.OriginalDefinition, wellKnownTypes.TaskOfT)) + { + AddDiagnosticWarning(context, anonymousFunction.Syntax.GetLocation(), returnType); + return; + } + } + } + } + }, OperationKind.AnonymousFunction); + }); + } + + private static void AddDiagnosticWarning(OperationAnalysisContext context, Location location, ITypeSymbol returnType) + { + context.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.DoNotReturnValueFromRequestDelegate, + location, + ((INamedTypeSymbol)returnType).TypeArguments[0].ToString())); + } + + private static IOperation WalkDownConversion(IOperation operation) + { + while (operation is IConversionOperation conversionOperation) + { + operation = conversionOperation.Operand; + } + + return operation; + } + + internal sealed class WellKnownTypes + { + public static bool TryCreate(Compilation compilation, [NotNullWhen(returnValue: true)] out WellKnownTypes? wellKnownTypes) + { + wellKnownTypes = default; + + const string RequestDelegate = "Microsoft.AspNetCore.Http.RequestDelegate"; + if (compilation.GetTypeByMetadataName(RequestDelegate) is not { } requestDelegate) + { + return false; + } + + const string TaskOfT = "System.Threading.Tasks.Task`1"; + if (compilation.GetTypeByMetadataName(TaskOfT) is not { } taskOfT) + { + return false; + } + + wellKnownTypes = new() + { + RequestDelegate = requestDelegate, + TaskOfT = taskOfT + }; + + return true; + } + + public INamedTypeSymbol RequestDelegate { get; private init; } + public INamedTypeSymbol TaskOfT { get; private init; } + } +} diff --git a/src/Framework/AspNetCoreAnalyzers/test/Http/RequestDelegateReturnTypeAnalyzerTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Http/RequestDelegateReturnTypeAnalyzerTests.cs new file mode 100644 index 000000000000..fb8b62e7cb5f --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/test/Http/RequestDelegateReturnTypeAnalyzerTests.cs @@ -0,0 +1,319 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.AspNetCore.Analyzers.Http; + +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.Testing; +using VerifyCS = Microsoft.AspNetCore.Analyzers.Verifiers.CSharpAnalyzerVerifier< + Microsoft.AspNetCore.Analyzers.Http.RequestDelegateReturnTypeAnalyzer>; + +public class RequestDelegateReturnTypeAnalyzerTests +{ + private string GetMessage(string type) => + $"The method used to create a RequestDelegate returns Task<{type}>. RequestDelegate discards this value. If this isn't intended then don't return a value or change the method signature to not match RequestDelegate."; + + [Fact] + public async Task AnonymousDelegate_RequestDelegate_ReturnType_EndpointCtor_ReportDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.Use(async (HttpContext context, Func next) => +{ + context.SetEndpoint(new Endpoint({|#0:c => { return Task.FromResult(DateTime.Now); }|}, EndpointMetadataCollection.Empty, ""Test"")); + await next(); +}); +", + new DiagnosticResult(DiagnosticDescriptors.DoNotReturnValueFromRequestDelegate) + .WithLocation(0) + .WithMessage(GetMessage("System.DateTime"))); + } + + [Fact] + public async Task AnonymousDelegate_RequestDelegate_ReturnType_AsTask_ReportDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.MapGet(""/"", {|#0:(HttpContext context) => +{ + return context.Request.ReadFromJsonAsync().AsTask(); +}|}); +", + new DiagnosticResult(DiagnosticDescriptors.DoNotReturnValueFromRequestDelegate) + .WithLocation(0) + .WithMessage(GetMessage("object?"))); + } + + [Fact] + public async Task AnonymousDelegate_RequestDelegate_ReturnType_DelegateCtor_ReportDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.Use(next => +{ + return new RequestDelegate({|#0:(HttpContext context) => + { + next(context).Wait(); + return Task.FromResult(""hello world""); + }|}); +}); +", + new DiagnosticResult(DiagnosticDescriptors.DoNotReturnValueFromRequestDelegate) + .WithLocation(0) + .WithMessage(GetMessage("string"))); + } + + [Fact] + public async Task AnonymousDelegate_RequestDelegate_ReturnTypeMethodCall_ReportDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.MapGet(""/"", {|#0:(HttpContext context) => Task.FromResult(""hello world"")|}); +", + new DiagnosticResult(DiagnosticDescriptors.DoNotReturnValueFromRequestDelegate) + .WithLocation(0) + .WithMessage(GetMessage("string"))); + } + + [Fact] + public async Task AnonymousDelegate_RequestDelegate_ReturnTypeVariable_ReportDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.MapGet(""/"",{|#0:(HttpContext context) => +{ + var t = Task.FromResult(""hello world""); + return t; +}|}); +", + new DiagnosticResult(DiagnosticDescriptors.DoNotReturnValueFromRequestDelegate) + .WithLocation(0) + .WithMessage(GetMessage("string"))); + } + + [Fact] + public async Task AnonymousDelegate_RequestDelegate_ReturnTypeTernary_ReportDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.MapGet(""/"", {|#0:(HttpContext context) => +{ + var t1 = Task.FromResult(""hello world""); + var t2 = Task.FromResult(""hello world""); + return t1.IsCompleted ? t1 : t2; +}|}); +", + new DiagnosticResult(DiagnosticDescriptors.DoNotReturnValueFromRequestDelegate) + .WithLocation(0) + .WithMessage(GetMessage("string"))); + } + + [Fact] + public async Task AnonymousDelegate_RequestDelegate_ReturnTypeCoalesce_ReportDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.MapGet(""/"", {|#0:(HttpContext context) => +{ + var t1 = Task.FromResult(""hello world""); + var t2 = Task.FromResult(""hello world""); + return t1 ?? t2; +}|}); +", + new DiagnosticResult(DiagnosticDescriptors.DoNotReturnValueFromRequestDelegate) + .WithLocation(0) + .WithMessage(GetMessage("string"))); + } + + [Fact] + public async Task AnonymousDelegate_RequestDelegate_MultipleReturns_ReportDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.MapGet(""/"", {|#0:(HttpContext context) => +{ + var t1 = Task.FromResult(""hello world""); + var t2 = Task.FromResult(""hello world""); + if (t1.IsCompleted) + { + return t1; + } + else + { + return t2; + } +}|}); +", + new DiagnosticResult(DiagnosticDescriptors.DoNotReturnValueFromRequestDelegate) + .WithLocation(0) + .WithMessage(GetMessage("string"))); + } + + [Fact] + public async Task AnonymousDelegate_RequestDelegate_MixReturnValues_ReportDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.MapGet(""/"", {|#0:(HttpContext context) => +{ + var t1 = Task.FromResult(""hello world""); + var t2 = Task.FromResult(1); + if (t1.IsCompleted) + { + return Task.CompletedTask; + } + else + { + return t2; + } +}|}); +", + new DiagnosticResult(DiagnosticDescriptors.DoNotReturnValueFromRequestDelegate) + .WithLocation(0) + .WithMessage(GetMessage("int"))); + } + + [Fact] + public async Task AnonymousDelegate_NotRequestDelegate_Async_HasReturnType_NoDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.MapGet(""/"", async (HttpContext context) => ""hello world""); +"); + } + + [Fact] + public async Task AnonymousDelegate_RequestDelegate_Async_HasReturns_NoReturnType_NoDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.MapGet(""/"", async (HttpContext context) => +{ + if (Task.CompletedTask.IsCompleted) + { + await Task.Yield(); + return; + } + else + { + await Task.Delay(1000); + return; + } +}); +"); + } + + [Fact] + public async Task AnonymousDelegate_RequestDelegate_NoReturnType_NoDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.MapGet(""/"", (HttpContext context) => Task.CompletedTask); +"); + } + + [Fact] + public async Task AnonymousDelegate_RequestDelegate_MultipleReturns_NoReturnType_NoDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.MapGet(""/"", (HttpContext context) => +{ + if (Task.CompletedTask.IsCompleted) + { + return Task.CompletedTask; + } + else + { + return Task.CompletedTask; + } +}); +"); + } + + [Fact] + public async Task MethodReference_RequestDelegate_HasReturnType_ReportDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.MapGet(""/"", {|#0:HttpMethod|}); + +static Task HttpMethod(HttpContext context) => Task.FromResult(""hello world""); +", + new DiagnosticResult(DiagnosticDescriptors.DoNotReturnValueFromRequestDelegate) + .WithLocation(0) + .WithMessage(GetMessage("string"))); + } + + [Fact] + public async Task MethodReference_RequestDelegate_NoReturnType_NoDiagnostics() + { + // Arrange & Act & Assert + await VerifyCS.VerifyAnalyzerAsync(@" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Builder; +var webApp = WebApplication.Create(); +webApp.MapGet(""/"", HttpMethod); + +static Task HttpMethod(HttpContext context) => Task.CompletedTask; +"); + } +} diff --git a/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs b/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs index e68cfa13156f..510a976b9922 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs @@ -61,6 +61,7 @@ internal static ReferenceAssemblies GetReferenceAssemblies() TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Http.IResult).Assembly.Location), TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Http.IHeaderDictionary).Assembly.Location), TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Http.HeaderDictionary).Assembly.Location), + TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Http.HttpRequestJsonExtensions).Assembly.Location), TrimAssemblyExtension(typeof(Microsoft.Extensions.Primitives.StringValues).Assembly.Location), TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Hosting.HostingAbstractionsWebHostBuilderExtensions).Assembly.Location), TrimAssemblyExtension(typeof(Microsoft.Extensions.Logging.ILoggingBuilder).Assembly.Location),