New flag --incompatible_bzl_disallow_load_after_statement

This disallows to have any statement in a .bzl file before a load() statement.

RELNOTES:
  load() statements should be called at the top of .bzl files, before any
  other statement. This convention will be enforced in the future.
PiperOrigin-RevId: 155636719

3 files changed, 65 insertions, 0 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
index 8653e83707..f25ec69794 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
@@ -76,4 +76,14 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable
     help = "If set to true, the `+` becomes disabled for dicts."
   )
   public boolean incompatibleDisallowDictPlus;
+
+  @Option(
+    name = "incompatible_bzl_disallow_load_after_statement",
+    defaultValue = "false",
+    category = "incompatible changes",
+    help =
+        "If set to true, all `load` must be called at the top of .bzl files, before any other "
+            + "statement."
+  )
+  public boolean incompatibleBzlDisallowLoadAfterStatement;
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
index b0e766d6bf..84a7cbbaaa 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
@@ -41,6 +41,8 @@ public final class ValidationEnvironment {
 
   private final Set<String> readOnlyVariables = new HashSet<>();
 
+  private final SkylarkSemanticsOptions semantics;
+
   // A stack of variable-sets which are read only but can be assigned in different
   // branches of if-else statements.
   private final Stack<Set<String>> futureReadOnlyVariables = new Stack<>();
@@ -54,6 +56,7 @@ public final class ValidationEnvironment {
     Set<String> builtinVariables = env.getVariableNames();
     variables.addAll(builtinVariables);
     readOnlyVariables.addAll(builtinVariables);
+    semantics = env.getSemantics();
   }
 
   /**
@@ -62,6 +65,7 @@ public final class ValidationEnvironment {
   public ValidationEnvironment(ValidationEnvironment parent) {
     // Don't copy readOnlyVariables: Variables may shadow global values.
     this.parent = parent;
+    semantics = parent.semantics;
   }
 
   /**
@@ -146,10 +150,45 @@ public final class ValidationEnvironment {
     readOnlyVariables.removeAll(futureReadOnlyVariables.peek());
   }
 
+  /** Throws EvalException if a load() appears after another kind of statement. */
+  private void checkLoadAfterStatement(List<Statement> statements) throws EvalException {
+    Location firstStatement = null;
+
+    for (Statement statement : statements) {
+      // Ignore string literals (e.g. docstrings).
+      if (statement instanceof ExpressionStatement
+          && ((ExpressionStatement) statement).getExpression() instanceof StringLiteral) {
+        continue;
+      }
+
+      if (statement instanceof LoadStatement) {
+        if (firstStatement == null) {
+          continue;
+        }
+        throw new EvalException(
+            statement.getLocation(),
+            "load() statements must be called before any other statement. "
+                + "First non-load() statement appears at "
+                + firstStatement
+                + ". Use --incompatible_bzl_disallow_load_after_statement to temporarily disable "
+                + "this check.");
+      }
+
+      if (firstStatement == null) {
+        firstStatement = statement.getLocation();
+      }
+    }
+  }
+
   /**
    * Validates the AST and runs static checks.
    */
   public void validateAst(List<Statement> statements) throws EvalException {
+    // Check that load() statements are on top.
+    if (semantics.incompatibleBzlDisallowLoadAfterStatement) {
+      checkLoadAfterStatement(statements);
+    }
+
     // Add every function in the environment before validating. This is
     // necessary because functions may call other functions defined
     // later in the file.
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
index 85a8f8c725..9b17d4b9e4 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
@@ -44,6 +44,22 @@ public class ValidationTest extends EvaluationTestCase {
   }
 
   @Test
+  public void testLoadAfterStatement() throws Exception {
+    env = newEnvironmentWithSkylarkOptions("--incompatible_bzl_disallow_load_after_statement=true");
+    checkError(
+        "load() statements must be called before any other statement",
+        "a = 5",
+        "load(':b.bzl', 'c')");
+  }
+
+  @Test
+  public void testAllowLoadAfterStatement() throws Exception {
+    env =
+        newEnvironmentWithSkylarkOptions("--incompatible_bzl_disallow_load_after_statement=false");
+    parse("a = 5", "load(':b.bzl', 'c')");
+  }
+
+  @Test
   public void testTwoFunctionsWithTheSameName() throws Exception {
     checkError(
         "Variable foo is read only", "def foo():", "  return 1", "def foo(x, y):", "  return 1");