#361 | adding warning messages on mutable method invocations on immutable collection

[naren2605]

fix realted to issue#361
context:
ide's like intellij show warnings on using immutable methods, incorporating warnings in our extension aswell.
created draft pr for initial review :

• logic to verify and show warnings on mutable methods invocation with test cases
  • attached intellij warnings and new change warnings in vscode extension screenshots
• other methods/collections to be added for the mutable method checks
• final content for warning messages

[lahodaj]

Nice work.

Some functional comments:

• I understand the argument of not consuming much resources by checking every .add, etc., method in absence of any List/Set.of. OTOH, note the hints infrastructure is not designed to warnings to be produced at a place distant from the place where the match happened. So, expect a possible fallout. E.g. if there's a suppress warnings key (there's a constant defined for one, but it is not used), the @SuppressWarnings may be added at a surprising place. That said, given the current nature of this check, the fallout is probably relatively small
• when considering performance, TreePath.getPath(CompilationUnitTree, Tree) is searching through the given CompilationUnitTree, looking for the given Tree. When called repeatedly, this method may cause performance problems. Use of this method should be a very last resort, when nothing else absolutely cannot work. In this case: the use of this method in getMit seems to be completely unnecessary (the TreePath is slowly computed, and then the leaf is taken - but the leaf is the same as the input Tree/mit, isn't it?); the use in markMethodInvocation is a bit more tricky, but, as a stop-gap measure, there's FlowResult.findPath, which precomputes the values only once. Even better might be to reconsider what the FlowResult.getValueUsers returns, and return a collection of TreePaths instead.
• detecting mutating methods by just names is probably mostly OK in this case, since we know the actual type and instance is produced by one of the .of() factories. I.e. there cannot be custom overloads of the methods. But in more general cases, it may be necessary to check the methods are the expected overloads/have the expected parameter types.

Some stylistic comments:

• Having 4 helper methods seems a bit excessive, and it was actually difficult for me to decode what it does. IMO: getInvocations could be folded into checkForMutableMethodInvocations, and the getMit can me dropped completely (both the invocation and declaration). In checkForMutableMethodInvocations, we know getPath().getLeaf() is a method invocation, otherwise it would not be called. markMethodInvocation can be folded into checkForUsagesAndMarkInvocations, and it (to me) makes things more clear.
• variablesToCheck in checkForUsagesAndMarkInvocations is a little bit misleading, as it obviously may contain non-variables.
• I am typically a bit wary about code like <TreePath>.getParentPath().getParentPath().getLeaf(), as for very broken code, the AST sometimes looks very weird, and it is not completely clear the getParentPath() returns non-null. Although, in this case, it is probably mostly safe.

I was experimenting with something like:

diff --git a/java/java.hints/src/org/netbeans/modules/java/hints/bugs/MutableMethodsOnImmutableCollections.java b/java/java.hints/src/org/netbeans/modules/java/hints/bugs/MutableMethodsOnImmutableCollections.java
index 6c6b1881f5..906baf271f 100644
--- a/java/java.hints/src/org/netbeans/modules/java/hints/bugs/MutableMethodsOnImmutableCollections.java
+++ b/java/java.hints/src/org/netbeans/modules/java/hints/bugs/MutableMethodsOnImmutableCollections.java
@@ -48,7 +48,8 @@ import org.netbeans.spi.java.hints.ErrorDescriptionFactory;
         category = "bugs",
         id = "ImmutableListCreation",
         severity = Severity.WARNING,
-        options = Options.QUERY)
+        options = Options.QUERY,
+        suppressWarnings = MutableMethodsOnImmutableCollections.SUPPRESS_WARNING_KEY)
 
 public class MutableMethodsOnImmutableCollections {
 
@@ -74,8 +75,10 @@ public class MutableMethodsOnImmutableCollections {
 
     private static List<ErrorDescription> checkForMutableMethodInvocations(HintContext ctx, Set<String> mutatingMethods, String warningMessage) {
         List<ErrorDescription> errors = new ArrayList<>();
+        FlowResult flow = Flow.assignmentsForUse(ctx.getInfo(), () -> ctx.isCanceled());
+        List<MemberSelectTree> invocations = checkForUsagesAndMarkInvocations(ctx.getInfo(), flow, ctx.getPath());
 
-        for (MemberSelectTree mst : getInvocations(ctx.getInfo(), ctx.getPath().getLeaf(), () -> ctx.isCanceled())) {
+        for (MemberSelectTree mst : invocations) {
             String method = mst.getIdentifier().toString();
             if (mutatingMethods.contains(method)) {
                 errors.add(ErrorDescriptionFactory.forName(
@@ -89,44 +92,22 @@ public class MutableMethodsOnImmutableCollections {
         return errors;
     }
 
-    private static Tree getMit(CompilationUnitTree cut, Tree patternTriggered) {
-        if (patternTriggered instanceof MethodInvocationTree mit) {
-            return TreePath.getPath(cut, mit).getLeaf();
-        } else {
-            return null;
-        }
-
-    }
-
-    private static List<MemberSelectTree> getInvocations(CompilationInfo info, Tree patternTriggered, Flow.Cancel cancel) {
-        var initializerMit = getMit(info.getCompilationUnit(), patternTriggered);
-        if (initializerMit != null) {
-            FlowResult flow = Flow.assignmentsForUse(info, cancel);
-            return checkForUsagesAndMarkInvocations(info, flow, initializerMit);
-        }
-        return List.of();
-    }
-
-    private static List<MemberSelectTree> checkForUsagesAndMarkInvocations(CompilationInfo info, FlowResult flow, Tree invokedMethodPattern) {
+    private static List<MemberSelectTree> checkForUsagesAndMarkInvocations(CompilationInfo info, FlowResult flow, TreePath invokedMethodPattern) {
         List<MemberSelectTree> usedInvocationsWithIdentifier = new ArrayList<>();
-        Collection<Tree> variablesToCheck = Set.of(invokedMethodPattern);
+        Collection<TreePath> variablesToCheck = Set.of(invokedMethodPattern);
         do {
-            Set<Tree> nextSetOfVariablesToCheck = new HashSet<>();          
+            Set<TreePath> nextSetOfVariablesToCheck = new HashSet<>();
             for(var variable:variablesToCheck){
-                markMethodInvocation(info, variable, usedInvocationsWithIdentifier);
-                nextSetOfVariablesToCheck.addAll(flow.getValueUsers(variable));   
+                var ancestor = variable.getParentPath().getParentPath().getLeaf(); //TODO: getParentPath() might be tricky for erroneous sources
+                if (ancestor instanceof MethodInvocationTree mit && mit.getMethodSelect() instanceof MemberSelectTree mst) {
+                    usedInvocationsWithIdentifier.add(mst);
+                }
+                flow.getValueUsers(variable.getLeaf())
+                    .forEach(t -> nextSetOfVariablesToCheck.add(flow.findPath(t, info.getCompilationUnit())));
             }
             variablesToCheck = nextSetOfVariablesToCheck;
         } while (!variablesToCheck.isEmpty());
         return usedInvocationsWithIdentifier;
     }
 
-    private static void markMethodInvocation(CompilationInfo info, Tree tree, List<MemberSelectTree> usedInvocationsWithIdentifier) {
-        var ancestor = TreePath.getPath(info.getCompilationUnit(), tree).getParentPath().getParentPath().getLeaf();
-        if (ancestor instanceof MethodInvocationTree mit && mit.getMethodSelect() instanceof MemberSelectTree mst) {
-            usedInvocationsWithIdentifier.add(mst);
-        }
-    }
-    
-
 }

[naren2605]

thanks @lahodaj for taking a look , will check on the observations you have pointed and make required changes,