SONARJAVA-4960 False positive for S1854 (#5378)

Author: rombirli

its/autoscan/src/test/resources/autoscan/diffs/diff_S1481.json

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S1481",
   "hasTruePositives": true,
-  "falseNegatives": 10,
+  "falseNegatives": 8,
   "falsePositives": 0
 }

java-checks-test-sources/default/src/main/files/non-compiling/checks/UnusedVariablesFPCheck.java

@@ -0,0 +1,105 @@
+package checks;
+
+
+public class UnusedVariablesFPCheck {
+  public class DeobfuscatedUpdateManager {
+    // @formatter:off
+// uncommenting the following code makes the issue disappear, as the semantic is fully resolved
+//    private interface DataContainer {
+//      Iterable<ItemElement> getItems();
+//    }
+//
+//    private interface ModelObject {
+//      void performAction();
+//    }
+//
+//    private interface ItemElement {
+//      ModelObject getDataModel();
+//    }
+//
+//    private static class SystemConfig {
+//      static ConfigMode getMode() {
+//        return ConfigMode.ENABLED;
+//      }
+//    }
+//
+//    private enum ConfigMode {
+//      ENABLED,
+//      DISABLED
+//    }
+//
+//    static class A {
+//      interface GenericCallback<T> { }
+//    }
+    // @formatter:on
+
+    void processUpdates(
+      DataContainer container
+      // REMARK : the issue arises from the A.GenericCallback<ModelObject> callback that is not even used (indirect type resolution problem)
+      , A.GenericCallback<X> callback
+    ) {
+      for (ItemElement element : container.getItems()) {
+        if (SystemConfig.getMode() == ConfigMode.ENABLED) {
+          ModelObject dataModel = element.getDataModel(); // Compliant - false positive was raised here, dataModel is used in the next line
+          dataModel.performAction();
+        }
+      }
+    }
+
+  }
+
+  static class StringConcatenation {
+    // @formatter:off
+// uncommenting the following code makes the issue disappear, as the semantic is fully resolved
+//    private class AClass {
+//      private class BClass<T> {
+//        public T b;
+//      }
+//    }
+    // @formatter:on
+
+    public String doSomething(AClass.BClass<String> instance) {
+      String c = "Hi"; // Compliant - false positive was raised here, c is used in the next line
+      return instance.b + c;
+    }
+  }
+
+/*
+  A user reported a FP on enhanced switch statements like the one below.
+  However I was not able to reproduce it in a minimal example.
+  https://community.sonarsource.com/t/false-positive-for-s1854-unused-assignments-should-be-removed/114110/12
+
+  static class EnhancedSwitch {
+//    private enum DocumentStatus {
+//      DOC01,
+//      DOC02
+//    }
+//
+//    private interface Document {
+//      void setStatus(DocumentStatus status);
+//    }
+//
+//    private interface Event {
+//    }
+//
+//    private class SimpleStatusChangedEvent implements Event {
+//    }
+//
+//    private class NeedClientRecheckEvent implements Event {
+//    }
+//    private interface DocumentRepository {
+//      void save(Document document);
+//    }
+
+    void ko(Event event, Document document, DocumentRepository documentRepository) {
+      final DocumentStatus status = switch (event) {
+        case SimpleStatusChangedEvent ignored -> DocumentStatus.DOC01;
+        case NeedClientRecheckEvent ignored -> DocumentStatus.DOC02;
+      };
+      document.setStatus(status);
+      // ...
+      documentRepository.save(document);
+    }
+
+  }*/
+}

java-checks/src/test/java/org/sonar/java/checks/DeadStoreCheckTest.java

@@ -51,6 +51,14 @@ void test_non_compiling() {
       .verifyIssues();
   }
 
+  @Test
+  void test_incomplete_semantic() {
+    CheckVerifier.newVerifier()
+      .onFile(TestUtils.nonCompilingTestSourcesPath("checks/UnusedVariablesFPCheck.java"))
+      .withCheck(new DeadStoreCheck())
+      .verifyNoIssues();
+  }
+
   private static class EraseSymbols extends BaseTreeVisitor {
 
     @Override

java-frontend/src/main/java/org/sonar/java/model/Symbols.java

@@ -99,7 +99,7 @@ public final boolean isTypeSymbol() {
     }
 
     @Override
-    public final boolean isMethodSymbol() {
+    public boolean isMethodSymbol() {
       return false;
     }
 
@@ -340,6 +340,11 @@ public boolean isVarArgsMethod() {
     public boolean isNativeMethod() {
       return false;
     }
+
+    @Override
+    public boolean isMethodSymbol() {
+      return true;
+    }
   }
 
   public static final class UnknownType implements Type {

java-frontend/src/test/java/org/sonar/java/model/JParserSemanticTest.java

@@ -1287,7 +1287,7 @@ private java.util.Collection<UnknownClass.Entry<E>> samples() {
 
     assertThat(recovered.isTypeSymbol()).isFalse();
     assertThat(recovered.isVariableSymbol()).isFalse();
-    assertThat(recovered.isMethodSymbol()).isFalse();
+    assertThat(recovered.isMethodSymbol()).isTrue();
     assertThat(recovered.isPackageSymbol()).isFalse();
 
     assertThat(recovered.isAbstract()).isFalse();

java-frontend/src/test/java/org/sonar/java/model/SymbolsTest.java

@@ -125,6 +125,7 @@ void unknown_symbol() {
     Symbol unknownSymbol = Symbol.UNKNOWN_SYMBOL;
 
     assertCommonProperties(unknownSymbol);
+    assertThat(unknownSymbol.isMethodSymbol()).isFalse();
     assertThat(unknownSymbol.name()).isEqualTo("!unknown!");
     assertThat(unknownSymbol.owner()).isEqualTo(Symbol.ROOT_PACKAGE);
     SymbolMetadata metadata = unknownSymbol.metadata();
@@ -137,6 +138,7 @@ void unknown_type_symbol() {
     Symbol.TypeSymbol unknownTypeSymbol = Symbol.TypeSymbol.UNKNOWN_TYPE;
 
     assertCommonProperties(unknownTypeSymbol);
+    assertThat(unknownTypeSymbol.isMethodSymbol()).isFalse();
     assertThat(unknownTypeSymbol.name()).isEqualTo("!unknown!");
     assertThat(unknownTypeSymbol.owner()).isEqualTo(Symbol.ROOT_PACKAGE);
 
@@ -154,6 +156,7 @@ void unknown_method_symbol() {
     Symbol.MethodSymbol unknownMethodSymbol = Symbol.MethodSymbol.UNKNOWN_METHOD;
 
     assertCommonProperties(unknownMethodSymbol);
+    assertThat(unknownMethodSymbol.isMethodSymbol()).isTrue();
     assertThat(unknownMethodSymbol.name()).isEqualTo("!unknownMethod!");
     assertThat(unknownMethodSymbol.owner()).isEqualTo(Symbol.TypeSymbol.UNKNOWN_TYPE);
 
@@ -171,7 +174,6 @@ private static void assertCommonProperties(Symbol unknownSymbol) {
     assertThat(unknownSymbol.isPackageSymbol()).isFalse();
     assertThat(unknownSymbol.isTypeSymbol()).isFalse();
     assertThat(unknownSymbol.isVariableSymbol()).isFalse();
-    assertThat(unknownSymbol.isMethodSymbol()).isFalse();
 
     assertThat(unknownSymbol.isStatic()).isFalse();
     assertThat(unknownSymbol.isFinal()).isFalse();