S3457/S2629: inconsistent reporting with multiline and getter

SonarLint Eclipse
, ,
1

Hello,

Versions used:

  • SonarLint for Eclipse 3.6.0.2018060712
  • Eclipse Photon Release 4.8.0

Here is a sample class, including the issues it raises:

protected final Logger logger = LoggerFactory.getLogger(this.getClass());

public void methodPojo(Person value) {
	// raise S3457: Format specifiers should be used instead of string concatenation.
	logger.info("My value " + value.getName());
	
	// no issue raised
	logger.info("My value "
		+ value.getName());
	
	// raise S2629: Use the built-in formatting to construct this argument.
	logger.info("My value " + value);
	
	// raise S2629: Use the built-in formatting to construct this argument.
	logger.info("My value "
		+ value);
}

public void methodString(String value) {
	// raise S2629: Use the built-in formatting to construct this argument.
	logger.info("My value " + value);
	
	// raise S2629: Use the built-in formatting to construct this argument.
	logger.info("My value "
		+ value);
}

public class Person {
	private String name;
	private int age;
	
	public String getName() {
		return name;
	}
	
	public int getAge() {
		return age;
	}
}

This snippet uses an SLF4J Logger.

  1. Why do I have different issues whether I use a getter or not ?
  2. No issue is raised when using a getter on a multi line statement. Is this expected ?

Thanks in advance for your answers.

2

I am not sure I understand your first point given your example.

Regarding the multi line : this is because of that (old) ticket : https://jira.sonarsource.com/browse/SONARJAVA-1153 but I suspect that this could be improved in the description of the rule.

3

For the first point. Admit value is a Pojo with some getter.

If I write logger.info("My value " + value);, the rule S2629 raises the issue Use the built-in formatting to construct this argument. The correct form is logger.info("My value {}", value);.

If I write logger.info("My value " + value.getName());, the rule S3457 raises the issue Format specifiers should be used instead of string concatenation. The correct form is logger.info("My value {}", value.getName());.

Why do I have two different rules for the same kind of error ?

If I write:

	logger.info("My value "
		+ value.getName());

none of the rules mentionned above raise an issue.

4

Hi @j.joslet

Thanks for your examples, helped me to figure out this not trivial rules:)
So there are 2 things.

First thing is a false negative for S3457 when there is a new line is indeed a bug, and it appeared probably due to with to avoid FP for cases like this:

logger.info("One long string " +
   "Another long string");

I’ve created the ticket to fix your FN:

logger.info("str" +
   x);

Second thing is a different behaviour for getters for rule S2629. This rule reports when there is a chance of some significant computations done for no reason. The idea is that when you use getter it will not require a lot of computations, just field value will be returned. But when you use simple variable (like value in your case), some toString operation could be performed. I agree, this approach is not always perfect, but without it the rule would be too noisy.

Btw for the 3rd case (logger.info("My value " + value);) there will be 2 issues, for both rules (S3457 and S2629).