False positive with changing reported issue

Rules and Languages Report False-positive / False-negative...
1

We have a case here where Sonar is reporting:
Remove this unnecessary cast to “Throwable” in the log statement.

If we remove the cast, then we get instead for the same line:
2nd argument is not used.
which is because the argument is not a Throwable that is handled differently

private void logError(HttpServletRequest req) {
	Object exception = req.getAttribute(RequestDispatcher.ERROR_EXCEPTION);
	String uri = "some uri";

	if (exception instanceof Throwable) {
		log.error("HTTP 404 - Not found: {}", uri, (Throwable)exception); //$NON-NLS-1$  
	}
	else {
		log.error("HTTP 404 - Not found: {} ", uri); //$NON-NLS-1$
	}
}
4

Hello @Alain_Picard,

Thank you for your message. I tried to reproduce this issue, but I didn’t get this issue. So it seems to me that I need some more details:

  • Are you talking about rule S1905
  • Which version of SonarQube you’re using or is it SonarCloud or SonarLint?
  • Do you see the same behaviour on the latest version?
  • Which log class you’re using (fully qualified name)?

Regards,
Margarita

6

Hi @Margarita_Nedzelska,

Please find enclosed a sample full java file that reproduces the problem with Sonar Lint
Info: SonarSource SonarLint for Eclipse 5.6.0.25634 org.sonarlint.eclipse.feature

Error400Servlet2.zip (470 Bytes)

7

Thanks for the clarifications @Alain_Picard,

Now I am able to get this issue, but in my context this is a True positive.

You’re right the cast is mandatory, if there are conflicting overloads of log.error() method, because the overloaded method is statically binded and is chosen at compile time. So it will work if Logger has 2 different methods:
error(String, Object, Object) and error(String, Object, Throwable)

But there is no such method as error(String, Object, Throwable) in slfg-api-1.7.30 version, so in any case method call log.error("HTTP 404 - Not found: {}", uri, (Throwable)exception); will be mapped to the error(String, Object, Object) even when cast is present. To pass Throwable correctly there is another method error(String, Throwable).

Please, let me know if this maps and explains your case or I’ve missed something and cast is mandatory in your case.

Regards,
Margarita

8

@Margarita_Nedzelska,

Yes and No.

As stated with the cast we get: Remove this unnecessary cast to “Throwable”.
But without the cast we get:
2arg not used

slf4j (and for us logback implementation) will always treat the last argument that is a Throwable as an argument that doesn’t need to be part of the string (no {}). Rule java:S3457 checks for the right number of braces to match your arguments, but here whatever you do you have a reported issue.

Is that clearer?

9

Hi @Alain_Picard,

Could you please tell me which version of logback implementation you’re using and which method you want to call, I can’t find a suitable one here

As far as I can understand, if signature of the method uses object, and the cast or instanceOf check is done in runtime, you don’t need the cast, because in runtime the type will “Throwable”. And if, as you said, the last argument is not a part of formatting message, there could be a FP in rule S3457.

So from you side I need some clarifications, that with and without cast runtime behaviour is different, or that the last argument is really treated as Throwable. Could you please provide a minimum reproducer of this behaviour so we can proceed?

Regards,
Margarita

10

Hi @Margarita_Nedzelska,

slf4j and hence an implementation such as Logback provide a large number of methods with various number of arguments or array of arguments to support any number of such arguments. Ultimately whether it chose a method with a throwable as the last argument or not, it will re-arrange things as seen here.

The problem is that Sonar shouldn’t report anything here, and even more importantly, fixing the reported cast, should not lead to S3457, damn if you do and damn if you don’t, i.e. no way but adding a //NOSONAR.

As for reproducer, I gave you the simplest java file to use and you can remove the cast and then you will get S3457. Not sure what more I can provide.

Regards,
Alain

11

Hi @Alain_Picard

I’ve got your point. You don’t need casts, so S1905 reported correctly. But without casts you’re getting a different issue, which is a FP, but happened due to our limitations on this rule. Regarding S3457:
We’re not reporting the issue if the last argument’s compile time type is Throwable. I created a ticket to relax this rule and do not report if there was an instanceof check before. Because in your example the compile time type without cast is still Object and that’s why you’re getting the issue with unused argument.

Here is the link to a ticket: https://jira.sonarsource.com/browse/SONARJAVA-3704

Thanks for your report.

Regards,
Margarita

12

@Margarita_Nedzelska,

Sounds good, thanks

13

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.