Remove duplicate checks in Environment
There was some duplication between Environment and LValue.
The check in the DynamicFrame was removed. The dynamic frame should be completely separate from user values (set a value with setupDynamic, read it with dynamicLookup), they shouldn't conflict with each other.
#5827
RELNOTES: None.
PiperOrigin-RevId: 211820447
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 01d33fb..a8158e3 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
@@ -478,7 +478,11 @@
/** The global Frame of the caller. */
final GlobalFrame globalFrame;
- /** The set of known global variables of the caller. */
+ /**
+ * 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(
@@ -1034,18 +1038,17 @@
* @return this Environment, in fluid style
*/
public Environment update(String varname, Object value) throws EvalException {
- Preconditions.checkNotNull(value, "update(value == null)");
- // prevents clashes between static and dynamic variables.
- if (dynamicFrame.get(varname) != null) {
- throw new EvalException(
- null, String.format("Trying to update special read-only global variable '%s'", varname));
- }
+ Preconditions.checkNotNull(value, "trying to assign null to '%s'", varname);
if (isKnownGlobalVariable(varname)) {
throw new EvalException(
- null, String.format("Trying to update read-only global variable '%s'", varname));
+ null,
+ String.format(
+ "Variable '%s' is referenced before assignment. "
+ + "The variable is defined in the global scope.",
+ varname));
}
try {
- lexicalFrame.put(this, varname, Preconditions.checkNotNull(value));
+ lexicalFrame.put(this, varname, value);
} catch (MutabilityException e) {
// Note that since at this time we don't accept the global keyword, and don't have closures,
// end users should never be able to mutate a frozen Environment, and a MutabilityException
@@ -1148,7 +1151,9 @@
* the current function).
*/
boolean isKnownGlobalVariable(String varname) {
- return knownGlobalVariables != null && knownGlobalVariables.contains(varname);
+ return !semantics.incompatibleStaticNameResolution()
+ && knownGlobalVariables != null
+ && knownGlobalVariables.contains(varname);
}
public SkylarkSemantics getSemantics() {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
index eb3c685..223e192 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
@@ -14,7 +14,6 @@
package com.google.devtools.build.lib.syntax;
-import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.syntax.SkylarkList.MutableList;
@@ -73,7 +72,7 @@
private static void assign(Expression expr, Object value, Environment env, Location loc)
throws EvalException, InterruptedException {
if (expr instanceof Identifier) {
- assignIdentifier((Identifier) expr, value, env, loc);
+ assignIdentifier((Identifier) expr, value, env);
} else if (expr instanceof IndexExpression) {
Object object = ((IndexExpression) expr).getObject().eval(env);
Object key = ((IndexExpression) expr).getKey().eval(env);
@@ -87,25 +86,9 @@
}
}
- /**
- * Binds a variable to the given value in the environment.
- *
- * @throws EvalException if we're currently in a function's scope, and the identifier has
- * previously resolved to a global variable in the same function
- */
- private static void assignIdentifier(
- Identifier ident, Object value, Environment env, Location loc)
+ /** Binds a variable to the given value in the environment. */
+ private static void assignIdentifier(Identifier ident, Object value, Environment env)
throws EvalException {
- Preconditions.checkNotNull(value, "trying to assign null to %s", ident);
-
- if (env.isKnownGlobalVariable(ident.getName())) {
- throw new EvalException(
- loc,
- String.format(
- "Variable '%s' is referenced before assignment. "
- + "The variable is defined in the global scope.",
- ident.getName()));
- }
env.update(ident.getName(), value);
}
@@ -178,7 +161,7 @@
Object result =
BinaryOperatorExpression.evaluateAugmented(
operator, expr.eval(env), rhs.eval(env), env, loc);
- assignIdentifier((Identifier) expr, result, env, loc);
+ assignIdentifier((Identifier) expr, result, env);
} else if (expr instanceof IndexExpression) {
IndexExpression indexExpression = (IndexExpression) expr;
// The object and key should be evaluated only once, so we don't use expr.eval().
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 25d3d5e..1b53776 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
@@ -161,7 +161,7 @@
update("some_name", null);
fail();
} catch (NullPointerException e) {
- assertThat(e).hasMessage("update(value == null)");
+ assertThat(e).hasMessage("trying to assign null to 'some_name'");
}
}
