From 3c05f3a8af2c4a72b351b0bfe09329ef3ffe23f4 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Mon, 23 Sep 2019 11:17:45 -0700 Subject: [PATCH] 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 https://github.com/aspnet/AspNetCore/issues/6945 --- .../src/DiagnosticDescriptors.cs | 3 ++- .../src/TopLevelParameterNameAnalyzer.cs | 27 ++++++++++++++++++- ...icParameter_ReturnsFalse_ForSimpleTypes.cs | 9 +++++++ .../test/TopLevelParameterNameAnalyzerTest.cs | 24 +++++++++++++++++ 4 files changed, 61 insertions(+), 2 deletions(-) create mode 100644 src/Mvc/Mvc.Analyzers/test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForSimpleTypes.cs diff --git a/src/Mvc/Mvc.Analyzers/src/DiagnosticDescriptors.cs b/src/Mvc/Mvc.Analyzers/src/DiagnosticDescriptors.cs index 1b7eace720..76b068a928 100644 --- a/src/Mvc/Mvc.Analyzers/src/DiagnosticDescriptors.cs +++ b/src/Mvc/Mvc.Analyzers/src/DiagnosticDescriptors.cs @@ -47,7 +47,8 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers 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, diff --git a/src/Mvc/Mvc.Analyzers/src/TopLevelParameterNameAnalyzer.cs b/src/Mvc/Mvc.Analyzers/src/TopLevelParameterNameAnalyzer.cs index c20466bfcc..2d391f55c8 100644 --- a/src/Mvc/Mvc.Analyzers/src/TopLevelParameterNameAnalyzer.cs +++ b/src/Mvc/Mvc.Analyzers/src/TopLevelParameterNameAnalyzer.cs @@ -89,12 +89,17 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers 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 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers 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)) diff --git a/src/Mvc/Mvc.Analyzers/test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForSimpleTypes.cs b/src/Mvc/Mvc.Analyzers/test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForSimpleTypes.cs new file mode 100644 index 0000000000..50aded8121 --- /dev/null +++ b/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) { } + } +} diff --git a/src/Mvc/Mvc.Analyzers/test/TopLevelParameterNameAnalyzerTest.cs b/src/Mvc/Mvc.Analyzers/test/TopLevelParameterNameAnalyzerTest.cs index 361f6d3cb5..b13e1eac9e 100644 --- a/src/Mvc/Mvc.Analyzers/test/TopLevelParameterNameAnalyzerTest.cs +++ b/src/Mvc/Mvc.Analyzers/test/TopLevelParameterNameAnalyzerTest.cs @@ -95,6 +95,30 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers 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() {