#20630 Cast is redundant on attribute with params fix #20743

MaStr11 · 2017-07-10T12:18:12Z

Customer scenario

False positive on IDE0004 "Cast is redundant" analyzer

Bugs this fixes:

Fixes #20630

Workarounds, if any

Disable IDE0004

Risk

Code fix changes behavior.

Performance impact

Low.

Is this a regression from a previous update?

Variation of: #18978
Related: #20741 #20742

Root cause analysis:

Analyzer misses test for prams in attribute constructor calls.

How was the bug found?

Customer reported

Pilchie · 2017-07-16T18:36:28Z

src/Workspaces/CSharp/Portable/Extensions/CastExpressionSyntaxExtensions.cs

-                    {
-                        return false;
-                    }
+        private static bool CheckConversionOfParamToParamArray(IParameterSymbol parameter, ITypeSymbol castType, SemanticModel semanticModel)


Check what? Is there a name that describes what is true if this method returns true?

Maybe ParameterTypeMatchesParamsElementType or something?

Pilchie · 2017-07-16T18:36:42Z

src/Workspaces/CSharp/Portable/Extensions/CastExpressionSyntaxExtensions.cs

-                    }
+        private static bool CheckConversionOfParamToParamArray(IParameterSymbol parameter, ITypeSymbol castType, SemanticModel semanticModel)
+        {
+            if (parameter != null && parameter.IsParams)


Could just use parameter?.IsParams

Pilchie · 2017-07-16T18:37:54Z

src/Workspaces/CSharp/Portable/Extensions/CastExpressionSyntaxExtensions.cs

-                    {
-                        return true;
-                    }
+                var parameterType = (IArrayTypeSymbol)parameter.Type;


Is this correct in broken code? What if I type a method that says M(params int something) Does the symbol still report IsParams?

I see this is just moved code, so I assume so, but worth double checking.

Pilchie · 2017-07-16T18:40:18Z

retest windows_debug_vs-integration_prtest please

Pilchie · 2017-07-16T18:40:28Z

retest windows_release_vs-integration_prtest please

Pilchie · 2017-07-16T18:40:36Z

Test results were gone :(

Pilchie · 2017-07-16T18:40:45Z

@dotnet/roslyn-ide can we get a second review here?

… always an array and added test for erroneous method definition.

MaStr11 · 2017-07-17T09:12:44Z

@Pilchie

Is this correct in broken code? What if I type a method that says M(params int something) Does the symbol still report IsParams?

This wasn't covered before by tests. I added tests for method calls and for attributes and it turned out, that both were failing with an InvalidCastException. I don't know what is the correct behavior in such a case but I decided to assume that in such cases we return true to cancel the analyzer. Unfortunately in this case the return true does not reflect what the method name ParameterTypeMatchesParamsElementType suggests.

Pilchie · 2017-07-17T16:00:09Z

Thanks! This looks good to me. Anyone else from @dotnet/roslyn-ide want to review?

Pilchie · 2017-07-25T00:17:16Z

Ping @dotnet/roslyn-ide for a second review.

sharwell · 2017-08-11T22:10:32Z

@MaStr11 Were you closing this because we were slow or because it was fixed in some other manner?

MaStr11 · 2017-08-11T22:35:08Z

Closing the issue was a mistake on my side. I don't know how this could happen. I'm sorry for the inconvenience.

sharwell · 2017-08-12T14:52:02Z

@MaStr11 No problem! Just wanted to make sure I didn't miss something.

jinujoseph · 2018-03-08T07:48:44Z

test windows_release_vs-integration_prtest

jinujoseph · 2018-03-27T21:25:10Z

test windows_coreclr_debug_prtest

sharwell · 2018-03-08T21:22:49Z

src/Workspaces/CSharp/Portable/Extensions/CastExpressionSyntaxExtensions.cs

-                    Debug.Assert(parameter.Type is IArrayTypeSymbol);
+                    // We don't check the position of the argument because in attributes it is allowed that 
+                    // params parameter are positioned in between if named arguments are used.
+                    // The *single* argument check above is also broken: https://github.com/dotnet/roslyn/issues/20742


❓ Can you not just check that the next AttributeArgumentSyntax has a null NameEquals property?

At this position we need to find the parameter that fits to the argument (DetermineParameter). We pass this information to ParameterTypeMatchesParamsElementType which is only interested in params parameters. We could avoid the call to DetermineParameter if we are sure that looking at the attributeArgumentList and the attributeArgument in it we can be sure that it

can't be a params parameter or

if it might be a params parameter the argument would be a single element array passed to the parameter.

I don't think it is possible to determine either of that conditions by only looking at the attributeArgumentList and the attributeArgument.

I was specifically referring to how this:

If there are any arguments to the right, we can assume that this is not a single argument passed to a params parameter.

Relates to this:

The single argument check above is also broken

It seems that you could identify a single argument as an argument meeting both of the following conditions:

The argument appears first in the list

The next argument, if any, has a non-null NameEquals property, indicating that it sets a named property instead of a parameter

➡️ Filed #25790 as a follow-up

The single argument comment is misleading. It's really asking for a single argument passed to a params parameter.

jinujoseph · 2018-03-27T21:27:51Z

@Pilchie for ask mode approval for 15.7 Preview 4

mavasani · 2018-03-27T21:33:32Z

src/Workspaces/CSharp/Portable/Extensions/AttributeArgumentSyntaxExtensions.cs

+        /// is true, the last parameter will be returned if it is params parameter and the index of
+        /// the specified argument is greater than the number of parameters.
+        /// </summary>
+        public static IParameterSymbol DetermineParameter(


@AlekseyTs @333fred I can't seem to recall: does GetOperation(AttributeArgumentSyntax) return an IArgumentOperation? If so, we can replace this method with IArgumentOperation.Parameter. If not, it would be good to file a tracking issue.

It doesn't look like we have any tests in this area. We should give it a try and see if it works.

This method is a copy of ArgumentSyntaxExtensions.DetermineParameter. If we change the logic here, we should also change it there.

It doesn't look like we have any tests in this area. We should give it a try and see if it works.

That's a confusing statement. I really don't know what I'm supposed to do here, especially given that this PR got approved to merge already.

CyrusNajmabadi · 2018-03-27T21:43:20Z

src/Workspaces/CSharp/Portable/Extensions/AttributeArgumentSyntaxExtensions.cs

+                return parameters.FirstOrDefault(p => p.Name == name);
+            }
+
+            // Handle positional argument


what if it is a "Name =" argument? That would not map to a parameter at all, regardless of position.

That's right. I addressed this in 514e4fd:

Added a test for a cast in a named argument

added a section explaining the handling of named arguments

Added an return at the top of the method for named arguments.

Pilchie · 2018-03-28T02:12:30Z

Approved - thanks @MaStr11 and sorry this took so long to get in. It's embarrassing! 😊

…arguments. Added test for cast in named argument.

MaStr11 added 2 commits July 10, 2017 13:26

Fix for dotnet#20630

4559dcb

Merged PR dotnet#20596

1de9f5a

dnfclas added the cla-already-signed label Jul 10, 2017

MaStr11 and others added 2 commits July 10, 2017 14:35

Try resolve merge conflict in RemoveUnnecessaryCastTests

a58f6ce

Merge branch 'master' into CastIsRedundantOnAttributeWithParamsFix

7f845f0

sharwell requested a review from a team July 10, 2017 22:39

Pilchie added the Area-IDE label Jul 12, 2017

Pilchie added this to the 15.5 milestone Jul 12, 2017

Pilchie added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jul 12, 2017

Pilchie approved these changes Jul 16, 2017

View reviewed changes

Pilchie requested a review from a team July 16, 2017 18:39

MaStr11 and others added 2 commits July 17, 2017 10:43

Merge branch 'master' into CastIsRedundantOnAttributeWithParamsFix

f3fe0ec

Incorporated code review. Changed assumption that params parameter is…

4c9a9f5

… always an array and added test for erroneous method definition.

MaStr11 closed this Aug 11, 2017

MaStr11 reopened this Aug 11, 2017

dnfclas added the cla-already-signed label Aug 11, 2017

sharwell self-assigned this Aug 31, 2017

sharwell requested a review from chborl October 5, 2017 15:29

Merge branch 'master' into CastIsRedundantOnAttributeWithParamsFix

1d7e0ce

Fixed error introduced by merge of master.

8d0e387

sharwell modified the milestones: 15.5, 15.7, 15.6 Oct 30, 2017

sharwell changed the base branch from master to post-dev15.5-contrib November 1, 2017 12:30

shyamnamboodiripad changed the base branch from post-dev15.5-contrib to master November 17, 2017 20:17

sharwell changed the base branch from master to dev15.6.x January 23, 2018 02:04

sharwell removed the request for review from chborl January 23, 2018 02:04

sharwell changed the base branch from dev15.6.x to dev15.7.x January 23, 2018 02:06

sharwell modified the milestones: 15.6, 15.7 Jan 23, 2018

jinujoseph approved these changes Mar 27, 2018

View reviewed changes

sharwell reviewed Mar 27, 2018

View reviewed changes

mavasani reviewed Mar 27, 2018

View reviewed changes

CyrusNajmabadi reviewed Mar 27, 2018

View reviewed changes

Pilchie added the Approved to merge label Mar 28, 2018

Added comments to DeterminePrameter to clarify the handling of named …

514e4fd

…arguments. Added test for cast in named argument.

sharwell mentioned this pull request Mar 28, 2018

Consider simplifying argument check for attributes in CastExpressionSyntaxExtensions #25790

Closed

sharwell merged commit 1804a31 into dotnet:dev15.7.x Mar 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#20630 Cast is redundant on attribute with params fix #20743

#20630 Cast is redundant on attribute with params fix #20743

MaStr11 commented Jul 10, 2017 •

edited by sharwell

Loading

Pilchie Jul 16, 2017

Pilchie Jul 16, 2017

Pilchie Jul 16, 2017

Pilchie Jul 16, 2017

Pilchie Jul 16, 2017

Pilchie commented Jul 16, 2017

Pilchie commented Jul 16, 2017

Pilchie commented Jul 16, 2017

Pilchie commented Jul 16, 2017

MaStr11 commented Jul 17, 2017

Pilchie commented Jul 17, 2017

Pilchie commented Jul 25, 2017

sharwell commented Aug 11, 2017

MaStr11 commented Aug 11, 2017

sharwell commented Aug 12, 2017

jinujoseph commented Mar 8, 2018

jinujoseph commented Mar 27, 2018

sharwell Mar 8, 2018

MaStr11 Mar 28, 2018

sharwell Mar 28, 2018

sharwell Mar 28, 2018

MaStr11 Mar 28, 2018

jinujoseph commented Mar 27, 2018

mavasani Mar 27, 2018

333fred Mar 27, 2018

MaStr11 Mar 28, 2018

CyrusNajmabadi Mar 27, 2018

MaStr11 Mar 28, 2018

CyrusNajmabadi Mar 28, 2018

Pilchie commented Mar 28, 2018

#20630 Cast is redundant on attribute with params fix #20743

#20630 Cast is redundant on attribute with params fix #20743

Conversation

MaStr11 commented Jul 10, 2017 • edited by sharwell Loading

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

Pilchie commented Jul 16, 2017

Pilchie commented Jul 16, 2017

Pilchie commented Jul 16, 2017

Pilchie commented Jul 16, 2017

MaStr11 commented Jul 17, 2017

Pilchie commented Jul 17, 2017

Pilchie commented Jul 25, 2017

sharwell commented Aug 11, 2017

MaStr11 commented Aug 11, 2017

sharwell commented Aug 12, 2017

jinujoseph commented Mar 8, 2018

jinujoseph commented Mar 27, 2018

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

jinujoseph commented Mar 27, 2018

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

Pilchie commented Mar 28, 2018

MaStr11 commented Jul 10, 2017 •

edited by sharwell

Loading