S2384 Mutable members should not be stored or returned directly
Environment:
SonarQube Version: 10.0.0.68432
Description:
In the provided code examples, SonarQube reports the violation of rule S2384 when the String[] arr field is private, but does not report the violation when the field is not private. However, it is still a good practice to make the field private and use the clone() method when assigning values to ensure the safety of the class state.
Here are the code samples:
With private field (SonarQube reports the violation):
Copy code
public class Foo {
private String[] arr;
void foo(String[] x) {
arr = x; // report here
}
void bar(Foo f) {
f.foo(arr);
}
public static void main(String[] args) {
Foo f1 = new Foo();
Foo f2 = new Foo();
f1.bar(f2);
}
}
Without private field (SonarQube does not report the violation):
Copy code
public class Foo {
String[] arr;
void foo(String[] x) {
arr = x; //FN, should report here
}
void bar(Foo f) {
f.foo(arr);
}
public static void main(String[] args) {
Foo f1 = new Foo();
Foo f2 = new Foo();
f1.bar(f2);
}
}
Hello Seren, first of all, thank you for reaching out and sharing your opinions.
Your concern about the scope of the Java:S2384 rule is valid and the reason why we decide to limit it to private mutable members is because, for such cases, the developer’s intention to restrict the access is clear hence the rule is perfectly applicable.
When a mutable field is non-private we can’t make any assumptions: it may be totally reasonable that the developer’s intent is to keep the field accessible and thus the rule would be inappropriate.
I agree that the Java:S2384 rule should be more explicit in mentioning that it applies only to private mutable members; I will take care of updating the title and the description.
I was taught, that one shouldn’t make fields accessible directly, but only with a pair of setters and getters and keep the fields themselves private.
If I’m reading it right, then SonarQube would warn against a “good” practice, where instead it should rather warn about non-private fields with getters and setters, where these getters and setters could be skipped and the fields modified without calling the setters.
The main point of setters and getters would be, that they could be easily changed lateron, to facilitate for logging and debugging - for this to make sense, all accesses from “outside” must use the setters and getters. The field is made private not to prevent any changes to it at all, but to prevent bypassing of the setter/getter.
SonarQube is not warning against any good practice, rather the goal of each rule is to favor good practices while keeping the false positive rates as low as possible (ideally zero).
Let me try to clarify what the discussion is about and give you more context to avoid possible confusion:
the discussion is about the direct accessibility of mutable members and not about the use of getters and setters
the rule S2384 basically says that: when you have a private mutable field then you should store it as a copy of the original mutable object, in the constructors and in the setters, and you should return a copy of such mutable field in the getters; see Effective Java - Make defensive copies when needed
we define static rules that are sure to be generally valid (i.e. no false positives)
we cannot define rules that make assumptions about the intent of the code: for this reason, when there are legitimate cases where a rule doesn’t apply then we don’t flag it
I hope my explanation helps you to have a clearer idea and a better understanding.