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

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.
+     *
+     * <p>TODO(laurentlb): Remove this when we use static name resolution.
+     */
     @Nullable final LinkedHashSet<String> 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() {

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().

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'");
     }
   }