Using aliases in load() may no longer lead to false positives in SkylarkEnvironment's recursion detection.

--
MOS_MIGRATED_REVID=101374341

3 files changed, 30 insertions, 11 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
index 3516bd2d7c..816110097b 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
@@ -27,6 +27,7 @@ import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 
 import javax.annotation.Nullable;
 
@@ -552,4 +553,20 @@ public abstract class BaseFunction {
     builder.append(")");
     return builder.toString();
   }
+
+  @Override
+  public boolean equals(@Nullable Object other) {
+    if (other instanceof BaseFunction) {
+      BaseFunction that = (BaseFunction) other;
+      // In theory, the location alone unambiguously identifies a given function. However, in
+      // some test cases the location might not have a valid value, thus we also check the name.
+      return Objects.equals(this.name, that.name) && Objects.equals(this.location, that.location);
+    }
+    return false;
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(name, location);
+  }
 }
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 1c599a8b15..5722e7df61 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
@@ -356,9 +356,9 @@ public class Environment {
   }
 
   /**
-   * Return the current stack trace (list of function names).
+   * Return the current stack trace (list of functions).
    */
-  public ImmutableList<String> getStackTrace() {
+  public ImmutableList<BaseFunction> getStackTrace() {
     // Empty list, since this environment does not allow function definition
     // (see SkylarkEnvironment)
     return ImmutableList.of();
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkEnvironment.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkEnvironment.java
index 99362b6b96..f9e1e30eb0 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkEnvironment.java
@@ -44,7 +44,7 @@ public class SkylarkEnvironment extends Environment implements Serializable {
    */
   private final Set<String> readGlobalVariables = new HashSet<>();
 
-  private ImmutableList<String> stackTrace;
+  private ImmutableList<BaseFunction> stackTrace;
 
   @Nullable private String fileContentHashCode;
 
@@ -55,13 +55,15 @@ public class SkylarkEnvironment extends Environment implements Serializable {
   public static SkylarkEnvironment createEnvironmentForFunctionCalling(
       Environment callerEnv, SkylarkEnvironment definitionEnv,
       UserDefinedFunction function) throws EvalException {
-    if (callerEnv.getStackTrace().contains(function.getName())) {
-      throw new EvalException(function.getLocation(), "Recursion was detected when calling '"
-          + function.getName() + "' from '" + Iterables.getLast(callerEnv.getStackTrace()) + "'");
+    if (callerEnv.getStackTrace().contains(function)) {
+      throw new EvalException(
+          function.getLocation(),
+          "Recursion was detected when calling '" + function.getName() + "' from '"
+          + Iterables.getLast(callerEnv.getStackTrace()).getName() + "'");
     }
-    ImmutableList<String> stackTrace = new ImmutableList.Builder<String>()
+    ImmutableList<BaseFunction> stackTrace = new ImmutableList.Builder<BaseFunction>()
         .addAll(callerEnv.getStackTrace())
-        .add(function.getName())
+        .add(function)
         .build();
     SkylarkEnvironment childEnv =
         // Always use the caller Environment's EventHandler. We cannot assume that the
@@ -79,8 +81,8 @@ public class SkylarkEnvironment extends Environment implements Serializable {
     return childEnv;
   }
 
-  private SkylarkEnvironment(SkylarkEnvironment definitionEnv, ImmutableList<String> stackTrace,
-      EventHandler eventHandler) {
+  private SkylarkEnvironment(SkylarkEnvironment definitionEnv,
+      ImmutableList<BaseFunction> stackTrace, EventHandler eventHandler) {
     super(definitionEnv.getGlobalEnvironment());
     this.stackTrace = stackTrace;
     this.eventHandler = Preconditions.checkNotNull(eventHandler,
@@ -109,7 +111,7 @@ public class SkylarkEnvironment extends Environment implements Serializable {
   }
 
   @Override
-  public ImmutableList<String> getStackTrace() {
+  public ImmutableList<BaseFunction> getStackTrace() {
     return stackTrace;
   }