Cleanups for call stacks.

* Implement equality in `StarlarkThread.CallStackEntry` to improve test assertions.
* Use a static factory method for creating `StarlarkThread.CallStackEntry` instances.
* Use a constant for the `<toplevel>` Starlark function name.

PiperOrigin-RevId: 522149898
Change-Id: Idb017e3ea2fd520972eb46e560628565d454a600

src/main/java/com/google/devtools/build/lib/analysis/starlark/BazelBuildApiGlobals.java

@@ -52,7 +52,7 @@ public void visibility(Object value, StarlarkThread thread) throws EvalException
     // for static tooling to mechanically find and modify visibility() declarations.)
     ImmutableList<StarlarkThread.CallStackEntry> callStack = thread.getCallStack();
     if (!(callStack.size() == 2
-        && callStack.get(0).name.equals("<toplevel>")
+        && callStack.get(0).name.equals(StarlarkThread.TOP_LEVEL)
         && callStack.get(1).name.equals("visibility"))) {
       throw Starlark.errorf(
           "load visibility may only be set at the top level, not inside a function");

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java

@@ -34,6 +34,7 @@
 import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Starlark;
 import net.starlark.java.eval.StarlarkSemantics;
+import net.starlark.java.eval.StarlarkThread;
 import net.starlark.java.eval.StarlarkThread.CallStackEntry;
 import net.starlark.java.syntax.Location;
 
@@ -71,7 +72,7 @@ public static Rule createRule(
     BuildLangTypedAttributeValuesMap attributeValues =
         new BuildLangTypedAttributeValuesMap(attributes);
     ImmutableList<CallStackEntry> callStack =
-        ImmutableList.of(new CallStackEntry(callStackEntry, Location.BUILTIN));
+        ImmutableList.of(StarlarkThread.callStackEntry(callStackEntry, Location.BUILTIN));
     Rule rule;
     try {
       rule =

src/main/java/com/google/devtools/build/lib/packages/CallStack.java

@@ -109,7 +109,7 @@ private Location toLocation() {
     }
 
     private StarlarkThread.CallStackEntry toCallStackEntry() {
-      return new StarlarkThread.CallStackEntry(name, toLocation());
+      return StarlarkThread.callStackEntry(name, toLocation());
     }
 
     @Nullable

src/main/java/net/starlark/java/eval/CpuProfiler.java

@@ -405,7 +405,7 @@ private long getFunctionID(StarlarkCallable fn, long addr) throws IOException {
         Location loc = fn.getLocation();
         String filename = loc.file(); // TODO(adonovan): make WORKSPACE-relative
         String name = fn.getName();
-        if (name.equals("<toplevel>")) {
+        if (name.equals(StarlarkThread.TOP_LEVEL)) {
           name = filename;
         }
 

src/main/java/net/starlark/java/eval/StarlarkThread.java

@@ -193,7 +193,7 @@ public ImmutableMap<String, Object> getLocals() {
           }
         }
       }
-      return env.build();
+      return env.buildOrThrow();
     }
 
     @Override
@@ -430,7 +430,7 @@ public StarlarkSemantics getSemantics() {
   }
 
   /** Reports whether this thread is allowed to make recursive calls. */
-  public boolean isRecursionAllowed() {
+  boolean isRecursionAllowed() {
     return allowRecursion;
   }
 
@@ -455,16 +455,22 @@ StarlarkFunction getInnermostEnclosingStarlarkFunction(int depth) {
     return null;
   }
 
-  @Nullable
-  StarlarkFunction getInnermostEnclosingStarlarkFunction() {
-    return getInnermostEnclosingStarlarkFunction(0);
-  }
-
   /** Returns the size of the callstack. This is needed for the debugger. */
   int getCallStackSize() {
     return callstack.size();
   }
 
+  /**
+   * The value of {@link CallStackEntry#name} for the implicit function that executes the top-level
+   * statements of a file.
+   */
+  public static final String TOP_LEVEL = "<toplevel>";
+
+  /** Creates a new {@link CallStackEntry}. */
+  public static CallStackEntry callStackEntry(String name, Location location) {
+    return new CallStackEntry(name, location);
+  }
+
   /**
    * A CallStackEntry describes the name and PC location of an active function call. See {@link
    * #getCallStack}.
@@ -474,15 +480,32 @@ public static final class CallStackEntry {
     public final String name;
     public final Location location;
 
-    public CallStackEntry(String name, Location location) {
-      this.location = location;
-      this.name = name;
+    private CallStackEntry(String name, Location location) {
+      this.name = Preconditions.checkNotNull(name);
+      this.location = Preconditions.checkNotNull(location);
     }
 
     @Override
     public String toString() {
       return name + "@" + location;
     }
+
+    @Override
+    public int hashCode() {
+      return 31 * name.hashCode() + location.hashCode();
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      }
+      if (!(o instanceof CallStackEntry)) {
+        return false;
+      }
+      CallStackEntry that = (CallStackEntry) o;
+      return name.equals(that.name) && location.equals(that.location);
+    }
   }
 
   /**
@@ -495,7 +518,7 @@ public ImmutableList<CallStackEntry> getCallStack() {
     ImmutableList.Builder<CallStackEntry> stack =
         ImmutableList.builderWithExpectedSize(callstack.size());
     for (Frame fr : callstack) {
-      stack.add(new CallStackEntry(fr.fn.getName(), fr.loc));
+      stack.add(callStackEntry(fr.fn.getName(), fr.loc));
     }
     return stack.build();
   }

src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java

@@ -120,12 +120,10 @@ private static Object exec(String... lines) {
 
   private static final ImmutableList<StarlarkThread.CallStackEntry> DUMMY_STACK =
       ImmutableList.of(
-          new StarlarkThread.CallStackEntry( //
-              "<toplevel>", Location.fromFileLineColumn("BUILD", 10, 1)),
-          new StarlarkThread.CallStackEntry( //
-              "foo", Location.fromFileLineColumn("foo.bzl", 42, 1)),
-          new StarlarkThread.CallStackEntry( //
-              "myrule", Location.fromFileLineColumn("bar.bzl", 30, 6)));
+          StarlarkThread.callStackEntry(
+              StarlarkThread.TOP_LEVEL, Location.fromFileLineColumn("BUILD", 10, 1)),
+          StarlarkThread.callStackEntry("foo", Location.fromFileLineColumn("foo.bzl", 42, 1)),
+          StarlarkThread.callStackEntry("myrule", Location.fromFileLineColumn("bar.bzl", 30, 6)));
 
   private void setUpContextForRule(
       Map<String, Object> kwargs,

src/test/java/com/google/devtools/build/lib/packages/CallStackTest.java

@@ -17,7 +17,6 @@
 import static com.google.common.truth.Truth.assertThat;
 
 import com.google.common.collect.ImmutableList;
-import com.google.common.truth.Correspondence;
 import com.google.protobuf.CodedInputStream;
 import com.google.protobuf.CodedOutputStream;
 import java.io.ByteArrayOutputStream;
@@ -33,15 +32,6 @@
 @RunWith(JUnit4.class)
 public class CallStackTest {
 
-  /**
-   * Compare {@link StarlarkThread.CallStackEntry} using string equality since (1) it doesn't
-   * currently implement equals and (2) it should have a faithful string representation anyway.
-   */
-  private static final Correspondence<StarlarkThread.CallStackEntry, StarlarkThread.CallStackEntry>
-      STACK_ENTRY_CORRESPONDENCE =
-          Correspondence.from(
-              (l, r) -> l.toString().equals(r.toString()), "String-representations equal");
-
   @Test
   public void testCreateFromEmptyCallStack() {
     CallStack result = CallStack.createFrom(ImmutableList.of());
@@ -151,10 +141,7 @@ public void testSerialization() throws Exception {
   /** Asserts the provided {@link CallStack} faithfully represents the expected stack. */
   private static void assertCallStackContents(CallStack result, List<CallStackEntry> expected) {
     assertThat(result.size()).isEqualTo(expected.size());
-    assertThat(result.toList())
-        .comparingElementsUsing(STACK_ENTRY_CORRESPONDENCE)
-        .containsExactlyElementsIn(expected)
-        .inOrder();
+    assertThat(result.toList()).isEqualTo(expected);
     // toList and getFrame use different code paths, make sure they agree.
     for (int i = 0; i < expected.size(); i++) {
       assertThat(result.getFrame(i).toString()).isEqualTo(expected.get(i).toString());
@@ -163,6 +150,6 @@ private static void assertCallStackContents(CallStack result, List<CallStackEntr
 
   private static StarlarkThread.CallStackEntry entryFromNameAndLocation(
       String name, String file, int line, int col) {
-    return new CallStackEntry(name, Location.fromFileLineColumn(file, line, col));
+    return StarlarkThread.callStackEntry(name, Location.fromFileLineColumn(file, line, col));
   }
 }

src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java

@@ -92,10 +92,10 @@ public final class RuleClassTest extends PackageLoadingTestCase {
 
   private static final ImmutableList<StarlarkThread.CallStackEntry> DUMMY_STACK =
       ImmutableList.of(
-          new StarlarkThread.CallStackEntry(
-              "<toplevel>", Location.fromFileLineColumn("BUILD", 10, 1)),
-          new StarlarkThread.CallStackEntry("bar", Location.fromFileLineColumn("bar.bzl", 42, 1)),
-          new StarlarkThread.CallStackEntry("rule", Location.BUILTIN));
+          StarlarkThread.callStackEntry(
+              StarlarkThread.TOP_LEVEL, Location.fromFileLineColumn("BUILD", 10, 1)),
+          StarlarkThread.callStackEntry("bar", Location.fromFileLineColumn("bar.bzl", 42, 1)),
+          StarlarkThread.callStackEntry("rule", Location.BUILTIN));
 
   private static final class DummyFragment extends Fragment {}
 

src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java

@@ -51,11 +51,10 @@ public final class RuleFactoryTest extends PackageLoadingTestCase {
 
   private static final ImmutableList<StarlarkThread.CallStackEntry> DUMMY_STACK =
       ImmutableList.of(
-          new StarlarkThread.CallStackEntry(
-              "<toplevel>", Location.fromFileLineColumn("BUILD", 42, 1)),
-          new StarlarkThread.CallStackEntry("foo", Location.fromFileLineColumn("foo.bzl", 10, 1)),
-          new StarlarkThread.CallStackEntry(
-              "myrule", Location.fromFileLineColumn("bar.bzl", 30, 6)));
+          StarlarkThread.callStackEntry(
+              StarlarkThread.TOP_LEVEL, Location.fromFileLineColumn("BUILD", 42, 1)),
+          StarlarkThread.callStackEntry("foo", Location.fromFileLineColumn("foo.bzl", 10, 1)),
+          StarlarkThread.callStackEntry("myrule", Location.fromFileLineColumn("bar.bzl", 30, 6)));
 
   private Package.Builder newBuilder(PackageIdentifier id, Path filename) {
     return packageFactory

src/test/java/com/google/devtools/build/lib/starlarkdebug/server/StarlarkDebugServerTest.java

@@ -331,7 +331,7 @@ public void testPauseAtInvalidConditionBreakpointWithError() throws Exception {
             .setPauseReason(PauseReason.CONDITIONAL_BREAKPOINT_ERROR)
             .setLocation(location.toBuilder().setColumnNumber(1))
             .setConditionalBreakpointError(
-                StarlarkDebuggingProtos.Error.newBuilder().setMessage("name \'z\' is not defined"))
+                StarlarkDebuggingProtos.Error.newBuilder().setMessage("name 'z' is not defined"))
             .build();
 
     assertThat(event).isEqualTo(DebugEventHelper.threadPausedEvent(expectedThreadState));
@@ -370,7 +370,7 @@ public void testSimpleListFramesRequest() throws Exception {
     assertFramesEqualIgnoringValueIdentifiers(
         frames.getFrame(0),
         Frame.newBuilder()
-            .setFunctionName("<toplevel>")
+            .setFunctionName(StarlarkThread.TOP_LEVEL)
             .setLocation(breakpoint.toBuilder().setColumnNumber(1))
             .addScope(
                 Scope.newBuilder()
@@ -463,7 +463,7 @@ public void testListFramesShadowedBinding() throws Exception {
     assertFramesEqualIgnoringValueIdentifiers(
         frames.getFrame(1),
         Frame.newBuilder()
-            .setFunctionName("<toplevel>")
+            .setFunctionName(StarlarkThread.TOP_LEVEL)
             .setLocation(
                 Location.newBuilder()
                     .setPath("/a/build/file/test.bzl")
@@ -802,7 +802,7 @@ private static Thread execInWorkerThread(ParserInput input, @Nullable Module mod
   /**
    * Asserts that the given frames are equal after clearing the identifier from all {@link Value}s.
    */
-  private void assertFramesEqualIgnoringValueIdentifiers(Frame frame1, Frame frame2) {
+  private static void assertFramesEqualIgnoringValueIdentifiers(Frame frame1, Frame frame2) {
     assertThat(clearIds(frame1)).isEqualTo(clearIds(frame2));
   }
 
@@ -822,7 +822,7 @@ private static Scope clearIds(Scope scope) {
     return builder.build();
   }
 
-  private void assertValuesEqualIgnoringId(Value value1, Value value2) {
+  private static void assertValuesEqualIgnoringId(Value value1, Value value2) {
     assertThat(clearId(value1)).isEqualTo(clearId(value2));
   }
 

src/test/java/net/starlark/java/eval/StarlarkThreadDebuggingTest.java

@@ -50,7 +50,7 @@ private static StarlarkFunction defineFunc() throws Exception {
   }
 
   @Test
-  public void testListFramesEmptyStack() throws Exception {
+  public void testListFramesEmptyStack() {
     StarlarkThread thread = newThread();
     assertThat(Debug.getCallStack(thread)).isEmpty();
     assertThat(thread.getCallStack()).isEmpty();
@@ -222,7 +222,7 @@ public void testEvaluateVariableInScope() throws Exception {
   }
 
   @Test
-  public void testEvaluateVariableNotInScopeFails() throws Exception {
+  public void testEvaluateVariableNotInScopeFails() {
     Module module = Module.create();
 
     SyntaxError.Exception e =