From 8ca245a3e175e1b3129d120d2d9858a1115979c8 Mon Sep 17 00:00:00 2001 From: Balyam muralidhar narendra kumar Date: Thu, 26 Jun 2025 18:32:42 +0530 Subject: [PATCH 1/2] 361 | adding warning messages on mutable method invocations on immutable collection --- build.xml | 1 + patches/jvsce-361-draft.diff | 256 +++++++++++++++++++++++++++++++++++ 2 files changed, 257 insertions(+) create mode 100644 patches/jvsce-361-draft.diff diff --git a/build.xml b/build.xml index d92bd7f..6035e26 100644 --- a/build.xml +++ b/build.xml @@ -83,6 +83,7 @@ patches/upgrade-lsp4j.diff patches/updated-show-input-params.diff patches/java-notebooks.diff + patches/jvsce-361-draft.diff diff --git a/patches/jvsce-361-draft.diff b/patches/jvsce-361-draft.diff new file mode 100644 index 0000000..179cc49 --- /dev/null +++ b/patches/jvsce-361-draft.diff @@ -0,0 +1,256 @@ +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 +new file mode 100644 +index 0000000000..6c6b1881f5 +--- /dev/null ++++ b/java/java.hints/src/org/netbeans/modules/java/hints/bugs/MutableMethodsOnImmutableCollections.java +@@ -0,0 +1,132 @@ ++/* ++ * Licensed to the Apache Software Foundation (ASF) under one ++ * or more contributor license agreements. See the NOTICE file ++ * distributed with this work for additional information ++ * regarding copyright ownership. The ASF licenses this file ++ * to you under the Apache License, Version 2.0 (the ++ * "License"); you may not use this file except in compliance ++ * with the License. You may obtain a copy of the License at ++ * ++ * http://www.apache.org/licenses/LICENSE-2.0 ++ * ++ * Unless required by applicable law or agreed to in writing, ++ * software distributed under the License is distributed on an ++ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY ++ * KIND, either express or implied. See the License for the ++ * specific language governing permissions and limitations ++ * under the License. ++ */ ++package org.netbeans.modules.java.hints.bugs; ++ ++import com.sun.source.tree.CompilationUnitTree; ++import com.sun.source.tree.MemberSelectTree; ++import com.sun.source.tree.MethodInvocationTree; ++import com.sun.source.tree.Tree; ++import com.sun.source.util.TreePath; ++import java.util.ArrayList; ++import java.util.Collection; ++import java.util.HashSet; ++import java.util.List; ++import java.util.Set; ++import org.netbeans.api.java.source.CompilationInfo; ++import org.netbeans.modules.java.hints.introduce.Flow; ++import org.netbeans.modules.java.hints.introduce.Flow.FlowResult; ++import org.netbeans.spi.editor.hints.ErrorDescription; ++import org.netbeans.spi.editor.hints.Severity; ++import org.netbeans.spi.java.hints.HintContext; ++import org.netbeans.spi.java.hints.Hint; ++import org.netbeans.spi.java.hints.Hint.Options; ++import org.netbeans.spi.java.hints.TriggerPattern; ++import org.netbeans.spi.java.hints.ErrorDescriptionFactory; ++ ++/** ++ * ++ * @author nbalyam ++ */ ++@Hint(displayName = "Track mutable methods on immutable collections", ++ description = "Track mutable methods on immutable collections", ++ category = "bugs", ++ id = "ImmutableListCreation", ++ severity = Severity.WARNING, ++ options = Options.QUERY) ++ ++public class MutableMethodsOnImmutableCollections { ++ ++ private static final Set MUTATING_METHODS_IN_LIST = Set.of( ++ "add", "addAll", "remove", "removeAll", "clear", "set", "replaceAll", "sort" ++ ); ++ ++ private static final Set MUTATING_METHODS_IN_SET = Set.of( ++ "add", "addAll", "remove", "removeAll", "retainAll", "clear" ++ ); ++ ++ public static final String SUPPRESS_WARNING_KEY = "immutable-collection-mutation"; ++ ++ @TriggerPattern(value = "java.util.List.of($args$)") ++ public static List immutableList(HintContext ctx) { ++ return checkForMutableMethodInvocations(ctx, MUTATING_METHODS_IN_LIST, "Attempting to modify an immutable List created via List.of()"); ++ } ++ ++ @TriggerPattern(value = "java.util.Set.of($args$)") ++ public static List immutableSet(HintContext ctx) { ++ return checkForMutableMethodInvocations(ctx, MUTATING_METHODS_IN_SET, "Attempting to modify an immutable Set created via Set.of()"); ++ } ++ ++ private static List checkForMutableMethodInvocations(HintContext ctx, Set mutatingMethods, String warningMessage) { ++ List errors = new ArrayList<>(); ++ ++ for (MemberSelectTree mst : getInvocations(ctx.getInfo(), ctx.getPath().getLeaf(), () -> ctx.isCanceled())) { ++ String method = mst.getIdentifier().toString(); ++ if (mutatingMethods.contains(method)) { ++ errors.add(ErrorDescriptionFactory.forName( ++ ctx, ++ TreePath.getPath(ctx.getInfo().getCompilationUnit(), mst), ++ warningMessage ++ )); ++ ++ } ++ } ++ 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 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 checkForUsagesAndMarkInvocations(CompilationInfo info, FlowResult flow, Tree invokedMethodPattern) { ++ List usedInvocationsWithIdentifier = new ArrayList<>(); ++ Collection variablesToCheck = Set.of(invokedMethodPattern); ++ do { ++ Set nextSetOfVariablesToCheck = new HashSet<>(); ++ for(var variable:variablesToCheck){ ++ markMethodInvocation(info, variable, usedInvocationsWithIdentifier); ++ nextSetOfVariablesToCheck.addAll(flow.getValueUsers(variable)); ++ } ++ variablesToCheck = nextSetOfVariablesToCheck; ++ } while (!variablesToCheck.isEmpty()); ++ return usedInvocationsWithIdentifier; ++ } ++ ++ private static void markMethodInvocation(CompilationInfo info, Tree tree, List usedInvocationsWithIdentifier) { ++ var ancestor = TreePath.getPath(info.getCompilationUnit(), tree).getParentPath().getParentPath().getLeaf(); ++ if (ancestor instanceof MethodInvocationTree mit && mit.getMethodSelect() instanceof MemberSelectTree mst) { ++ usedInvocationsWithIdentifier.add(mst); ++ } ++ } ++ ++ ++} +diff --git a/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/bugs/MutableMethodsOnImmutableCollectionsTest.java b/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/bugs/MutableMethodsOnImmutableCollectionsTest.java +new file mode 100644 +index 0000000000..229b730615 +--- /dev/null ++++ b/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/bugs/MutableMethodsOnImmutableCollectionsTest.java +@@ -0,0 +1,111 @@ ++/* ++ * Licensed to the Apache Software Foundation (ASF) under one ++ * or more contributor license agreements. See the NOTICE file ++ * distributed with this work for additional information ++ * regarding copyright ownership. The ASF licenses this file ++ * to you under the Apache License, Version 2.0 (the ++ * "License"); you may not use this file except in compliance ++ * with the License. You may obtain a copy of the License at ++ * ++ * http://www.apache.org/licenses/LICENSE-2.0 ++ * ++ * Unless required by applicable law or agreed to in writing, ++ * software distributed under the License is distributed on an ++ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY ++ * KIND, either express or implied. See the License for the ++ * specific language governing permissions and limitations ++ * under the License. ++ */ ++package org.netbeans.modules.java.hints.bugs; ++ ++import org.netbeans.junit.NbTestCase; ++import org.netbeans.modules.java.hints.test.api.HintTest; ++ ++/** ++ * ++ * @author nbalyamm ++ */ ++public class MutableMethodsOnImmutableCollectionsTest extends NbTestCase { ++ ++ public MutableMethodsOnImmutableCollectionsTest(String name) { ++ super(name); ++ } ++ public void testCaseWithMutlipleVariablesAndNoAssigmentChange() throws Exception{ ++ ++ HintTest ++ .create() ++ .input(""" ++ package test; ++ ++ import java.util.*; ++ ++ public class Test { ++ private void test () { ++ var l=List.of("foo","bar"); ++ var l2=List.of("fool2","barl2"); ++ l.add("bar2"); ++ l.clear(); ++ l2.clear(); ++ } ++ ++ } ++ """) ++ .sourceLevel(10) ++ .run(MutableMethodsOnImmutableCollections.class) ++ .assertWarnings( ++ "8:13-8:16:warning:Attempting to modify an immutable List created via List.of()", ++ "9:13-9:18:warning:Attempting to modify an immutable List created via List.of()", ++ "10:14-10:19:warning:Attempting to modify an immutable List created via List.of()"); ++ } ++ ++ public void testCaseWithAssignmentChange() throws Exception{ ++ ++ HintTest ++ .create() ++ .input(""" ++ package test; ++ ++ import java.util.*; ++ ++ public class Test { ++ private void test () { ++ var l=List.of("foo","bar"); ++ var l2=List.of("foo2","bar2"); ++ l.add("bar2"); ++ l.clear(); ++ l2.clear(); ++ l2 = new ArrayList(); ++ l2.clear(); ++ l2 = List.of("foo3","bar3"); ++ l2.clear(); ++ l2 = l; ++ l2.clear(); ++ if(true){ ++ l.clear(); ++ } ++ List l3 = new ArrayList(); ++ l3 = l2; ++ l3.clear(); ++ List l4 = new ArrayList(); ++ l4 = l3; ++ l4.clear(); ++ var s1 = Set.of("sfoo1","sbar1"); ++ s1.clear(); ++ } ++ } ++ """) ++ .sourceLevel(10) ++ .run(MutableMethodsOnImmutableCollections.class) ++ .assertWarnings( ++ "8:13-8:16:warning:Attempting to modify an immutable List created via List.of()", ++ "9:13-9:18:warning:Attempting to modify an immutable List created via List.of()", ++ "10:14-10:19:warning:Attempting to modify an immutable List created via List.of()", ++ "14:14-14:19:warning:Attempting to modify an immutable List created via List.of()", ++ "16:14-16:19:warning:Attempting to modify an immutable List created via List.of()", ++ "18:15-18:20:warning:Attempting to modify an immutable List created via List.of()", ++ "22:14-22:19:warning:Attempting to modify an immutable List created via List.of()", ++ "25:14-25:19:warning:Attempting to modify an immutable List created via List.of()", ++ "27:14-27:19:warning:Attempting to modify an immutable Set created via Set.of()" ++ ); ++ } ++} +\ No newline at end of file From f14f44ee96fa37cac801dcd8368be51d4c513bc9 Mon Sep 17 00:00:00 2001 From: Balyam muralidhar narendra kumar Date: Tue, 14 Oct 2025 10:05:25 +0530 Subject: [PATCH 2/2] updated changes addressing some review comments --- build.xml | 2 +- ...361-draft.diff => jvsce-361-draft-v2.diff} | 85 +++++++++---------- 2 files changed, 42 insertions(+), 45 deletions(-) rename patches/{jvsce-361-draft.diff => jvsce-361-draft-v2.diff} (81%) diff --git a/build.xml b/build.xml index 6035e26..1fea918 100644 --- a/build.xml +++ b/build.xml @@ -83,7 +83,7 @@ patches/upgrade-lsp4j.diff patches/updated-show-input-params.diff patches/java-notebooks.diff - patches/jvsce-361-draft.diff + patches/jvsce-361-draft-v2.diff diff --git a/patches/jvsce-361-draft.diff b/patches/jvsce-361-draft-v2.diff similarity index 81% rename from patches/jvsce-361-draft.diff rename to patches/jvsce-361-draft-v2.diff index 179cc49..93066b3 100644 --- a/patches/jvsce-361-draft.diff +++ b/patches/jvsce-361-draft-v2.diff @@ -1,9 +1,9 @@ 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 new file mode 100644 -index 0000000000..6c6b1881f5 +index 0000000000..5d1b1f2fde --- /dev/null +++ b/java/java.hints/src/org/netbeans/modules/java/hints/bugs/MutableMethodsOnImmutableCollections.java -@@ -0,0 +1,132 @@ +@@ -0,0 +1,129 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file @@ -24,16 +24,17 @@ index 0000000000..6c6b1881f5 + */ +package org.netbeans.modules.java.hints.bugs; + -+import com.sun.source.tree.CompilationUnitTree; ++import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; +import com.sun.source.util.TreePath; +import java.util.ArrayList; -+import java.util.Collection; -+import java.util.HashSet; +import java.util.List; ++import java.util.Optional; +import java.util.Set; ++import java.util.function.Function; ++import java.util.stream.Collectors; +import org.netbeans.api.java.source.CompilationInfo; +import org.netbeans.modules.java.hints.introduce.Flow; +import org.netbeans.modules.java.hints.introduce.Flow.FlowResult; @@ -52,7 +53,7 @@ index 0000000000..6c6b1881f5 +@Hint(displayName = "Track mutable methods on immutable collections", + description = "Track mutable methods on immutable collections", + category = "bugs", -+ id = "ImmutableListCreation", ++ id = "MutableMethodsOnImmutableCollections", + severity = Severity.WARNING, + options = Options.QUERY) + @@ -66,8 +67,6 @@ index 0000000000..6c6b1881f5 + "add", "addAll", "remove", "removeAll", "retainAll", "clear" + ); + -+ public static final String SUPPRESS_WARNING_KEY = "immutable-collection-mutation"; -+ + @TriggerPattern(value = "java.util.List.of($args$)") + public static List immutableList(HintContext ctx) { + return checkForMutableMethodInvocations(ctx, MUTATING_METHODS_IN_LIST, "Attempting to modify an immutable List created via List.of()"); @@ -80,8 +79,10 @@ index 0000000000..6c6b1881f5 + + private static List checkForMutableMethodInvocations(HintContext ctx, Set mutatingMethods, String warningMessage) { + List errors = new ArrayList<>(); ++ FlowResult flow = Flow.assignmentsForUse(ctx.getInfo(), () -> ctx.isCanceled()); ++ List 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( @@ -95,43 +96,39 @@ index 0000000000..6c6b1881f5 + 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 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 checkForUsagesAndMarkInvocations(CompilationInfo info, FlowResult flow, Tree invokedMethodPattern) { ++ private static List checkForUsagesAndMarkInvocations(CompilationInfo info, FlowResult flow, TreePath initPattern) { + List usedInvocationsWithIdentifier = new ArrayList<>(); -+ Collection variablesToCheck = Set.of(invokedMethodPattern); -+ do { -+ Set nextSetOfVariablesToCheck = new HashSet<>(); -+ for(var variable:variablesToCheck){ -+ markMethodInvocation(info, variable, usedInvocationsWithIdentifier); -+ nextSetOfVariablesToCheck.addAll(flow.getValueUsers(variable)); -+ } -+ variablesToCheck = nextSetOfVariablesToCheck; -+ } while (!variablesToCheck.isEmpty()); -+ return usedInvocationsWithIdentifier; -+ } -+ -+ private static void markMethodInvocation(CompilationInfo info, Tree tree, List usedInvocationsWithIdentifier) { -+ var ancestor = TreePath.getPath(info.getCompilationUnit(), tree).getParentPath().getParentPath().getLeaf(); -+ if (ancestor instanceof MethodInvocationTree mit && mit.getMethodSelect() instanceof MemberSelectTree mst) { -+ usedInvocationsWithIdentifier.add(mst); ++ Function> findIdentifierTreePaths = (Tree tree) -> { ++ return flow.getValueUsers(tree) ++ .stream() ++ .filter(IdentifierTree.class::isInstance) ++ .map(t -> flow.findPath(t, info.getCompilationUnit())) ++ .filter(treePath -> treePath.getLeaf() instanceof IdentifierTree) ++ .collect(Collectors.toSet()); ++ }; ++ Set identfiersPointingToInitializer = Optional.of(initPattern.getLeaf()) ++ .map(findIdentifierTreePaths) ++ .orElse(Set.of()); ++ while (!identfiersPointingToInitializer.isEmpty()) { ++ identfiersPointingToInitializer.forEach(indentifierPath -> { ++ var ancestorPath = Optional.of(indentifierPath) ++ .map(tpath -> tpath.getParentPath()) ++ .map(tpath -> tpath.getParentPath()) ++ .map(tpath -> tpath.getLeaf()); ++ ancestorPath.ifPresent(ancestor -> { ++ if (ancestor instanceof MethodInvocationTree mit && mit.getMethodSelect() instanceof MemberSelectTree mst) { ++ usedInvocationsWithIdentifier.add(mst); ++ } ++ }); ++ }); ++ identfiersPointingToInitializer = identfiersPointingToInitializer ++ .parallelStream() ++ .map(tpath->tpath.getLeaf()) ++ .map(findIdentifierTreePaths) ++ .flatMap(tpaths->tpaths.parallelStream()) ++ .collect(Collectors.toSet()); + } ++ return usedInvocationsWithIdentifier; + } + +