I am using SonarQube for IDE (JetBrains Rider) (version 10.20.0.80985) in connected mode to SonarQube Server (Enterprise Edition, v10.8.1 (101195)).
In projects using net9.0 the analyzer reports rule S3236 when a message string is supplied to a System.Diagnostics.Debug.Assert(bool condition, string? message = null) call.
Full signature of the Debug.Assert:
I believe that as a developer I am free to supply a custom message to Debug.Assert and this is one of the intended usages (before net9.0 it was the only way). The CallerArgumentExpression attribute (added to Debug.Assert in net9.0) is just a fallback in case there is no message supplied to Debug.Assert. That is why I think this report is a false positive.
Now as to how to prevent this false positive, I’m not sure. It’s unfortunate that the CallerArgumentExpression attribute (and other attributes like it) requires an optional parameter to let the compiler do its magic. Maybe hard-coded exceptions to this rule are the only way out? For Debug.Assert this would in my opinion be warranted as it’s such a specific use case.
A reasonable (to me) alternative would be to have some way to suppress the rule for specific methods (or method overloads), such that I can exclude the Debug.Assert overload mentioned above at one place in my solution.
Suppressing the inspection at all call sites is not feasible.
I considered creating a custom static method that calls the framework Debug.Assert with a specified message and suppressing that one call. This however is also not great as that will put the debugger in that static method instead of at the call site, which is distracting.
I confirm this is a false positive. In any case, there’s an overload of Debug.Assert that just gets as parameters the condition and the message - so it should not consider the CallerArgumentExpression at all. I’ll add a ticket to our backlog to fix this.
My bad - the overload is the one with [CallerArgumentExpression(nameof(condition))]. I got confused by the IDE inspection tool.
I’ll go a bit deeper and come back soon.
Apologies for the mess earlier, I got really confused hovering on the method in VS22 - it does not show the CallerArgumentExpression annotation unless you go to the implementation.
From our side this is not considered an FP.
The Caller information annotation is not really a fallback, as if the Debug.Assert fails it offers more information that just a message (which assertion fails and at which line exactly).
If you find the custom message more useful would disabling this rule be an option for you?
I don’t quite agree on the Caller information offering more information than a (custom) message. It will provide the assertion expression, that is true. But a custom message can provide more context, for example it can provide a reason why the asserted expression is expected to be true at that point in the code.
I’m hesitant turning this rule off entirely as I think in most cases it can be super useful. That’s why I suggested making the suppression configurable per method (signature) in my original post. But I also understand if that’s more work than you are willing to invest into this inspection rule.
P.S. I don’t think de Caller information will provide line numbers…
I don’t quite agree on the Caller information offering more information than a (custom) message. It will provide the assertion expression, that is true. But a custom message can provide more context, for example it can provide a reason why the asserted expression is expected to be true at that point in the code.
I took time to think about this and I agree with you. I had a chat with my team and we agreed to except this particular method as the change is recent (NET9), and the automatic message provided might not be enough to give a good explanation to the developer indeed.
As a workaround until this is implemented, there’s an overload for Debug.Assert that won’t raise this issue. public static void Assert([DoesNotReturnIf(false)] bool condition, string? message, string? detailMessage).
P.S. I don’t think de Caller information will provide line numbers…
It provides the line number of the failed Assertion.
That’s why I suggested making the suppression configurable per method (signature) in my original post.
This, implementation wise, requires a lot of changes to most of our products, not only the analyzer, so it’s not really an option for the time being.
