Do not return an top lever parameter analyzer warning for some types (#13496)

* Do not return an top lever parameter analyzer warning for some types

Fixes #6945

Author: pranavkm

src/Mvc/Mvc.Analyzers/src/DiagnosticDescriptors.cs

@@ -47,7 +47,8 @@ public static class DiagnosticDescriptors
             new DiagnosticDescriptor(
                 "MVC1004",
                 "Rename model bound parameter.",
-                "Property on type '{0}' has the same name as parameter '{1}'. This may result in incorrect model binding. Consider renaming the parameter or using a model binding attribute to override the name.",
+                "Property on type '{0}' has the same name as parameter '{1}'. This may result in incorrect model binding. " +
+                "Consider renaming the parameter or the property to avoid conflicts. If the type '{0}' has a custom type converter or custom model binder, you can suppress this message.",
                 "Naming",
                 DiagnosticSeverity.Warning,
                 isEnabledByDefault: true,

src/Mvc/Mvc.Analyzers/src/TopLevelParameterNameAnalyzer.cs

@@ -89,12 +89,17 @@ internal static bool IsProblematicParameter(in SymbolCache symbolCache, IParamet
                 return false;
             }
 
-            if (SpecifiesModelType(symbolCache, parameter))
+            if (SpecifiesModelType(in symbolCache, parameter))
             {
                 // Ignore parameters that specify a model type.
                 return false;
             }
 
+            if (!IsComplexType(parameter.Type))
+            {
+                return false;
+            }
+
             var parameterName = GetName(symbolCache, parameter);
 
             var type = parameter.Type;
@@ -122,6 +127,26 @@ internal static bool IsProblematicParameter(in SymbolCache symbolCache, IParamet
             return false;
         }
 
+        private static bool IsComplexType(ITypeSymbol type)
+        {
+            // This analyzer should not apply to simple types. In MVC, a simple type is any type that has a type converter that returns true for TypeConverter.CanConvertFrom(typeof(string)).
+            // Unfortunately there isn't a Roslyn way of determining if a TypeConverter exists for a given symbol or if the converter allows string conversions.
+            // https://github.com/dotnet/corefx/blob/v3.0.0-preview8.19405.3/src/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs#L103-L141
+            // provides a list of types that have built-in converters.
+            // We'll use a simpler heuristic in the analyzer: A type is simple if it's a value type or if it's in the "System.*" namespace hierarchy.
+
+            var @namespace = type.ContainingNamespace?.ToString();
+            if (@namespace != null)
+            {
+                // Things in the System.* namespace hierarchy don't count as complex types. This workarounds
+                // the problem of discovering type converters on types in mscorlib.
+                return @namespace != "System" &&
+                    !@namespace.StartsWith("System.", StringComparison.Ordinal);
+            }
+
+            return true;
+        }
+
         internal static string GetName(in SymbolCache symbolCache, ISymbol symbol)
         {
             foreach (var attribute in symbol.GetAttributes(symbolCache.IModelNameProvider))

src/Mvc/Mvc.Analyzers/test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForSimpleTypes.cs

@@ -0,0 +1,9 @@
+using System;
+
+namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles
+{
+    public class IsProblematicParameter_ReturnsFalse_ForSimpleTypes
+    {
+        public void ActionMethod(DateTime date, DateTime? day, Uri absoluteUri, Version majorRevision, DayOfWeek sunday) { }
+    }
+}

src/Mvc/Mvc.Analyzers/test/TopLevelParameterNameAnalyzerTest.cs

@@ -95,6 +95,30 @@ public async Task IsProblematicParameter_ReturnsFalse_ForParametersWithCustomMod
             Assert.False(result);
         }
 
+        // Test for https://github.com/aspnet/AspNetCore/issues/6945
+        [Fact]
+        public async Task IsProblematicParameter_ReturnsFalse_ForSimpleTypes()
+        {
+            var testName = nameof(IsProblematicParameter_ReturnsFalse_ForSimpleTypes);
+            var testSource = MvcTestSource.Read(GetType().Name, testName);
+            var project = DiagnosticProject.Create(GetType().Assembly, new[] { testSource.Source });
+
+            var compilation = await project.GetCompilationAsync();
+
+            var modelType = compilation.GetTypeByMetadataName($"Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles.{testName}");
+            var method = (IMethodSymbol)modelType.GetMembers("ActionMethod").First();
+
+            Assert.True(TopLevelParameterNameAnalyzer.SymbolCache.TryCreate(compilation, out var symbolCache));
+
+            Assert.Collection(
+                method.Parameters,
+                p => Assert.False(TopLevelParameterNameAnalyzer.IsProblematicParameter(symbolCache, p)),
+                p => Assert.False(TopLevelParameterNameAnalyzer.IsProblematicParameter(symbolCache, p)),
+                p => Assert.False(TopLevelParameterNameAnalyzer.IsProblematicParameter(symbolCache, p)),
+                p => Assert.False(TopLevelParameterNameAnalyzer.IsProblematicParameter(symbolCache, p)),
+                p => Assert.False(TopLevelParameterNameAnalyzer.IsProblematicParameter(symbolCache, p)));
+        }
+
         [Fact]
         public async Task IsProblematicParameter_IgnoresStaticProperties()
         {