Introduce flag --incompatible_static_name_resolution_in_build_files

Implements #8022

When the flag is enabled, BUILD files are statically checked. This can find errors (undefined symbols) in code path that is not executed.

RELNOTES: Flag `--incompatible_static_name_resolution_in_build_files` is added. See #8022
PiperOrigin-RevId: 243355609

src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java

@@ -484,6 +484,20 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
               + "will be available")
   public boolean incompatibleRemoveNativeMavenJar;
 
+  @Option(
+      name = "incompatible_static_name_resolution_in_build_files",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
+      metadataTags = {
+        OptionMetadataTag.INCOMPATIBLE_CHANGE,
+        OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+      },
+      help =
+          "If set to true, BUILD files use static name resolution (which can find errors in code "
+              + "that is not executed). See https://github.com/bazelbuild/bazel/issues/8022")
+  public boolean incompatibleStaticNameResolutionInBuildFiles;
+
   /** Used in an integration test to confirm that flags are visible to the interpreter. */
   @Option(
       name = "internal_skylark_flag_test_canary",
@@ -546,6 +560,7 @@ public StarlarkSemantics toSkylarkSemantics() {
         .incompatibleNoTransitiveLoads(incompatibleNoTransitiveLoads)
         .incompatibleRemapMainRepo(incompatibleRemapMainRepo)
         .incompatibleRemoveNativeMavenJar(incompatibleRemoveNativeMavenJar)
+        .incompatibleStaticNameResolutionInBuildFiles(incompatibleStaticNameResolutionInBuildFiles)
         .incompatibleStringJoinRequiresStrings(incompatibleStringJoinRequiresStrings)
         .internalSkylarkFlagTestCanary(internalSkylarkFlagTestCanary)
         .incompatibleDoNotSplitLinkingCmdline(incompatibleDoNotSplitLinkingCmdline)

src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java

@@ -176,6 +176,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) {
 
   public abstract boolean incompatibleStringJoinRequiresStrings();
 
+  public abstract boolean incompatibleStaticNameResolutionInBuildFiles();
+
   public abstract boolean internalSkylarkFlagTestCanary();
 
 
@@ -227,6 +229,7 @@ public static Builder builderWithDefaults() {
           .incompatibleNoTransitiveLoads(true)
           .incompatibleRemapMainRepo(false)
           .incompatibleRemoveNativeMavenJar(false)
+          .incompatibleStaticNameResolutionInBuildFiles(false)
           .incompatibleStringJoinRequiresStrings(false)
           .internalSkylarkFlagTestCanary(false)
           .incompatibleDoNotSplitLinkingCmdline(false)
@@ -301,6 +304,7 @@ public abstract static class Builder {
 
     public abstract Builder incompatibleStringJoinRequiresStrings(boolean value);
 
+    public abstract Builder incompatibleStaticNameResolutionInBuildFiles(boolean value);
 
     public abstract Builder internalSkylarkFlagTestCanary(boolean value);
 

src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java

@@ -88,11 +88,14 @@ private static class ValidationException extends RuntimeException {
   private final Environment env;
   private Block block;
   private int loopCount;
+  /** In BUILD files, we have a slightly different behavior for legacy reasons. */
+  private final boolean isBuildFile;
 
   /** Create a ValidationEnvironment for a given global Environment (containing builtins). */
-  ValidationEnvironment(Environment env) {
+  private ValidationEnvironment(Environment env, boolean isBuildFile) {
     Preconditions.checkArgument(env.isGlobal());
     this.env = env;
+    this.isBuildFile = isBuildFile;
     block = new Block(Scope.Universe, null);
     Set<String> builtinVariables = env.getVariableNames();
     block.variables.addAll(builtinVariables);
@@ -173,12 +176,17 @@ public void visit(Identifier node) {
       // If this is the case, output a more helpful error message than 'not found'.
       FlagGuardedValue result = env.getRestrictedBindings().get(node.getName());
       if (result != null) {
-        throw new ValidationException(result.getEvalExceptionFromAttemptingAccess(
-            node.getLocation(), env.getSemantics(), node.getName()));
+        throw new ValidationException(
+            result.getEvalExceptionFromAttemptingAccess(
+                node.getLocation(), env.getSemantics(), node.getName()));
       }
       throw new ValidationException(node.createInvalidIdentifierException(getAllSymbols()));
     }
-    node.setScope(b.scope);
+    // TODO(laurentlb): In BUILD files, calling setScope will throw an exception. This happens
+    // because some AST nodes are shared across multipe ASTs (due to the prelude file).
+    if (!isBuildFile) {
+      node.setScope(b.scope);
+    }
   }
 
   @Override
@@ -271,7 +279,8 @@ public void visit(AugmentedAssignmentStatement node) {
 
   /** Declare a variable and add it to the environment. */
   private void declare(String varname, Location location) {
-    if (block.scope == Scope.Module && block.variables.contains(varname)) {
+    // TODO(laurentlb): Forbid reassignment in BUILD files.
+    if (!isBuildFile && block.scope == Scope.Module && block.variables.contains(varname)) {
       // Symbols defined in the module scope cannot be reassigned.
       throw new ValidationException(
           location,
@@ -333,7 +342,7 @@ private static void checkLoadAfterStatement(List<Statement> statements) {
   /** Validates the AST and runs static checks. */
   private void validateAst(List<Statement> statements) {
     // Check that load() statements are on top.
-    if (env.getSemantics().incompatibleBzlDisallowLoadAfterStatement()) {
+    if (!isBuildFile && env.getSemantics().incompatibleBzlDisallowLoadAfterStatement()) {
       checkLoadAfterStatement(statements);
     }
 
@@ -350,7 +359,7 @@ private void validateAst(List<Statement> statements) {
 
   public static void validateAst(Environment env, List<Statement> statements) throws EvalException {
     try {
-      ValidationEnvironment venv = new ValidationEnvironment(env);
+      ValidationEnvironment venv = new ValidationEnvironment(env, false);
       venv.validateAst(statements);
       // Check that no closeBlock was forgotten.
       Preconditions.checkState(venv.block.parent == null);
@@ -392,6 +401,17 @@ public static boolean checkBuildSyntax(
       List<Statement> statements, final EventHandler eventHandler, Environment env) {
     // Wrap the boolean inside an array so that the inner class can modify it.
     final boolean[] success = new boolean[] {true};
+
+    if (env.getSemantics().incompatibleStaticNameResolutionInBuildFiles()) {
+      ValidationEnvironment venv = new ValidationEnvironment(env, true);
+      try {
+        venv.validateAst(statements);
+      } catch (ValidationException e) {
+        eventHandler.handle(Event.error(e.exception.getLocation(), e.exception.getMessage()));
+        return false;
+      }
+    }
+
     // TODO(laurentlb): Merge with the visitor above when possible (i.e. when BUILD files use it).
     SyntaxTreeVisitor checker =
         new SyntaxTreeVisitor() {

src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java

@@ -155,6 +155,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E
         "--incompatible_no_transitive_loads=" + rand.nextBoolean(),
         "--incompatible_remap_main_repo=" + rand.nextBoolean(),
         "--incompatible_remove_native_maven_jar=" + rand.nextBoolean(),
+        "--incompatible_static_name_resolution_in_build_files=" + rand.nextBoolean(),
         "--incompatible_string_join_requires_strings=" + rand.nextBoolean(),
         "--internal_skylark_flag_test_canary=" + rand.nextBoolean(),
         "--incompatible_do_not_split_linking_cmdline=" + rand.nextBoolean());
@@ -200,6 +201,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
         .incompatibleNoTransitiveLoads(rand.nextBoolean())
         .incompatibleRemapMainRepo(rand.nextBoolean())
         .incompatibleRemoveNativeMavenJar(rand.nextBoolean())
+        .incompatibleStaticNameResolutionInBuildFiles(rand.nextBoolean())
         .incompatibleStringJoinRequiresStrings(rand.nextBoolean())
         .internalSkylarkFlagTestCanary(rand.nextBoolean())
         .incompatibleDoNotSplitLinkingCmdline(rand.nextBoolean())

src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java

@@ -679,6 +679,16 @@ public void testArgBothPosKey() throws Exception {
             "'banana'.replace('a', 'o', 3, old='a')");
   }
 
+  @Test
+  public void testStaticNameResolution() throws Exception {
+    newTest("--incompatible_static_name_resolution_in_build_files=true")
+        .testIfErrorContains("name 'foo' is not defined", "[foo for x in []]");
+
+    // legacy
+    new BuildTest("--incompatible_static_name_resolution_in_build_files=false")
+        .testStatement("str([foo for x in []])", "[]");
+  }
+
   @Test
   public void testDefInBuild() throws Exception {
     new BuildTest()