Skip to content

[GITHUB-9198] Do not remove for each variables, even if unused. #9233

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lahodaj wants to merge 1 commit into
base: master
Choose a base branch
from
+257 −11
Open

[GITHUB-9198] Do not remove for each variables, even if unused. #9233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion java/java.hints/src/org/netbeans/modules/java/hints/Feature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,12 @@ public enum Feature {
VIRTUAL_THREADS(19, 21),

/// https://openjdk.org/jeps/440
RECORD_PATTERN(19, 21);
RECORD_PATTERN(19, 21),

/// https://openjdk.org/jeps/456
UNNAMED_VARIABLES(21, 22),

;

/// preview begin
public final int PREVIEW;
Expand Down
53 changes: 47 additions & 6 deletions java/java.hints/src/org/netbeans/modules/java/hints/bugs/Unused.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,25 @@
package org.netbeans.modules.java.hints.bugs;

import com.sun.source.tree.Tree.Kind;
import com.sun.source.util.TreePath;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import javax.lang.model.SourceVersion;
import javax.lang.model.element.ElementKind;
import org.netbeans.api.java.source.CompilationInfo;
import org.netbeans.api.java.source.TreeMaker;
import org.netbeans.api.java.source.WorkingCopy;
import org.netbeans.modules.java.editor.base.semantic.UnusedDetector;
import org.netbeans.modules.java.editor.base.semantic.UnusedDetector.UnusedDescription;
import org.netbeans.modules.java.hints.Feature;
import org.netbeans.spi.editor.hints.ErrorDescription;
import org.netbeans.spi.editor.hints.Fix;
import org.netbeans.spi.java.hints.BooleanOption;
import org.netbeans.spi.java.hints.ErrorDescriptionFactory;
import org.netbeans.spi.java.hints.Hint;
import org.netbeans.spi.java.hints.HintContext;
import org.netbeans.spi.java.hints.JavaFix;
import org.netbeans.spi.java.hints.JavaFixUtilities;
import org.netbeans.spi.java.hints.TriggerTreeKind;
import org.openide.util.NbBundle.Messages;
Expand All @@ -46,6 +55,9 @@
})
public class Unused {

private static final SourceVersion UNNAMED_VARIABLES = SourceVersion.RELEASE_22;
Copy link
Member

@mbien mbien Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now unused

private static final Set<ElementKind> SUPPORT_UNNAMED = Set.of(ElementKind.BINDING_VARIABLE, ElementKind.EXCEPTION_PARAMETER, ElementKind.LOCAL_VARIABLE, ElementKind.RESOURCE_VARIABLE);

private static final boolean DETECT_UNUSED_PACKAGE_PRIVATE_DEFAULT = true;

@BooleanOption(displayName="#LBL_UnusedPackagePrivate", tooltip="#TP_UnusedPackagePrivate", defaultValue=DETECT_UNUSED_PACKAGE_PRIVATE_DEFAULT)
Expand Down Expand Up @@ -108,10 +120,7 @@ private static ErrorDescription convertUnused(HintContext ctx, UnusedDescription
case NOT_WRITTEN: message = Bundle.ERR_NotWritten(name);
break;
case NOT_READ: message = Bundle.ERR_NotRead(name);
//unclear what can be done with unused binding variables currently (before "_"):
if (ud.unusedElementPath().getParentPath().getLeaf().getKind() != Kind.BINDING_PATTERN) {
fix = JavaFixUtilities.safelyRemoveFromParent(ctx, Bundle.FIX_RemoveUsedElement(name), ud.unusedElementPath());
}
fix = JavaFixUtilities.safelyRemoveFromParent(ctx, Bundle.FIX_RemoveUsedElement(name), ud.unusedElementPath());
break;
case NOT_USED:
if (ud.unusedElement().getKind() == ElementKind.CONSTRUCTOR) {
Expand All @@ -125,7 +134,39 @@ private static ErrorDescription convertUnused(HintContext ctx, UnusedDescription
default:
throw new IllegalStateException("Unknown unused type: " + ud.reason());
}
return fix != null ? ErrorDescriptionFactory.forName(ctx, ud.unusedElementPath(), message, fix)
: ErrorDescriptionFactory.forName(ctx, ud.unusedElementPath(), message);

List<Fix> fixes = new ArrayList<>();

if (fix != null) {
fixes.add(fix);
}

if (Feature.UNNAMED_VARIABLES.isEnabled(ctx.getInfo()) &&
SUPPORT_UNNAMED.contains(ud.unusedElement().getKind())) {
fixes.add(new RenameToUnderscore(ctx.getInfo(), ctx.getPath()).toEditorFix());
}

return ErrorDescriptionFactory.forName(ctx, ud.unusedElementPath(), message, fixes.toArray(Fix[]::new));
}

private static final class RenameToUnderscore extends JavaFix {

public RenameToUnderscore(CompilationInfo info, TreePath tp) {
super(info, tp);
}

@Override
@Messages("FIX_RenameToUnderscore=Rename the variable to '_'")
protected String getText() {
return Bundle.FIX_RenameToUnderscore();
}

@Override
protected void performRewrite(TransformationContext ctx) throws Exception {
WorkingCopy wc = ctx.getWorkingCopy();
TreeMaker make = wc.getTreeMaker();
wc.rewrite(ctx.getPath().getLeaf(), make.setLabel(ctx.getPath().getLeaf(), "_"));
}

}
}
70 changes: 70 additions & 0 deletions java/java.hints/test/unit/src/org/netbeans/modules/java/hints/bugs/UnusedTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,4 +135,74 @@ record Test() {
.assertWarnings();
}

public void testUnusedForEach() throws Exception {
HintTest.create()
.input("""
package test;
public class Test {
public void test(String[] args) {
for (String a : args) {}
}
}
""")
.run(Unused.class)
.assertWarnings("3:20-3:21:verifier:" + Bundle.ERR_NotRead("a"))
.findWarning("3:20-3:21:verifier:" + Bundle.ERR_NotRead("a"))
.assertFixes();
}

public void testToUndescore1() throws Exception {
HintTest.create()
.sourceLevel("22")
.input("""
package test;
public class Test {
public void test(String[] args) {
String u = "";
}
}
""")
.run(Unused.class)
.findWarning("3:15-3:16:verifier:" + Bundle.ERR_NotRead("u"))
.applyFix(Bundle.FIX_RenameToUnderscore())
.assertCompilable()
.assertOutput("""
package test;
public class Test {
public void test(String[] args) {
String _ = "";
}
}
""");
}

public void testToUndescore2() throws Exception {
HintTest.create()
.sourceLevel("22")
.input("""
package test;
public class Test {
String u = "";
}
""")
.run(Unused.class)
.findWarning("2:11-2:12:verifier:" + Bundle.ERR_NotRead("u"))
.assertFixes(Bundle.FIX_RemoveUsedElement("u"));
}

public void testToUndescore3() throws Exception {
HintTest.create()
.sourceLevel("21")
.input("""
package test;
public class Test {
public void test(String[] args) {
String u = "";
}
}
""")
.run(Unused.class)
.findWarning("3:15-3:16:verifier:" + Bundle.ERR_NotRead("u"))
.assertFixes(Bundle.FIX_RemoveUsedElement("u"));
}
}
16 changes: 16 additions & 0 deletions java/spi.java.hints/src/org/netbeans/spi/java/hints/JavaFixUtilities.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.sun.source.tree.CompilationUnitTree;
import com.sun.source.tree.CompoundAssignmentTree;
import com.sun.source.tree.DoWhileLoopTree;
import com.sun.source.tree.EnhancedForLoopTree;
import com.sun.source.tree.ExpressionStatementTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IdentifierTree;
Expand Down Expand Up @@ -1835,6 +1836,21 @@ private void doRemoveFromParent(WorkingCopy wc, TreePath what) {
}

private static boolean canSafelyRemove(CompilationInfo info, TreePath tp) {
if (tp.getParentPath() != null) {
//cannot remove from some parent trees:
switch (tp.getParentPath().getLeaf().getKind()) {
case BINDING_PATTERN -> {
return false;
}
case ENHANCED_FOR_LOOP -> {
EnhancedForLoopTree loop = (EnhancedForLoopTree) tp.getParentPath().getLeaf();

if (loop.getVariable() == tp.getLeaf()) {
return false;
}
}
}
}
AtomicBoolean ret = new AtomicBoolean(true);
Element el = info.getTrees().getElement(tp);
if (el != null) {
Expand Down
122 changes: 118 additions & 4 deletions java/spi.java.hints/test/unit/src/org/netbeans/spi/java/hints/JavaFixUtilitiesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.netbeans.modules.java.hints.spiimpl.TestBase;
import org.netbeans.modules.java.hints.spiimpl.hints.HintsInvoker;
import org.netbeans.modules.java.hints.providers.spi.Trigger.PatternDescription;
import org.netbeans.modules.java.hints.spiimpl.SyntheticFix;
import org.netbeans.modules.java.hints.spiimpl.options.HintsSettings;
import org.netbeans.modules.java.hints.spiimpl.pm.PatternCompilerUtilities;
import org.netbeans.modules.java.source.parsing.JavacParser;
Expand Down Expand Up @@ -509,6 +510,102 @@ public void testRemoveFromParentExpressionStatement206116() throws Exception {
"}\n");
}

public void testRemovedFromForEach1() throws Exception {
performRemoveFromParentTest("""
package test;
import java.io.InputStream;
public class Test {
private void t(String[] args) throws Exception {
for (String a : args)
System.err.println();
}
}
""",
"System.err.println();",
"""
package test;
import java.io.InputStream;
public class Test {
private void t(String[] args) throws Exception {
for (String a : args) {
}
}
}
""",
true);
}

public void testRemovedFromForEach2() throws Exception {
performRemoveFromParentTest("""
package test;
import java.io.InputStream;
public class Test {
private void t(String[] args) throws Exception {
for (String a : args)
System.err.println();
}
}
""",
"System.err.println();",
"""
package test;
import java.io.InputStream;
public class Test {
private void t(String[] args) throws Exception {
for (String a : args) {
}
}
}
""",
true);
}

public void testRemovedFromForEach3() throws Exception {
performRemoveFromParentTest("""
package test;
import java.io.InputStream;
public class Test {
private void t(String[] args) throws Exception {
for (String a : args)
System.err.println();
}
}
""",
"String a",
null,
true);
}

public void testRemovedFromForEach4() throws Exception {
performRemoveFromParentTest("""
package test;
import java.util.List;
public class Test {
private void t() throws Exception {
for (String a : List.of("")
System.err.println();
}
}
""",
"java.util.List.of($args)",
null,
true);
}

public void testRemoveBindingPattern1() throws Exception {
performRemoveFromParentTest("""
package test;
public class Test {
private void t(Object o) throws Exception {
if (o instanceof String str) {}
}
}
""",
"String str",
null,
true);
}

public void testUnresolvableTarget() throws Exception {
performRewriteTest("package test;\n" +
"public class Test extends java.util.ArrayList {\n" +
Expand Down Expand Up @@ -1396,25 +1493,42 @@ public void performRewriteTest(String code, String rule, String golden, String s
}

public void performRemoveFromParentTest(String code, String rule, String golden) throws Exception {
performRemoveFromParentTest(code, rule, golden, false);
}

public void performRemoveFromParentTest(String code, String rule, String golden, boolean safelyRemove) throws Exception {
prepareTest("test/Test.java", code);

HintDescription hd = HintDescriptionFactory.create()
.setTrigger(PatternDescription.create(rule, Collections.<String, String>emptyMap()))
.setWorker(new HintDescription.Worker() {
@Override public Collection<? extends ErrorDescription> createErrors(HintContext ctx) {
return Collections.singletonList(ErrorDescriptionFactory.forName(ctx, ctx.getPath(), "", JavaFixUtilities.removeFromParent(ctx, "", ctx.getPath())));
Fix fix = safelyRemove ? JavaFixUtilities.safelyRemoveFromParent(ctx, "", ctx.getPath())
: JavaFixUtilities.removeFromParent(ctx, "", ctx.getPath());
return Collections.singletonList(ErrorDescriptionFactory.forName(ctx, ctx.getPath(), "", fix));
}
}).produce();

List<ErrorDescription> computeHints = new HintsInvoker(HintsSettings.getGlobalSettings(), new AtomicBoolean()).computeHints(info, Collections.singleton(hd));

assertEquals(computeHints.toString(), 1, computeHints.size());

Fix fix = computeHints.get(0).getFixes().getFixes().get(0);
List<Fix> fixes = computeHints.get(0).getFixes().getFixes();

fix.implement();
assertEquals(String.valueOf(fixes), 1, fixes.size());

assertEquals(golden, doc.getText(0, doc.getLength()));
if (golden != null) {
Fix fix = fixes.get(0);

fix.implement();

assertEquals(golden, doc.getText(0, doc.getLength()));
} else {
Fix fix = fixes.get(0);

assertTrue(String.valueOf(fixes),
fix instanceof SyntheticFix);
}
}

static {
Expand Down
Loading