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,7 @@ public static bool IsAssignableFrom(this ITypeSymbol source, ITypeSymbol target)
  return false;
  }
 
+
  private static bool HasAttribute(this ISymbol symbol, ITypeSymbol attribute)
  {
  foreach (var declaredAttribute in symbol.GetAttributes())
@@ -145,5 +146,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_AttributeAvoidUsingRequiredAndBindRequired =
+ 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 '{1}'.",
+ "Usage",
+ DiagnosticSeverity.Warning,
+ isEnabledByDefault: true);
  }
 }

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

@@ -0,0 +1,95 @@
+using System;
+using System.Collections.Generic;
+using System.Collections.Immutable;
+using System.Text;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Diagnostics;
+
+namespace Microsoft.AspNetCore.Mvc.Analyzers
+{
+ [DiagnosticAnalyzer(LanguageNames.CSharp)]
+ public class ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzer : DiagnosticAnalyzer
+ {
+ public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(
+ DiagnosticDescriptors.MVC1005_AttributeAvoidUsingRequiredAndBindRequired);
+
+ public override void Initialize(AnalysisContext context)
+ {
+ context.EnableConcurrentExecution();
+ context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
+
+ context.RegisterCompilationStartAction(compilationStartAnalysisContext =>
+ {
+ InitializeWorker(compilationStartAnalysisContext);
+ });
+ }
+
+ private void InitializeWorker(CompilationStartAnalysisContext compilationStartAnalysisContext)
+ {
+ compilationStartAnalysisContext.RegisterSymbolAction(symbolAnalysisContext =>
+ {
+ var property = (IPropertySymbol)symbolAnalysisContext.Symbol;
+
+ if (property.DeclaredAccessibility != Accessibility.Public ||
+ property.IsStatic )
+ {
+ return;
+ }
+
+ if (property.GetAttributes().Length == 0)
+ {
+ return;
+ }
+
+ var typeCache = new SymbolCache(symbolAnalysisContext.Compilation);
+ var location = property.Locations.Length != 0 ?
+ property.Locations[0] :
+ Location.None;
+
+ if (IsNonNullableValueType(property.Type, typeCache.SystemNullableType) && property.HasAttribute(typeCache.RequiredAttribute, inherit: false))
+ {
+ {
+ symbolAnalysisContext.ReportDiagnostic(
+ Diagnostic.Create(
+ DiagnosticDescriptors.MVC1005_AttributeAvoidUsingRequiredAndBindRequired,
+ location,
+ "RequiredAttribute", "BindRequiredAttribute"));
+ }
+ }
+ else if (IsComplexType(property.Type) && property.HasAttribute(typeCache.BindRequiredAttribute, inherit: false))
+ {
+ symbolAnalysisContext.ReportDiagnostic(
+ Diagnostic.Create(
+ DiagnosticDescriptors.MVC1005_AttributeAvoidUsingRequiredAndBindRequired,
+ location,
+ "BindRequiredAttribute", "RequiredAttribute"));
+ }
+
+
+ }, SymbolKind.Property);
+ }
+
+ public static bool IsComplexType(ITypeSymbol type) => type.SpecialType == SpecialType.None;
+
+ public static bool IsNonNullableValueType(ITypeSymbol type, INamedTypeSymbol systemNullableType)
+ {
+ return !type.IsReferenceType && !(type.OriginalDefinition.Equals(systemNullableType));
+ }
+
+ internal readonly struct SymbolCache
+ {
+ public SymbolCache(Compilation compilation)
+ {
+ RequiredAttribute = compilation.GetTypeByMetadataName(SymbolNames.RequiredAttribute);
+ BindRequiredAttribute = compilation.GetTypeByMetadataName(SymbolNames.BindRequiredAttribute);
+ SystemNullableType = compilation.GetSpecialType(SpecialType.System_Nullable_T);
+ }
+
+ public ITypeSymbol BindRequiredAttribute { get; }
+ public INamedTypeSymbol SystemNullableType { get; }
+ public INamedTypeSymbol RequiredAttribute { get; }
+ }
+ }
+}

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 = "System.ComponentModel.DataAnnotations.RequiredAttribute";
+ public const string BindRequiredAttribute = "Microsoft.AspNetCore.Mvc.ModelBinding.BindRequiredAttribute";
+ public const string NullableType = "System.Nullable<T>";
  }
 }

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

@@ -0,0 +1,126 @@
+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 TopLevelParameterRequiredAttributeAnalyzer : DiagnosticAnalyzer
+ {
+ public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(
+ DiagnosticDescriptors.MVC1005_AttributeAvoidUsingRequiredAndBindRequired);
+
+ 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 (IsEnumerable(parameter.Type, symbolCache.IEnumerableInterface))
+ {
+ if (IsProblematicParameter(parameter, symbolCache.RequiredAttribute))
+ {
+ var location = parameter.Locations.Length != 0 ?
+ parameter.Locations[0] :
+ Location.None;
+
+ symbolAnalysisContext.ReportDiagnostic(
+ Diagnostic.Create(
+ DiagnosticDescriptors.MVC1005_AttributeAvoidUsingRequiredAndBindRequired,
+ location,
+ "RequiredAttribute","MinLength(1)"));
+ }
+ }
+ }
+
+
+ }, SymbolKind.Method);
+ }
+
+ public static bool IsEnumerable(ITypeSymbol source, INamedTypeSymbol iEnumerableInterface)
+ {
+ if (source.Kind == SymbolKind.ArrayType)
+ {
+ return true;
+ }
+ foreach (var @interface in source.AllInterfaces)
+ {
+ if (@interface == iEnumerableInterface)
+ {
+ return true;
+ }
+ }
+
+ return false;
+ }
+
+ private bool IsProblematicParameter(IParameterSymbol parameter, INamedTypeSymbol requiredAttribute)
+ {
+ return parameter.GetAttributes(requiredAttribute).Any();
+ }
+
+ internal readonly struct SymbolCache
+ {
+ public SymbolCache(Compilation compilation)
+ {
+ ControllerAttribute = compilation.GetTypeByMetadataName(SymbolNames.ControllerAttribute);
+ NonControllerAttribute = compilation.GetTypeByMetadataName(SymbolNames.NonControllerAttribute);
+ NonActionAttribute = compilation.GetTypeByMetadataName(SymbolNames.NonActionAttribute);
+
+ IEnumerableInterface = compilation.GetSpecialType(SpecialType.System_Collections_IEnumerable);
+ RequiredAttribute = compilation.GetTypeByMetadataName(SymbolNames.RequiredAttribute);
+
+ 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 INamedTypeSymbol IEnumerableInterface { get; }
+ public INamedTypeSymbol RequiredAttribute { get; }
+ public IMethodSymbol IDisposableDispose { get; }
+ }
+ }
+}

test/Mvc.Analyzers.Test/ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzerTest.cs

@@ -0,0 +1,43 @@
+using System.Runtime.CompilerServices;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Analyzer.Testing;
+using Xunit;
+
+namespace Microsoft.AspNetCore.Mvc.Analyzers
+{
+ public class ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzerTest
+ {
+ private MvcDiagnosticAnalyzerRunner Runner { get; } = new MvcDiagnosticAnalyzerRunner(new ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzer());
+
+ [Fact]
+ public Task DiagnosticsAreReturned_ForNonNullableTypeProperty() 
+ => RunTest(new[] { "RequiredAttribute", "BindRequiredAttribute" });
+
+ [Fact]
+ public Task DiagnosticsAreReturned_ForComplexTypeProperty()
+ => RunTest(new[] {"BindRequiredAttribute", "RequiredAttribute" });
+
+ private async Task RunTest(string[] attributes,[CallerMemberName] string testMethod = "")
+ {
+ // Arrange
+ var descriptor = DiagnosticDescriptors.MVC1005_AttributeAvoidUsingRequiredAndBindRequired;
+ 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(), attributes[0], attributes[1]), diagnostic.GetMessage());
+ });
+ }
+
+ }
+}

test/Mvc.Analyzers.Test/TestFiles/ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzerTest/DiagnosticsAreReturned_ForComplexTypeProperty.cs

@@ -0,0 +1,16 @@
+using System;
+using System.Collections.Generic;
+using System.Text;
+using Microsoft.AspNetCore.Mvc.ModelBinding;
+
+namespace Microsoft.AspNetCore.Mvc.Analyzers.TestFiles.ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzerTest
+{
+ public class DiagnosticsAreReturned_ForComplexTypeProperty
+ {
+ [BindRequired]
+ public DiagnosticsAreReturned_ForComplexTypeProperty /*MM*/MyProperty { get; set; }
+
+ [BindRequired]
+ public string name { get; set; }
+ }
+}

test/Mvc.Analyzers.Test/TestFiles/ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzerTest/DiagnosticsAreReturned_ForNonNullableTypeProperty.cs

@@ -0,0 +1,20 @@
+using System;
+using System.Collections.Generic;
+using System.ComponentModel.DataAnnotations;
+using System.Text;
+
+namespace Microsoft.AspNetCore.Mvc.Analyzers.TestFiles.ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzerTest
+{
+ public class DiagnosticsAreReturned_ForNonNullableTypeProperty
+ {
+ [Required]
+ public int /*MM*/id { get; set; }
+
+ protected static int static_x { get; set; }
+
+ protected int x { get; set; }
+
+ [Required]
+ public int? y { get; set; }
+ }
+}

test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterRequiredAttributeAnalyzerTest/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/TopLevelParameterRequiredAttributeAnalyzerTest/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/TopLevelParameterRequiredAttributeAnalyzerTest/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/TopLevelParameterRequiredAttributeAnalyzerTest/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;
+ }
+}