False pose in Mutable fields should not be "public static" with immutable collections (squid:S2386)

[Michał Kordas]

Hi,

I have the following code:

public static final Set<String> SET = ImmutableSet.of("a"); // no violation - OK
public static final Set<String> OTHER_SET = SET; // violation Make this member "protected". coming from squid:S2386

Both sets in this case are Guava's immutable sets, so no violation is expected.

Regards,

Michal Kordas

[Nicolas Peru]

Hi Michal, 

This is indeed a false positive and also a known limitation. To overcome this limitation we would have to track effective type of variables (which is doable in case of constant but can become quickly overly complex in other cases). However, before I create a ticket I am also interested to know if you encountered this case in a real piece of code (and in that case in which context because the reduced example you shared does not seem to make sense as a real piece of code) or if you trick the analyzer on purpose (which is completely fine and actually nice :) ) .

Cheers, 

Nicolas PERU | SonarSource

Senior Developer
http://sonarsource.com

https://twitter.com/benzonico

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/40441059-0804-46a0-bc70-f8f48360a00e%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Michał Kordas]

Hi Nicolas,

We use this pattern only in classes containing constants for test purposes to increase readablity. We usually have already defined constants for some particular messages/states/actions/anything, but if from test perspective it's not important that it is specific one, we give it alias name. Examples:

public static final List<Action> SOME_ACTIONS = USER_LOGIN_ACTIONS;

public static final List<Message> ANY_MESSAGES = SPECIFIC_MESSAGE_CHAIN;

public static final Set<Decision> DUMMY_DECISIONS = DECISIONS_NEEDED_TO_REPORT;

Thanks,

Michal Kordas

[Nicolas Peru]

Thanks for the precision. 

Just to go deeper in details : are those aliases defined in the same source file ? or are you importing them  ?

Cheers, 

Nicolas PERU | SonarSource

Senior Developer
http://sonarsource.com

https://twitter.com/benzonico

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/0e803535-cf42-4ac5-a5ca-7b6acd9b2cd4%40googlegroups.com.

[Michał Kordas]

Usually they are in the same file, but sometimes they are in different ones. Please see example:

TestConstants.java

public static final List<Action> USER_LOGIN_ACTIONS = ImmutableList.of(AUTH_PHASE_1, AUTH_PHASE_2);

public static final List<Action> SOME_ACTIONS = USER_LOGIN_ACTIONS;

public static final Set<Decision> DECISIONS_NEEDED_TO_REPORT = ImmutableSet.of(DECISION_GB, DECISION_US);

TestDummies.java

import static TestConstants.DECISIONS_NEEDED_TO_REPORT;

public static final Set<Decision> DUMMY_DECISIONS = DECISIONS_NEEDED_TO_REPORT;

Thanks,
Michal Kordas

[Michel Pawlak]

Hi,

The rule triggers on test sources ? 

If so why don't you exclude it on test sources directory  ?

Cheers,

Michel

[Michał Kordas]

Hi Michel,

Of course we can exclude it for tests or disable completely, but we find it very valuable for our code - it just has some false-positives, which shouldn't be the case for 'Critical' rule.

Best,

Michal K

[Nicolas Peru]

Hi Michal, 

Sorry for the long delay of answer. 

This is rather complex case for sonar java analyzer as tracking effective type is advanced work and requires way more than "just" fixing the rule. 

So as a  first approximation I would only focus on types that are accessible within the same source file. 

Ticket created to handle this case : https://jira.sonarsource.com/browse/SONARJAVA-1576

Cheers, 

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/11a647a7-14aa-46bf-9f4e-2be2a9a80726%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

[Michal Kordas]

Hi Nicolas,

That's perfect, thanks!

MK