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(
