From a3339c8ca2e824757afe698f591770f4232af530 Mon Sep 17 00:00:00 2001 From: Taras Tsugrii Date: Tue, 13 Feb 2018 09:12:12 -0800 Subject: [Skylint] Do not crash ControlFlowChecker on nested functions. ControlFlowChecker has a precondition check that assumes nested functions do not occur. While this assumption is reasonable for a valid Skylark syntax, linter can actually be invoked on malformed files and users would get a stack trace instead of a human-readable linter error. Alternative and possibly a better strategy would to not run CFChecker in case parse errors are detected. fixes #4511 Closes #4512. PiperOrigin-RevId: 185538897 --- scripts/ij.bazelproject | 1 + .../skylark/skylint/ControlFlowChecker.java | 15 +++++++++-- .../com/google/devtools/skylark/skylint/BUILD | 1 + .../skylark/skylint/ControlFlowCheckerTest.java | 30 +++++++++++++++++----- 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/scripts/ij.bazelproject b/scripts/ij.bazelproject index 4cdd223ef1..7b6a97946b 100644 --- a/scripts/ij.bazelproject +++ b/scripts/ij.bazelproject @@ -19,3 +19,4 @@ targets: //src/java_tools/singlejar:SingleJar //src/test/... //src/tools/remote/... + //src/tools/skylark/... 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 db28c52f6d..5421193e5c 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 @@ -46,7 +46,8 @@ import javax.annotation.Nullable; // TODO(skylark-team): Check for unreachable statements public class ControlFlowChecker extends SyntaxTreeVisitor { private static final String MISSING_RETURN_VALUE_CATEGORY = "missing-return-value"; - public static final String UNREACHABLE_STATEMENT_CATEGORY = "unreachable-statement"; + private static final String UNREACHABLE_STATEMENT_CATEGORY = "unreachable-statement"; + private static final String NESTED_FUNCTION_CATEGORY = "nested-function"; private final List issues = new ArrayList<>(); @@ -156,7 +157,17 @@ public class ControlFlowChecker extends SyntaxTreeVisitor { @Override public void visit(FunctionDefStatement node) { - Preconditions.checkState(cfi == null); + if (cfi != null) { + issues.add( + Issue.create( + NESTED_FUNCTION_CATEGORY, + node.getIdentifier() + + " is a nested function which is not allowed." + + " Consider inlining it or moving it to top-level." + + " For more details, have a look at the Skylark documentation.", + node.getLocation())); + return; + } cfi = ControlFlowInfo.entry(); super.visit(node); if (cfi.hasReturnWithValue && (!cfi.returnsAlwaysExplicitly || cfi.hasReturnWithoutValue)) { diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BUILD b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BUILD index 2178ad4f2b..719b437c70 100644 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BUILD +++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BUILD @@ -5,6 +5,7 @@ java_test( srcs = glob(["*.java"]), test_class = "com.google.devtools.skylark.skylint.SkylintTests", deps = [ + "//src/main/java/com/google/devtools/build/lib:events", "//src/main/java/com/google/devtools/build/lib:packages", "//src/test/java/com/google/devtools/build/lib:testutil", "//src/tools/skylark/java/com/google/devtools/skylark/skylint:skylint_lib", 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 6d7a673acc..3424be9494 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 @@ -15,6 +15,7 @@ package com.google.devtools.skylark.skylint; import com.google.common.truth.Truth; +import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.syntax.BuildFileAST; import java.util.List; import org.junit.Test; @@ -23,17 +24,20 @@ import org.junit.runners.JUnit4; @RunWith(JUnit4.class) public class ControlFlowCheckerTest { - private static List findIssues(String... lines) { + private static List findIssues(EventHandler eventHandler, String... lines) { String content = String.join("\n", lines); - BuildFileAST ast = - BuildFileAST.parseString( - event -> { - throw new IllegalArgumentException(event.getMessage()); - }, - content); + BuildFileAST ast = BuildFileAST.parseString(eventHandler, content); return ControlFlowChecker.check(ast); } + private static List findIssues(String... lines) { + return findIssues( + event -> { + throw new IllegalArgumentException(event.getMessage()); + }, + lines); + } + @Test public void testAnalyzerToleratesTopLevelFail() throws Exception { Truth.assertThat( @@ -59,6 +63,18 @@ public class ControlFlowCheckerTest { + " For more details, have a look at the documentation. [missing-return-value]"); } + @Test + public void testNestedFunction() { + Truth.assertThat( + findIssues(event -> {}, "def foo():", " def bar():", " pass", " return") + .toString()) + .contains( + "2:3-3:8: bar is a nested function which is not allowed." + + " Consider inlining it or moving it to top-level." + + " For more details, have a look at the Skylark documentation." + + " [nested-function]"); + } + @Test public void testIfElseReturnValueMissing() throws Exception { String messages = -- cgit v1.2.3