Repository: bazelbuild/bazel
Issue: Skylint crashes when invoked on a file with a nested function.

### Description of the problem / feature request:

> Skylint crashes with an error below when analyzing a .bzl file with a nested function. A file with
```
def yo():
  def bar():
    pass
```
will cause Skylint crash:
```
Exception in thread "main" java.lang.IllegalStateException
	at com.google.common.base.Preconditions.checkState(Preconditions.java:485)
	at com.google.devtools.skylark.skylint.ControlFlowChecker.visit(ControlFlowChecker.java:159)
	at com.google.devtools.build.lib.syntax.FunctionDefStatement.accept(FunctionDefStatement.java:78)
	at com.google.devtools.build.lib.syntax.SyntaxTreeVisitor.visit(SyntaxTreeVisitor.java:28)
	at com.google.devtools.skylark.skylint.ControlFlowChecker.visitBlock(ControlFlowChecker.java:86)
	at com.google.devtools.build.lib.syntax.SyntaxTreeVisitor.visit(SyntaxTreeVisitor.java:141)
	at com.google.devtools.skylark.skylint.ControlFlowChecker.visit(ControlFlowChecker.java:161)
	at com.google.devtools.build.lib.syntax.FunctionDefStatement.accept(FunctionDefStatement.java:78)
	at com.google.devtools.build.lib.syntax.SyntaxTreeVisitor.visit(SyntaxTreeVisitor.java:28)
	at com.google.devtools.build.lib.syntax.SyntaxTreeVisitor.visitAll(SyntaxTreeVisitor.java:34)
	at com.google.devtools.build.lib.syntax.SyntaxTreeVisitor.visitBlock(SyntaxTreeVisitor.java:47)
	at com.google.devtools.skylark.skylint.ControlFlowChecker.visitBlock(ControlFlowChecker.java:75)
	at com.google.devtools.build.lib.syntax.SyntaxTreeVisitor.visit(SyntaxTreeVisitor.java:62)
	at com.google.devtools.skylark.skylint.ControlFlowChecker.check(ControlFlowChecker.java:68)
	at com.google.devtools.skylark.skylint.Linter.lint(Linter.java:122)
	at com.google.devtools.skylark.skylint.Skylint.main(Skylint.java:48)
ERROR: Non-zero return code '1' from command: Process exited with status 1
```

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

> run Skylint on file with content described above

### Any other information, logs, or outputs that you want to share?

> Technically nested functions are already covered by Skylint linter, because EventHandler errors that happen during build file parsing are recorded as parse-error, but CFChecker does not know about this, so either CFChecker should be made resilient to this issue, or parse errors should prevent other checkers from running. I have a patch for the first option, but believe that second option may be a better one.


Issue comments:

Pull request review comments:

Patch that resolved the issue:
diff --git a/scripts/ij.bazelproject b/scripts/ij.bazelproject
index e90868242d2cf0..f13282c5b8ced5 100644
--- a/scripts/ij.bazelproject
+++ b/scripts/ij.bazelproject
@@ -20,3 +20,4 @@ targets:
   //src/test/...
   -//src/test/docker/...
   //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 db28c52f6dba94..729e3e3f08d7ed 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 @@
 // 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<Issue> issues = new ArrayList<>();
 
@@ -156,7 +157,16 @@ public static boolean isFail(Expression expression) {
 
   @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/ControlFlowCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/ControlFlowCheckerTest.java
index 6d7a673accc5e0..839e47c3490920 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,15 +24,20 @@
 
 @RunWith(JUnit4.class)
 public class ControlFlowCheckerTest {
-  private static List<Issue> findIssues(String... lines) {
+  private static List<Issue> findIssues(EventHandler eventHandler, String... lines) {
     String content = String.join("\n", lines);
     BuildFileAST ast =
-        BuildFileAST.parseString(
-            event -> {
+            BuildFileAST.parseString(
+                    eventHandler,
+                    content);
+    return ControlFlowChecker.check(ast);
+  }
+
+  private static List<Issue> findIssues(String... lines) {
+    return findIssues(event -> {
               throw new IllegalArgumentException(event.getMessage());
             },
-            content);
-    return ControlFlowChecker.check(ast);
+            lines);
   }
 
   @Test
@@ -59,6 +65,24 @@ public void testIfElseReturnMissing() throws Exception {
                 + " 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 =


Test patch: