Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- correction of technical problem with Integration tests (because of Maven format in technical answer to "sonar-orchestrator-junit5" library)
- upgrade JDK from 11 to 17
- [#4](https://github.com/green-code-initiative/creedengo-java/issues/4) Improvement: "++i" statement is not so bad
- [#113](https://github.com/green-code-initiative/creedengo-java/pull/113) fix GCI82 rule to handle Lombok generated setters

### Deleted

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -440,8 +440,8 @@ void testGCI82() {
String filePath = "src/main/java/org/greencodeinitiative/creedengo/java/checks/MakeNonReassignedVariablesConstants.java";
String ruleId = "creedengo-java:GCI82";
String ruleMsg = "The variable is never reassigned and can be 'final'";
int[] startLines = new int[]{7, 12, 13, 45};
int[] endLines = new int[]{7, 12, 13, 45};
int[] startLines = new int[]{13, 18, 19, 51, 82, 91};
int[] endLines = new int[]{13, 18, 19, 51, 83, 92};

checkIssuesForFile(filePath, ruleId, ruleMsg, startLines, endLines);
}
Expand Down
27 changes: 26 additions & 1 deletion src/it/test-projects/creedengo-java-plugin-test-project/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,31 @@
<artifactId>spring-beans</artifactId>
<version>5.3.25</version>
</dependency>
<dependency>
<groupId>org.projectlombok</groupId>
<artifactId>lombok</artifactId>
<version>1.18.38</version>
</dependency>
</dependencies>

</project>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.14.0</version>
<configuration>
<source>17</source>
<target>17</target>
<annotationProcessorPaths>
<path>
<groupId>org.projectlombok</groupId>
<artifactId>lombok</artifactId>
<version>1.18.38</version>
</path>
</annotationProcessorPaths>
</configuration>
</plugin>
</plugins>
</build>
</project>
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
import java.util.logging.Logger;
import lombok.Setter;
import lombok.Data;
import lombok.AccessLevel;

public class MakeNonReassignedVariablesConstants {

private final Logger logger = Logger.getLogger(""); // Compliant

@Setter
private String myLombokManagedString = "initialValue"; // Compliant

private Object myNonFinalAndNotReassignedObject = new Object(); // Noncompliant {{The variable is never reassigned and can be 'final'}}
private Object myNonFinalAndReassignedObject = new Object(); // Compliant
private final Object myFinalAndNotReassignedObject = new Object(); // Compliant
Expand Down Expand Up @@ -66,4 +72,22 @@ void classVariableReassignedBis() {
logger.info(myFinalAndNotReassignedObject.toString());
}

}
}

@Setter
class myExtraClassWithLombokSetter {
private String myExtraClassString = "initialValue"; // Compliant
private final String myExtraClassFinalString = "initialValue"; // Compliant

@Setter(AccessLevel.NONE) // Noncompliant {{The variable is never reassigned and can be 'final'}}
private String myExtraClassSetterNoneString = "initialValue";
}

@Data
class myExtraClassWithLombokData {
private String myExtraClassString = "initialValue"; // Compliant
private final String myExtraClassFinalString = "initialValue"; // Compliant

@Setter(AccessLevel.NONE) // Noncompliant {{The variable is never reassigned and can be 'final'}}
private String myExtraClassSetterNoneString = "initialValue";
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,28 @@ public class MakeNonReassignedVariablesConstants extends IssuableSubscriptionVis

private static final Logger LOGGER = Loggers.get(MakeNonReassignedVariablesConstants.class);

private final String LOMBOK_SETTER = "Setter";
private final String LOMBOK_DATA = "Data";

private boolean hasParsedImports = false;
private boolean hasLombokSetterImport = false;
private boolean hasLombokDataImport = false;


@Override
public List<Kind> nodesToVisit() {
return List.of(Kind.VARIABLE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can improve more efficiently the import checks. You can add "Kind.IMPORT" in the list of nodes to check. Because Imports are the first statements analyezed in the code, you can know if lombok importrs are present or not.

}

@Override
public void visitNode(@Nonnull Tree tree) {
VariableTree variableTree = (VariableTree) tree;
LOGGER.debug("Variable > " + getVariableNameForLogger(variableTree));
LOGGER.debug(" => isNotFinalAndNotStatic(variableTree) = " + isNotFinalAndNotStatic(variableTree));
LOGGER.debug(" => usages = " + variableTree.symbol().usages().size());
LOGGER.debug(" => isNotReassigned = " + isNotReassigned(variableTree));

if (isNotFinalAndNotStatic(variableTree) && isNotReassigned(variableTree)) {
VariableTree variableTree = (VariableTree) tree;
LOGGER.info("Variable > " + getVariableNameForLogger(variableTree));
LOGGER.info(" => isNotFinalAndNotStatic(variableTree) = " + isNotFinalAndNotStatic(variableTree));
LOGGER.info(" => usages = " + variableTree.symbol().usages().size());
LOGGER.info(" => isNotReassigned = " + isNotReassigned(variableTree));
if (hasNoLombokSetter(variableTree) && isNotFinalAndNotStatic(variableTree) && isNotReassigned(variableTree)) {
reportIssue(tree, MESSAGE_RULE);
} else {
super.visitNode(tree);
Expand Down Expand Up @@ -103,6 +111,77 @@ public static boolean hasModifier(ModifiersTree modifiersTree, Modifier expected
return false;
}

private boolean hasNoLombokSetter(VariableTree variableTree) {
// Check if the variable is annotated with @Setter

for (AnnotationTree annotation : variableTree.modifiers().annotations()) {
if (annotation.annotationType().toString().equals(LOMBOK_SETTER)) {
if (hasLombokImport(variableTree, LOMBOK_SETTER)) {

// Ignore if the annotation has AccessLevel.NONE
if (!annotation.arguments().isEmpty()) {
for (ExpressionTree argument : annotation.arguments()) {
if (argument.is(Kind.MEMBER_SELECT)) {
MemberSelectExpressionTree memberSelectExpressionTree = (MemberSelectExpressionTree) argument;
if (memberSelectExpressionTree.expression().toString().equals("AccessLevel")
&& memberSelectExpressionTree.identifier().name().equals("NONE")) {
return true;
}
}
}
}
return false;
}
}
}
// Check if the variable is in a class with @Setter or with @Data
if( variableTree.parent() != null && !variableTree.parent().is(Kind.CLASS)){
return true;
}
if (variableTree.parent() != null && variableTree.parent().is(Kind.CLASS)) {
ClassTree classTree = (ClassTree) variableTree.parent();
for (AnnotationTree annotation : classTree.modifiers().annotations()) {
if (annotation.annotationType().toString().equals(LOMBOK_SETTER) && hasLombokImport(variableTree, LOMBOK_SETTER)) {
return false;
}
if (annotation.annotationType().toString().equals(LOMBOK_DATA) && hasLombokImport(variableTree, LOMBOK_DATA)) {
return false;
}
}
}

return true;
}

private boolean hasLombokImport(VariableTree variableTree, String lombokImport) {
if (!hasParsedImports) {
Tree currentTree = variableTree;
while (currentTree.parent() != null && !currentTree.parent().is(Kind.COMPILATION_UNIT)) {
currentTree = currentTree.parent();
}
if (currentTree != null) {
CompilationUnitTree rootNode = (CompilationUnitTree) currentTree.parent();
for (var importClauseTree : rootNode.imports()) {
ImportTree importTree = (ImportTree) importClauseTree;
MemberSelectExpressionTree identifier = (MemberSelectExpressionTree) importTree.qualifiedIdentifier();

if ("lombok".equals(identifier.expression().toString())) {
if ("*".equals(identifier.identifier().name())) {
hasLombokSetterImport = true;
hasLombokDataImport = true;
} else if (LOMBOK_SETTER.equals(identifier.identifier().name())) {
hasLombokSetterImport = true;
} else if (LOMBOK_DATA.equals(identifier.identifier().name())) {
hasLombokDataImport = true;
}
}
}
}
hasParsedImports = true;
}
return LOMBOK_SETTER.equals(lombokImport) ? hasLombokSetterImport : hasLombokDataImport;
}

private String getVariableNameForLogger(VariableTree variableTree) {
String name = variableTree.simpleName().name();

Expand Down
26 changes: 25 additions & 1 deletion src/test/files/MakeNonReassignedVariablesConstants.java
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
import java.util.logging.Logger;
import lombok.Setter;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the first use case ... using real imports like that.
but there is a second use case missing : if you don't use import but "@lombok.Setter" directly for example
could you add this second use case for all lombok imports ? you can add a second test file and a second unit test to check it.

import lombok.Data;
import lombok.AccessLevel;

public class MakeNonReassignedVariablesConstants {

private final Logger logger = Logger.getLogger(""); // Compliant

@Setter
private String myLombokManagedString = "initialValue"; // Compliant

private Object myNonFinalAndNotReassignedObject = new Object(); // Noncompliant {{The variable is never reassigned and can be 'final'}}
private Object myNonFinalAndReassignedObject = new Object(); // Compliant
private final Object myFinalAndNotReassignedObject = new Object(); // Compliant
Expand Down Expand Up @@ -66,4 +72,22 @@ void classVariableReassignedBis() {
logger.info(myFinalAndNotReassignedObject.toString());
}

}
}

@Setter
class myExtraClassWithLombokSetter {
private String myExtraClassString = "initialValue"; // Compliant
private final String myExtraClassFinalString = "initialValue"; // Compliant

@Setter(AccessLevel.NONE) // Noncompliant {{The variable is never reassigned and can be 'final'}}
private String myExtraClassSetterNoneString = "initialValue";
}

@Data
class myExtraClassWithLombokData {
private String myExtraClassString = "initialValue"; // Compliant
private final String myExtraClassFinalString = "initialValue"; // Compliant

@Setter(AccessLevel.NONE) // Noncompliant {{The variable is never reassigned and can be 'final'}}
private String myExtraClassSetterNoneString = "initialValue";
}