Pass correct parameterName to ArgumentException

[terrajobst]

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);
    }
}

[jeffhandley]

Estimates:

• Analyzer: Medium
• Fixer: Not Applicable

[jeffhandley]

A fixer is tough here because we don't know what message to insert into the parameter list. Perhaps we could pass in nameof(...) also as the message.

[Edit by @bartonjs: Digging in a little deeper, it looks like new ArgumentException(nameof(something)) => new ArgumentException(null, nameof(something)) is probably the right answer]

[jeffhandley]

Based on this guidance, we can update the estimates:

• Analyzer: Medium
• Fixer: Small

[buyaa-n]

Just found we have analyzer for ArgumentException and its descendants, some rules might not covered and no fixer implemented I assume i would update this analyzer (if needed) and add fixer implementation. @mavasani please let me know what you think

[buyaa-n]

There is case not covered above:

 public async Task ArgumentException_NoArguments_Warns()
 {
     await VerifyCS.VerifyAnalyzerAsync(@"
         public class Class
         {
             public void Test(string first)
             {
                 throw new System.ArgumentException();
             }
         }",
         GetCSharpNoArgumentsExpectedResult(6, 31, "ArgumentException"));
}

The question is do we want fixer for this (possibly new ArgumentException() => new ArgumentException(null, nameof(first)))

[buyaa-n]

Same question for other similar cases with empty string or white space, should we provide fixer?

[Fact]
public async Task ArgumentException_EmptyParameterNameArgument_Warns()
{
    await VerifyCS.VerifyAnalyzerAsync(@"
        public class Class
        {
            public void Test(string first)
            {
                throw new System.ArgumentNullException("""");
            }
        }",
        GetCSharpIncorrectParameterNameExpectedResult(6, 31, "Test", "", "paramName", "ArgumentNullException"));
}

[Fact]
public async Task ArgumentNullException_SpaceParameterArgument_Warns()
{
    await VerifyCS.VerifyAnalyzerAsync(@"
        public class Class
        {
            public void Test(string first)
            {
                throw new System.ArgumentNullException("" "");
            }
        }",
        GetBasicIncorrectParameterNameExpectedResult(4, 31, "Test", " ", "paramName", "ArgumentNullException"));
}

[danmoseley]

I searched for \WArgumentException\(nameof in our own tree and there are 64 hits...

[bartonjs]

I don't know that there's enough cleverness to guess as to what argument someone meant if they didn't give a name (vs gave a name, but passed it as the wrong parameter).

[danmoseley]

Two argument call, paramName is a literal, but not a parameter name. (No fixer, just squiggle the argument.)

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

        private static Exception HashAlgorithmNameNullOrEmpty() =>
            new ArgumentException(SR.Cryptography_HashAlgorithmNameNullOrEmpty, "hashAlgorithm");

Would I be expected to suppress the warning on this line?

[bartonjs]

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.

[buyaa-n]

Would I be expected to suppress the warning on this line?

@danmosemsft in addition to @bartonjs's comment our "Definition of Done" steps for analyzers might give you more info

1. Analyzer implemented locally (but no PR yet)
2. Rule run locally against the runtime repo with failures assessed to guide implementation details
3. Matching and non-matching scenarios captured and documented (in the issue comments to start, but Jeff will follow up with Manish for permanent documentation plans); this will illustrate the nuance taken into account for the analyzer
4. Follow the standard API review process, including the attribute if applicable, and discussing categorization and generalization. We are still working out the details of this exact process for analyzers.
5. Determine what to do with each of the runtime failures
6. Fixer implemented for C# (if applicable) - we will not implement VB fixers
7. Decision made for severity/default/categorization/numbering, typically on by default for new APIs
8. Write unit tests for all cases documented in bullet 3
9. Submit PR for Analyzer/Fixer and get it merged into roslyn-analyzers
10. SDK config committed to reflect the severity/default decision for old APIs
11. Runtime failures fixed or suppressed

[mavasani]

Just found we have analyzer for ArgumentException and its descendants, some rules might not covered and no fixer implemented I assume i would update this analyzer (if needed) and add fixer implementation. @mavasani please let me know what you think

@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:

1. The default analyzer implementation is too strict/pessimistic to avoid false positives, but we have large user requests to allow configuration to execute it in lenient/optimistic mode where user want no false negatives, but are fine to suppress false positives.
2. The analyzer can be implemented with more then reasonable semantics, but only one can be chosen for default implementation, but it seems reasonable to expose an option to allow users to switch between any of these. For example, see single-letter-type-parameters option.
3. End users want to be able to customize the analysis scope, such as API surface (public versus internal) on which the analysis executes. For example, see analyzed-api-surface option.
4. Analyzer can take end-user configurable symbol names/signatures as inputs for analysis. For example see additional-string-formatting-methods option or null-check-validation-methods option.

[jeffhandley]

@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.

• Implement the Analyzer locally (but no PR yet)
• Run the analyzer locally against the dotnet/runtime, dotnet/aspnetcore, and dotnet/roslyn repositories using the failures there to discover nuance and guide the implementation details
• Document the matching and non-matching scenarios and all of the nuance discovered (as issue comments for now, until we have a more formal place to document these details)
• Review each of the failures in those repositories and determine the course of action for each
• Implement the fixer for C# (if applicable). We will not implement fixers for VB.
• Solidify decision for severity/default/categorization/numbering. Ideally, analyzers for new APIs will be on by default, but this requires very high confidence in avoiding false positives
• Follow the standard API review process for anything that has changed since the analyzer concept was approved, such as the categorization and generalization of the analyzer or changes to an attribute which informs the analyzer. We will solidify this process as we work through the first handful of analyzers
• Write unit tests for all cases documented in bullet 3 (matching and non-matching scenarios)
• Submit a PR for Analyzer/Fixer and get it merged into roslyn-analyzers
• Submit a PR for the SDK config to reflect the severity/default determination
• Fix or suppress the failures in the dotnet/runtime repo
• File issues in dotnet/aspnetcore and dotnet/roslyn for the failures in those repositories

[buyaa-n]

I have added some more tests to see what analyzer scenarios are not covered and found one scenario not covered. Which is Two argument call, paramName is a literal, but not nameof. (Fixer: use nameof) was trying to add analizer rule for that scenario but found there is another with fixer analyzer for that https://github.com/dotnet/roslyn-analyzers/blob/master/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/UseNameofInPlaceOfString.cs which is not specifically for ArgumentException but for any arguments passed as string literal. I assume we wouldn't like to add another rule for that for ArgumentException analyzer

[bartonjs]

Yep, if there's already an analyzer for it then there's no need to repeat it.

[stephentoub]

We have that rule enabled in dotnet/runtime:

runtime/eng/CodeAnalysis.ruleset

Line 57 in d1006ce

<Rule Id="CA1507" Action="Warning" /> <!-- Use nameof to express symbol names -->

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.

[bartonjs]

[Dan] told me he found some occurrences of this in our codebase

I feel like I've seen some, too. But maybe it was just in test projects?

[buyaa-n]

We have that rule enabled in dotnet/runtime:

I was planning to test this and ArgmentException analyzer against runtime by increasing severity level, I thought they are not warning because of the severity level low, but didn't know it was enabled like this, anyways I will look into that and test them with runtime to get some numbers which would help us with potential augmetning

[buyaa-n]

I might be remembering incorrectly, but I thought Dan 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.

The UseNameofInPlaceOfString analyzer is enabled in the runtime repo and running just fine, as @bartonjs said i assume those occurrences were on test projects .

Also I had enabled InstantiateArgumentExceptionsCorrectlyAnalyzer to test against runtime repo and got lots of valid/invalid warnings, sent the report by email. Please let me know your thoughts about it

[danmoseley]

I thought Dan told me he found some occurrences of this in our codebase

I have no memory of anything I may have said 😺 Using simple regexes right now, I only see some minor hits like throw new ArgumentException(SR.InvalidQuery, "*"); which presumably this rule would not fire for since it isn't a parameter. It is a little "odd" perhaps to have anything other than nameof in the 2nd position, but there are potential valid reasons.
[edited to fix mistake]

[buyaa-n]

• According to test run results discussion following updates made to analyzer:

1. Increased severity from hidden to info (IdeHidden_BulkConfigurable => IdeSuggestion)
2. Don't warn when the enclosing method has no parameter

• Plus while implementing analyzer decided not to show multiple diagnostics for reversed arguments, it looks ugly and reporting the same issue for 2 reversed parameters, one of which has a fixer. Now only showing the diagnosing having fixer instead:

• The fixer implemented for CSharp only, please suggest if there is better way to make it more language-agnostic Updated to work for CSharp and VB both Instantiate ArgumentExceptions Correctly analyzer updates and fixer implementation roslyn-analyzers#3500

Question: what we want/need to do with the test failures on runtime CC @bartonjs @jeffhandley @stephentoub @terrajobst

[buyaa-n]

In the "Fix warnings found by the analyzer" PR got more suggestions for improving the analyzer:

• maybe we only want the analyzer to report for public/protected/protected ( do not warn for internal private methods).

• We could include the generic method parameters (and possibly the generic type parameters for constructors) as legal targets.

I am planning to add these improvements, please let me know if you have any concerns with the above @terrajobst.

@mavasani is there any config option for setting the target method accessibility level? Any suggestions about that?

[mavasani]

maybe we only want the analyzer to report for public/protected/protected ( do not warn for internal private methods).
@mavasani is there any config option for setting the target method accessibility level? Any suggestions about that?

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%20Configuration.md#analyzed-api-surface

API: You can invoke ISymbolExtensions.MatchesConfiguredVisibility method 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-L230

Example Unit test: https://github.com/dotnet/roslyn-analyzers/blob/0cbdd68e14722e911d50eee64ee95823b3ffa09c/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/Maintainability/ReviewUnusedParametersTests.cs#L820-L873

[buyaa-n]

Great, thank you @mavasani!

[buyaa-n]

Reopening the issue as we need to fine tune the rules more and also for tracking the fix warnings in runtime repo and update the doc accordingly. Gonna add following to the rule:

• Allow using nameof(parameter) + "." + nameof(parameter.Property) or "parameter.Property" as parameterName for exceptions not having exception message to embed the parameter.Property

• Allow using the Property name for setters in addition to using value as the name of the implicit value parameter of property

[bartonjs]

Allow using the Property name for setters in addition to using value as the name of the implicit value parameter of property

This explicitly violates Framework Design Guidelines, we should not do it.

Allow using nameof(parameter) + "." + nameof(parameter.Property) or "parameter.Property" as parameterName for exceptions not having exception message to embed the parameter.Property

You'll want PM signoff on that... since I recall it being something @terrajobst explicitly was against on the call.

[buyaa-n]

This explicitly violates Framework Design Guidelines, we should not do it.

Oops sorry, I thought it would be more convenient 😛, canceling this

You'll want PM signoff on that... since I recall it being something @terrajobst explicitly was against on the call.

Sure thing, @terrajobst havign issue with the following case, doesn't make sense to throw with the argument again, doesn't have a message to embed the property info, for now only can suppress

public ChainedConfigurationProvider(ChainedConfigurationSource source)
{
    if (source == null)
    {
        throw new ArgumentNullException(nameof(source));
    }
    if (source.Configuration == null)
    {
#pragma warning disable CA2208 // Instantiate argument exceptions correctly
         throw new ArgumentNullException(nameof(source) + "." + nameof(source.Configuration));
#pragma warning restore CA2208
     }
}

[buyaa-n]

Immo's suggestion for the later case: ANE is not right thing to throw here as arguments was not null, better to use argument exception with appropriate message and keep nameof(source) as paramName. I will apply this suggestion in my PR, would double check the doc as these are public APIs, i think i shouldn't change the exception if it is documented so.

So the analyzer will not be updated, but gonna keep this open until runtime fixes merged

[buyaa-n]

Another suggestion came up from the "Fix warnings found in runtime PR" was apply the analyzer only for desired ArgumentException descendants to exclude user-defined types or apply to all ArgumentException-derived types (including itself) that have a constructor accepting a paramName parameter, including user-defined types.

[mavasani]

analyzer only for desired ArgumentException descendants to exclude user-defined types or apply to all ArgumentException-derived types (including itself) that have a constructor accepting a paramName parameter, including user-defined types

Seems like a good candidate for an analyzer option to allow users to select either configuration.

[buyaa-n]

@mavasani I was thinking to apply whichever implementation would more efficient/faster, but let the user choose which one to apply might be even better

[buyaa-n]

After looking constrcutors of ArgumentException derived types in .Net
Some of them have a constructor with paramName, some have not. If we apply rules only for some chosen exception types it might be easier and faster with less code change but if we want to add/update the supported types later would need code change, if we make it user-configurable that could solve the later problem but the original issue could happen in case user added an exception type that not having an overload that accepts paramName. With that being said i think @bartonjs's suggestion to apply the rules to "all ArgumentException-derived types (including itself) that have a constructor accepting a paramName parameter, including user-defined types" is better approach

[buyaa-n]

Need to turn on the analyzer on runtime repo after its updates shipped to nugget, but i think we don't need to keep it open for that