Fix value collision in builtins registry

If two values compared equal (e.g., MethodLibrary#bool and SkylarkAttr#bool), we were dropping one of them in favor of the other.

RELNOTES: None
PiperOrigin-RevId: 182057611

2 files changed, 25 insertions, 8 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java b/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java
index 839b3114cf..3decbebff5 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.syntax;
 
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
@@ -203,9 +204,9 @@ public final class Runtime {
       functions.get(namespace).put(function.getName(), function);
     }
 
-    /** Returns a set of all registered builtins, in a deterministic order. */
-    public ImmutableSet<Object> getBuiltins() {
-      return ImmutableSet.copyOf(allBuiltins.values());
+    /** Returns a list of all registered builtins, in a deterministic order. */
+    public ImmutableList<Object> getBuiltins() {
+      return ImmutableList.copyOf(allBuiltins.values());
     }
 
     @Nullable
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/RuntimeTest.java b/src/test/java/com/google/devtools/build/lib/syntax/RuntimeTest.java
index a559f34d1b..60df36ba70 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/RuntimeTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/RuntimeTest.java
@@ -19,7 +19,7 @@ import static com.google.common.truth.Truth.assertThat;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
 import java.lang.reflect.Field;
-import java.util.Set;
+import java.util.List;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -28,7 +28,6 @@ import org.junit.runners.JUnit4;
 @RunWith(JUnit4.class)
 public final class RuntimeTest {
 
-  private static final Object DUMMY = new Object();
   private static final BuiltinFunction DUMMY_FUNC = new BuiltinFunction("dummyFunc") {
     // This would normally be done by @SkylarkSignature annotation and configure(), but a simple
     // stub suffices.
@@ -47,9 +46,10 @@ public final class RuntimeTest {
 
   @Test
   public void checkRegistry_GetBuiltins() {
+    Object dummy = new Object();
     Runtime.BuiltinRegistry reg = new Runtime.BuiltinRegistry();
-    reg.registerBuiltin(DummyType.class, "dummy", DUMMY);
-    assertThat(reg.getBuiltins()).contains(DUMMY);
+    reg.registerBuiltin(DummyType.class, "dummy", dummy);
+    assertThat(reg.getBuiltins()).contains(dummy);
   }
 
   @Test
@@ -66,6 +66,22 @@ public final class RuntimeTest {
     assertThat(reg.getFunctionNames(DummyType.class)).contains("dummyFunc");
   }
 
+  /** Ensures that we still register all builtins, even when some are equal to one another. */
+  @Test
+  public void checkRegistry_EqualBuiltinsDontClash() {
+    // Create two distinct objects that compare equal under Object#equals. Use toCharArray() to
+    // not worry about whether the JVM does string interning.
+    String equalValue1 = "abc";
+    String equalValue2 = new String(equalValue1.toCharArray());
+    Runtime.BuiltinRegistry reg = new Runtime.BuiltinRegistry();
+    reg.registerBuiltin(DummyType.class, "eq1", equalValue1);
+    reg.registerBuiltin(DummyType.class, "eq2", equalValue2);
+    List<Object> values = reg.getBuiltins();
+    assertThat(values).hasSize(2);
+    assertThat(values.get(0)).isSameAs(equalValue1);
+    assertThat(values.get(1)).isSameAs(equalValue2);
+  }
+
   @Test
   public void checkStaticallyRegistered_Method() throws Exception {
     Field splitField = MethodLibrary.class.getDeclaredField("split");
@@ -80,7 +96,7 @@ public final class RuntimeTest {
     Field lenField = MethodLibrary.class.getDeclaredField("len");
     lenField.setAccessible(true);
     Object lenFieldValue = lenField.get(null);
-    Set<Object> builtins = Runtime.getBuiltinRegistry().getBuiltins();
+    List<Object> builtins = Runtime.getBuiltinRegistry().getBuiltins();
     assertThat(builtins).contains(lenFieldValue);
   }
 }