From 74c6054b39bf1ac5895f3f3aab403b20efcce7e6 Mon Sep 17 00:00:00 2001 From: leonardo-pilastri-sonarsource <115481625+leonardo-pilastri-sonarsource@users.noreply.github.com> Date: Wed, 17 Apr 2024 09:08:01 +0200 Subject: [PATCH] SONARJAVA-4937 S1118 Add support for lombok generated constructors (#4768) --- .../java/org/sonar/java/it/AutoScanTest.java | 2 +- .../resources/autoscan/diffs/diff_S1118.json | 4 +- ...ClassWithPublicConstructorCheckSample.java | 206 ++++++++++++++++++ ...tilityClassWithPublicConstructorCheck.java | 46 +++- ...tilityClassWithPublicConstructorCheck.java | 156 ------------- ...tyClassWithPublicConstructorCheckTest.java | 4 +- 6 files changed, 253 insertions(+), 165 deletions(-) create mode 100644 java-checks-test-sources/default/src/main/java/checks/UtilityClassWithPublicConstructorCheckSample.java delete mode 100644 java-checks/src/test/files/checks/UtilityClassWithPublicConstructorCheck.java diff --git a/its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java b/its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java index 6c0407d0ea3..838a5c7673b 100644 --- a/its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java +++ b/its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java @@ -197,7 +197,7 @@ public void javaCheckTestSources() throws Exception { SoftAssertions softly = new SoftAssertions(); softly.assertThat(newDiffs).containsExactlyInAnyOrderElementsOf(knownDiffs.values()); softly.assertThat(newTotal).isEqualTo(knownTotal); - softly.assertThat(rulesCausingFPs).hasSize(8); + softly.assertThat(rulesCausingFPs).hasSize(9); softly.assertThat(rulesNotReporting).hasSize(10); /** diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S1118.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S1118.json index eb28be56c4f..b4cfbf310ee 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S1118.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S1118.json @@ -2,5 +2,5 @@ "ruleKey": "S1118", "hasTruePositives": true, "falseNegatives": 0, - "falsePositives": 0 -} \ No newline at end of file + "falsePositives": 3 +} diff --git a/java-checks-test-sources/default/src/main/java/checks/UtilityClassWithPublicConstructorCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/UtilityClassWithPublicConstructorCheckSample.java new file mode 100644 index 00000000000..dedc6062caf --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/UtilityClassWithPublicConstructorCheckSample.java @@ -0,0 +1,206 @@ +package checks; + +import java.io.Serializable; +import javax.annotation.CheckForNull; +import lombok.AccessLevel; +import lombok.AllArgsConstructor; +import lombok.NoArgsConstructor; +import lombok.RequiredArgsConstructor; + +import static lombok.AccessLevel.PRIVATE; + +class UtilityClassWithPublicConstructorCheckSample { + + @CheckForNull + class Coverage { // Noncompliant + public static void foo() { + } + } + + @NoArgsConstructor(access = AccessLevel.PRIVATE, force = true) + class LombokClass1 { // Compliant, a private constructor will be generated + public static void foo() { + } + } + + @NoArgsConstructor(access = AccessLevel.PUBLIC) + class LombokClass2 { // Noncompliant + public static void foo() { + } + } + + @NoArgsConstructor(force = true) + class LombokClass6 { // Noncompliant + public static void foo() { + } + } + + @AllArgsConstructor(access = PRIVATE) + class LombokClass3 { // Compliant, a private constructor will be generated + public static void foo() { + } + } + + @RequiredArgsConstructor(access = PRIVATE) + class LombokClass4 { // Compliant, a private constructor will be generated + public static void foo() { + } + } + + class Foo1 { + } + + class Foo2 { + public void foo() { + } + } + + class Foo3 { // Noncompliant [[sc=9;ec=13]] {{Add a private constructor to hide the implicit public one.}} + public static void foo() { + } + } + + class Foo4 { + public static void foo() { + } + + public void bar() { + } + } + + class Foo5 { + public Foo5() { // Noncompliant {{Hide this public constructor.}} + } + + public static void foo() { + } + } + + class Foo6 { + private Foo6() { + } + + public static void foo() { + } + + int foo; + + static int bar; + } + + class Foo7 { + + public Foo7(T foo) { // Noncompliant + } + + public static void foo(T foo) { + } + + } + + class Foo8 extends Bar { + + public static void f() { + } + + } + + class Foo9 { + + public int foo; + + public static void foo() { + } + + } + + class Foo10 { // Noncompliant + + public static int foo; + + ; + + } + + class Foo11 { + + protected Foo11() { + } + + public static int a; + + } + + class Foo12 { // Noncompliant + static class plop { + int a; + } + } + + class Foo13 { + + private Foo13() { + } + + ; + } + + class Foo14 { // Noncompliant [[sc=9;ec=14]] {{Add a private constructor to hide the implicit public one.}} + static { + } + } + + class Foo15 { + public Object o = new Object() { + public static void foo() { + } + }; + } + + class Foo16 implements Serializable { // Compliant + private static final long serialVersionUID = 1L; + } + + class Foo17 { + public Foo17() { + // do something + } + } + + class Main { // Compliant - contains main method + public static void main(String[] args) throws Exception { + System.out.println("Hello world!"); + } + } + + class NotMain { // Noncompliant + static void main(String[] args) throws Exception { + System.out.println("Hello world!"); + } + + static void main2(String[] args) { + System.out.println("Hello world!"); + } + } + + public class MySingleton { + private void MySingleton2() { + // use getInstance() + } + + private static class InitializationOnDemandHolderMySingleton { // compliant inner class is private, adding a private constructor won't change anything + static final checks.MySingleton INSTANCE = new checks.MySingleton(); + } + static class InitializationOnDemandHolderMySingleton2 { // Noncompliant + static final checks.MySingleton INSTANCE = new checks.MySingleton(); + } + private class InitializationOnDemandHolderMySingleton3 { + static final checks.MySingleton INSTANCE = new checks.MySingleton(); + } + + public static checks.MySingleton getInstance() { + return InitializationOnDemandHolderMySingleton.INSTANCE; + } + } + +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/UtilityClassWithPublicConstructorCheck.java b/java-checks/src/main/java/org/sonar/java/checks/UtilityClassWithPublicConstructorCheck.java index f5ef0a25a13..218feb18d3d 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/UtilityClassWithPublicConstructorCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/UtilityClassWithPublicConstructorCheck.java @@ -21,19 +21,29 @@ import java.util.Collections; import java.util.List; +import java.util.Set; import org.sonar.check.Rule; import org.sonar.java.checks.helpers.ClassPatternsUtils; import org.sonar.java.model.ModifiersUtils; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.tree.AnnotationTree; +import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; import org.sonar.plugins.java.api.tree.MethodTree; import org.sonar.plugins.java.api.tree.Modifier; import org.sonar.plugins.java.api.tree.Tree; - @Rule(key = "S1118") public class UtilityClassWithPublicConstructorCheck extends IssuableSubscriptionVisitor { + private static final Set LOMBOK_CONSTRUCTOR_GENERATORS = Set.of( + "lombok.NoArgsConstructor", + "lombok.AllArgsConstructor", + "lombok.RequiredArgsConstructor"); + @Override public List nodesToVisit() { return Collections.singletonList(Tree.Kind.CLASS); @@ -52,16 +62,16 @@ public void visitNode(Tree tree) { reportIssue(explicitConstructor.simpleName(), "Hide this public constructor."); } } - if (hasImplicitPublicConstructor) { + if (hasImplicitPublicConstructor && !hasCompliantGeneratedConstructors(classTree)) { reportIssue(classTree.simpleName(), "Add a private constructor to hide the implicit public one."); } } private static List getExplicitConstructors(ClassTree classTree) { return classTree.members().stream() - .filter(UtilityClassWithPublicConstructorCheck::isConstructor) - .map(MethodTree.class::cast) - .toList(); + .filter(UtilityClassWithPublicConstructorCheck::isConstructor) + .map(MethodTree.class::cast) + .toList(); } private static boolean isConstructor(Tree tree) { @@ -76,4 +86,30 @@ private static boolean hasPublicModifier(MethodTree methodTree) { return ModifiersUtils.hasModifier(methodTree.modifiers(), Modifier.PUBLIC); } + private static boolean hasCompliantGeneratedConstructors(ClassTree classTree) { + return classTree.modifiers().annotations().stream().anyMatch(it -> isLombokConstructorGenerator(it) && !hasPublicAccess(it)); + } + + private static boolean isLombokConstructorGenerator(AnnotationTree annotation) { + return LOMBOK_CONSTRUCTOR_GENERATORS.contains(annotation.annotationType().symbolType().fullyQualifiedName()); + } + + private static boolean hasPublicAccess(AnnotationTree annotation) { + return annotation.arguments().stream().noneMatch(it -> + isAccessLevelNotPublic(((AssignmentExpressionTree) it).expression()) + ); + } + + private static boolean isAccessLevelNotPublic(ExpressionTree tree) { + String valueName; + if (tree instanceof MemberSelectExpressionTree mset) { + valueName = mset.identifier().name(); + } else if (tree instanceof IdentifierTree identifier) { + valueName = identifier.name(); + } else { + return false; + } + return !"PUBLIC".equals(valueName); + } + } diff --git a/java-checks/src/test/files/checks/UtilityClassWithPublicConstructorCheck.java b/java-checks/src/test/files/checks/UtilityClassWithPublicConstructorCheck.java deleted file mode 100644 index 3db82a63027..00000000000 --- a/java-checks/src/test/files/checks/UtilityClassWithPublicConstructorCheck.java +++ /dev/null @@ -1,156 +0,0 @@ -import java.io.Serializable; -import java.lang.Object; - -class Foo1 { -} - -class Foo2 { - public int foo() { - } -} - -class Foo3 { // Noncompliant [[sc=7;ec=11]] {{Add a private constructor to hide the implicit public one.}} - public static void foo() { - } -} - -class Foo4 { - public static int foo() { - } - - public void bar() { - } -} - -class Foo5 { - public Foo5() { // Noncompliant {{Hide this public constructor.}} - } - - public static int foo() { - } -} - -class Foo6 { - private Foo6() { - } - - public static int foo() { - } - - int foo; - - static int bar; -} - -class Foo7 { - - public Foo7(T foo) { // Noncompliant - } - - public static void foo(T foo) { - } - -} - -class Foo8 extends Bar { - - public static void f() { - } - -} - -class Foo9 { - - public int foo; - - public static void foo() { - } - -} - -class Foo10 { // Noncompliant - - public static int foo; - - ; - -} - -class Foo11 { - - protected Foo11() { - } - - public static int a; - -} - -class Foo12 { // Noncompliant - static class plop { - int a; - } -} - -class Foo13 { - - private Foo13() { - } - - ; -} - -class Foo14 { // Noncompliant [[sc=7;ec=12]] {{Add a private constructor to hide the implicit public one.}} - static { - } -} - -class Foo15 { - public Object o = new Object() { - public static void foo() {} - }; -} - -class Foo16 implements Serializable { // Compliant - private static final long serialVersionUID = 1L; -} - -class Foo17 { - public Foo17() { - // do something - } -} - -public class Main { // Compliant - contains main method - public static void main(String[] args) throws Exception { - System.out.println("Hello world!"); - } -} - -public class NotMain { // Noncompliant - static void main(String[] args) throws Exception { - System.out.println("Hello world!"); - } - - static int main(String[] args) { - System.out.println("Hello world!"); - } -} - -public class MySingleton { - private MySingleton() { - // use getInstance() - } - - private static class InitializationOnDemandHolderMySingleton { // compliant inner class is private, adding a private constructor won't change anything - static final MySingleton INSTANCE = new MySingleton(); - } - static class InitializationOnDemandHolderMySingleton2 { // Noncompliant - static final MySingleton INSTANCE = new MySingleton(); - } - private class InitializationOnDemandHolderMySingleton3 { - static final MySingleton INSTANCE = new MySingleton(); - } - public static MySingleton getInstance() { - return InitializationOnDemandHolderMySingleton.INSTANCE; - } -} diff --git a/java-checks/src/test/java/org/sonar/java/checks/UtilityClassWithPublicConstructorCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/UtilityClassWithPublicConstructorCheckTest.java index ea707da64ae..90b3d53d70a 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/UtilityClassWithPublicConstructorCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/UtilityClassWithPublicConstructorCheckTest.java @@ -22,12 +22,14 @@ import org.junit.jupiter.api.Test; import org.sonar.java.checks.verifier.CheckVerifier; +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; + class UtilityClassWithPublicConstructorCheckTest { @Test void test() { CheckVerifier.newVerifier() - .onFile("src/test/files/checks/UtilityClassWithPublicConstructorCheck.java") + .onFile(mainCodeSourcesPath("checks/UtilityClassWithPublicConstructorCheckSample.java")) .withCheck(new UtilityClassWithPublicConstructorCheck()) .verifyIssues(); }