[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

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<Issue> 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<Issue> findIssues(String... lines) {
+  private static List<Issue> 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<Issue> findIssues(String... lines) {
+    return findIssues(
+        event -> {
+          throw new IllegalArgumentException(event.getMessage());
+        },
+        lines);
+  }
+
   @Test
   public void testAnalyzerToleratesTopLevelFail() throws Exception {
     Truth.assertThat(
@@ -60,6 +64,18 @@ public class ControlFlowCheckerTest {
   }
 
   @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 =
         findIssues(