runtime: Pass correct parameterName to ArgumentException
Find places where nameof(...) is passed as the first (rather than second) argument to ArgumentException. Also places where nameof is used as the argument name in an ArgumentException (or derived type) but where it’s not referring to a parameter.
Category: Reliability Usage
Examples to detect:
Single-argument nameof (or a literal string that matches the name of a parameter). Fixer: change to call the two argument constructor, pass null for the first parameter.
public void SomeMethod(string formatted)
{
if (!Helper.TryParse(arg, out Parsed parsed))
{
throw new ArgumentException(nameof(formatted));
}
}
Two argument call, first one looks like the parameter. Flip the argument order.
public void SomeMethod(string formatted)
{
if (!Helper.TryParse(arg, out Parsed parsed))
{
throw new ArgumentException(
nameof(formatted),
string.Format(Resources.DidNotParse, formatted));
}
}
Two argument call, paramName is a literal, but not a parameter name. (No fixer, just squiggle the argument.)
public void SomeMethod(string formatted)
{
if (!Helper.TryParse(arg, out Parsed parsed))
{
throw new ArgumentException(
string.Format(Resources.DidNotParse, formatted),
"input");
}
}
Two argument call, paramName is a literal, but not nameof. (Fixer: use nameof)
public void SomeMethod(string formatted)
{
if (!Helper.TryParse(arg, out Parsed parsed))
{
throw new ArgumentException(
string.Format(Resources.DidNotParse, formatted),
"formatted");
}
}
Examples to not detect:
Probably wrong (based on the parameter names), but probably shouldn’t warn; since we’re kinda guessing.
public static void ThrowArgumentException(string param, string msg)
{
throw new ArgumentException(param, msg);
}
When the parameter name comes from a variable rather than a literal, don’t squiggle.
public static void ThrowArgumentException(string msg, string param)
{
throw new ArgumentException(msg, param);
}
public void SomeMethod(string formatted, int idx)
{
string argFail = null;
string msg = null;
if (!Helper.TryParse(arg, out Parsed parsed))
{
argFail = "formatted";
msg = string.Format(Resources.DidNotParse, formatted);
}
else if (parsed.Length < idx)
{
// Disregard that ArgumentOutOfRangeException is better here.
argFail = "idx";
msg = string.Format(Resources.OutOfRange, idx, parsed.Length);
}
if (argFail != null)
{
throw new ArgumentException(msg, argFail);
}
}
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Comments: 33 (33 by maintainers)
We have that rule enabled in dotnet/runtime: https://github.com/dotnet/runtime/blob/d1006ce95052e609252756999596652e3f5d0757/eng/CodeAnalysis.ruleset#L57 I might be remembering incorrectly, but I thought @danmosemsft told me he found some occurrences of this in our codebase; if that’s true, it’d be good to understand what the rule is missing, and potentially augment it.
@buyaa-n / @danmosemsft - I’ve revised our definition of done a bit. Here’s the updated list. We’ll get this documented somewhere centrally very soon.
dotnet/runtime,dotnet/aspnetcore, anddotnet/roslynrepositories using the failures there to discover nuance and guide the implementation detailsroslyn-analyzersdotnet/runtimerepodotnet/aspnetcoreanddotnet/roslynfor the failures in those repositoriesSeems like a good candidate for an analyzer option to allow users to select either configuration.
Yes, we have tons of analyzers that only want to run on public API surface by default, but want to give end users the option to configure the analyzed API surface. We have an extension method and an existing editorconfig option that should exactly meet your requirements.
Option: https://github.com/dotnet/roslyn-analyzers/blob/master/docs/Analyzer Configuration.md#analyzed-api-surface
API: You can invoke
ISymbolExtensions.MatchesConfiguredVisibilitymethod on the symbol that needs to be checked, for example: https://github.com/dotnet/roslyn-analyzers/blob/0cbdd68e14722e911d50eee64ee95823b3ffa09c/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/ReviewUnusedParameters.cs#L221-L230Example Unit test: https://github.com/dotnet/roslyn-analyzers/blob/0cbdd68e14722e911d50eee64ee95823b3ffa09c/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/Maintainability/ReviewUnusedParametersTests.cs#L820-L873
@buyaa-n That sounds good. This rule should likely tune or supersede CA2208.
Going through the discussions in this issue, I wanted to point out that the rules written in roslyn-analyzers repo have the ability to define end-user configurable .editorconfig options. We have had large number of scenarios where options are very helpful when there is no single, perfect analyzer implementation:
@danmosemsft in addition to @bartonjs’s comment our “Definition of Done” steps for analyzers might give you more info
My personal refinement/rise-and-repeat would say “hmm, if there are no parameters to a method and it’s throwing an ArgumentException, there’s not really anything to report on; it must be a helper”.
If the helper took the message as a parameter, then it’d require suppression… assuming that’s not a commonish pattern.
But the general flow on anything reactive like this is definitely “write it, see too many violations, figure out how to get it down to a manageable number”; like with stackalloc in a loop.
Are we holding the bar to “essentially no noise” ? As this is a pattern we use in a fair number of places. Eg., (1) because the parameter name was historically different, although we should probably just ‘break’ those (2) because it’s in a throw helper eg
Would I be expected to suppress the warning on this line?
I searched for
\WArgumentException\(nameofin our own tree and there are 64 hits…