Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SONARJAVA-4972 S1118 Fix FPs on Lombok annotations in automatic analysis #5035

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,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(9);
softly.assertThat(rulesCausingFPs).hasSize(8);
softly.assertThat(rulesNotReporting).hasSize(11);

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@
"ruleKey": "S1118",
"hasTruePositives": true,
"falseNegatives": 0,
"falsePositives": 3
"falsePositives": 0
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,31 +18,49 @@ public static void foo() {
}

@NoArgsConstructor(access = AccessLevel.PRIVATE, force = true)
class LombokClass1 { // Compliant, a private constructor will be generated
class LombokClassNoArgsPrivate { // Compliant, a private constructor will be generated
public static void foo() {
}
}

@NoArgsConstructor(access = AccessLevel.PUBLIC)
class LombokClass2 { // Noncompliant
class LombokClassNoArgsPublic { // Noncompliant
public static void foo() {
}
}

@NoArgsConstructor(access = AccessLevel.NONE)
class LombokClassNoArgsNone { // Compliant
public static void foo() {
}
}

@lombok.NoArgsConstructor(access = AccessLevel.PUBLIC)
class LombokClassFullyQualifiedNoArgsPublic { // Noncompliant
public static void foo() {
}
}

@NoArgsConstructor(force = true)
class LombokClass6 { // Noncompliant
class LombokClassNoArgs { // Noncompliant
public static void foo() {
}
}

@AllArgsConstructor(access = PRIVATE)
class LombokClass3 { // Compliant, a private constructor will be generated
class LombokClassAllArgsPrivate { // Compliant, a private constructor will be generated
public static void foo() {
}
}

@lombok.AllArgsConstructor(access = PRIVATE)
class LombokClassFullyQualifiedAllArgsPrivate { // Compliant, a private constructor will be generated
public static void foo() {
}
}

@RequiredArgsConstructor(access = PRIVATE)
class LombokClass4 { // Compliant, a private constructor will be generated
class LombokClassRequiresPrivate { // Compliant, a private constructor will be generated
public static void foo() {
}
}
Expand Down Expand Up @@ -205,4 +223,24 @@ public static checks.MySingleton getInstance() {
}
}

static class CustomLombokLikeAnnotation {
// Custom Lombok-like annotation.
@interface NoArgsConstructor {
AccessLevel access() default AccessLevel.PUBLIC;
}

// This one is tricky - the annotation is not the real one, so it is noncompliant.
@NoArgsConstructor(access = AccessLevel.PRIVATE)
class CustomPrivate { // Noncompliant
public static void foo() {
}
}

@NoArgsConstructor(access = AccessLevel.PUBLIC)
class CustomPublic { // Noncompliant
public static void foo() {
}
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,13 @@
import java.util.Collections;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
import org.sonar.check.Rule;
import org.sonar.java.checks.helpers.AnnotationsHelper;
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.semantic.Type;
import org.sonar.plugins.java.api.tree.AnnotationTree;
import org.sonar.plugins.java.api.tree.AssignmentExpressionTree;
import org.sonar.plugins.java.api.tree.ClassTree;
Expand All @@ -36,11 +39,19 @@
@Rule(key = "S1118")
public class UtilityClassWithPublicConstructorCheck extends IssuableSubscriptionVisitor {

/**
* See also {@link org.sonar.java.filters.LombokFilter}.
*/
private static final Set<String> LOMBOK_CONSTRUCTOR_GENERATORS = Set.of(
"lombok.NoArgsConstructor",
"lombok.AllArgsConstructor",
"lombok.RequiredArgsConstructor");

private static final Set<String> LOMBOK_CONSTRUCTOR_GENERATOR_NAMES =
LOMBOK_CONSTRUCTOR_GENERATORS.stream()
.map(AnnotationsHelper::annotationTypeIdentifier)
.collect(Collectors.toSet());

@Override
public List<Tree.Kind> nodesToVisit() {
return Collections.singletonList(Tree.Kind.CLASS);
Expand Down Expand Up @@ -84,11 +95,17 @@ private static boolean hasPublicModifier(MethodTree methodTree) {
}

private static boolean hasCompliantGeneratedConstructors(ClassTree classTree) {
return classTree.modifiers().annotations().stream().anyMatch(it -> isLombokConstructorGenerator(it) && !hasPublicAccess(it));
return classTree.modifiers().annotations().stream()
.anyMatch(it -> isLombokConstructorGenerator(it.symbolType()) && !hasPublicAccess(it));
}

private static boolean isLombokConstructorGenerator(AnnotationTree annotation) {
return LOMBOK_CONSTRUCTOR_GENERATORS.contains(annotation.annotationType().symbolType().fullyQualifiedName());
private static boolean isLombokConstructorGenerator(Type symbolType) {
// This happens in automatic analysis. We match only the last part of the name.
if (symbolType.isUnknown()) {
return LOMBOK_CONSTRUCTOR_GENERATOR_NAMES.contains(symbolType.name());
}

return LOMBOK_CONSTRUCTOR_GENERATORS.contains(symbolType.fullyQualifiedName());
}

private static boolean hasPublicAccess(AnnotationTree annotation) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,13 @@ void test() {
.withCheck(new UtilityClassWithPublicConstructorCheck())
.verifyIssues();
}

@Test
void test_without_semantic() {
CheckVerifier.newVerifier()
.onFile(mainCodeSourcesPath("checks/UtilityClassWithPublicConstructorCheckSample.java"))
.withCheck(new UtilityClassWithPublicConstructorCheck())
.withoutSemantic()
.verifyIssues();
}
}