Skip to content

Adding enhanced switch support to ThrowableNotThrown #8717

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
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Adding enhanced switch support to ThrowableNotThrown #8717

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
47 changes: 38 additions & 9 deletions java/java.hints/src/org/netbeans/modules/java/hints/bugs/ThrowableNotThrown.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -90,13 +96,6 @@ public static ErrorDescription newThrowable(HintContext ctx) {
return null;
}

private static final EnumSet<ElementKind> 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();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<? extends CaseTree> cases) {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: collectVarAssignments potentially a more descriptive name?

//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);
}
}
}
58 changes: 32 additions & 26 deletions java/java.hints/src/org/netbeans/modules/java/hints/introduce/Flow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,16 @@
*/
public class Flow {

public static final Set<ElementKind> LOCAL_VARIABLES = Set.of(ElementKind.BINDING_VARIABLE, ElementKind.EXCEPTION_PARAMETER, ElementKind.LOCAL_VARIABLE, ElementKind.PARAMETER, ElementKind.RESOURCE_VARIABLE);
private static final Set<ElementKind> IMPLICITLY_INITIALIZED_VARIABLES = Set.of(ElementKind.BINDING_VARIABLE, ElementKind.EXCEPTION_PARAMETER, ElementKind.PARAMETER);
Comment on lines +117 to +118
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: since this got moved; maybe add some line breaks for mortals without panoramic screens ;)

private static final Set<ElementKind> 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));
}
Expand Down Expand Up @@ -382,7 +392,6 @@ private static final class VisitorImpl extends CancellableTreeScanner<Boolean, C
private Map<Tree, Collection<Map<Element, State>>> resumeBefore = new IdentityHashMap<Tree, Collection<Map<Element, State>>>();
private Map<Tree, Collection<Map<Element, State>>> resumeAfter = new IdentityHashMap<Tree, Collection<Map<Element, State>>>();
private Map<TypeMirror, Map<Element, State>> resumeOnExceptionHandler = new IdentityHashMap<TypeMirror, Map<Element, State>>();
private boolean inParameters;
private Tree nearestMethod;
private Set<Element> currentMethodVariables = Collections.newSetFromMap(new IdentityHashMap<Element, Boolean>());
private final Set<Tree> deadBranches = new HashSet<Tree>();
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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<Tree> additionalTrees = p != null ? p.initializers : Collections.<Tree>emptyList();
Expand Down Expand Up @@ -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<Element> assigned = new HashSet<Element>();
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -1810,9 +1819,6 @@ private void handleInitializers(List<Tree> additionalTrees) {
}
}

private static final Set<ElementKind> SUPPORTED_VARIABLES = EnumSet.of(ElementKind.EXCEPTION_PARAMETER, ElementKind.LOCAL_VARIABLE, ElementKind.PARAMETER, ElementKind.FIELD);
private static final Set<ElementKind> LOCAL_VARIABLES = EnumSet.of(ElementKind.EXCEPTION_PARAMETER, ElementKind.LOCAL_VARIABLE, ElementKind.PARAMETER);

static class State {
private final Set<TreePath> assignments;
private final boolean reassigned;
Expand Down
126 changes: 126 additions & 0 deletions ...java.hints/test/unit/src/org/netbeans/modules/java/hints/bugs/ThrowableNotThrownTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}

}
55 changes: 55 additions & 0 deletions java/java.hints/test/unit/src/org/netbeans/modules/java/hints/introduce/FlowTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Loading