New rule proposal: use nameof instead of magic strings to refer to parameters

[AArnott]

With C# 6 using nameof(fooparam) instead of "fooparam" is preferred because it allows the language service to help keep it up to date in refactorings. A StyleCop rule that finds these magic strings that match parameter names and offers an auto-fix would be awesome.

This needs some discussion for how to best detect when a string whose value matches a parameter name should be considered an intended match rather than just a coincidentally same value.

[sharwell]

I think this is a great diagnostic, but I would prefer to see it placed in a package of "migration analyzers" instead of this project (see DotNetAnalyzers/Proposals#19). For this project, I would prefer to see the following two rules:

1. The paramName should be specified when instantiating an ArgumentException (or derived).
2. The value passed for paramName should match the name of an in-scope argument.

These rules are not limited to C# 6, and validate the two aspects of ArgumentException which tend to be violated.

[AArnott]

Thanks for reviewing.

I think this analyzer is better than just for migration. Folks who have already migrated still need to ensure that old habits or new developers don't regress into using magic strings again.

ArgumentException(string) doesn't take the parameter name. It takes the exception message. Another overload adds the parameter name, but it can be appropriate to call the single string overload when the argument validation failed due to a combination of arguments that are incompatible (and thus no single argument can be blamed).
But a StyleCop rule that checks that the single arg overload is not passed the parameter name would be very useful because folks often accidentally use throw new ArgumentException("argname") which produces bad results in the Exception message.

[sharwell]

If the migration analyzer I described previously were available and I were working on a C# 6 project, I would likely install it and set that particular diagnostic to severity Error. I see it as very similar to the way this project already uses AsyncUsageAnalyzers to prevent regressions.

I could likely be convinced to change the list above to the following:

1. Argument exceptions should be instantiated with a message and/or parameter name.
   • I'll leave the details regarding the use of a parameter name as the message for the discussion if/when this rule is proposed.
2. If a parameter name is specified, it should match the name of an in-scope parameter.

I definitely don't see this project as the best place for the nameof analyzer, at least at this time. The primary reason for this is it would be the first rule that is incompatible with C# prior to version 6, and I don't believe the majority of StyleCop users are ready to migrate their code to C# 6. (Yes, I know the rule can be disabled.) The second point is it would inevitably lead to duplicated effort and/or code between this project and the C# 6 migration analyzers project.

[AArnott]

Fair enough. :)

[otac0n]

@sharwell I was just about to request this rule as well. Is it possible to detect the language version in the rule, and just NO-OP? I would vote to add this directly to StyleCop, if at all possible. I don't see this as analogous to AsyncUsageAnalyzers, because this is style guidance, not usage guidance.

[dlemstra]

@sharwell and @AArnott I would love to get a rule that checks if an ArgumentException is using the correct name and also make sure that an ArgumentException always includes a message. This might break some code after an upgrade but I personally think that not using a message and the name of the argument is abuse of the ArgumentException class. I have seen a lot of code where an InvalidOperationException should be thrown instead of an ArgumentException. But maybe there should be an SX version of the rule to make it easy to disable this? Would either of you be willing to create a pull request that describes this rule so we can continue the design of the rule there? I could help out with coding the rule.

[JohanLarsson]

There are analyzers for this here and here. Should be a built in VS analyzer imo.