[release/6.0] Complain when returning IActionResult from MinimalAction (#35710)

* Complain when returning IActionResult from MinimalAction

* Update src/Framework/Analyzer/test/MinimalActions/DisallowReturningActionResultsFromMapMethodsTest.cs

Co-authored-by: Pranav K <[email protected]>

Author: github-actions[bot]

src/Framework/Analyzer/src/DelegateEndpoints/DelegateEndpointAnalyzer.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Immutable;
+using System.Diagnostics;
 using System.Linq;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.Diagnostics;
@@ -16,6 +17,7 @@ public partial class DelegateEndpointAnalyzer : DiagnosticAnalyzer
     public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(new[]
     {
         DiagnosticDescriptors.DoNotUseModelBindingAttributesOnDelegateEndpointParameters,
+        DiagnosticDescriptors.DoNotReturnActionResultsFromMapActions,
     });
 
     public override void Initialize(AnalysisContext context)
@@ -28,6 +30,7 @@ public override void Initialize(AnalysisContext context)
             var compilation = compilationStartAnalysisContext.Compilation;
             if (!WellKnownTypes.TryCreate(compilation, out var wellKnownTypes))
             {
+                Debug.Fail("One or more types could not be found. This usually means you are bad at spelling C# type names.");
                 return;
             }
 
@@ -50,11 +53,52 @@ public override void Initialize(AnalysisContext context)
                 {
                     var lambda = ((IAnonymousFunctionOperation)delegateCreation.Target);
                     DisallowMvcBindArgumentsOnParameters(in operationAnalysisContext, wellKnownTypes, invocation, lambda.Symbol);
+                    DisallowReturningActionResultFromMapMethods(in operationAnalysisContext, wellKnownTypes, invocation, lambda);
                 }
                 else if (delegateCreation.Target.Kind == OperationKind.MethodReference)
                 {
                     var methodReference = (IMethodReferenceOperation)delegateCreation.Target;
                     DisallowMvcBindArgumentsOnParameters(in operationAnalysisContext, wellKnownTypes, invocation, methodReference.Method);
+
+                    var foundMethodReferenceBody = false;
+                    if (!methodReference.Method.DeclaringSyntaxReferences.IsEmpty)
+                    {
+                        var syntaxReference = methodReference.Method.DeclaringSyntaxReferences[0];
+                        var methodOperation = invocation.SemanticModel.GetOperation(syntaxReference.GetSyntax(operationAnalysisContext.CancellationToken));
+                        if (methodOperation is ILocalFunctionOperation { Body: not null } localFunction)
+                        {
+                            foundMethodReferenceBody = true;
+                            DisallowReturningActionResultFromMapMethods(
+                                in operationAnalysisContext,
+                                wellKnownTypes,
+                                invocation,
+                                methodReference.Method,
+                                localFunction.Body);
+                        }
+                        else if (methodOperation is IMethodBodyOperation methodBody)
+                        {
+                            foundMethodReferenceBody = true;
+                            DisallowReturningActionResultFromMapMethods(
+                                in operationAnalysisContext,
+                                wellKnownTypes,
+                                invocation,
+                                methodReference.Method,
+                                methodBody.BlockBody ?? methodBody.ExpressionBody);
+                        }
+                    }
+
+                    if (!foundMethodReferenceBody)
+                    {
+                        // it's possible we couldn't find the operation for the method reference. In this case,
+                        // try and provide less detailed diagnostics to the extent we can
+                        DisallowReturningActionResultFromMapMethods(
+                                in operationAnalysisContext,
+                                wellKnownTypes,
+                                invocation,
+                                methodReference.Method,
+                                methodBody: null);
+
+                    }
                 }
             }, OperationKind.Invocation);
         });

src/Framework/Analyzer/src/DelegateEndpoints/DiagnosticDescriptors.cs

@@ -16,5 +16,14 @@ internal static class DiagnosticDescriptors
             DiagnosticSeverity.Warning,
             isEnabledByDefault: true,
             helpLinkUri: "https://aka.ms/aspnet/analyzers");
+
+        internal static readonly DiagnosticDescriptor DoNotReturnActionResultsFromMapActions = new(
+            "ASP0004",
+            "Do not use action results with Map actions",
+            "IActionResult instances should not be returned from a {0} Delegate parameter. Consider returning an equivalent result from Microsoft.AspNetCore.Http.Results.",
+            "Usage",
+            DiagnosticSeverity.Warning,
+            isEnabledByDefault: true,
+            helpLinkUri: "https://aka.ms/aspnet/analyzers");
     }
 }

src/Framework/Analyzer/src/DelegateEndpoints/DisallowReturningActionResultFromMapMethods.cs

@@ -0,0 +1,93 @@
+// 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.Diagnostics;
+using Microsoft.CodeAnalysis.Operations;
+
+namespace Microsoft.AspNetCore.Analyzers.DelegateEndpoints;
+
+public partial class DelegateEndpointAnalyzer : DiagnosticAnalyzer
+{
+    private static void DisallowReturningActionResultFromMapMethods(
+        in OperationAnalysisContext context,
+        WellKnownTypes wellKnownTypes,
+        IInvocationOperation invocationOperation,
+        IAnonymousFunctionOperation anonymousFunction)
+    {
+        DisallowReturningActionResultFromMapMethods(in context, wellKnownTypes, invocationOperation, anonymousFunction.Symbol, anonymousFunction.Body);
+    }
+
+    private static void DisallowReturningActionResultFromMapMethods(
+        in OperationAnalysisContext context,
+        WellKnownTypes wellKnownTypes,
+        IInvocationOperation invocationOperation,
+        IMethodSymbol methodSymbol,
+        IBlockOperation? methodBody)
+    {
+        var returnType = UnwrapPossibleAsyncReturnType(methodSymbol.ReturnType);
+
+        if (wellKnownTypes.IResult.IsAssignableFrom(returnType))
+        {
+            // This type returns some form of IResult. Nothing to do here.
+            return;
+        }
+
+        if (methodBody is null &&
+            (wellKnownTypes.IActionResult.IsAssignableFrom(returnType) ||
+            wellKnownTypes.IConvertToActionResult.IsAssignableFrom(returnType)))
+        {
+            // if we don't have a method body, and the action is IResult or ActionResult<T> returning, produce diagnostics for the entire method.
+            context.ReportDiagnostic(Diagnostic.Create(
+                DiagnosticDescriptors.DoNotReturnActionResultsFromMapActions,
+                invocationOperation.Arguments[2].Syntax.GetLocation(),
+                invocationOperation.TargetMethod.Name));
+            return;
+        }
+
+        foreach (var returnOperation in methodBody.Descendants().OfType<IReturnOperation>())
+        {
+            if (returnOperation.ReturnedValue is null or IInvalidOperation)
+            {
+                continue;
+            }
+
+            var returnedValue = returnOperation.ReturnedValue;
+            if (returnedValue is IConversionOperation conversionOperation)
+            {
+                returnedValue = conversionOperation.Operand;
+            }
+
+            var type = returnedValue.Type;
+
+            if (type is null)
+            {
+                continue;
+            }
+
+            if (wellKnownTypes.IResult.IsAssignableFrom(type))
+            {
+                continue;
+            }
+
+            if (wellKnownTypes.IActionResult.IsAssignableFrom(type))
+            {
+                context.ReportDiagnostic(Diagnostic.Create(
+                    DiagnosticDescriptors.DoNotReturnActionResultsFromMapActions,
+                    returnOperation.Syntax.GetLocation(),
+                    invocationOperation.TargetMethod.Name));
+            }
+        }
+    }
+
+    private static ITypeSymbol UnwrapPossibleAsyncReturnType(ITypeSymbol returnType)
+    {
+        if (returnType is not INamedTypeSymbol { Name: "Task" or "ValueTask", IsGenericType: true, TypeArguments: { Length: 1 } } taskLike)
+        {
+            return returnType;
+        }
+
+        return taskLike.TypeArguments[0];
+    }
+}

src/Framework/Analyzer/src/DelegateEndpoints/WellKnownTypes.cs

@@ -31,11 +31,32 @@ public static bool TryCreate(Compilation compilation, [NotNullWhen(true)] out We
             return false;
         }
 
+        const string IResult = "Microsoft.AspNetCore.Http.IResult";
+        if (compilation.GetTypeByMetadataName(IResult) is not { } iResult)
+        {
+            return false;
+        }
+
+        const string IActionResult = "Microsoft.AspNetCore.Mvc.IActionResult";
+        if (compilation.GetTypeByMetadataName(IActionResult) is not { } iActionResult)
+        {
+            return false;
+        }
+
+        const string IConvertToActionResult = "Microsoft.AspNetCore.Mvc.Infrastructure.IConvertToActionResult";
+        if (compilation.GetTypeByMetadataName(IConvertToActionResult) is not { } iConvertToActionResult)
+        {
+            return false;
+        }
+
         wellKnownTypes = new WellKnownTypes
         {
             DelegateEndpointRouteBuilderExtensions = delegateEndpointRouteBuilderExtensions,
             IBinderTypeProviderMetadata = ibinderTypeProviderMetadata,
             BindAttribute = bindAttribute,
+            IResult = iResult,
+            IActionResult = iActionResult,
+            IConvertToActionResult = iConvertToActionResult,
         };
 
         return true;
@@ -44,4 +65,7 @@ public static bool TryCreate(Compilation compilation, [NotNullWhen(true)] out We
     public ITypeSymbol DelegateEndpointRouteBuilderExtensions { get; private init; }
     public INamedTypeSymbol IBinderTypeProviderMetadata { get; private init; }
     public INamedTypeSymbol BindAttribute { get; private init; }
+    public INamedTypeSymbol IResult { get; private init; }
+    public INamedTypeSymbol IActionResult { get; private init; }
+    public INamedTypeSymbol IConvertToActionResult { get; private init; }
 }

src/Framework/Analyzer/test/Microsoft.AspNetCore.App.Analyzers.Test.csproj

@@ -15,6 +15,7 @@
     <ProjectReference Include="$(RepoRoot)src\Analyzers\Microsoft.AspNetCore.Analyzer.Testing\src\Microsoft.AspNetCore.Analyzer.Testing.csproj" />
     <Reference Include="Microsoft.AspNetCore" />
     <Reference Include="Microsoft.AspNetCore.Mvc" />
+    <Reference Include="Microsoft.AspNetCore.Http.Results" />
   </ItemGroup>
 
 </Project>

src/Framework/Analyzer/test/MinimalActions/DisallowMvcBindArgumentsOnParametersTest.cs

@@ -5,11 +5,11 @@
 using Microsoft.AspNetCore.Analyzer.Testing;
 using Xunit;
 
-namespace Microsoft.AspNetCore.Analyzers.MinimalActions;
+namespace Microsoft.AspNetCore.Analyzers.DelegateEndpoints;
 
 public partial class DisallowMvcBindArgumentsOnParametersTest
 {
-    private TestDiagnosticAnalyzerRunner Runner { get; } = new(new MinimalActionAnalyzer());
+    private TestDiagnosticAnalyzerRunner Runner { get; } = new(new DelegateEndpointAnalyzer());
 
     [Fact]
     public async Task MinimalAction_WithoutBindAttributes_Works()
@@ -59,9 +59,9 @@ public async Task MinimalAction_Lambda_WithBindAttributes_ProducesDiagnostics()
 
         // Assert
         var diagnostic = Assert.Single(diagnostics);
-        Assert.Same(DiagnosticDescriptors.DoNotUseModelBindingAttributesOnMinimalActionParameters, diagnostic.Descriptor);
+        Assert.Same(DiagnosticDescriptors.DoNotUseModelBindingAttributesOnDelegateEndpointParameters, diagnostic.Descriptor);
         AnalyzerAssert.DiagnosticLocation(source.DefaultMarkerLocation, diagnostic.Location);
-        Assert.Equal("BindAttribute should not be specified for a MapGet delegate parameter", diagnostic.GetMessage(CultureInfo.InvariantCulture));
+        Assert.Equal("BindAttribute should not be specified for a MapGet Delegate parameter", diagnostic.GetMessage(CultureInfo.InvariantCulture));
     }
 
     [Fact]
@@ -81,9 +81,9 @@ static void PostWithBind(/*MM*/[ModelBinder] string name) {}
 
         // Assert
         var diagnostic = Assert.Single(diagnostics);
-        Assert.Same(DiagnosticDescriptors.DoNotUseModelBindingAttributesOnMinimalActionParameters, diagnostic.Descriptor);
+        Assert.Same(DiagnosticDescriptors.DoNotUseModelBindingAttributesOnDelegateEndpointParameters, diagnostic.Descriptor);
         AnalyzerAssert.DiagnosticLocation(source.DefaultMarkerLocation, diagnostic.Location);
-        Assert.Equal("ModelBinderAttribute should not be specified for a MapPost delegate parameter", diagnostic.GetMessage(CultureInfo.InvariantCulture));
+        Assert.Equal("ModelBinderAttribute should not be specified for a MapPost Delegate parameter", diagnostic.GetMessage(CultureInfo.InvariantCulture));
     }
 }