aboutsummaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java36
-rw-r--r--src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java83
2 files changed, 103 insertions, 16 deletions
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java
index 821749ca3d..a75a4905f8 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/ControlFlowChecker.java
@@ -18,6 +18,7 @@ import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.syntax.BuildFileAST;
import com.google.devtools.build.lib.syntax.Expression;
import com.google.devtools.build.lib.syntax.ExpressionStatement;
+import com.google.devtools.build.lib.syntax.FlowStatement;
import com.google.devtools.build.lib.syntax.ForStatement;
import com.google.devtools.build.lib.syntax.FuncallExpression;
import com.google.devtools.build.lib.syntax.FunctionDefStatement;
@@ -64,6 +65,22 @@ public class ControlFlowChecker extends SyntaxTreeVisitor {
}
@Override
+ public void visitBlock(List<Statement> statements) {
+ if (cfi == null) {
+ super.visitBlock(statements);
+ return;
+ }
+ boolean alreadyReported = false;
+ for (Statement stmt : statements) {
+ if (!cfi.reachable && !alreadyReported) {
+ issues.add(new Issue("unreachable statement", stmt.getLocation()));
+ alreadyReported = true;
+ }
+ visit(stmt);
+ }
+ }
+
+ @Override
public void visit(IfStatement node) {
if (cfi == null) {
return;
@@ -92,9 +109,16 @@ public class ControlFlowChecker extends SyntaxTreeVisitor {
}
@Override
+ public void visit(FlowStatement node) {
+ Preconditions.checkNotNull(cfi);
+ cfi.reachable = false;
+ }
+
+ @Override
public void visit(ReturnStatement node) {
// Should be rejected by parser, but we may have been fed a bad AST.
Preconditions.checkState(cfi != null, "AST has illegal top-level return statement");
+ cfi.reachable = false;
cfi.returnsAlwaysExplicitly = true;
if (node.getReturnExpression() != null) {
cfi.hasReturnWithValue = true;
@@ -110,6 +134,7 @@ public class ControlFlowChecker extends SyntaxTreeVisitor {
return;
}
if (isFail(node.getExpression())) {
+ cfi.reachable = false;
cfi.returnsAlwaysExplicitly = true;
}
}
@@ -169,16 +194,19 @@ public class ControlFlowChecker extends SyntaxTreeVisitor {
}
private static class ControlFlowInfo {
+ private boolean reachable;
private boolean hasReturnWithValue;
private boolean hasReturnWithoutValue;
private boolean returnsAlwaysExplicitly;
private final LinkedHashSet<Return> returnStatementsWithoutValue;
private ControlFlowInfo(
+ boolean reachable,
boolean hasReturnWithValue,
boolean hasReturnWithoutValue,
boolean returnsAlwaysExplicitly,
LinkedHashSet<Return> returnStatementsWithoutValue) {
+ this.reachable = reachable;
this.hasReturnWithValue = hasReturnWithValue;
this.hasReturnWithoutValue = hasReturnWithoutValue;
this.returnsAlwaysExplicitly = returnsAlwaysExplicitly;
@@ -187,12 +215,13 @@ public class ControlFlowChecker extends SyntaxTreeVisitor {
/** Create a CFI corresponding to an entry point in the control-flow graph. */
static ControlFlowInfo entry() {
- return new ControlFlowInfo(false, false, false, new LinkedHashSet<>());
+ return new ControlFlowInfo(true, false, false, false, new LinkedHashSet<>());
}
/** Creates a copy of a CFI, including the {@code returnStatementsWithoutValue} collection. */
static ControlFlowInfo copy(ControlFlowInfo existing) {
return new ControlFlowInfo(
+ existing.reachable,
existing.hasReturnWithValue,
existing.hasReturnWithoutValue,
existing.returnsAlwaysExplicitly,
@@ -201,9 +230,10 @@ public class ControlFlowChecker extends SyntaxTreeVisitor {
/** Joins the CFIs for several alternative paths together. */
static ControlFlowInfo join(List<ControlFlowInfo> infos) {
- ControlFlowInfo result = new ControlFlowInfo(
- false, false, true, new LinkedHashSet<>());
+ ControlFlowInfo result =
+ new ControlFlowInfo(false, false, false, true, new LinkedHashSet<>());
for (ControlFlowInfo info : infos) {
+ result.reachable |= info.reachable;
result.hasReturnWithValue |= info.hasReturnWithValue;
result.hasReturnWithoutValue |= info.hasReturnWithoutValue;
result.returnsAlwaysExplicitly &= info.returnsAlwaysExplicitly;
diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java
index 12d1125366..b6297a423c 100644
--- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java
+++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java
@@ -154,6 +154,62 @@ public class ControlFlowCheckerTest {
}
@Test
+ public void testUnreachableAfterIf() throws Exception {
+ String messages =
+ findIssues(
+ "def some_function(x):",
+ " if x:",
+ " return",
+ " else:",
+ " fail('fail')",
+ " print('This line is unreachable')")
+ .toString();
+ Truth.assertThat(messages).contains(":6:3: unreachable statement");
+ }
+
+ @Test
+ public void testNoUnreachableDuplicates() throws Exception {
+ List<Issue> messages =
+ findIssues(
+ "def some_function():",
+ " return",
+ " print('unreachable1')",
+ " print('unreachable2')");
+ Truth.assertThat(messages).hasSize(1);
+ }
+
+ @Test
+ public void testUnreachableAfterBreakContinue() throws Exception {
+ String messages =
+ findIssues(
+ "def some_function(x):",
+ " for y in x:",
+ " if y:",
+ " break",
+ " else:",
+ " continue",
+ " print('unreachable')")
+ .toString();
+ Truth.assertThat(messages).contains(":7:5: unreachable statement");
+ }
+
+ @Test
+ public void testReachableStatements() throws Exception {
+ Truth.assertThat(
+ findIssues(
+ "def some_function(x):",
+ " if x:",
+ " return",
+ " for y in []:",
+ " if y:",
+ " continue",
+ " else:",
+ " fail('fail')",
+ " return"))
+ .isEmpty();
+ }
+
+ @Test
public void testIfBeforeReturn() throws Exception {
Truth.assertThat(
findIssues(
@@ -211,19 +267,20 @@ public class ControlFlowCheckerTest {
" else:",
" return y"))
.isEmpty();
- Truth.assertThat(
- findIssues(
- "def f(x,y):",
- " if x:",
- " return x",
- " else:",
- " return x",
- " # from now on everything's unreachable",
- " print('bar')",
- " if y:",
- " return x",
- " # no else branch but doesn't matter since it's unreachable"))
- .isEmpty();
+ List<Issue> issues =
+ findIssues(
+ "def f(x,y):",
+ " if x:",
+ " return x",
+ " else:",
+ " return x",
+ " # from now on everything's unreachable",
+ " print('bar')",
+ " if y:",
+ " return x",
+ " # no else branch but doesn't matter since it's unreachable");
+ Truth.assertThat(issues).hasSize(1);
+ Truth.assertThat(issues.toString()).contains(":7:3: unreachable statement");
}
@Test
generated by cgit v1.2.3 (git 2.39.1) at 2024-07-02 15:45:45 +0000