Skip to content
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

Handling of switch expressions and rule cases in Flow and NPECheck hint. #35

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Handling of switch expressions and rule cases in Flow and NPECheck hint. #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5d143fe
Handling of switch expressions and rule cases in Flow and NPECheck hint.
jlahoda Aug 1, 2020
29b3fb7
Using a correct overload if TreeScanner.scan
jlahoda Aug 2, 2020
ad7dc29
Fixing FlowTest on JDK 8.
jlahoda Aug 2, 2020
2dbc768
Attempting to improve stability of HideFieldByVarTest.
jlahoda Aug 3, 2020
edd000c
Using a diamond instead of explicit type parameters.
jlahoda Aug 5, 2020
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
4 changes: 4 additions & 0 deletions java/java.hints/nbproject/project.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,10 @@
<code-name-base>org.netbeans.core</code-name-base>
<compile-dependency/>
</test-dependency>
<test-dependency>
<code-name-base>org.netbeans.core.startup</code-name-base>
<compile-dependency/>
</test-dependency>
<test-dependency>
<code-name-base>org.netbeans.insane</code-name-base>
<compile-dependency/>
Expand Down
97 changes: 87 additions & 10 deletions java/java.hints/src/org/netbeans/modules/java/hints/bugs/NPECheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -542,7 +543,7 @@ public interface AnnotationMirrorGetter {
public Iterable<? extends AnnotationMirror> getAnnotationMirrors(CompilationInfo info, Element el);
}

private static final class VisitorImpl extends CancellableTreePathScanner<State, Void> {
private static final class VisitorImpl extends CancellableTreeScanner<State, Void> {

private final HintContext ctx;
private final CompilationInfo info;
Expand All @@ -564,6 +565,7 @@ private static final class VisitorImpl extends CancellableTreePathScanner<State,
private Map<TypeMirror, Map<VariableElement, State>> resumeOnExceptionHandler = new IdentityHashMap<>();
private final Map<Tree, State> expressionState = new IdentityHashMap<>();
private final List<TreePath> pendingFinally = new LinkedList<>();
private List<State> pendingYields = new ArrayList<>();
private boolean not;
private boolean doNotRecord;
private final TypeElement throwableEl;
Expand Down Expand Up @@ -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.getLeaf(), 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;

Expand Down Expand Up @@ -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<State> 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<? extends CaseTree> cases) {
scan(expression, null);

Map<VariableElement, State> 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
Expand All @@ -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<TypeMirror, Map<VariableElement, State>> oldResumeOnExceptionHandler = resumeOnExceptionHandler;
Expand Down
79 changes: 70 additions & 9 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 @@ -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;
Copy link
Owner Author

Choose a reason for hiding this comment

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

Line 105.

import org.netbeans.modules.java.hints.errors.Utilities;
import org.netbeans.spi.java.hints.HintContext;

Expand Down Expand Up @@ -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<Boolean, ConstructorData> {
private static final class VisitorImpl extends CancellableTreeScanner<Boolean, ConstructorData> {

private final CompilationInfo info;
private final TypeElement undefinedSymbolScope;
Expand Down Expand Up @@ -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;
Copy link
Owner Author

Choose a reason for hiding this comment

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

Line 455.

try {
currentPath = path;
return super.scan(path.getLeaf(), 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);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Line 477.

} else {
result = super.scan(tree, p);
}
} finally {
currentPath = oldPath;
}
} else {
result = null;
}

resume(tree, resumeAfter);

Expand Down Expand Up @@ -1172,37 +1207,63 @@ public Boolean visitBreak(BreakTree node, ConstructorData p) {

Tree target = info.getTreeUtilities().getBreakContinueTargetTree(getCurrentPath());

resumeAfter(target, variable2State);
breakTo(target);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Line 1210.


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);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Line 1220.


return null;
}

private void breakTo(Tree target) {
resumeAfter(target, variable2State);

variable2State = new HashMap<>();
}

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<? extends CaseTree> cases) {
scan(expression, null);

Map< Element, State> origVariable2State = new HashMap< Element, State>(variable2State);

variable2State = new HashMap< Element, State>();

boolean exhaustive = false;

for (CaseTree ct : node.getCases()) {
for (CaseTree ct : cases) {
variable2State = mergeOr(variable2State, origVariable2State);

if (ct.getExpression() == null) {
exhaustive = true;
}

scan(ct, null);

if (TreeShims.isRuleCase(ct)) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Line 1259.

breakTo(switchTree);
}
}

if (!exhaustive) {
variable2State = mergeOr(variable2State, origVariable2State);
}

return null;
}

public Boolean visitEnhancedForLoop(EnhancedForLoopTree node, ConstructorData p) {
Expand Down
15 changes: 13 additions & 2 deletions java/java.hints/test/unit/src/org/netbeans/modules/java/hints/HideFieldByVarTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -116,5 +116,16 @@ protected List<ErrorDescription> 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;
}

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