Introduce flag --incompatible_static_name_resolution_in_build_files Implements https://github.com/bazelbuild/bazel/issues/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 https://github.com/bazelbuild/bazel/issues/8022 PiperOrigin-RevId: 243355609
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java index 7a1b53d..a590fdf 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
@@ -484,6 +484,20 @@ + "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 @@ .incompatibleNoTransitiveLoads(incompatibleNoTransitiveLoads) .incompatibleRemapMainRepo(incompatibleRemapMainRepo) .incompatibleRemoveNativeMavenJar(incompatibleRemoveNativeMavenJar) + .incompatibleStaticNameResolutionInBuildFiles(incompatibleStaticNameResolutionInBuildFiles) .incompatibleStringJoinRequiresStrings(incompatibleStringJoinRequiresStrings) .internalSkylarkFlagTestCanary(internalSkylarkFlagTestCanary) .incompatibleDoNotSplitLinkingCmdline(incompatibleDoNotSplitLinkingCmdline)
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java index d4cb286..ce47ee6 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
@@ -176,6 +176,8 @@ public abstract boolean incompatibleStringJoinRequiresStrings(); + public abstract boolean incompatibleStaticNameResolutionInBuildFiles(); + public abstract boolean internalSkylarkFlagTestCanary(); @@ -227,6 +229,7 @@ .incompatibleNoTransitiveLoads(true) .incompatibleRemapMainRepo(false) .incompatibleRemoveNativeMavenJar(false) + .incompatibleStaticNameResolutionInBuildFiles(false) .incompatibleStringJoinRequiresStrings(false) .internalSkylarkFlagTestCanary(false) .incompatibleDoNotSplitLinkingCmdline(false) @@ -301,6 +304,7 @@ public abstract Builder incompatibleStringJoinRequiresStrings(boolean value); + public abstract Builder incompatibleStaticNameResolutionInBuildFiles(boolean value); public abstract Builder internalSkylarkFlagTestCanary(boolean value);
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 e53fde0..590e9540 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
@@ -88,11 +88,14 @@ 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 @@ // 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 @@ /** 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 @@ /** 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 @@ 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 @@ 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() {
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java index 2b533f9..08aad7f 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
@@ -155,6 +155,7 @@ "--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 @@ .incompatibleNoTransitiveLoads(rand.nextBoolean()) .incompatibleRemapMainRepo(rand.nextBoolean()) .incompatibleRemoveNativeMavenJar(rand.nextBoolean()) + .incompatibleStaticNameResolutionInBuildFiles(rand.nextBoolean()) .incompatibleStringJoinRequiresStrings(rand.nextBoolean()) .internalSkylarkFlagTestCanary(rand.nextBoolean()) .incompatibleDoNotSplitLinkingCmdline(rand.nextBoolean())
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java index 78700ea..2d25e1a 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -680,6 +680,16 @@ } @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() .testIfErrorContains(