Roslyn: Fix `ArgumentException`s where parameterName is sent as message

[Youssef1313]

Example (That's not the only occurrence)

[RikkiGibson]

What needs to be fixed in the example?

[Youssef1313]

@RikkiGibson Oh sorry. It seems like I copied a wrong URL. Here is an example of what I meant. The called constructor takes the message, but the paramName was sent.

See CA2208

[jaredpar]

@Youssef1313 the second example that you showed here is likely not a bug. Yes indeed the document value is null but the cause was the caller providing a bad filePath parameter to the function. Hence the exception is correct.

Are there other examples u think need to be fixde here?

[Youssef1313]

@jaredpar The paramName that caused the error is indeed filePath. But the exception doesn't set paramName at all.

The called constructor takes message, not paramName.

So, the example I referred to sets the message as filePath and paramName as null.

[jaredpar]

@Youssef1313 understand. That is likely intended behavior here (code model is a strange place). Are there examples of this outside the code model layer?

[Youssef1313]

@jaredpar

https://github.com/dotnet/roslyn/blob/462eac607741023e5c2d518ac1045f4c6dabd501/src/Workspaces/VisualBasic/Portable/Utilities/IntrinsicOperators/PredefinedCastExpressionDocumentation.vb#L37

[jaredpar]

Yes that's a case we could consider changing.

In general though I'm still not quite sure what the ask is here. Is the expectation that we look at every single use of ArgumentException in the code base and review it? Or do u have some tooling that we think we should be using as a guide here?

Also one thing to keep in mind is that the current pattern, while less than ideal, also isn't wrong. The parameter name as the error isn't ideal but it's also valid.

[Youssef1313]

@jaredpar there is an analyzer for that.

https://github.com/dotnet/roslyn-analyzers/blob/master/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/InstantiateArgumentExceptionsCorrectly.cs

dotnet/runtime repo has fixed that issue in https://github.com/dotnet/runtime/pull/38578

[jaredpar]

That seems reasonable if someone wanted to apply it to our repo. I'd be wary of changes to the code model but probably not enough to reject a change that did this.

@jasonmalinowski @sharwell could comment on the IDE side of taking a change like this.

[jasonmalinowski]

As far as CodeModel is concerned, changing the _type_ of exception is probably going to break something; changing the string of the message I wouldn't be too worried about, but then again you never know what terrible code exists out there. People catch CodeModel exceptions all the time and do strange things with them.

[jaredpar]

People catch CodeModel exceptions all the time and do strange things with them.

My first job at Microsoft was being one of those people :smile:

[stephentoub]

Practically any change, any bug fix, any performance fix, even any new API, can break someone. We consider changes to exception messages acceptable, even if someone managed to take a dependency on one.

[CyrusNajmabadi]

Note; our codemodel bar is pretty stringent. As Jason mentions, we absolutely see people doing all sorts of hacky stuff when calling into these api. I can certain assure you that people catch very specific exceptions, and it's quite likely that messages are looked at as well to allow people to keep their codemodel code still working over time.

[Youssef1313]

Is there a problem if codemodel was excluded from the fix?