Merge pull request #160 from green-code-initiative/PR_70_DDC

[EC79] NullPointer exception in eco code java Sonar plugin

Author: dedece35

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- [#69](https://github.com/green-code-initiative/creedengo-java/issues/69) correction of NullPointer in GCI79 rule + technical refactoring of GCI79
 - update integration tests system to use the new component "creedengo-integration-test"
 
 ### Deleted

IA_description.md

@@ -0,0 +1,88 @@
+This file contains the technical description of the project
+
+Global description
+---
+This project is a SonarQube plugin project and it is named "creedengo-java".
+This plugin is for Java project analysis. 
+When this plugin is installed inside SonarQube, it adds some new rules for Java language analysis.
+The new rules can be added to an existing Java quality profile or to a new Java quality profile.
+A Sonarqube quality profile must be associated to only one programming language.
+A quality profile can be the default quality profile for one language (here Java language), thus 
+all future Java project analysis will be done by default with this quality profile.
+If a quality profile is not the default, a specific project (already analysed) can be associated with this
+quality profile.
+
+Technical description
+---
+Important elements of the plugin :
+- the plugin contains a list of rule implementations
+- each rule has a unique rule id
+- each rule is implemented by one class
+- there are several tests for each implementation rule class with several ressource files containing compliant code or / and non compliant code
+
+The implementation code is in src/main/java.
+
+Sonarqube analysis are based on the navigation inside the code to detect bad practices and to raise an issue when detected.
+The code navigation is done through the AST principle.
+
+Implementation Structure
+---
+- rule implementations : inside the package org.greencodeinitiative.creedengo.java.checks
+- each implementation class has the same code template :
+  - "Rule" annotation : to give the rule id
+    - the rule id is previously defined in another maven component named "creedengo-rules-specifications"
+    - to enable a rule, the rule id must be added in the resources file named "creedengo_way_profile.json"
+  - extends a SonarQube API class "IssuableSubscriptionVisitor"
+  - "initialize" method (forom super-class) to declare for which AST node type, the analysis will deeply analyse the code.
+    - each "registerSyntaxNodeConsumer" method call implies that the node will be deeply analyzed. 
+    - For each, a new private method is given to implement the deeply analysis and raise an issue if needed.
+- to have a plugin working with activated rules, the Sonarqube plugin development guidelines is followed
+  - "JavaPlugin" : enable 2 following extensions
+  - "JavaRuleRepository" : extension containing the definition of the plugin and the list of implemented rule classes
+  - "JavaCreedengoWayProfile" : extension containing the definition a the quality profile (and rules activated inside) created with the plugin installation
+
+Unit Tests Implementation Structure
+---
+Inside the "test/java" directory, there are unit tests for each class of the plugin.
+The package "org.greencodeinitiative.creedengo.java.checks" contains one unit test class for each rule implementation class
+
+Each unit test class checking rule implementation class has the same template of code :
+- at least one test method using the "CheckVerifier" class to check and simulate a SonarQube analysis
+  - usage of "CheckVerifier.verify" to check if there is some issues raised or not 
+  - usage of "CheckVerifier.verifyNoIssues" to check there is no issues raised
+- each call to "CheckVerifier" needs a test resource file : this one is the resource code file for the simulated analysis
+
+each test resource file is in the "src/test/files" directory (or sub-directories).
+each test resource file contains compliuant code or / and non compliant code.
+If there is non compliant code on which Sonarqube analysis should raise an issue, the line with the issue has a comment at the end of the line with the following template : 
+"// Noncompliant {{ERROR_MESSAGE_TO_DISPLAY}}"
+    - the "ERROR_MESSAGE_TO_DISPLAY" in the previous template is replaced by the real error message.
+    - this comment gives the information to "CheckVerifier", the simulation tool, that an error is expected at this line
+
+Integration Tests Implementation Structure
+---
+Inside the "it/java" directory, there are integration tests for each class of the plugin.
+The package "org.greencodeinitiative.creedengo.java.integration.tests" contains one unit test class with one method for each rule.
+
+The integration test class extends the common class "GCIRulesBase" to use the system to initialize integration test.
+
+Integration test system process exists to check in a local and real environment that all implemented rules do the raises expected issues.
+
+The "src/it/test-projects" directory contains a real project to analyse.
+
+Here is the process 
+- build the plugin project
+- download and launch a specific sonarqube version in local machine
+- install the built plugin inside sonarqube
+- create a specific default quality profile with all rules of the plugin installed
+- launch the analysis of the test-project
+- send analysis result to local sonarqube
+- check the result of the analysis in front of expected results describe inside each test method
+
+Each test method describe different elements to check inside the result analysis :
+- the relative path of each resource test file inside de test-project
+- the complete rule id containing 2 parts : the plugin id ("creedengo-java") and the rule id (ex : "GCI2")
+- the rule error message
+- the lines where errors shoudl be raised : there are 2 arrays with the same size representing the start line and the end line of each issue raised
+
+Each test method ends with a call of a common method with all these parameters input.

IA_rule.md

@@ -0,0 +1,33 @@
+Rule Implementation Plan
+---
+
+# Rule Implementation Methodology
+
+Use the TDD (Test-Driven Development) methodology to implement a rule in an efficient and structured way.
+
+# Rule Implementation Process
+
+## STEP 1: Define unit test resources + technical shell of the rule
+- Define test resources for the rule to be implemented with all possible cases:
+  - Compliant code
+  - Non-compliant code
+  - Code with elements that should not be analyzed
+- Location of test resource files:
+    - Files for unit tests: in "src/test/files" or subdirectories
+    - Files for integration tests: in "src/it/test-projects" or subdirectories
+- Create the rule implementation class in the package "org.greencodeinitiative.creedengo.java.checks"
+    - Add the "Rule" annotation with the correct rule id (the rule is previously defined in another maven component named "creedengo-rules-specifications")
+    - Extend the "IssuableSubscriptionVisitor" class
+    - Override the "nodesToVisit", "visitNode" and (optionally) "leaveNode" methods, which will be used respectively to declare the AST node types to analyze in depth and to implement the analysis logic, but leave their bodies empty for now
+- Create the rule unit test class in the package "org.greencodeinitiative.creedengo.java.checks"
+- Verify that unit tests fail with non-compliant test resources
+
+## STEP 2: Implement the rule + unit test validation
+- Implement the rule in the implementation class created in step 1
+- Implement the "nodesToVisit" method to declare the AST node types to analyze in depth and implement the "visitNode" and, if needed, "leaveNode" methods to perform the analysis on those nodes
+- Implement private methods to analyze in depth the declared AST nodes and raise issues if needed
+- Verify that unit tests pass with both compliant and non-compliant test resources, and call them from "visitNode"/"leaveNode"
+
+## STEP 3: Implement integration tests
+- Add a test method in the integration class "GCIRulesIT" for the implemented rule using the same pattern as other existing test methods
+- Verify that integration tests pass with both compliant and non-compliant test resources

src/it/java/org/greencodeinitiative/creedengo/java/integration/tests/GCIRulesIT.java

@@ -408,7 +408,7 @@ void testGCI76_good() {
     void testGCI79() {
         String filePath = "src/main/java/org/greencodeinitiative/creedengo/java/checks/FreeResourcesOfAutoCloseableInterface.java";
         String ruleId = "creedengo-java:GCI79";
-        String ruleMsg = "try-with-resources Statement needs to be implemented for any object that implements the AutoClosable interface.";
+        String ruleMsg = "try-with-resources Statement needs to be implemented for any object that implements the AutoCloseable interface.";
         int[] startLines = new int[]{23};
         int[] endLines = new int[]{36};
 

src/it/test-projects/creedengo-java-plugin-test-project/src/main/java/org/greencodeinitiative/creedengo/java/checks/FreeResourcesOfAutoCloseableInterface.java

@@ -35,4 +35,25 @@ public void foo2() throws IOException {
             }
         }
     }
+
+    /**
+     * The first method adds a "try" in the stack used to follow if the code is in a try
+     */
+    public void callingMethodWithTheTry() throws IOException {
+        try { // Compliant
+            calledMethodWithoutTry();
+        } finally {
+            // Empty block of code
+        }
+    }
+
+    /**
+     * The "try" should have been popped from the stack before entering here
+     */
+    private void calledMethodWithoutTry() throws IOException {
+        FileWriter myWriter = new FileWriter("somefilepath");
+        myWriter.write("something");
+        myWriter.flush();
+        myWriter.close();
+    }
 }

src/main/java/org/greencodeinitiative/creedengo/java/checks/FreeResourcesOfAutoCloseableInterface.java

@@ -17,77 +17,146 @@
  */
 package org.greencodeinitiative.creedengo.java.checks;
 
+import java.util.ArrayDeque;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Deque;
-import java.util.LinkedList;
 import java.util.List;
 
+import javax.annotation.Nonnull;
 import javax.annotation.ParametersAreNonnullByDefault;
 
 import org.sonar.check.Rule;
 import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
 import org.sonar.plugins.java.api.JavaFileScannerContext;
-import org.sonar.plugins.java.api.JavaVersion;
 import org.sonar.plugins.java.api.tree.NewClassTree;
 import org.sonar.plugins.java.api.tree.Tree;
 import org.sonar.plugins.java.api.tree.TryStatementTree;
 import org.sonarsource.analyzer.commons.annotations.DeprecatedRuleKey;
 
-
+/**
+ * This rule checks that objects implementing AutoCloseable interface are properly managed
+ * using try-with-resources statement instead of try-finally blocks.
+ * <p>
+ * Try-with-resources ensures proper resource management and reduces the risk of resource leaks.
+ * It also reduces boilerplate code and improves code readability.
+ * <p>
+ * From an environmental perspective, proper resource management prevents resource leaks
+ * which can lead to increased memory consumption and unnecessary CPU cycles.
+ *
+ * @see <a href="https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html">Try-with-resources</a>
+ */
 @Rule(key = "GCI79")
 @DeprecatedRuleKey(repositoryKey = "ecocode-java", ruleKey = "EC79")
 @DeprecatedRuleKey(repositoryKey = "greencodeinitiative-java", ruleKey = "S79")
 public class FreeResourcesOfAutoCloseableInterface extends IssuableSubscriptionVisitor {
-    private final Deque<TryStatementTree> withinTry = new LinkedList<>();
-    private final Deque<List<Tree>> toReport = new LinkedList<>();
+
+    /**
+     * Stack to track nested try statements while traversing the AST
+     */
+    private final Deque<TryStatementContext> tryStack = new ArrayDeque<>();
 
     private static final String JAVA_LANG_AUTOCLOSEABLE = "java.lang.AutoCloseable";
-    protected static final String MESSAGE_RULE = "try-with-resources Statement needs to be implemented for any object that implements the AutoClosable interface.";
+    protected static final String MESSAGE_RULE = "try-with-resources Statement needs to be implemented for any object that implements the AutoCloseable interface.";
 
     @Override
     @ParametersAreNonnullByDefault
     public void leaveFile(JavaFileScannerContext context) {
-        withinTry.clear();
-        toReport.clear();
+        tryStack.clear();
     }
 
     @Override
+    @Nonnull
     public List<Tree.Kind> nodesToVisit() {
-        return Arrays.asList(Tree.Kind.TRY_STATEMENT, Tree.Kind.NEW_CLASS);
+        return List.of(Tree.Kind.TRY_STATEMENT, Tree.Kind.NEW_CLASS);
     }
 
     @Override
-    public void visitNode(Tree tree) {
+    public void visitNode(@Nonnull Tree tree) {
         if (tree.is(Tree.Kind.TRY_STATEMENT)) {
-            withinTry.push((TryStatementTree) tree);
-            if (withinTry.size() != toReport.size()) {
-                toReport.push(new ArrayList<>());
-            }
-        }
-        if (tree.is(Tree.Kind.NEW_CLASS) && ((NewClassTree) tree).symbolType().isSubtypeOf(JAVA_LANG_AUTOCLOSEABLE) && withinStandardTryWithFinally()) {
-            assert toReport.peek() != null;
-            toReport.peek().add(tree);
+            handleTryStatement((TryStatementTree) tree);
+        } else if (tree.is(Tree.Kind.NEW_CLASS)) {
+            handleNewClass((NewClassTree) tree);
         }
     }
 
     @Override
-    public void leaveNode(Tree tree) {
+    public void leaveNode(@Nonnull Tree tree) {
         if (tree.is(Tree.Kind.TRY_STATEMENT)) {
-            List<Tree> secondaryTrees = toReport.pop();
-            if (!secondaryTrees.isEmpty()) {
-                reportIssue(tree, MESSAGE_RULE);
+            leaveTryStatement();
+        }
+    }
+
+    /**
+     * Handle entering a try statement by pushing it onto the stack
+     */
+    private void handleTryStatement(@Nonnull TryStatementTree tryStatement) {
+        tryStack.push(new TryStatementContext(tryStatement));
+    }
+
+    /**
+     * Handle leaving a try statement by popping it from the stack and reporting issues if needed
+     */
+    private void leaveTryStatement() {
+        if (!tryStack.isEmpty()) {
+            TryStatementContext context = tryStack.pop();
+            if (!context.autoCloseableInstances.isEmpty()) {
+                reportIssue(context.tryStatement, MESSAGE_RULE);
+            }
+        }
+    }
+
+    /**
+     * Handle new class instantiation to detect AutoCloseable objects
+     * that are created inside a try-finally block (without try-with-resources)
+     */
+    private void handleNewClass(@Nonnull NewClassTree newClass) {
+        // Check if the new instance is an AutoCloseable
+        if (!newClass.symbolType().isSubtypeOf(JAVA_LANG_AUTOCLOSEABLE)) {
+            return;
+        }
+
+        // Check if we are inside a non-compliant try statement
+        if (isInNonCompliantTry()) {
+            TryStatementContext context = tryStack.peek();
+            if (context != null) {
+                context.autoCloseableInstances.add(newClass);
             }
         }
     }
 
-    private boolean withinStandardTryWithFinally() {
-        if (withinTry.isEmpty() || !withinTry.peek().resourceList().isEmpty()) return false;
-        assert withinTry.peek() != null;
-        return withinTry.peek().finallyBlock() != null;
+    /**
+     * Check if we are currently inside a try statement that:
+     * - Does NOT use try-with-resources (no resource list)
+     * - Has a finally block (indicating manual resource management)
+     *
+     * @return true if inside a non-compliant try statement
+     */
+    private boolean isInNonCompliantTry() {
+        if (tryStack.isEmpty()) {
+            return false;
+        }
+
+        TryStatementTree currentTry = tryStack.peek().tryStatement;
+
+        // If try-with-resources is already used, it's compliant
+        if (!currentTry.resourceList().isEmpty()) {
+            return false;
+        }
+
+        // If there's a finally block, it suggests manual resource management
+        return currentTry.finallyBlock() != null;
     }
 
-    public boolean isCompatibleWithJavaVersion(JavaVersion version) {
-        return version.isJava7Compatible();
+    /**
+     * Context class to track information about a try statement during AST traversal
+     */
+    private static class TryStatementContext {
+        final TryStatementTree tryStatement;
+        final List<Tree> autoCloseableInstances;
+
+        TryStatementContext(@Nonnull TryStatementTree tryStatement) {
+            this.tryStatement = tryStatement;
+            this.autoCloseableInstances = new ArrayList<>();
+        }
     }
 }

src/test/files/FreeResourcesOfAutoCloseableInterface.java

@@ -27,15 +27,15 @@ class FreeResourcesOfAutoCloseableInterface {
     public void foo1() {
         String fileName = "./FreeResourcesOfAutoCloseableInterface.java";
         try (FileReader fr = new FileReader(fileName);
-             BufferedReader br = new BufferedReader(fr)) {
+             BufferedReader br = new BufferedReader(fr)) { // Compliant
         } catch (IOException e) {
             System.err.println(e.getMessage());
         }
     }
 
     public void foo2() {
         String fileName = "./FreeResourcesOfAutoCloseableInterface.java";
-        try { // Noncompliant
+        try { // Noncompliant {{try-with-resources Statement needs to be implemented for any object that implements the AutoCloseable interface.}}
             FileReader fr = new FileReader(fileName);
             BufferedReader br = new BufferedReader(fr);
             System.out.printl(br.readLine());
@@ -50,4 +50,25 @@ public void foo2() {
             }
         }
     }
+
+    /**
+     * The first method adds a "try" in the stack used to follow if the code is in a try
+     */
+    public void callingMethodWithTheTry() throws IOException {
+        try { // Compliant
+            calledMethodWithoutTry();
+        } finally {
+            // Empty block of code
+        }
+    }
+
+    /**
+     * The "try" should have been popped from the stack before entering here
+     */
+    private void calledMethodWithoutTry() throws IOException {
+        FileWriter myWriter = new FileWriter("somefilepath");
+        myWriter.write("something");
+        myWriter.flush();
+        myWriter.close();
+    }
 }