Automated rollback of commit f1013485d41efd8503f9d4f937e17d1b4bc91ed3.

*** Reason for rollback ***

Introduced a bug in skylark that caused intellij TAP test to fail. The bug was not caught by any skylark/blaze/bazel tests.

*** Original change description ***

Optimize GC churn of parameter bindings performed before each user defined function call.

RELNOTES: None
PiperOrigin-RevId: 188249713

2 files changed, 21 insertions, 51 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 fa6f7e8bc6..d4e6403312 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
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.syntax;
 
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -143,21 +142,6 @@ public final class Environment implements Freezable {
           ? ImmutableEmptyLexicalFrame.INSTANCE
           : new MutableLexicalFrame(mutability);
     }
-
-    static LexicalFrame createForUserDefinedFunctionCall(
-        ImmutableList<String> argumentNames, Object[] argumentValues, Environment env)
-        throws EvalException {
-      Preconditions.checkState(!env.mutability().isFrozen());
-      MutableLexicalFrame frame = new MutableLexicalFrame(
-          env.mutability(), /*initialCapacity=*/ argumentNames.size());
-      for (int i = 0; i < argumentNames.size(); i++) {
-        String argumentName = argumentNames.get(i);
-        Object argumentValue = argumentValues[i];
-        env.sanityCheckBindingUpdate(argumentName, argumentValue);
-        frame.putUnsafe(argumentName, argumentValue);
-      }
-      return frame;
-    }
   }
 
   private static final class ImmutableEmptyLexicalFrame implements LexicalFrame {
@@ -200,16 +184,10 @@ public final class Environment implements Freezable {
   private static final class MutableLexicalFrame implements LexicalFrame {
     private final Mutability mutability;
     /** Bindings are maintained in order of creation. */
-    private final LinkedHashMap<String, Object> bindings;
+    private final LinkedHashMap<String, Object> bindings = new LinkedHashMap<>();
 
-    private MutableLexicalFrame(Mutability mutability, int initialCapacity) {
+    public MutableLexicalFrame(Mutability mutability) {
       this.mutability = mutability;
-      this.bindings = new LinkedHashMap<>(initialCapacity);
-    }
-
-    private MutableLexicalFrame(Mutability mutability) {
-      this.mutability = mutability;
-      this.bindings = new LinkedHashMap<>();
     }
 
     @Override
@@ -226,10 +204,6 @@ public final class Environment implements Freezable {
     @Override
     public void put(Environment env, String varname, Object value) throws MutabilityException {
       Mutability.checkMutable(this, env.mutability());
-      putUnsafe(varname, value);
-    }
-
-    private void putUnsafe(String varname, Object value) {
       bindings.put(varname, value);
     }
 
@@ -976,19 +950,6 @@ public final class Environment implements Freezable {
     }
   }
 
-  private void sanityCheckBindingUpdate(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));
-    }
-    if (isKnownGlobalVariable(varname)) {
-      throw new EvalException(
-          null, String.format("Trying to update read-only global variable '%s'", varname));
-    }
-  }
-
   /**
    * Modifies a binding in the current Frame of this Environment, as would an
    * {@link AssignmentStatement}. Does not try to modify an inherited binding.
@@ -999,7 +960,16 @@ public final class Environment implements Freezable {
    * @return this Environment, in fluid style
    */
   public Environment update(String varname, Object value) throws EvalException {
-    sanityCheckBindingUpdate(varname, value);
+    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));
+    }
+    if (isKnownGlobalVariable(varname)) {
+      throw new EvalException(
+          null, String.format("Trying to update read-only global variable '%s'", varname));
+    }
     try {
       currentFrame().put(this, varname, Preconditions.checkNotNull(value));
     } catch (MutabilityException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
index 54fdab2989..b4fc95f5aa 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
@@ -65,15 +65,15 @@ public class UserDefinedFunction extends BaseFunction {
 
     Profiler.instance().startTask(ProfilerTask.SKYLARK_USER_FN, getName());
     try {
-      ImmutableList<String> argumentNames = signature.getSignature().getNames();
-      env.enterScope(
-          this,
-          LexicalFrame.createForUserDefinedFunctionCall(
-              argumentNames,
-              arguments,
-              env),
-          ast,
-          definitionGlobals);
+      env.enterScope(this, LexicalFrame.create(env.mutability()), ast, definitionGlobals);
+      ImmutableList<String> names = signature.getSignature().getNames();
+
+      // Registering the functions's arguments as variables in the local Environment
+      int i = 0;
+      for (String name : names) {
+        env.update(name, arguments[i++]);
+      }
+
       Eval eval = new Eval(env);
       try {
         for (Statement stmt : statements) {