From 9dc4e20048af36a61dda50e1ff1a2f36c101a4ea Mon Sep 17 00:00:00 2001 From: laurentlb Date: Thu, 6 Sep 2018 09:34:13 -0700 Subject: [PATCH] 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 --- .../build/lib/syntax/Environment.java | 25 +++++++++++-------- .../devtools/build/lib/syntax/LValue.java | 25 +++---------------- .../build/lib/syntax/EnvironmentTest.java | 2 +- 3 files changed, 20 insertions(+), 32 deletions(-) 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 01d33fb2fa274d..a8158e3f32f0f2 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 @@ private static final class Continuation { /** 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. + * + *

TODO(laurentlb): Remove this when we use static name resolution. + */ @Nullable final LinkedHashSet knownGlobalVariables; Continuation( @@ -1034,18 +1038,17 @@ void removeLocalBinding(String varname) { * @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 @@ Object lookup(String varname) { * 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 eb3c6850638d20..223e192eedf6e6 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 @@ public void assign(Object value, Environment env, Location loc) 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 @@ private static void assign(Expression expr, Object value, Environment env, Locat } } - /** - * 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 @@ public void assignAugmented(Operator operator, Expression rhs, Environment env, 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 25d3d5ebc21f1e..1b53776bd2ecf3 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 @@ public void testBindToNullThrowsException() throws Exception { update("some_name", null); fail(); } catch (NullPointerException e) { - assertThat(e).hasMessage("update(value == null)"); + assertThat(e).hasMessage("trying to assign null to 'some_name'"); } }