Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CA2208: false positive when exception message contains parameter name (but only in some cases) #6863

Closed
MaceWindu opened this issue Aug 19, 2023 · 1 comment · Fixed by #7065
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design False_Positive A diagnostic is reported for non-problematic case

Comments

@MaceWindu
Copy link

Analyzer

Diagnostic ID: CA2208: Instantiate argument exceptions correctly

Analyzer source

NuGet Package: Microsoft.CodeAnalysis.NetAnalyzers

Version: 8.0.0-preview.23364.2 (Latest)

Describe the bug

False positive triggered when exception message argument contains substring equal to any parameter name with some unclear additions

Steps To Reproduce

public class Test
{
	public static void Mthod(string parameter)
	{
		// CA2208 Method Mthod passes parameter name 'parameter.Length blah-blah-blah.' as the message argument to a ArgumentException constructor.
		// Replace this argument with a descriptive message and pass the parameter name in the correct position.
		if (string.IsNullOrEmpty(parameter))
			throw new ArgumentException("parameter.Length blah-blah-blah.", nameof(parameter));
	}
}

Expected behavior

no errors

Actual behavior

error

Additional context

@Youssef1313
Copy link
Member

Looking at the code and history, this might be a regression from #6553

This PR was intended to make the analyzer less strict, but if I understand correctly, the MatchesParameter method is used in two different contexts:

bool matchesParameter = MatchesParameter(targetSymbol, creation, stringArgument);
if (IsMessage(parameter) && matchesParameter)
{
var dictBuilder = ImmutableDictionary.CreateBuilder<string, string?>();
dictBuilder.Add(MessagePosition, parameter.Ordinal.ToString(CultureInfo.InvariantCulture));
return context.Operation.CreateDiagnostic(RuleIncorrectMessage, dictBuilder.ToImmutable(), targetSymbol.Name, stringArgument, parameter.Name, creation.Type!.Name);
}
else if (HasParameters(targetSymbol) && IsParameterName(parameter) && !matchesParameter)
{
// Allow argument exceptions in accessors to use the associated property symbol name.
if (!MatchesAssociatedSymbol(targetSymbol, stringArgument))
{
return context.Operation.CreateDiagnostic(RuleIncorrectParameterName, targetSymbol.Name, stringArgument, parameter.Name, creation.Type!.Name);
}
}
return null;

  1. For a "message" argument, in this case, a match will report a diagnostic.
  2. For a "paramName/parameterName", in this case, a non-match will report a diagnostic.

The PR #6553 appears to make MatchesParameter return true in more cases, meaning that it makes the first case more strict (unintended) and the second case less strict (intended)

Note I'm only telling by reading through the code. I didn't go through debugging this.

cc @stephentoub

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design False_Positive A diagnostic is reported for non-problematic case
Projects
None yet
2 participants