Create an Analyzer to warn customers to not user Required attribute opn top-level parameters #7415

src/Microsoft.AspNetCore.Mvc.Analyzers/CodeAnalysisExtensions.cs

@@ -123,6 +123,24 @@ public static bool IsAssignableFrom(this ITypeSymbol source, ITypeSymbol target)
  return false;
  }
 
+ public static bool IsCollection(this ITypeSymbol source)
+ {
+ if (source.Kind == SymbolKind.ArrayType)
+ {
+ return true;
+ }
+
+ foreach (var @interface in source.AllInterfaces)
+ {
+ if (@interface.MetadataName == "IEnumerable")
+ {
+ return true;
+ }
+ }
+
+ return false;
+ }
+
  private static bool HasAttribute(this ISymbol symbol, ITypeSymbol attribute)
  {
  foreach (var declaredAttribute in symbol.GetAttributes())
@@ -145,5 +163,6 @@ private static IEnumerable<ITypeSymbol> GetTypeHierarchy(this ITypeSymbol typeSy
  typeSymbol = typeSymbol.BaseType;
  }
  }
+
  }
 }

src/Microsoft.AspNetCore.Mvc.Analyzers/DiagnosticDescriptors.cs

@@ -52,5 +52,14 @@ public static class DiagnosticDescriptors
  DiagnosticSeverity.Warning,
  isEnabledByDefault: true,
  helpLinkUri: "https://aka.ms/AA20pbc");
+
+ public static readonly DiagnosticDescriptor MVC1005_ParameterAttributeAvoidUsingRequiredOncollections =
+ new DiagnosticDescriptor(
+ "MVC1005",
+ "Use of '{0}' should be avoided on collections.",
+ "Use of '{0}' should be avoided. This always made model validation is true. Consider using [MinLength(1)].",
+ "Usage",
+ DiagnosticSeverity.Warning,
+ isEnabledByDefault: true);
  }
 }

src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs

@@ -58,5 +58,9 @@ internal static class SymbolNames
  public const string ProducesResponseTypeAttribute = "Microsoft.AspNetCore.Mvc.ProducesResponseTypeAttribute";
 
  public const string RenderPartialMethod = "RenderPartial";
+
+ public const string RequiredAttribute = "RequiredAttribute";
+
+ public const string CollectionType= "SourceComplexParameterSymbol";
  }
 }

src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterAttributeAnalyzer.cs

@@ -0,0 +1,106 @@
+using System;
+using System.Collections.Immutable;
+using System.Linq;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.Diagnostics;
+
+namespace Microsoft.AspNetCore.Mvc.Analyzers
+{
+ [DiagnosticAnalyzer(LanguageNames.CSharp)]
+ public class TopLevelParameterAttributeAnalyzer : DiagnosticAnalyzer
+ {
+ public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(
+ DiagnosticDescriptors.MVC1005_ParameterAttributeAvoidUsingRequiredOncollections);
+
+ public override void Initialize(AnalysisContext context)
+ {
+ context.EnableConcurrentExecution();
+ context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
+
+ context.RegisterCompilationStartAction(compilationStartAnalysisContext =>
+ {
+ var typeCache = new SymbolCache(compilationStartAnalysisContext.Compilation);
+ if (typeCache.ControllerAttribute == null || typeCache.ControllerAttribute.TypeKind == TypeKind.Error)
+ {
+ // No-op if we can't find types we care about.
+ return;
+ }
+
+ InitializeWorker(compilationStartAnalysisContext, typeCache);
+ });
+ }
+
+ private void InitializeWorker(CompilationStartAnalysisContext compilationStartAnalysisContext, SymbolCache symbolCache)
+ {
+ compilationStartAnalysisContext.RegisterSymbolAction(symbolAnalysisContext =>
+ {
+
+ var method = (IMethodSymbol)symbolAnalysisContext.Symbol;
+ if (method.MethodKind != MethodKind.Ordinary)
+ {
+ return;
+ }
+
+ if (method.Parameters.Length == 0)
+ {
+ return;
+ }
+
+ if (!MvcFacts.IsController(method.ContainingType, symbolCache.ControllerAttribute, symbolCache.NonControllerAttribute) ||
+ !MvcFacts.IsControllerAction(method, symbolCache.NonActionAttribute, symbolCache.IDisposableDispose))
+ {
+ return;
+ }
+
+ for (var i = 0; i < method.Parameters.Length; i++)
+ {
+ var parameter = method.Parameters[i];
+
+ if (parameter.Type.IsCollection())
+ {
+ if (IsProblematicParameter(parameter))
+ {
+ var location = parameter.Locations.Length != 0 ?
+ parameter.Locations[0] :
+ Location.None;
+ var descriptor = DiagnosticDescriptors.MVC1005_ParameterAttributeAvoidUsingRequiredOncollections;
+
+ symbolAnalysisContext.ReportDiagnostic(
+ Diagnostic.Create(
+ DiagnosticDescriptors.MVC1005_ParameterAttributeAvoidUsingRequiredOncollections,
+ location,
+ SymbolNames.RequiredAttribute));
+ }
+ }
+ }
+
+
+ }, SymbolKind.Method);
+ }
+
+ private bool IsProblematicParameter(IParameterSymbol parameter)
+ {
+ return parameter.GetAttributes().Any(attribute => Equals(attribute.AttributeClass.MetadataName, SymbolNames.RequiredAttribute));
+ }
+
+ internal readonly struct SymbolCache
+ {
+ public SymbolCache(Compilation compilation)
+ {
+ ControllerAttribute = compilation.GetTypeByMetadataName(SymbolNames.ControllerAttribute);
+ NonControllerAttribute = compilation.GetTypeByMetadataName(SymbolNames.NonControllerAttribute);
+ NonActionAttribute = compilation.GetTypeByMetadataName(SymbolNames.NonActionAttribute);
+
+ var disposable = compilation.GetSpecialType(SpecialType.System_IDisposable);
+ var members = disposable.GetMembers(nameof(IDisposable.Dispose));
+ IDisposableDispose = members.Length == 1 ? (IMethodSymbol)members[0] : null;
+ }
+
+ public INamedTypeSymbol ControllerAttribute { get; }
+ public INamedTypeSymbol NonControllerAttribute { get; }
+ public INamedTypeSymbol NonActionAttribute { get; }
+
+ public IMethodSymbol IDisposableDispose { get; }
+ }
+ }
+}

test/Mvc.Analyzers.Test/Mvc.Analyzers.Test.csproj

@@ -7,6 +7,11 @@
  </PropertyGroup>
 
  <ItemGroup>
+ <Compile Remove="TestFiles\TopLevelParameterAttributeAnalyzerTest\DiagnosticsAreReturned_ForControllerActionsWithParametersWithOutRequiredAttributeForIEnumerable.cs" />
+ </ItemGroup>
+
+ <ItemGroup>
+ <None Include="TestFiles\TopLevelParameterAttributeAnalyzerTest\DiagnosticsAreReturned_ForControllerActionsWithParametersWithOutRequiredAttributeForIEnumerable.cs" />
  <None Include="xunit.runner.json" CopyToOutputDirectory="PreserveNewest" />
  </ItemGroup>
 

test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterAttributeAnalyzerTest/DiagnosticsAreReturned_ForControllerActionsWithParametersWithOutRequiredAttribute.cs

@@ -0,0 +1,8 @@
+namespace Microsoft.AspNetCore.Mvc.Analyzers.TestFiles.TopLevelParameterAttributeAnalyzerTest
+{
+ [Controller]
+ public class DiagnosticsAreReturned_ForControllerActionsWithParametersWithOutRequiredAttribute
+ {
+ public IActionResult Index(int[] collection) => null;
+ }
+}

test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterAttributeAnalyzerTest/DiagnosticsAreReturned_ForControllerActionsWithParametersWithOutRequiredAttributeForIEnumerable.cs

@@ -0,0 +1,11 @@
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Microsoft.AspNetCore.Mvc.Analyzers.TestFiles.TopLevelParameterAttributeAnalyzerTest
+{
+ public class DiagnosticsAreReturned_ForControllerActionsWithParametersWithOutRequiredAttributeForIEnumerable : Controller
+ {
+ public IActionResult Index(IEnumerable<int> /*MM*/collection) => null;
+ }
+}

test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterAttributeAnalyzerTest/DiagnosticsAreReturned_ForControllerActionsWithParametersWithRequiredAttributeForArray.cs

@@ -0,0 +1,9 @@
+using System.ComponentModel.DataAnnotations;
+
+namespace Microsoft.AspNetCore.Mvc.Analyzers.TestFiles.TopLevelParameterAttributeAnalyzerTest
+{
+ public class DiagnosticsAreReturned_ForControllerActionsWithParametersWithRequiredAttributeForArray: Controller
+ {
+ public IActionResult Index([Required]int[] /*MM*/collection) => null; 
+ }
+}

test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterAttributeAnalyzerTest/DiagnosticsAreReturned_ForControllerActionsWithParametersWithRequiredAttributeForIEnumerable.cs

@@ -0,0 +1,12 @@
+using System;
+using System.Collections.Generic;
+using System.ComponentModel.DataAnnotations;
+using System.Text;
+
+namespace Microsoft.AspNetCore.Mvc.Analyzers.TestFiles.TopLevelParameterAttributeAnalyzerTest
+{
+ public class DiagnosticsAreReturned_ForControllerActionsWithParametersWithRequiredAttributeForIEnumerable : Controller
+ {
+ public IActionResult Index([Required]IEnumerable<int> /*MM*/collection) => null;
+ }
+}

test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterAttributeAnalyzerTest/RunNoDiagnosticsAreReturnedForNonCollectionsParameterWithRequired.cs

@@ -0,0 +1,9 @@
+using System.ComponentModel.DataAnnotations;
+
+namespace Microsoft.AspNetCore.Mvc.Analyzers.TestFiles.TopLevelParameterAttributeAnalyzerTest
+{
+ public class RunNoDiagnosticsAreReturnedForNonCollectionsParameterWithRequired : Controller
+ {
+ public IActionResult Index([Required] int id) => null;
+ }
+}

test/Mvc.Analyzers.Test/TopLevelParameterAttributeAnalyzerTest.cs

@@ -0,0 +1,76 @@
+using System.Runtime.CompilerServices;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Analyzer.Testing;
+using Xunit;
+
+namespace Microsoft.AspNetCore.Mvc.Analyzers
+{
+ public class TopLevelParameterAttributeAnalyzerTest
+ {
+ private MvcDiagnosticAnalyzerRunner Runner { get; } = new MvcDiagnosticAnalyzerRunner(new TopLevelParameterAttributeAnalyzer());
+
+ [Fact]
+ public Task DiagnosticsAreReturned_ForControllerActionsWithParametersWithRequiredAttributeForArray()
+ => RunTest();
+
+ [Fact]
+ public Task DiagnosticsAreReturned_ForControllerActionsWithParametersWithRequiredAttributeForIEnumerable()
+ => RunTest();
+
+ [Fact]
+ public Task DiagnosticsAreReturned_ForControllerActionsWithParametersWithOutRequiredAttribute()
+ => RunNoDiagnosticsAreReturned(nameof(TopLevelParameterAttributeAnalyzerTest));
+
+
+ [Fact]
+ public Task NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttribute()
+ => RunNoDiagnosticsAreReturned(nameof(TopLevelParameterNameAnalyzerTest));
+
+ [Fact]
+ public Task NoDiagnosticsAreReturnedForNonActions()
+ => RunNoDiagnosticsAreReturned(nameof(TopLevelParameterNameAnalyzerTest));
+
+ [Fact]
+ public Task RunNoDiagnosticsAreReturnedForNonCollectionsParameterWithRequired()
+ => RunNoDiagnosticsAreReturned(nameof(TopLevelParameterAttributeAnalyzerTest));
+
+ [Fact]
+ public Task DiagnosticsAreReturned_ForControllerActionsWithParametersWithOutRequiredAttributeForIEnumerable()
+ => RunNoDiagnosticsAreReturned(nameof(TopLevelParameterAttributeAnalyzerTest));
+
+ private async Task RunNoDiagnosticsAreReturned(string typename, [CallerMemberName] string testMethod = "")
+ {
+ // Arrange
+ var testSource = MvcTestSource.Read(typename, testMethod);
+ var expectedLocation = testSource.DefaultMarkerLocation;
+
+ // Act
+ var result = await Runner.GetDiagnosticsAsync(testSource.Source);
+
+ // Assert
+ Assert.Empty(result);
+ }
+
+ private async Task RunTest([CallerMemberName] string testMethod = "")
+ {
+ // Arrange
+ var descriptor = DiagnosticDescriptors.MVC1005_ParameterAttributeAvoidUsingRequiredOncollections;
+ var testSource = MvcTestSource.Read(GetType().Name, testMethod);
+ var expectedLocation = testSource.DefaultMarkerLocation;
+
+ // Act
+ var result = await Runner.GetDiagnosticsAsync(testSource.Source);
+
+ // Assert
+ Assert.Collection(
+ result,
+ diagnostic =>
+ {
+ Assert.Equal(descriptor.Id, diagnostic.Id);
+ Assert.Same(descriptor, diagnostic.Descriptor);
+ AnalyzerAssert.DiagnosticLocation(expectedLocation, diagnostic.Location);
+ Assert.Equal(string.Format(descriptor.MessageFormat.ToString(), SymbolNames.RequiredAttribute), diagnostic.GetMessage());
+ });
+ }
+ }
+}