I’m cleaning up a lot of issues in one of our products. Now I’m running into rule S3928: Parameter names used into ArgumentException constructors should match an existing one.
The code we have spread around our codebase looks like this:
public bool DoSomething(Guid[] things)
{
return WithExceptionHandling(() =>
{
if (things == null)
{
throw new ArgumentNullException(nameof(things));
We get the same issue when using “things” as parameter on the exception.
In my opinion this should be valid, so I’m wondering if this is a bug or that I might be mistaken?
Hello,
Could you please let us know what version of the analyser you are using ?
Please also attach a screenshot of the issue raised so we can see on which line it is raised.
Finally, a code snippet that compiles would be much appreciated so we can work with.
Regards,
Alex.
Sorry for my late reply.
I still think this is something that needs to be supported by SonarCloud. This lamba is part of a containing function and SonarCloud should also use that scope for rule 3928 in my opinion.
Do you have other solutions to fix the warnings we get?
Could you please give us a bit more details about your exact use case and why this check needs to be inside a nested lambda ?
What prevents you from from doing this check outside this lambda ?
Nothing prevents us from doing this check outside the lamba. But for certain reasons we do some validation and throw an argument exception inside that lamba. In the example of the first post, the lamba we use is responsible for certain logging and other stuff.
If we move this outside the lamba, we need to change the architecture, just to satisfy SonarCloud. We can do this if we are using a really bad pattern, I have not been given a good explanation why this is bad and I still feel this is correct code…
I agree that you shouldn’t change your architecture just to please the tool. I also agree that in your particular case, this is a FP. I don’t think it’s a bad pattern if used judiciously.
However, I don’t fully agree that captured variables can be considered as arguments for a lambda (from a generic programming point of view, the lambda is validating a captured variable, not its own argument).
Also, the Action you’re passing to WithExceptionHandling could be passed on to other methods. And when the ArgumentNullException will be thrown, it could be several frames up the stack, and it won’t be very clear
where that “argument” got initially captured
whether the captured variable was an argument or not (maybe it was just a captured local variable)
I’m sure that in your case you don’t have such problems, but we try to make our rules as generic as possible and IMHO this case is not generic enough to modify the rule.
If there are some widely used frameworks that use this pattern for validating arguments, we can reconsider our position.
I can only imagine the challenges you guys have to make these rule fit all cases everybody is presenting.
But… in this case I think it should be about the scope of the variable. The example I have given, the scope is valid and thus the nameof(xxx) should also be considered as valid. At least visual studio says it is fine.
I’m curious about your comment of passing the Action around in respect of the example I have given you. Could you provide me with an example with what you are thinking about? (Maybe I’m on the wrong thinking-path)
And what if we would write the code like this? things is still captured here.
public void DoSomething(Guid[] things)
{
Action action = () =>
{
if (things == null)
{
throw new ArgumentNullException(nameof(things));
throw new ArgumentNullException("things");
}
};
WithExceptionHandling(action);
}
This rule is about not throwing ArgumentException for variables which are not arguments. In the case of the lambda, the variable is not an argument, it’s a captured variable.
It’s not related to the example you gave me. I mentioned a generic example where an Action can be passed as argument in a chain of method calls. Then throwing ArgumentException for captured variables can be difficult to understand. When you see an ArgumentException you can look at the top of the stack and quickly identify where that argument got passed. In the case of captured variables, you have to track where that Action is coming from.
It’s not an ArgumentException anymore, it’s something else. You cannot look at the lambda signature and its arguments, because it has no arguments. Or even worse if the lambda has arguments and also captured variables, you look at the signature, see the arguments, but actually the exception is thrown for a captured variable. How can you tell that from the stack trace?
This is the same snippet presented at the beginning of this thread (a bit refactored). I don’t understand the question.
But the captured variable is still an argument of the current function.
This is why I knock at your door I only can give you as much information from my perspective. I’m not thinking in stack traces at all.
Also, I would help me if you guys can tell me why this needs to be the focus of the lamba and not the function?
I really want to cleanup these warnings, but I’m not agreeing with them as you are presenting the rules here in this thread, because I think sonarcloud isn’t correct in this case.
If you guys disagree, let’s agree on our disagreement and close this discussion…
Sorry, Ignore this please … I though it would help when I was replying back
Yes, and the exception is thrown outside of the scope of the current function. In your particular case, you are using lambdas to capture function arguments.
We could, of course, add a check inside the rule to make sure that those captured variables are the arguments of the outer function… but as I said I feel this is a corner case and there’s a trade-off when adding complexity to the rule (on the performance of analysis and also on maintenance).
And what if we would write the code like this? things is still captured here.
I’ll try to illustrate the problem on different example. things (or input in this case) is still captured and the code works. And that’s exactly the problem:
public static string DoSomething(string html, string defaultImage)
{
var evaluator = CreateEvaluator(defaultImage, "Image");
return Replace(html, @"<img src=""(?<Image>[^""]+)""[^>]*>", RegexOptions.ExplicitCapture, evaluator);
}
public static MatchEvaluator CreateEvaluator(string input, string key) =>
(match) =>
{
if (input == null)
{
throw new ArgumentNullException(nameof(input));
}
var image = match.Groups[key].Value;
if (image.StartsWith("cid:"))
{
return match.Value.Replace(image, input);
}
return match.Value;
};
protected static string Replace(string input, string pattern, RegexOptions options, MatchEvaluator evaluator)
{
var regex = new Regex(pattern, options);
return Replace(input, regex, evaluator);
}
protected static string Replace(string input, Regex regex, MatchEvaluator evaluator)
{
//Do some logging or logic here
return regex.Replace(input, evaluator);
}
This would produce ArgumentNullException: Value cannot be null. (Parameter 'input') on the last line.
If you’ll try to debug it and understand what’s going on:
You’ll see that input variable has some value in Replace function.
You’ll not see any other input variable in your call stack.
