SONARJAVA-4964 Fix FP in S1941 in case of Unknown symbol referencing declared variable (#5121)

Author: romainbrenguier

java-checks/src/main/java/org/sonar/java/checks/VariableDeclarationScopeCheck.java

@@ -55,12 +55,16 @@ public void visitNode(Tree tree) {
     }
   }
 
+  /**
+   * If there is a {@code return} or {@code throw} in {@code body} at index greater or equal to {@code next} and before any reference to
+   * {@code variable} then report an issue.
+   */
   private void check(VariableTree variable, List<StatementTree> body, int bodySize, int next) {
     Symbol symbol = variable.symbol();
     ReferenceVisitor referenceVisitor = new ReferenceVisitor(symbol);
     for (int i = next; i < bodySize; i++) {
       referenceVisitor.visit(body.get(i));
-      if (referenceVisitor.referenceSymbol) {
+      if (referenceVisitor.referencesSymbol) {
         return;
       }
       if (referenceVisitor.hasBreakingStatement) {
@@ -71,41 +75,37 @@ private void check(VariableTree variable, List<StatementTree> body, int bodySize
   }
 
   private static class ReferenceVisitor extends BaseTreeVisitor {
-    Symbol symbol;
-    boolean referenceSymbol;
+    private final Symbol symbol;
+    boolean referencesSymbol;
     boolean hasBreakingStatement;
 
     ReferenceVisitor(Symbol symbol) {
       this.symbol = symbol;
     }
 
     void visit(StatementTree node) {
-      referenceSymbol = false;
+      referencesSymbol = false;
       hasBreakingStatement = false;
       node.accept(this);
     }
 
     @Override
     public void visitReturnStatement(ReturnStatementTree tree) {
-      if (!hasBreakingStatement) {
-        hasBreakingStatement = true;
-      }
+      hasBreakingStatement = true;
       super.visitReturnStatement(tree);
     }
 
     @Override
     public void visitThrowStatement(ThrowStatementTree tree) {
-      if (!hasBreakingStatement) {
-        hasBreakingStatement = true;
-      }
+      hasBreakingStatement = true;
       super.visitThrowStatement(tree);
     }
 
     @Override
     public void visitIdentifier(IdentifierTree tree) {
-      if (!referenceSymbol && symbol.equals(tree.symbol())) {
-        referenceSymbol = true;
-      }
+      referencesSymbol |= symbol.equals(tree.symbol());
+      // If the symbol is "Unknown" and has same name, we assume it's the same to avoid FPs.
+      referencesSymbol |= tree.symbol().isUnknown() && symbol.name().equals(tree.name());
       super.visitIdentifier(tree);
     }
   }

java-checks/src/test/files/checks/VariableDeclarationScopeCheck.java

@@ -1,3 +1,6 @@
+import java.util.List;
+import java.util.ArrayList;
+
 abstract class A {
 
   void m1();
@@ -55,4 +58,23 @@ void unused() {
       return;
     }
   }
+
+  void withLambdaUsingVar() {
+    List<String> outsideLambda = new ArrayList<>(); // Compliant: this cannot be declared anywhere else
+    Collections.singletonList(23).stream.map(x -> {
+        outsideLambda.add(x);
+        return x;
+      })
+      .collect(Collectors.toList());
+    return outsideLambda;
+  }
+
+  void withLambdaNotUsingVar() {
+    List<String> outsideLambda = new ArrayList<>(); // Noncompliant
+    Collections.singletonList(23).stream.map(x -> {
+        return x;
+      })
+      .collect(Collectors.toList());
+    return outsideLambda;
+  }
 }