From 5d143fe57839e8ae9f4a04bfbb21f30269a26430 Mon Sep 17 00:00:00 2001 From: Jan Lahoda Date: Sat, 1 Aug 2020 06:34:04 +0200 Subject: [PATCH 1/5] Handling of switch expressions and rule cases in Flow and NPECheck hint. --- .../modules/java/hints/bugs/NPECheck.java | 97 +++++++++++++++++-- .../modules/java/hints/introduce/Flow.java | 79 +++++++++++++-- .../modules/java/hints/bugs/NPECheckTest.java | 38 ++++++++ .../java/hints/introduce/FlowTest.java | 43 ++++++++ .../api/java/source/TreeUtilities.java | 6 +- .../modules/java/source/TreeShims.java | 25 +++++ 6 files changed, 268 insertions(+), 20 deletions(-) diff --git a/java/java.hints/src/org/netbeans/modules/java/hints/bugs/NPECheck.java b/java/java.hints/src/org/netbeans/modules/java/hints/bugs/NPECheck.java index 17316324b137..6d29808abf94 100644 --- a/java/java.hints/src/org/netbeans/modules/java/hints/bugs/NPECheck.java +++ b/java/java.hints/src/org/netbeans/modules/java/hints/bugs/NPECheck.java @@ -48,7 +48,8 @@ import javax.lang.model.util.ElementFilter; import org.netbeans.api.annotations.common.CheckForNull; import org.netbeans.api.java.source.CompilationInfo; -import org.netbeans.api.java.source.support.CancellableTreePathScanner; +import org.netbeans.api.java.source.support.CancellableTreeScanner; +import org.netbeans.modules.editor.java.TreeShims; import org.netbeans.spi.editor.hints.ErrorDescription; import org.openide.util.NbBundle; @@ -542,7 +543,7 @@ public interface AnnotationMirrorGetter { public Iterable getAnnotationMirrors(CompilationInfo info, Element el); } - private static final class VisitorImpl extends CancellableTreePathScanner { + private static final class VisitorImpl extends CancellableTreeScanner { private final HintContext ctx; private final CompilationInfo info; @@ -564,6 +565,7 @@ private static final class VisitorImpl extends CancellableTreePathScanner> resumeOnExceptionHandler = new IdentityHashMap<>(); private final Map expressionState = new IdentityHashMap<>(); private final List pendingFinally = new LinkedList<>(); + private List pendingYields = new ArrayList<>(); private boolean not; private boolean doNotRecord; private final TypeElement throwableEl; @@ -617,11 +619,45 @@ protected boolean isCanceled() { } + private TreePath currentPath; + + public TreePath getCurrentPath() { + return currentPath; + } + + public State scan(TreePath path, Void p) { + TreePath oldPath = currentPath; + try { + currentPath = path; + return super.scan(path, p); + } finally { + currentPath = oldPath; + } + } + @Override public State scan(Tree tree, Void p) { resume(tree, resumeBefore); - State r = super.scan(tree, p); + State r; + + if (tree != null) { + TreePath oldPath = currentPath; + try { + currentPath = new TreePath(currentPath, tree); + if (TreeShims.SWITCH_EXPRESSION.equals(tree.getKind().name())) { + r = visitSwitchExpression(tree, p); + } else if (TreeShims.YIELD.equals(tree.getKind().name())) { + r = visitYield(tree, p); + } else { + r = super.scan(tree, p); + } + } finally { + currentPath = oldPath; + } + } else { + r = null; + } TypeMirror currentType = tree != null ? info.getTrees().getTypeMirror(new TreePath(getCurrentPath(), tree)) : null; @@ -1223,27 +1259,54 @@ public State visitArrayAccess(ArrayAccessTree node, Void p) { @Override public State visitSwitch(SwitchTree node, Void p) { - scan(node.getExpression(), null); + handleGeneralizedSwitch(node, node.getExpression(), node.getCases()); + return null; + } + + public State visitSwitchExpression(Tree node, Void p) { + List oldPendingYields = pendingYields; + try { + pendingYields = new ArrayList<>(); + handleGeneralizedSwitch(node, TreeShims.getExpressions(node).get(0), TreeShims.getCases(node)); + if (pendingYields.isEmpty()) { + //should not happen (for valid source) + return State.POSSIBLE_NULL; + } + State result = pendingYields.get(0); + for (State s : pendingYields.subList(1, pendingYields.size())) { + result = State.collect(result, s); + } + return result; + } finally { + pendingYields = oldPendingYields; + } + } + + private void handleGeneralizedSwitch(Tree switchTree, ExpressionTree expression, List cases) { + scan(expression, null); Map origVariable2State = new HashMap<>(variable2State); boolean exhaustive = false; - for (CaseTree ct : node.getCases()) { + for (CaseTree ct : cases) { mergeIntoVariable2State(origVariable2State); if (ct.getExpression() == null) { exhaustive = true; } - scan(ct, null); + State caseResult = scan(ct, null); + + if (TreeShims.isRuleCase(ct)) { + pendingYields.add(caseResult); + breakTo(switchTree); + } } if (!exhaustive) { mergeIntoVariable2State(origVariable2State); } - - return null; } @Override @@ -1252,13 +1315,27 @@ public State visitBreak(BreakTree node, Void p) { Tree target = info.getTreeUtilities().getBreakContinueTargetTree(getCurrentPath()); - resumeAfter(target, variable2State); + breakTo(target); - variable2State = new HashMap<>(); //XXX: fields? + return null; + } + + public State visitYield(Tree node, Void p) { + pendingYields.add(scan(TreeShims.getYieldValue(node), p)); + + Tree target = info.getTreeUtilities().getBreakContinueTargetTree(getCurrentPath()); + breakTo(target); + return null; } + private void breakTo(Tree target) { + resumeAfter(target, variable2State); + + variable2State = new HashMap<>(); //XXX: fields? + } + @Override public State visitTry(TryTree node, Void p) { Map> oldResumeOnExceptionHandler = resumeOnExceptionHandler; 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 46df84ff7f7d..578ed5160557 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 @@ -101,7 +101,8 @@ import javax.lang.model.type.TypeMirror; import org.netbeans.api.java.source.CompilationInfo; import org.netbeans.api.java.source.CompilationInfo.CacheClearPolicy; -import org.netbeans.api.java.source.support.CancellableTreePathScanner; +import org.netbeans.api.java.source.support.CancellableTreeScanner; +import org.netbeans.modules.editor.java.TreeShims; import org.netbeans.modules.java.hints.errors.Utilities; import org.netbeans.spi.java.hints.HintContext; @@ -353,7 +354,7 @@ public AV(TreePath path, State state) { * that are qualified to belong to other scopes are ignored at the moment. * */ - private static final class VisitorImpl extends CancellableTreePathScanner { + private static final class VisitorImpl extends CancellableTreeScanner { private final CompilationInfo info; private final TypeElement undefinedSymbolScope; @@ -444,11 +445,45 @@ protected boolean isCanceled() { return cancel.isCanceled(); } + private TreePath currentPath; + + public TreePath getCurrentPath() { + return currentPath; + } + + public Boolean scan(TreePath path, ConstructorData p) { + TreePath oldPath = currentPath; + try { + currentPath = path; + return super.scan(path, p); + } finally { + currentPath = oldPath; + } + } + @Override public Boolean scan(Tree tree, ConstructorData p) { resume(tree, resumeBefore); - Boolean result = super.scan(tree, p); + Boolean result; + + if (tree != null) { + TreePath oldPath = currentPath; + try { + currentPath = new TreePath(currentPath, tree); + if (TreeShims.SWITCH_EXPRESSION.equals(tree.getKind().name())) { + result = visitSwitchExpression(tree, p); + } else if (TreeShims.YIELD.equals(tree.getKind().name())) { + result = visitYield(tree, p); + } else { + result = super.scan(tree, p); + } + } finally { + currentPath = oldPath; + } + } else { + result = null; + } resume(tree, resumeAfter); @@ -1172,15 +1207,39 @@ public Boolean visitBreak(BreakTree node, ConstructorData p) { Tree target = info.getTreeUtilities().getBreakContinueTargetTree(getCurrentPath()); - resumeAfter(target, variable2State); + breakTo(target); - variable2State = new HashMap< Element, State>(); + return null; + } + + public Boolean visitYield(Tree node, ConstructorData p) { + scan(TreeShims.getYieldValue(node), p); + + Tree target = info.getTreeUtilities().getBreakContinueTargetTree(getCurrentPath()); + breakTo(target); + return null; } + private void breakTo(Tree target) { + resumeAfter(target, variable2State); + + variable2State = new HashMap< Element, State>(); + } + public Boolean visitSwitch(SwitchTree node, ConstructorData p) { - scan(node.getExpression(), null); + generalizedSwitch(node, node.getExpression(), node.getCases()); + return null; + } + + public Boolean visitSwitchExpression(Tree node, ConstructorData p) { + generalizedSwitch(node, TreeShims.getExpressions(node).get(0), TreeShims.getCases(node)); + return null; //never a constant expression + } + + private void generalizedSwitch(Tree switchTree, ExpressionTree expression, List cases) { + scan(expression, null); Map< Element, State> origVariable2State = new HashMap< Element, State>(variable2State); @@ -1188,7 +1247,7 @@ public Boolean visitSwitch(SwitchTree node, ConstructorData p) { boolean exhaustive = false; - for (CaseTree ct : node.getCases()) { + for (CaseTree ct : cases) { variable2State = mergeOr(variable2State, origVariable2State); if (ct.getExpression() == null) { @@ -1196,13 +1255,15 @@ public Boolean visitSwitch(SwitchTree node, ConstructorData p) { } scan(ct, null); + + if (TreeShims.isRuleCase(ct)) { + breakTo(switchTree); + } } if (!exhaustive) { variable2State = mergeOr(variable2State, origVariable2State); } - - return null; } public Boolean visitEnhancedForLoop(EnhancedForLoopTree node, ConstructorData p) { diff --git a/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/bugs/NPECheckTest.java b/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/bugs/NPECheckTest.java index 6e9dc30de338..e62a098374c6 100644 --- a/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/bugs/NPECheckTest.java +++ b/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/bugs/NPECheckTest.java @@ -1760,6 +1760,44 @@ public void testExceptionIsNonNullNETBEANS734c() throws Exception { .assertWarnings("6:29-6:37:verifier:DN"); } + public void testRuleCases() throws Exception { + HintTest.create() + .sourceLevel("14") + .input("package test;\n" + + "public class Test {\n" + + " public void test(int i) {\n" + + " Object o;\n" + + " switch (i) {\n" + + " case 0 -> o = null;\n" + + " default -> { o = 12; }\n" + + " }\n" + + " o.toString();\n" + + " }\n" + + "}") + .run(NPECheck.class) + .assertWarnings("8:6-8:14:verifier:Possibly Dereferencing null"); + } + + public void testSwitchExpression1() throws Exception { + HintTest.create() + .sourceLevel("14") + .input("package test;\n" + + "public class Test {\n" + + " public void test(int i) {\n" + + " Object o1;\n" + + " Object o2 = switch (i) {\n" + + " case 0 -> o1 = null;\n" + + " default -> { yield o1 = 12; }\n" + + " };\n" + + " o1.toString();\n" + + " o2.toString();\n" + + " }\n" + + "}") + .run(NPECheck.class) + .assertWarnings("8:7-8:15:verifier:Possibly Dereferencing null", + "9:7-9:15:verifier:Possibly Dereferencing null"); + } + private void performAnalysisTest(String fileName, String code, String... golden) throws Exception { HintTest.create() .input(fileName, code) 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 b7fdad34e100..21ef1c7df524 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 @@ -1552,6 +1552,49 @@ public void testDoWhileUsedNextIteration241808() throws Exception { "di"); } + public void testRuleCases() throws Exception { + sourceLevel = "14"; + performTest("package test;\n" + + "public class Test {\n" + + " public void test(int i) {\n" + + " int val;\n" + + " switch (i) {\n" + + " case 0 -> val = 0;\n" + + " case 1 -> val = 1;\n" + + " case 2 -> { val = 2; }\n" + + " case 3 -> { val = 3; }\n" + + " default -> { val = -1; }\n" + + " }\n" + + " System.err.println(val`);\n" + + " }\n" + + "}", + "0", + "1", + "2", + "3", + "-1"); + } + + public void testSwitchExpression1() throws Exception { + sourceLevel = "14"; + performTest("package test;\n" + + "public class Test {\n" + + " static void t(int p) {\n" + + " int ii;\n" + + " int x = switch (p) {\n" + + " case 0: ii = 1; yield 0;\n" + + " case 1: ii = 2;\n" + + " case 2: ii = 3; yield 0;\n" + + " default: ii = 4; yield 0;\n" + + " };\n" + + " System.err.println(i`i);\n" + + " }\n" + + "}\n", + "1", + "3", + "4"); + } + private void performFinalCandidatesTest(String code, boolean allowErrors, String... finalCandidates) throws Exception { prepareTest(code, allowErrors); diff --git a/java/java.source.base/src/org/netbeans/api/java/source/TreeUtilities.java b/java/java.source.base/src/org/netbeans/api/java/source/TreeUtilities.java index dc13204df020..e7c31ab55b9e 100644 --- a/java/java.source.base/src/org/netbeans/api/java/source/TreeUtilities.java +++ b/java/java.source.base/src/org/netbeans/api/java/source/TreeUtilities.java @@ -103,6 +103,7 @@ import org.netbeans.lib.nbjavac.services.NBParserFactory; import org.netbeans.lib.nbjavac.services.NBResolve; import org.netbeans.lib.nbjavac.services.NBTreeMaker.IndexedClassDecl; +import org.netbeans.modules.java.source.TreeShims; import org.netbeans.modules.java.source.TreeUtilitiesAccessor; import org.netbeans.modules.java.source.builder.CommentHandlerService; import org.netbeans.modules.java.source.builder.CommentSetImpl; @@ -1377,7 +1378,7 @@ private int[] findNameSpan(String name, Tree t, JavaTokenId... allowedTokens) { * * @param breakOrContinue {@link TreePath} to the tree that should be inspected. * The breakOrContinue.getLeaf().getKind() - * has to be either {@link Kind#BREAK} or {@link Kind#CONTINUE}, or + * has to be one of {@link Kind#BREAK}, {@link Kind#CONTINUE}, or {@link Kind#YIELD}, or * an IllegalArgumentException is thrown * @return the tree that is the "target" for the given break or continue statement, or null if there is none. Tree can be of type StatementTree or ExpressionTree * @throws IllegalArgumentException if the given tree is not a break or continue tree or if the given {@link CompilationInfo} @@ -1416,6 +1417,9 @@ public Tree getBreakContinueTargetTree(TreePath breakOrContinue) throws IllegalA return target; } default: + if (TreeShims.YIELD.equals(leaf.getKind().name())) { + return TreeShims.getTarget(leaf); + } throw new IllegalArgumentException("Unsupported kind: " + leaf.getKind()); } } diff --git a/java/java.source.base/src/org/netbeans/modules/java/source/TreeShims.java b/java/java.source.base/src/org/netbeans/modules/java/source/TreeShims.java index 88504aa1708b..59d00bfe015b 100644 --- a/java/java.source.base/src/org/netbeans/modules/java/source/TreeShims.java +++ b/java/java.source.base/src/org/netbeans/modules/java/source/TreeShims.java @@ -27,6 +27,7 @@ import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.TreeMaker; import com.sun.tools.javac.util.ListBuffer; +import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.ArrayList; @@ -69,6 +70,17 @@ public static Tree getBody(CaseTree node) { } } + public static boolean isRuleCase(CaseTree node) { + try { + Method getCaseKind = CaseTree.class.getDeclaredMethod("getCaseKind"); + return "RULE".equals(String.valueOf(getCaseKind.invoke(node))); + } catch (NoSuchMethodException ex) { + return false; + } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException ex) { + throw TreeShims.throwAny(ex); + } + } + public static Tree getPattern(InstanceOfTree node) { try { Method getPattern = InstanceOfTree.class.getDeclaredMethod("getPattern"); @@ -237,4 +249,17 @@ public static ElementKind getRecordKind() { } } + public static Tree getTarget(Tree node) { + if (!node.getKind().name().equals(YIELD)) { + throw new IllegalStateException(); + } + try { + Field target = node.getClass().getField("target"); + return (Tree) target.get(node); + } catch (NoSuchFieldException ex) { + return null; + } catch (IllegalAccessException | IllegalArgumentException ex) { + throw TreeShims.throwAny(ex); + } + } } From 29b3fb796df9277d4949f20dad4bc4c78cc93bbd Mon Sep 17 00:00:00 2001 From: Jan Lahoda Date: Sun, 2 Aug 2020 08:20:25 +0200 Subject: [PATCH 2/5] Using a correct overload if TreeScanner.scan --- .../src/org/netbeans/modules/java/hints/bugs/NPECheck.java | 2 +- .../src/org/netbeans/modules/java/hints/introduce/Flow.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/java/java.hints/src/org/netbeans/modules/java/hints/bugs/NPECheck.java b/java/java.hints/src/org/netbeans/modules/java/hints/bugs/NPECheck.java index 6d29808abf94..d8bec2557bb2 100644 --- a/java/java.hints/src/org/netbeans/modules/java/hints/bugs/NPECheck.java +++ b/java/java.hints/src/org/netbeans/modules/java/hints/bugs/NPECheck.java @@ -629,7 +629,7 @@ public State scan(TreePath path, Void p) { TreePath oldPath = currentPath; try { currentPath = path; - return super.scan(path, p); + return super.scan(path.getLeaf(), p); } finally { currentPath = oldPath; } 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 578ed5160557..1113473a4213 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 @@ -455,7 +455,7 @@ public Boolean scan(TreePath path, ConstructorData p) { TreePath oldPath = currentPath; try { currentPath = path; - return super.scan(path, p); + return super.scan(path.getLeaf(), p); } finally { currentPath = oldPath; } From ad7dc29e3b859ee565db863e58ef5b6e35ff0de7 Mon Sep 17 00:00:00 2001 From: Jan Lahoda Date: Sun, 2 Aug 2020 18:13:32 +0200 Subject: [PATCH 3/5] Fixing FlowTest on JDK 8. --- java/java.hints/nbproject/project.xml | 4 ++++ .../org/netbeans/modules/java/hints/introduce/FlowTest.java | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/java/java.hints/nbproject/project.xml b/java/java.hints/nbproject/project.xml index a328b255f557..a94078f3aea8 100644 --- a/java/java.hints/nbproject/project.xml +++ b/java/java.hints/nbproject/project.xml @@ -486,6 +486,10 @@ org.netbeans.core + + org.netbeans.core.startup + + org.netbeans.insane 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 21ef1c7df524..14973c5e7b34 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 @@ -39,6 +39,7 @@ import org.netbeans.api.java.source.JavaSource.Phase; import org.netbeans.api.java.source.SourceUtilsTestUtil; import org.netbeans.api.lexer.Language; +import org.netbeans.core.startup.Main; import org.netbeans.junit.NbTestCase; import org.netbeans.modules.java.hints.introduce.Flow.FlowResult; import org.netbeans.modules.java.hints.spiimpl.TestUtilities; @@ -62,6 +63,7 @@ public FlowTest(String name) { @Override protected void setUp() throws Exception { + JavacParser.DISABLE_SOURCE_LEVEL_DOWNGRADE = true; SourceUtilsTestUtil .prepareTest(new String[0], new Object[0]); super @@ -1609,4 +1611,8 @@ private void performFinalCandidatesTest(String code, boolean allowErrors, String assertEquals(Arrays.asList(finalCandidates), computedCandidates); } + + static { + Main.initializeURLFactory(); + } } From 2dbc768528dcd822d55592ee0da1567280fe522a Mon Sep 17 00:00:00 2001 From: Jan Lahoda Date: Mon, 3 Aug 2020 08:45:41 +0200 Subject: [PATCH 4/5] Attempting to improve stability of HideFieldByVarTest. --- .../modules/java/hints/HideFieldByVarTest.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/HideFieldByVarTest.java b/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/HideFieldByVarTest.java index 3115b7a058aa..e0696ad13a32 100644 --- a/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/HideFieldByVarTest.java +++ b/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/HideFieldByVarTest.java @@ -19,6 +19,7 @@ package org.netbeans.modules.java.hints; import com.sun.source.util.TreePath; +import java.io.File; import java.util.List; import java.util.Locale; import org.netbeans.api.java.source.CompilationInfo; @@ -77,9 +78,8 @@ public void testLocaVarInStaticMethod() throws Exception { "}"; for (int i = 0; i < text.length(); i++) { - clearWorkDir(); + workDirIndex = i; performAnalysisTest("test/Test.java", "// index: " + i + "\n" + text, i); - SourceUtils.waitScanFinished(); } } public void testLocaVarAgainsInhVar() throws Exception { @@ -116,5 +116,16 @@ protected List computeErrors(CompilationInfo info, TreePath pa } private String sourceLevel = "1.5"; + + private int workDirIndex = -1; + + @Override + public String getWorkDirPath() { + String basePath = super.getWorkDirPath(); + if (workDirIndex != (-1)) { + basePath += File.separator + workDirIndex; + } + return basePath; + } } From edd000c6b6f60a0243dd304e11859923bbaea814 Mon Sep 17 00:00:00 2001 From: Jan Lahoda Date: Wed, 5 Aug 2020 07:29:13 +0200 Subject: [PATCH 5/5] Using a diamond instead of explicit type parameters. --- .../src/org/netbeans/modules/java/hints/introduce/Flow.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 1113473a4213..8e521409dbbb 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 @@ -1225,7 +1225,7 @@ public Boolean visitYield(Tree node, ConstructorData p) { private void breakTo(Tree target) { resumeAfter(target, variable2State); - variable2State = new HashMap< Element, State>(); + variable2State = new HashMap<>(); } public Boolean visitSwitch(SwitchTree node, ConstructorData p) {