From f8b344a250ab06f5c60af1dfd40339583947d321 Mon Sep 17 00:00:00 2001 From: Jan Lahoda Date: Wed, 6 Aug 2025 15:50:06 +0200 Subject: [PATCH] Adding enhanced switch support to ThrowableNotThrown --- .../java/hints/bugs/ThrowableNotThrown.java | 47 +++++-- .../modules/java/hints/introduce/Flow.java | 58 ++++---- .../hints/bugs/ThrowableNotThrownTest.java | 126 ++++++++++++++++++ .../java/hints/introduce/FlowTest.java | 55 ++++++++ 4 files changed, 251 insertions(+), 35 deletions(-) diff --git a/java/java.hints/src/org/netbeans/modules/java/hints/bugs/ThrowableNotThrown.java b/java/java.hints/src/org/netbeans/modules/java/hints/bugs/ThrowableNotThrown.java index da87859dd987..e3b711251f95 100644 --- a/java/java.hints/src/org/netbeans/modules/java/hints/bugs/ThrowableNotThrown.java +++ b/java/java.hints/src/org/netbeans/modules/java/hints/bugs/ThrowableNotThrown.java @@ -19,16 +19,22 @@ package org.netbeans.modules.java.hints.bugs; import com.sun.source.tree.AssignmentTree; +import com.sun.source.tree.BindingPatternTree; +import com.sun.source.tree.CaseTree; import com.sun.source.tree.ConditionalExpressionTree; import com.sun.source.tree.ForLoopTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.PatternCaseLabelTree; +import com.sun.source.tree.SwitchExpressionTree; +import com.sun.source.tree.SwitchTree; import com.sun.source.tree.Tree; +import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.VariableTree; import com.sun.source.util.TreePath; import java.util.Collection; -import java.util.EnumSet; import java.util.HashSet; +import java.util.List; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import javax.lang.model.element.Element; @@ -90,13 +96,6 @@ public static ErrorDescription newThrowable(HintContext ctx) { return null; } - private static final EnumSet LOCAL_VARIABLES = EnumSet.of( - ElementKind.LOCAL_VARIABLE, - ElementKind.PARAMETER, - ElementKind.RESOURCE_VARIABLE, - ElementKind.EXCEPTION_PARAMETER - ); - private static TreePath findEnclosingMethodPath(TreePath path) { TreePath enclosingMethodPath = path; TreePath nextPath = enclosingMethodPath.getParentPath(); @@ -303,7 +302,7 @@ Boolean processEnclosingStatement(TreePath excPath) { Element el = info.getTrees().getElement(new TreePath(excPath, var)); if (el == null || el.getKind() == ElementKind.FIELD) { return true; - } else if (LOCAL_VARIABLES.contains(el.getKind())) { + } else if (Flow.LOCAL_VARIABLES.contains(el.getKind())) { varAssignments.add(as.getExpression()); } process = true; @@ -359,10 +358,40 @@ Boolean processEnclosingStatement(TreePath excPath) { break; } + case SWITCH: { + SwitchTree st = (SwitchTree) leaf; + + if (st.getExpression() == prevLeaf) { + handleCases(st.getCases()); + } + + break; + } + case SWITCH_EXPRESSION: { + SwitchExpressionTree st = (SwitchExpressionTree) leaf; + + if (st.getExpression() == prevLeaf) { + handleCases(st.getCases()); + process = true; + } + break; + } } prevLeaf = excPath.getLeaf(); } while (process); return varAssignments.isEmpty() ? Boolean.FALSE : null; } + + private void handleCases(List cases) { + //all binding patterns should be considered as new target variables for the selector value: + cases.stream() + .flatMap(cse -> cse.getLabels().stream()) + .filter(label -> label.getKind() == Kind.PATTERN_CASE_LABEL) + .map(label -> ((PatternCaseLabelTree) label).getPattern()) + .filter(pattern -> pattern.getKind() == Kind.BINDING_PATTERN) + .map(pattern -> ((BindingPatternTree) pattern).getVariable()) + .filter(var -> !var.getName().isEmpty()) + .forEach(varAssignments::add); + } } } diff --git a/java/java.hints/src/org/netbeans/modules/java/hints/introduce/Flow.java b/java/java.hints/src/org/netbeans/modules/java/hints/introduce/Flow.java index 860bd8b07c1b..d4e85f87bbe6 100644 --- a/java/java.hints/src/org/netbeans/modules/java/hints/introduce/Flow.java +++ b/java/java.hints/src/org/netbeans/modules/java/hints/introduce/Flow.java @@ -114,6 +114,16 @@ */ public class Flow { + public static final Set LOCAL_VARIABLES = Set.of(ElementKind.BINDING_VARIABLE, ElementKind.EXCEPTION_PARAMETER, ElementKind.LOCAL_VARIABLE, ElementKind.PARAMETER, ElementKind.RESOURCE_VARIABLE); + private static final Set IMPLICITLY_INITIALIZED_VARIABLES = Set.of(ElementKind.BINDING_VARIABLE, ElementKind.EXCEPTION_PARAMETER, ElementKind.PARAMETER); + private static final Set SUPPORTED_VARIABLES; + + static { + SUPPORTED_VARIABLES = EnumSet.noneOf(ElementKind.class); + SUPPORTED_VARIABLES.addAll(LOCAL_VARIABLES); + SUPPORTED_VARIABLES.add(ElementKind.FIELD); + } + public static FlowResult assignmentsForUse(CompilationInfo info, AtomicBoolean cancel) { return assignmentsForUse(info, new AtomicBooleanCancel(cancel)); } @@ -382,7 +392,6 @@ private static final class VisitorImpl extends CancellableTreeScanner>> resumeBefore = new IdentityHashMap>>(); private Map>> resumeAfter = new IdentityHashMap>>(); private Map> resumeOnExceptionHandler = new IdentityHashMap>(); - private boolean inParameters; private Tree nearestMethod; private Set currentMethodVariables = Collections.newSetFromMap(new IdentityHashMap()); private final Set deadBranches = new HashSet(); @@ -645,17 +654,32 @@ public Boolean visitVariable(VariableTree node, ConstructorData p) { Element e = info.getTrees().getElement(getCurrentPath()); if (e != null && SUPPORTED_VARIABLES.contains(e.getKind())) { - // note: if the variable does not have an initializer and is not a parameter (inParameters = true), + // note: if the variable does not have an initializer and is not an implicitly initialized variable, // the State will receive null as an assignment to indicate an uninitializer + boolean implicitlyInitialized = IMPLICITLY_INITIALIZED_VARIABLES.contains(e.getKind()); TreePath tp = node.getInitializer() != null ? new TreePath(getCurrentPath(), node.getInitializer()) : - inParameters ? getCurrentPath() : null; + implicitlyInitialized ? getCurrentPath() : null; recordVariableState(e, tp); currentMethodVariables.add((Element) e); - TreePath pp = getCurrentPath().getParentPath(); - if (pp != null) { - addScopedVariable(pp.getLeaf(), e); + TreePath scopePath; + if (e.getKind() == ElementKind.BINDING_VARIABLE) { + //a wider scope does not matter too much: + scopePath = getCurrentPath().getParentPath(); + SCOPE_SEARCH: while (scopePath != null) { + switch (scopePath.getLeaf().getKind()) { + case VARIABLE, BLOCK: break SCOPE_SEARCH; + default: + scopePath = scopePath.getParentPath(); + continue SCOPE_SEARCH; + } + } + } else { + scopePath = getCurrentPath().getParentPath(); + } + if (scopePath != null) { + addScopedVariable(scopePath.getLeaf(), e); } } @@ -921,15 +945,7 @@ public Boolean visitMethod(MethodTree node, ConstructorData p) { scan(node.getModifiers(), null); scan(node.getReturnType(), null); scan(node.getTypeParameters(), null); - - inParameters = true; - - try { - scan(node.getParameters(), null); - } finally { - inParameters = false; - } - + scan(node.getParameters(), null); scan(node.getThrows(), null); List additionalTrees = p != null ? p.initializers : Collections.emptyList(); @@ -1393,13 +1409,8 @@ public Boolean visitLambdaExpression(LambdaExpressionTree node, ConstructorData } try { - inParameters = true; + scan(node.getParameters(), null); - try { - scan(node.getParameters(), null); - } finally { - inParameters = false; - } b = scan(node.getBody(), p); Set assigned = new HashSet(); @@ -1723,9 +1734,7 @@ public Boolean visitCompilationUnit(CompilationUnitTree node, ConstructorData p) } public Boolean visitCatch(CatchTree node, ConstructorData p) { - inParameters = true; scan(node.getParameter(), p); - inParameters = false; scan(node.getBlock(), p); return null; } @@ -1810,9 +1819,6 @@ private void handleInitializers(List additionalTrees) { } } - private static final Set SUPPORTED_VARIABLES = EnumSet.of(ElementKind.EXCEPTION_PARAMETER, ElementKind.LOCAL_VARIABLE, ElementKind.PARAMETER, ElementKind.FIELD); - private static final Set LOCAL_VARIABLES = EnumSet.of(ElementKind.EXCEPTION_PARAMETER, ElementKind.LOCAL_VARIABLE, ElementKind.PARAMETER); - static class State { private final Set assignments; private final boolean reassigned; diff --git a/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/bugs/ThrowableNotThrownTest.java b/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/bugs/ThrowableNotThrownTest.java index 482c8bc168a7..a7a6641c81af 100644 --- a/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/bugs/ThrowableNotThrownTest.java +++ b/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/bugs/ThrowableNotThrownTest.java @@ -347,4 +347,130 @@ public void testThrowableAssignedToExceptionParameter() throws Exception { .run(ThrowableNotThrown.class) .assertWarnings(); } + + public void testEnhancedSwitch1() throws Exception { + HintTest.create() + .sourceLevel(25) + .input(""" + package test; + import java.io.FileInputStream; + import java.io.IOException; + + public class Test { + void test() throws Throwable { + try { + new FileInputStream("/foo"); + } catch (IOException ex) { + switch (ex.getCause()) { + case IOException ioe -> throw ioe; + case Throwable _ -> throw ex; + } + } + } + } + """) + .run(ThrowableNotThrown.class) + .assertWarnings(); + } + + public void testEnhancedSwitch2() throws Exception { + HintTest.create() + .sourceLevel(25) + .input(""" + package test; + import java.io.FileInputStream; + import java.io.IOException; + + public class Test { + void test() throws Throwable { + try { + new FileInputStream("/foo"); + } catch (IOException ex) { + switch (ex.getCause()) { + case IOException ioe -> throw ex; + case Throwable _ -> throw ex; + } + } + } + } + """) + .run(ThrowableNotThrown.class) + .assertWarnings("9:20-9:33:verifier:Throwable method result is ignored"); + } + + public void testEnhancedSwitchExpressionNoWarn1() throws Exception { + HintTest.create() + .sourceLevel(25) + .input(""" + package test; + import java.io.FileInputStream; + import java.io.IOException; + + public class Test { + void test() throws Throwable { + try { + new FileInputStream("/foo"); + } catch (IOException ex) { + throw switch (ex.getCause()) { + case IOException ioe -> ioe; + case Throwable _ -> ex; + }; + } + } + } + """) + .run(ThrowableNotThrown.class) + .assertWarnings(); + } + + public void testEnhancedSwitchExpressionNoWarn2() throws Exception { + HintTest.create() + .sourceLevel(25) + .input(""" + package test; + import java.io.FileInputStream; + import java.io.IOException; + + public class Test { + void test() throws Throwable { + try { + new FileInputStream("/foo"); + } catch (IOException ex) { + Throwable t = switch (ex.getCause()) { + case IOException ioe -> ioe; + case Throwable tt -> throw tt; + }; + } + } + } + """) + .run(ThrowableNotThrown.class) + .assertWarnings(); + } + + public void testEnhancedSwitchExpressionWarn() throws Exception { + HintTest.create() + .sourceLevel(25) + .input(""" + package test; + import java.io.FileInputStream; + import java.io.IOException; + + public class Test { + void test() throws Throwable { + try { + new FileInputStream("/foo"); + } catch (IOException ex) { + Throwable t = switch (ex.getCause()) { + case IOException ioe -> ioe; + case Throwable tt -> tt; + }; + } + } + } + """) + .run(ThrowableNotThrown.class) + .assertWarnings("9:34-9:47:verifier:Throwable method result is ignored"); + } + } diff --git a/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/introduce/FlowTest.java b/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/introduce/FlowTest.java index 14973c5e7b34..50535c79b6b0 100644 --- a/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/introduce/FlowTest.java +++ b/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/introduce/FlowTest.java @@ -1597,6 +1597,61 @@ public void testSwitchExpression1() throws Exception { "4"); } + public void testParameters() throws Exception { + performTest(""" + package test; + public class Test { + static void t(int ii) { + System.err.println(i`i); + } + } + """, + "int ii"); + } + + public void testLambdaParameters() throws Exception { + performTest(""" + package test; + public class Test { + static void t() { + I i = ii -> System.err.println(i`i); + } + interface I { + public void consume(int ii); + } + } + """, + "int ii"); + } + + public void testCatchParameters() throws Exception { + performTest(""" + package test; + public class Test { + static void t() { + try { + } catch (Throwable tt) { + System.err.println(t`t); + } + } + } + """, + "Throwable tt"); + } + + public void testBindingVariables() throws Exception { + sourceLevel = "21"; + performTest(""" + package test; + public class Test { + static void t(Object o) { + boolean b = o instanceof Integer ii && i`i == 0; + } + } + """, + "Integer ii"); + } + private void performFinalCandidatesTest(String code, boolean allowErrors, String... finalCandidates) throws Exception { prepareTest(code, allowErrors);