Skip to content

Commit 74c6054

Browse files
leonardo-pilastri-sonarsourceleonardo-pilastri-sonarsource
authored
SONARJAVA-4937 S1118 Add support for lombok generated constructors (#4768)
1 parent ca46a10 commit 74c6054

6 files changed

Lines changed: 253 additions & 165 deletions

File tree

‎its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ public void javaCheckTestSources() throws Exception {
197197
SoftAssertions softly = new SoftAssertions();
198198
softly.assertThat(newDiffs).containsExactlyInAnyOrderElementsOf(knownDiffs.values());
199199
softly.assertThat(newTotal).isEqualTo(knownTotal);
200-
softly.assertThat(rulesCausingFPs).hasSize(8);
200+
softly.assertThat(rulesCausingFPs).hasSize(9);
201201
softly.assertThat(rulesNotReporting).hasSize(10);
202202

203203
/**

‎its/autoscan/src/test/resources/autoscan/diffs/diff_S1118.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,5 +2,5 @@
22
"ruleKey": "S1118",
33
"hasTruePositives": true,
44
"falseNegatives": 0,
5-
"falsePositives": 0
6-
}
5+
"falsePositives": 3
6+
}

‎java-checks-test-sources/default/src/main/java/checks/UtilityClassWithPublicConstructorCheckSample.java‎

Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,206 @@
1+
package checks;
2+
3+
import java.io.Serializable;
4+
import javax.annotation.CheckForNull;
5+
import lombok.AccessLevel;
6+
import lombok.AllArgsConstructor;
7+
import lombok.NoArgsConstructor;
8+
import lombok.RequiredArgsConstructor;
9+
10+
import static lombok.AccessLevel.PRIVATE;
11+
12+
class UtilityClassWithPublicConstructorCheckSample {
13+
14+
@CheckForNull
15+
class Coverage { // Noncompliant
16+
public static void foo() {
17+
}
18+
}
19+
20+
@NoArgsConstructor(access = AccessLevel.PRIVATE, force = true)
21+
class LombokClass1 { // Compliant, a private constructor will be generated
22+
public static void foo() {
23+
}
24+
}
25+
26+
@NoArgsConstructor(access = AccessLevel.PUBLIC)
27+
class LombokClass2 { // Noncompliant
28+
public static void foo() {
29+
}
30+
}
31+
32+
@NoArgsConstructor(force = true)
33+
class LombokClass6 { // Noncompliant
34+
public static void foo() {
35+
}
36+
}
37+
38+
@AllArgsConstructor(access = PRIVATE)
39+
class LombokClass3 { // Compliant, a private constructor will be generated
40+
public static void foo() {
41+
}
42+
}
43+
44+
@RequiredArgsConstructor(access = PRIVATE)
45+
class LombokClass4 { // Compliant, a private constructor will be generated
46+
public static void foo() {
47+
}
48+
}
49+
50+
class Foo1 {
51+
}
52+
53+
class Foo2 {
54+
public void foo() {
55+
}
56+
}
57+
58+
class Foo3 { // Noncompliant [[sc=9;ec=13]] {{Add a private constructor to hide the implicit public one.}}
59+
public static void foo() {
60+
}
61+
}
62+
63+
class Foo4 {
64+
public static void foo() {
65+
}
66+
67+
public void bar() {
68+
}
69+
}
70+
71+
class Foo5 {
72+
public Foo5() { // Noncompliant {{Hide this public constructor.}}
73+
}
74+
75+
public static void foo() {
76+
}
77+
}
78+
79+
class Foo6 {
80+
private Foo6() {
81+
}
82+
83+
public static void foo() {
84+
}
85+
86+
int foo;
87+
88+
static int bar;
89+
}
90+
91+
class Foo7 {
92+
93+
public <T> Foo7(T foo) { // Noncompliant
94+
}
95+
96+
public static <T> void foo(T foo) {
97+
}
98+
99+
}
100+
101+
class Foo8 extends Bar {
102+
103+
public static void f() {
104+
}
105+
106+
}
107+
108+
class Foo9 {
109+
110+
public int foo;
111+
112+
public static void foo() {
113+
}
114+
115+
}
116+
117+
class Foo10 { // Noncompliant
118+
119+
public static int foo;
120+
121+
;
122+
123+
}
124+
125+
class Foo11 {
126+
127+
protected Foo11() {
128+
}
129+
130+
public static int a;
131+
132+
}
133+
134+
class Foo12 { // Noncompliant
135+
static class plop {
136+
int a;
137+
}
138+
}
139+
140+
class Foo13 {
141+
142+
private Foo13() {
143+
}
144+
145+
;
146+
}
147+
148+
class Foo14 { // Noncompliant [[sc=9;ec=14]] {{Add a private constructor to hide the implicit public one.}}
149+
static {
150+
}
151+
}
152+
153+
class Foo15 {
154+
public Object o = new Object() {
155+
public static void foo() {
156+
}
157+
};
158+
}
159+
160+
class Foo16 implements Serializable { // Compliant
161+
private static final long serialVersionUID = 1L;
162+
}
163+
164+
class Foo17 {
165+
public Foo17() {
166+
// do something
167+
}
168+
}
169+
170+
class Main { // Compliant - contains main method
171+
public static void main(String[] args) throws Exception {
172+
System.out.println("Hello world!");
173+
}
174+
}
175+
176+
class NotMain { // Noncompliant
177+
static void main(String[] args) throws Exception {
178+
System.out.println("Hello world!");
179+
}
180+
181+
static void main2(String[] args) {
182+
System.out.println("Hello world!");
183+
}
184+
}
185+
186+
public class MySingleton {
187+
private void MySingleton2() {
188+
// use getInstance()
189+
}
190+
191+
private static class InitializationOnDemandHolderMySingleton { // compliant inner class is private, adding a private constructor won't change anything
192+
static final checks.MySingleton INSTANCE = new checks.MySingleton();
193+
}
194+
static class InitializationOnDemandHolderMySingleton2 { // Noncompliant
195+
static final checks.MySingleton INSTANCE = new checks.MySingleton();
196+
}
197+
private class InitializationOnDemandHolderMySingleton3 {
198+
static final checks.MySingleton INSTANCE = new checks.MySingleton();
199+
}
200+
201+
public static checks.MySingleton getInstance() {
202+
return InitializationOnDemandHolderMySingleton.INSTANCE;
203+
}
204+
}
205+
206+
}

‎java-checks/src/main/java/org/sonar/java/checks/UtilityClassWithPublicConstructorCheck.java‎

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,19 +21,29 @@
2121

2222
import java.util.Collections;
2323
import java.util.List;
24+
import java.util.Set;
2425
import org.sonar.check.Rule;
2526
import org.sonar.java.checks.helpers.ClassPatternsUtils;
2627
import org.sonar.java.model.ModifiersUtils;
2728
import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
29+
import org.sonar.plugins.java.api.tree.AnnotationTree;
30+
import org.sonar.plugins.java.api.tree.AssignmentExpressionTree;
2831
import org.sonar.plugins.java.api.tree.ClassTree;
32+
import org.sonar.plugins.java.api.tree.ExpressionTree;
33+
import org.sonar.plugins.java.api.tree.IdentifierTree;
34+
import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree;
2935
import org.sonar.plugins.java.api.tree.MethodTree;
3036
import org.sonar.plugins.java.api.tree.Modifier;
3137
import org.sonar.plugins.java.api.tree.Tree;
3238

33-
3439
@Rule(key = "S1118")
3540
public class UtilityClassWithPublicConstructorCheck extends IssuableSubscriptionVisitor {
3641

42+
private static final Set<String> LOMBOK_CONSTRUCTOR_GENERATORS = Set.of(
43+
"lombok.NoArgsConstructor",
44+
"lombok.AllArgsConstructor",
45+
"lombok.RequiredArgsConstructor");
46+
3747
@Override
3848
public List<Tree.Kind> nodesToVisit() {
3949
return Collections.singletonList(Tree.Kind.CLASS);
@@ -52,16 +62,16 @@ public void visitNode(Tree tree) {
5262
reportIssue(explicitConstructor.simpleName(), "Hide this public constructor.");
5363
}
5464
}
55-
if (hasImplicitPublicConstructor) {
65+
if (hasImplicitPublicConstructor && !hasCompliantGeneratedConstructors(classTree)) {
5666
reportIssue(classTree.simpleName(), "Add a private constructor to hide the implicit public one.");
5767
}
5868
}
5969

6070
private static List<MethodTree> getExplicitConstructors(ClassTree classTree) {
6171
return classTree.members().stream()
62-
.filter(UtilityClassWithPublicConstructorCheck::isConstructor)
63-
.map(MethodTree.class::cast)
64-
.toList();
72+
.filter(UtilityClassWithPublicConstructorCheck::isConstructor)
73+
.map(MethodTree.class::cast)
74+
.toList();
6575
}
6676

6777
private static boolean isConstructor(Tree tree) {
@@ -76,4 +86,30 @@ private static boolean hasPublicModifier(MethodTree methodTree) {
7686
return ModifiersUtils.hasModifier(methodTree.modifiers(), Modifier.PUBLIC);
7787
}
7888

89+
private static boolean hasCompliantGeneratedConstructors(ClassTree classTree) {
90+
return classTree.modifiers().annotations().stream().anyMatch(it -> isLombokConstructorGenerator(it) && !hasPublicAccess(it));
91+
}
92+
93+
private static boolean isLombokConstructorGenerator(AnnotationTree annotation) {
94+
return LOMBOK_CONSTRUCTOR_GENERATORS.contains(annotation.annotationType().symbolType().fullyQualifiedName());
95+
}
96+
97+
private static boolean hasPublicAccess(AnnotationTree annotation) {
98+
return annotation.arguments().stream().noneMatch(it ->
99+
isAccessLevelNotPublic(((AssignmentExpressionTree) it).expression())
100+
);
101+
}
102+
103+
private static boolean isAccessLevelNotPublic(ExpressionTree tree) {
104+
String valueName;
105+
if (tree instanceof MemberSelectExpressionTree mset) {
106+
valueName = mset.identifier().name();
107+
} else if (tree instanceof IdentifierTree identifier) {
108+
valueName = identifier.name();
109+
} else {
110+
return false;
111+
}
112+
return !"PUBLIC".equals(valueName);
113+
}
114+
79115
}

0 commit comments

Comments
 (0)