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

kishanAnem · 2018-08-18T06:32:14Z

#7415

#7415 this is only about Action parameter in next commit will work on Properties. Please let me know, is this is fine?
Thanks @pranavkm, used your framework a lot.

@dougbu

…n top-level parameters #7415 Hi @dougbu #7415 this is only about Action parameter in next commit will work on Properties. Thanks @pranavkm, used your framework a lot.

dougbu

@kishanAnem are you planning to implement CodeFixProviders as well?

dougbu · 2018-08-18T19:06:19Z

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

+ public static readonly DiagnosticDescriptor MVC1005_ParameterAttributeAvoidUsingRequiredOncollections =
+ new DiagnosticDescriptor(
+ "MVC1005",
+ "Use of 'RequiredAttribute' should be avoided on collection",


change RequiredAttribute to {0}

"collections"

end the sentence with a period

dougbu · 2018-08-18T19:37:38Z

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

+
+ private bool IsProblematicParameter(IParameterSymbol parameter)
+ {
+ return parameter.GetAttributes().Any(Attribute => Equals(Attribute.AttributeClass.MetadataName, SymbolNames.RequiredAttribute));


attribute (lowercase first character)

dougbu · 2018-08-18T20:00:11Z

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

+
+ public const string RequiredAttribute = "RequiredAttribute";
+
+ public const string CollectionType= "SourceComplexParameterSymbol";


Doesn't this symbol both exclude SourceComplexParameterSymbolWithCustomModifiers and indicate only that the parameter is not simple e.g. that it has associated attributes? That is, what about this type indicates a collection?

@pranavkm

I don't think you're using this anymore.

dougbu · 2018-08-18T20:38:55Z

...gnosticsAreReturned_ForControllerActionsWithParametersWithRequiredAttributeForIEnumerable.cs

+{
+ public class DiagnosticsAreReturned_ForControllerActionsWithParametersWithRequiredAttributeForIEnumerable : Controller
+ {
+ public IActionResult Index([Required]IEnumerable<int> /*MM*/collection) => null;


IEnumerable<T> does not inherit from ICollection. So, I suspect the analyzer would also complain about a [Required] int nonCollection parameter.

So, I suspect the analyzer would also complain about a [Required] int nonCollection parameter.

Yes, you're right @dougbu will fix this.

dougbu · 2018-08-18T20:43:24Z

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

+ new DiagnosticDescriptor(
+ "MVC1005",
+ "Use of 'RequiredAttribute' should be avoided on collection",
+ "Use of '{0}' should be avoided. This always made model validation is true. Consider using [MinLength(1)].",


This advice works only for arrays in .NET Framework projects. Even in .NET Core, this advice doesn't work for IEnumerable<T> but IEnumerable<T> has the same issues w/ [Required]. Probably need an additional diagnostic without this advice for the excluded cases.

…pn top-level parameters #7415 #7415 I fixed all.

kishanAnem · 2018-08-19T07:27:55Z

@kishanAnem are you planning to implement CodeFixProviders as well?

@dougbu Yes, I started working on it.

dougbu

@kishanAnem my earlier question about CodeFixProviders was just a question. We can file issues for fixes later. #7415 alone doesn't require them though they would be nice to have.

Initial priority is covering #7415 in full, especially covering all top-level models and providing correct advice. Well, the "might be worthwhile" point is optional.

dougbu · 2018-08-19T21:10:33Z

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

+
+ public const string RequiredAttribute = "RequiredAttribute";
+
+ public const string CollectionType= "SourceComplexParameterSymbol";


I don't think you're using this anymore.

dougbu · 2018-08-19T21:25:49Z

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

+namespace Microsoft.AspNetCore.Mvc.Analyzers
+{
+ [DiagnosticAnalyzer(LanguageNames.CSharp)]
+ public class TopLevelParameterAttributeAnalyzer : DiagnosticAnalyzer


Are you planning a separate analyzer or analyzer pair for [Required] on complex types? #7415 covers that case too.

Yes I'm planning seperate analyzer for that.

We'll writer analyzer as well in this issue. If it require please update issue.

@kishanAnem #7415 is already clear complex types must be covered. See https://github.com/aspnet/Mvc/issues/7415#issuecomment-369413264

Sorry, i mean codefixer.

Not doing fixers is fine for this PR. As I said earlier,

Initial priority is covering #7415 in full, especially covering all top-level models and providing correct advice. Well, the "might be worthwhile" point is optional.

Could we call this RequiredAttributeAnalyzer? There's already an analyzer by the same name in the Api.Analyzers package

dougbu · 2018-08-19T21:28:20Z

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

 return false;
 }

+ public static bool IsCollection(this ITypeSymbol source)


IsEnumerable would be a better name. May also need IsCollection and IsArray to distinguish the cases where current MVC1005_ParameterAttributeAvoidUsingRequiredOncollections advice is actionable under .NET Core and .NET Framework (respectively).

Could we move this method to the analyzer? The methods on this type are meant to be more general purpose.

pranavkm · 2018-08-20T17:42:34Z

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

 return false;
 }

+ public static bool IsCollection(this ITypeSymbol source)


Could we move this method to the analyzer? The methods on this type are meant to be more general purpose.

pranavkm · 2018-08-20T17:47:38Z

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

+
+ foreach (var @interface in source.AllInterfaces)
+ {
+ if (@interface.MetadataName == "IEnumerable")


I'd recommend checking the actual type @interface == symbolCache.IEnumerable. You can get to IEnumerable using
Compilation.GetSpecialType(SpecialType.System_Collections_IEnumerable)

pranavkm · 2018-08-20T17:49:00Z

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


 public const string RenderPartialMethod = "RenderPartial";
+
+ public const string RequiredAttribute = "RequiredAttribute";


Fully qualify this and use that to look up the type.

Will change

pranavkm · 2018-08-20T17:49:55Z

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

+namespace Microsoft.AspNetCore.Mvc.Analyzers
+{
+ [DiagnosticAnalyzer(LanguageNames.CSharp)]
+ public class TopLevelParameterAttributeAnalyzer : DiagnosticAnalyzer


Could we call this RequiredAttributeAnalyzer? There's already an analyzer by the same name in the Api.Analyzers package

pranavkm · 2018-08-20T17:50:59Z

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

+ var location = parameter.Locations.Length != 0 ?
+ parameter.Locations[0] :
+ Location.None;
+ var descriptor = DiagnosticDescriptors.MVC1005_ParameterAttributeAvoidUsingRequiredOncollections;


Yes. I will remove that.

pranavkm · 2018-08-20T17:51:25Z

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

+
+ private bool IsProblematicParameter(IParameterSymbol parameter)
+ {
+ return parameter.GetAttributes().Any(attribute => Equals(attribute.AttributeClass.MetadataName, SymbolNames.RequiredAttribute));


parameter.GetAttributes(symbolCache.RequiredAttribute).Any()

pranavkm · 2018-08-20T17:51:41Z

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

 </PropertyGroup>

 <ItemGroup>
+ <Compile Remove="TestFiles\TopLevelParameterAttributeAnalyzerTest\DiagnosticsAreReturned_ForControllerActionsWithParametersWithOutRequiredAttributeForIEnumerable.cs" />


Undo changes to the file

kishanAnem · 2018-08-20T22:24:08Z

sorry, I did accidently.

@dougbu

…n top-level parameters #7415 hi @dougbu @pranavkm #7415 I made changes and added new Analyzer for properties.

dougbu

More comments though I believe you're not done yet. Please let us know when you're code complete.

dougbu · 2018-08-22T18:27:26Z

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

+ 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}'.",


"made" -> "makes"

dougbu · 2018-08-22T18:39:36Z

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

 return false;
 }

+


Why is this file changing at all?

should we keep this method here?

What method? I see only two new blank lines in this file.

dougbu · 2018-08-22T18:41:40Z

...osoft.AspNetCore.Mvc.Analyzers/ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzer.cs

+ var property = (IPropertySymbol)symbolAnalysisContext.Symbol;
+
+ if (property.DeclaredAccessibility != Accessibility.Public ||
+ property.IsStatic )


Odd indentation here and in a few other places. Please avoid over-indentation because it is just extra work to maintain.

(In this particular case, the whole condition fits easily on one line.)

dougbu · 2018-08-22T18:42:44Z

...osoft.AspNetCore.Mvc.Analyzers/ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzer.cs

+
+ if (IsNonNullableValueType(property.Type, typeCache.SystemNullableType) && property.HasAttribute(typeCache.RequiredAttribute, inherit: false))
+ {
+ {


Why is this statement wrapped in an additional block?

dougbu · 2018-08-22T18:50:53Z

...osoft.AspNetCore.Mvc.Analyzers/ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzer.cs

+
+ private void InitializeWorker(CompilationStartAnalysisContext compilationStartAnalysisContext)
+ {
+ compilationStartAnalysisContext.RegisterSymbolAction(symbolAnalysisContext =>


While #7415 mentions users continue to confuse [BindRequired] and [Required] on int and other value types, let's focus on top-level properties (and parameters) in this PR.

dougbu · 2018-08-22T18:58:54Z

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

+ context.RegisterCompilationStartAction(compilationStartAnalysisContext =>
+ {
+ var typeCache = new SymbolCache(compilationStartAnalysisContext.Compilation);
+ if (typeCache.ControllerAttribute == null || typeCache.ControllerAttribute.TypeKind == TypeKind.Error)


I may be missing something. Why doesn't this case exist in ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzer.Initialize?

Here we're checking type is the Controller, in my understanding whether it is controller or Model on properties we should advice.

Here we're checking type is the Controller

As far as I know, this checks the SymbolCache, not the specific symbol. My question was why this check doesn't exist in ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzer because that also has a SymbolCache.

whether it is controller or Model on properties we should advice.

No, #7415 is specifically about top-level parameters and properties. Specifically

model-bound properties in a controller

parameters to an action

model-bound properties in a page model

parameters to a page handler

Again, ignore my optional "might be worthwhile warning" point in #7415 at least until the core work is done.

then it should has this there as well. i will add.

dougbu · 2018-08-22T19:03:47Z

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

+ {
+
+ var method = (IMethodSymbol)symbolAnalysisContext.Symbol;
+ if (method.MethodKind != MethodKind.Ordinary)


Why is MethodKind.LambdaMethod unsupported? Customers often write actions as lambda methods e.g.

public IActionResult<int> First([Required] int[] numbers) => numbers.FirstOrDefault();

add this case as well in the next commit, with current implementation this test case got success. I didn't find IActionResult<int>, got ActionResult<int>.

Please correct If am wrong, up to my understanding following both are same .

public ActionResult<int> First([Required] int[] numbers) => numbers.FirstOrDefault();

and

public ActionResult<int> First([Required] int[] numbers) { return numbers.FirstOrDefault(); }

And both are MethodKind.Ordinary

I didn't find IActionResult<int>, got ActionResult<int>

Yes, that was a typo.

both are MethodKind.Ordinary

Please test this assumption. I'm not sure.

dougbu · 2018-08-22T19:06:33Z

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

+ Diagnostic.Create(
+ DiagnosticDescriptors.MVC1005_AttributeAvoidUsingRequiredAndBindRequired,
+ location,
+ "RequiredAttribute","MinLength(1)"));


MinLength(1)will only work in a subset of cases where a problem is detected i.e. arrays in the .NET Framework case and ICollection implementations in .NET Core. Should avoid giving incorrect advice otherwise.

dougbu · 2018-08-22T19:13:44Z

...osoft.AspNetCore.Mvc.Analyzers/ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzer.cs

+namespace Microsoft.AspNetCore.Mvc.Analyzers
+{
+ [DiagnosticAnalyzer(LanguageNames.CSharp)]
+ public class ModelPropertyRequiredAttribueBindRequiredAttributeAnalyzer : DiagnosticAnalyzer


Suggest creating a small base or helpers class that inspects an individual top-level parameter or property symbol. This would reduce the duplication between TopLevelParameterRequiredAttributeAnalyzer and TopLevelPropertyRequiredAttributeAnalyzer. (Yes, I suggest renaming this analyzer and narrowing its behaviour to match TopLevelParameterRequiredAttributeAnalyzer.)

As a smaller detail, Attribue -> Attribute.

dougbu · 2018-08-22T19:22:52Z

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

+ {
+ var parameter = method.Parameters[i];
+
+ if (IsEnumerable(parameter.Type, symbolCache.IEnumerableInterface))


Need to warn about [Required] on non-Nullable<T> value types and on complex types too.

Separately, MVC treats all types with conversions from string as "simple" types. Treating all classes as "complex" may result in false warnings. And, user-defined value types will not necessarily have conversions from string -- leading to missed warnings. @pranavkm does Roslyn support anything similar to the following (in ModelMetadata?

Mvc/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs

Line 447 in a67d936

IsComplexType = !TypeDescriptor.GetConverter(ModelType).CanConvertFrom(typeof(string));

dougbu · 2018-09-02T04:47:33Z

@kishanAnem this branch has gotten stale and needs to be rebased on the current release/2.2.

More generally, what are your thoughts, given this PR has grown larger and more complicated than you likely expected? Might frustrate you less for us to take over work on this issue and for you (if you want) to pick up an up-for-grabs issue.

kishanAnem · 2018-09-02T05:28:33Z

Please take over work on this issue

Create an Analyzer to warn customers to not user Required attribute o…

5899cab

…n top-level parameters #7415 Hi @dougbu #7415 this is only about Action parameter in next commit will work on Properties. Thanks @pranavkm, used your framework a lot.

dougbu reviewed Aug 18, 2018

View reviewed changes

Create an Analyzer to warn customers to not user Required attribute o…

ad2bcd7

…pn top-level parameters #7415 #7415 I fixed all.

dougbu reviewed Aug 19, 2018

View reviewed changes

pranavkm reviewed Aug 20, 2018

View reviewed changes

kishanAnem closed this Aug 20, 2018

kishanAnem reopened this Aug 20, 2018

Create an Analyzer to warn customers to not user Required attribute o…

8730e6c

…n top-level parameters #7415 hi @dougbu @pranavkm #7415 I made changes and added new Analyzer for properties.

dougbu reviewed Aug 22, 2018

View reviewed changes

kishanAnem closed this Sep 2, 2018


		public const string RequiredAttribute = "RequiredAttribute";

		public const string CollectionType= "SourceComplexParameterSymbol";


		public const string RenderPartialMethod = "RenderPartial";

		public const string RequiredAttribute = "RequiredAttribute";

Uh oh!

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

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

Uh oh!

Conversation

kishanAnem commented Aug 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishanAnem Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishanAnem commented Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishanAnem commented Aug 20, 2018

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Sep 2, 2018

kishanAnem commented Sep 2, 2018

Labels

3 participants

kishanAnem commented Aug 18, 2018 •

edited

Loading

kishanAnem Aug 19, 2018 •

edited

Loading

kishanAnem commented Aug 19, 2018 •

edited

Loading