Code simplification in the environment, remove old incompatible flags
Remove --incompatible_static_name_resolution and --incompatible_package_name_is_a_function
#5827
#5637
RELNOTES: None.
PiperOrigin-RevId: 224140026
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index e800976..4f5afc9 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -1529,10 +1529,7 @@
return StructProvider.STRUCT.create(builder.build(), "no native function or rule '%s'");
}
- private void buildPkgEnv(
- Environment pkgEnv,
- PackageContext context,
- PackageIdentifier packageId) {
+ private void buildPkgEnv(Environment pkgEnv, PackageContext context) {
// TODO(bazel-team): remove the naked functions that are redundant with the nativeModule,
// or if not possible, at least make them straight copies from the native module variant.
// or better, use a common Environment.Frame for these common bindings
@@ -1569,10 +1566,6 @@
}
pkgEnv.setupDynamic(PKG_CONTEXT, context);
- if (!pkgEnv.getSemantics().incompatiblePackageNameIsAFunction()) {
- pkgEnv.setupDynamic(Runtime.PKG_NAME, packageId.getPackageFragment().getPathString());
- pkgEnv.setupDynamic(Runtime.REPOSITORY_NAME, packageId.getRepository().toString());
- }
}
/**
@@ -1647,7 +1640,7 @@
PackageContext context =
new PackageContext(
pkgBuilder, globber, eventHandler, ruleFactory.getAttributeContainerFactory());
- buildPkgEnv(pkgEnv, context, packageId);
+ buildPkgEnv(pkgEnv, context);
if (!validatePackageIdentifier(packageId, buildFileAST.getLocation(), eventHandler)) {
pkgBuilder.setContainsErrors();
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
index 653a99d..008f892 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
@@ -422,20 +422,6 @@
public boolean incompatibleNoTransitiveLoads;
@Option(
- name = "incompatible_package_name_is_a_function",
- defaultValue = "true",
- documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS,
- effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
- metadataTags = {
- OptionMetadataTag.INCOMPATIBLE_CHANGE,
- OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
- },
- help =
- "If set to true, the values PACKAGE_NAME and REPOSITORY_NAME are not available. "
- + "Use the package_name() or repository_name() functions instead.")
- public boolean incompatiblePackageNameIsAFunction;
-
- @Option(
name = "incompatible_range_type",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS,
@@ -491,21 +477,6 @@
public boolean incompatibleRemoveNativeMavenJar;
@Option(
- name = "incompatible_static_name_resolution",
- defaultValue = "true",
- documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS,
- effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
- metadataTags = {
- OptionMetadataTag.INCOMPATIBLE_CHANGE,
- OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
- },
- help =
- "If set to true, the interpreter follows the semantics related to name resolution, "
- + "scoping, and shadowing, as defined in "
- + "https://github.com/bazelbuild/proposals/blob/master/docs/2018-06-18-name-resolution.md")
- public boolean incompatibleStaticNameResolution;
-
- @Option(
name = "incompatible_strict_argument_ordering",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS,
@@ -576,12 +547,10 @@
.incompatibleNoSupportToolsInActionInputs(incompatibleNoSupportToolsInActionInputs)
.incompatibleNoTargetOutputGroup(incompatibleNoTargetOutputGroup)
.incompatibleNoTransitiveLoads(incompatibleNoTransitiveLoads)
- .incompatiblePackageNameIsAFunction(incompatiblePackageNameIsAFunction)
.incompatibleRangeType(incompatibleRangeType)
.incompatibleRemoveNativeGitRepository(incompatibleRemoveNativeGitRepository)
.incompatibleRemoveNativeHttpArchive(incompatibleRemoveNativeHttpArchive)
.incompatibleRemoveNativeMavenJar(incompatibleRemoveNativeMavenJar)
- .incompatibleStaticNameResolution(incompatibleStaticNameResolution)
.incompatibleStricArgumentOrdering(incompatibleStricArgumentOrdering)
.incompatibleStringIsNotIterable(incompatibleStringIsNotIterable)
.internalSkylarkFlagTestCanary(internalSkylarkFlagTestCanary)
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
index 81c4883..b8d20f5 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
@@ -541,26 +541,17 @@
/** The global Frame of the caller. */
final GlobalFrame globalFrame;
- /**
- * The set of known global variables of the caller.
- *
- * <p>TODO(laurentlb): Remove this when we use static name resolution.
- */
- @Nullable final LinkedHashSet<String> knownGlobalVariables;
-
Continuation(
@Nullable Continuation continuation,
BaseFunction function,
@Nullable FuncallExpression caller,
Frame lexicalFrame,
- GlobalFrame globalFrame,
- @Nullable LinkedHashSet<String> knownGlobalVariables) {
+ GlobalFrame globalFrame) {
this.continuation = continuation;
this.function = function;
this.caller = caller;
this.lexicalFrame = lexicalFrame;
this.globalFrame = globalFrame;
- this.knownGlobalVariables = knownGlobalVariables;
}
}
@@ -767,15 +758,6 @@
private final Map<String, Extension> importedExtensions;
/**
- * When in a lexical (Skylark) Frame, this set contains the variable names that are global, as
- * determined not by global declarations (not currently supported), but by previous lookups that
- * ended being global or dynamic. This is necessary because if in a function definition something
- * reads a global variable after which a local variable with the same name is assigned an
- * Exception needs to be thrown.
- */
- @Nullable private LinkedHashSet<String> knownGlobalVariables;
-
- /**
* When in a lexical (Skylark) frame, this lists the names of the functions in the call stack. We
* currently use it to artificially disable recursion.
*/
@@ -801,12 +783,9 @@
Frame lexical,
@Nullable FuncallExpression caller,
GlobalFrame globals) {
- continuation =
- new Continuation(
- continuation, function, caller, lexicalFrame, globalFrame, knownGlobalVariables);
+ continuation = new Continuation(continuation, function, caller, lexicalFrame, globalFrame);
lexicalFrame = lexical;
globalFrame = globals;
- knownGlobalVariables = new LinkedHashSet<>();
}
/** Exits a scope by restoring state from the current continuation */
@@ -814,7 +793,6 @@
Preconditions.checkNotNull(continuation);
lexicalFrame = continuation.lexicalFrame;
globalFrame = continuation.globalFrame;
- knownGlobalVariables = continuation.knownGlobalVariables;
continuation = continuation.continuation;
}
@@ -1087,14 +1065,6 @@
*/
public Environment update(String varname, Object value) throws EvalException {
Preconditions.checkNotNull(value, "trying to assign null to '%s'", varname);
- if (isKnownGlobalVariable(varname)) {
- throw new EvalException(
- null,
- String.format(
- "Variable '%s' is referenced before assignment. "
- + "The variable is defined in the global scope.",
- varname));
- }
try {
lexicalFrame.put(this, varname, value);
} catch (MutabilityException e) {
@@ -1158,14 +1128,7 @@
/** Returns the value of a variable defined in the Universe scope (builtins). */
public Object universeLookup(String varname) {
// TODO(laurentlb): look only at globalFrame.universe.
- Object result = globalFrame.get(varname);
-
- if (result == null) {
- // TODO(laurentlb): Remove once PACKAGE_NAME and REPOSITOYRY_NAME are removed (they are the
- // only two user-visible values that use the dynamicFrame).
- return dynamicLookup(varname);
- }
- return result;
+ return globalFrame.get(varname);
}
/** Returns the value of a variable defined with setupDynamic. */
@@ -1186,17 +1149,10 @@
return lexicalValue;
}
Object globalValue = globalFrame.get(varname);
- Object dynamicValue = dynamicFrame.get(varname);
- if (globalValue == null && dynamicValue == null) {
+ if (globalValue == null) {
return null;
}
- if (knownGlobalVariables != null) {
- knownGlobalVariables.add(varname);
- }
- if (globalValue != null) {
- return globalValue;
- }
- return dynamicValue;
+ return globalValue;
}
/**
@@ -1209,16 +1165,6 @@
return globalFrame.restrictedBindings;
}
- /**
- * Returns true if varname is a known global variable (i.e., it has been read in the context of
- * the current function).
- */
- boolean isKnownGlobalVariable(String varname) {
- return !semantics.incompatibleStaticNameResolution()
- && knownGlobalVariables != null
- && knownGlobalVariables.contains(varname);
- }
-
public SkylarkSemantics getSemantics() {
return semantics;
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
index dcd39b6..98e7d5b 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
@@ -81,7 +81,7 @@
@Override
Object doEval(Environment env) throws EvalException {
Object result;
- if (scope == null || !env.getSemantics().incompatibleStaticNameResolution()) {
+ if (scope == null) {
// Legacy behavior, to be removed.
result = env.lookup(name);
if (result == null) {
@@ -107,12 +107,9 @@
if (result == null) {
// Since Scope was set, we know that the variable is defined in the scope.
// However, the assignment was not yet executed.
- EvalException e = getSpecialException();
- throw e != null
- ? e
- : new EvalException(
- getLocation(),
- scope.getQualifier() + " variable '" + name + "' is referenced before assignment.");
+ throw new EvalException(
+ getLocation(),
+ scope.getQualifier() + " variable '" + name + "' is referenced before assignment.");
}
return result;
@@ -128,39 +125,11 @@
return Kind.IDENTIFIER;
}
- /** Exception to provide a better error message for using PACKAGE_NAME or REPOSITORY_NAME. */
- private EvalException getSpecialException() {
- if (name.equals("PACKAGE_NAME")) {
- return new EvalException(
- getLocation(),
- "The value 'PACKAGE_NAME' has been removed in favor of 'package_name()', "
- + "please use the latter ("
- + "https://docs.bazel.build/versions/master/skylark/lib/native.html#package_name). "
- + "You can temporarily allow the old name "
- + "by using --incompatible_package_name_is_a_function=false");
- }
- if (name.equals("REPOSITORY_NAME")) {
- return new EvalException(
- getLocation(),
- "The value 'REPOSITORY_NAME' has been removed in favor of 'repository_name()', "
- + "please use the latter ("
- + "https://docs.bazel.build/versions/master/skylark/lib/native.html#repository_name)."
- + " You can temporarily allow the old name "
- + "by using --incompatible_package_name_is_a_function=false");
- }
- return null;
- }
-
EvalException createInvalidIdentifierException(Set<String> symbols) {
if (name.equals("$error$")) {
return new EvalException(getLocation(), "contains syntax error(s)", true);
}
- EvalException e = getSpecialException();
- if (e != null) {
- return e;
- }
-
String suggestion = SpellChecker.didYouMean(name, symbols);
return new EvalException(getLocation(), "name '" + name + "' is not defined" + suggestion);
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
index ff92277..ac3fa3a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
@@ -168,8 +168,6 @@
public abstract boolean incompatibleNoTransitiveLoads();
- public abstract boolean incompatiblePackageNameIsAFunction();
-
public abstract boolean incompatibleRangeType();
public abstract boolean incompatibleRemoveNativeGitRepository();
@@ -178,8 +176,6 @@
public abstract boolean incompatibleRemoveNativeMavenJar();
- public abstract boolean incompatibleStaticNameResolution();
-
public abstract boolean incompatibleStricArgumentOrdering();
public abstract boolean incompatibleStringIsNotIterable();
@@ -229,12 +225,10 @@
.incompatibleNoSupportToolsInActionInputs(false)
.incompatibleNoTargetOutputGroup(false)
.incompatibleNoTransitiveLoads(false)
- .incompatiblePackageNameIsAFunction(true)
.incompatibleRangeType(true)
.incompatibleRemoveNativeGitRepository(true)
.incompatibleRemoveNativeHttpArchive(true)
.incompatibleRemoveNativeMavenJar(false)
- .incompatibleStaticNameResolution(true)
.incompatibleStricArgumentOrdering(false)
.incompatibleStringIsNotIterable(false)
.internalSkylarkFlagTestCanary(false)
@@ -301,8 +295,6 @@
public abstract Builder incompatibleNoTransitiveLoads(boolean value);
- public abstract Builder incompatiblePackageNameIsAFunction(boolean value);
-
public abstract Builder incompatibleRangeType(boolean value);
public abstract Builder incompatibleRemoveNativeGitRepository(boolean value);
@@ -311,8 +303,6 @@
public abstract Builder incompatibleRemoveNativeMavenJar(boolean value);
- public abstract Builder incompatibleStaticNameResolution(boolean value);
-
public abstract Builder incompatibleStricArgumentOrdering(boolean value);
public abstract Builder incompatibleStringIsNotIterable(boolean value);
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
index 4da0436..d99c5f8 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -239,34 +239,6 @@
}
@Test
- public void testPackageConstant() throws Exception {
- Path buildFile =
- scratch.file("/pina/BUILD", "cc_library(name=PACKAGE_NAME + '-colada')");
-
- Package pkg =
- packages.createPackage(
- "pina",
- RootedPath.toRootedPath(root, buildFile),
- "--incompatible_package_name_is_a_function=false");
- events.assertNoWarningsOrErrors();
- assertThat(pkg.containsErrors()).isFalse();
- assertThat(pkg.getRule("pina-colada")).isNotNull();
- assertThat(pkg.getRule("pina-colada").containsErrors()).isFalse();
- assertThat(Sets.newHashSet(pkg.getTargets(Rule.class)).size()).isSameAs(1);
- }
-
- @Test
- public void testPackageConstantIsForbidden() throws Exception {
- events.setFailFast(false);
- Path buildFile = scratch.file("/pina/BUILD", "cc_library(name=PACKAGE_NAME + '-colada')");
- packages.createPackage(
- "pina",
- RootedPath.toRootedPath(root, buildFile),
- "--incompatible_package_name_is_a_function=true");
- events.assertContainsError("The value 'PACKAGE_NAME' has been removed");
- }
-
- @Test
public void testPackageNameFunction() throws Exception {
Path buildFile = scratch.file("/pina/BUILD", "cc_library(name=package_name() + '-colada')");
@@ -279,36 +251,6 @@
}
@Test
- public void testPackageConstantInExternalRepository() throws Exception {
- Path buildFile =
- scratch.file(
- "/external/a/b/BUILD",
- "genrule(name='c', srcs=[], outs=['ao'], cmd=REPOSITORY_NAME + ' ' + PACKAGE_NAME)");
- Package pkg =
- packages.createPackage(
- PackageIdentifier.create("@a", PathFragment.create("b")),
- RootedPath.toRootedPath(root, buildFile),
- events.reporter(),
- "--incompatible_package_name_is_a_function=false");
- Rule c = pkg.getRule("c");
- assertThat(AggregatingAttributeMapper.of(c).get("cmd", Type.STRING)).isEqualTo("@a b");
- }
-
- @Test
- public void testPackageConstantInExternalRepositoryIsForbidden() throws Exception {
- events.setFailFast(false);
- Path buildFile =
- scratch.file(
- "/external/a/b/BUILD", "genrule(name='c', srcs=[], outs=['ao'], cmd=REPOSITORY_NAME)");
- packages.createPackage(
- PackageIdentifier.create("@a", PathFragment.create("b")),
- RootedPath.toRootedPath(root, buildFile),
- events.reporter(),
- "--incompatible_package_name_is_a_function=true");
- events.assertContainsError("The value 'REPOSITORY_NAME' has been removed");
- }
-
- @Test
public void testPackageFunctionInExternalRepository() throws Exception {
Path buildFile =
scratch.file(
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 fe41bf7..ac6fc39 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
@@ -149,12 +149,10 @@
"--incompatible_no_support_tools_in_action_inputs=" + rand.nextBoolean(),
"--incompatible_no_target_output_group=" + rand.nextBoolean(),
"--incompatible_no_transitive_loads=" + rand.nextBoolean(),
- "--incompatible_package_name_is_a_function=" + rand.nextBoolean(),
"--incompatible_range_type=" + rand.nextBoolean(),
"--incompatible_remove_native_git_repository=" + rand.nextBoolean(),
"--incompatible_remove_native_http_archive=" + rand.nextBoolean(),
"--incompatible_remove_native_maven_jar=" + rand.nextBoolean(),
- "--incompatible_static_name_resolution=" + rand.nextBoolean(),
"--incompatible_strict_argument_ordering=" + rand.nextBoolean(),
"--incompatible_string_is_not_iterable=" + rand.nextBoolean(),
"--internal_skylark_flag_test_canary=" + rand.nextBoolean());
@@ -196,12 +194,10 @@
.incompatibleNoSupportToolsInActionInputs(rand.nextBoolean())
.incompatibleNoTargetOutputGroup(rand.nextBoolean())
.incompatibleNoTransitiveLoads(rand.nextBoolean())
- .incompatiblePackageNameIsAFunction(rand.nextBoolean())
.incompatibleRangeType(rand.nextBoolean())
.incompatibleRemoveNativeGitRepository(rand.nextBoolean())
.incompatibleRemoveNativeHttpArchive(rand.nextBoolean())
.incompatibleRemoveNativeMavenJar(rand.nextBoolean())
- .incompatibleStaticNameResolution(rand.nextBoolean())
.incompatibleStricArgumentOrdering(rand.nextBoolean())
.incompatibleStringIsNotIterable(rand.nextBoolean())
.internalSkylarkFlagTestCanary(rand.nextBoolean())
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java
index bbad014..152480c 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java
@@ -201,9 +201,7 @@
@Test
public void testBuiltinsCanBeShadowed() throws Exception {
- Environment env =
- newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=true")
- .setup("special_var", 42);
+ Environment env = newEnvironmentWithSkylarkOptions().setup("special_var", 42);
BuildFileAST.eval(env, "special_var = 41");
assertThat(env.moduleLookup("special_var")).isEqualTo(41);
}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
index e2325ce..e7f3861 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -1807,7 +1807,7 @@
@Test
public void testShadowisNotInitialized() throws Exception {
- new SkylarkTest("--incompatible_static_name_resolution=true")
+ new SkylarkTest()
.testIfErrorContains(
/* error message */ "local variable 'gl' is referenced before assignment",
"gl = 5",
@@ -1818,22 +1818,8 @@
}
@Test
- public void testLegacyGlobalVariableNotShadowed() throws Exception {
- new SkylarkTest("--incompatible_static_name_resolution=false")
- .setUp(
- "gl = 5",
- "def foo():",
- " if False: gl = 2",
- // The legacy behavior is that the global variable is returned.
- // With --incompatible_static_name_resolution set to true, this becomes an error.
- " return gl",
- "res = foo()")
- .testLookup("res", 5);
- }
-
- @Test
public void testShadowBuiltin() throws Exception {
- new SkylarkTest("--incompatible_static_name_resolution=true")
+ new SkylarkTest()
.testIfErrorContains(
"global variable 'len' is referenced before assignment",
"x = len('abc')",
@@ -1842,13 +1828,6 @@
}
@Test
- public void testLegacyShadowBuiltin() throws Exception {
- new SkylarkTest("--incompatible_static_name_resolution=false")
- .setUp("x = len('abc')", "len = 2", "y = x + len")
- .testLookup("y", 5);
- }
-
- @Test
public void testFunctionCallRecursion() throws Exception {
new SkylarkTest().testIfErrorContains("Recursion was detected when calling 'f' from 'g'",
"def main():",
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 c8e1be5..ea58193 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
@@ -133,13 +133,13 @@
@Test
public void testGlobalDefinedBelow() throws Exception {
- env = newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=true");
+ env = newEnvironmentWithSkylarkOptions();
parse("def bar(): return x", "x = 5\n");
}
@Test
public void testLocalVariableDefinedBelow() throws Exception {
- env = newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=true");
+ env = newEnvironmentWithSkylarkOptions();
parse(
"def bar():",
" for i in range(5):",
