Add a new optional param to fail() to allow pretty-printing user errors without a stack trace.

This allows a new fail(stack_trace = False, 'Message') which will not include a
stack trace.  This allows an intentional failure that is user facing to be a bit
more user friendly since they're unlikely to want to see the stack trace.  A
flag --force_eval_stack_trace can be used to unconditionally include the stack
trace for debugging.

RELNOTES:
Added an optional parameter to allow eliding the stack trace when calling fail().
PiperOrigin-RevId: 834908566
Change-Id: Ia7fd1673ec90e068b1a23578bd1094c662b94312

Author: kkress

src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java

@@ -845,6 +845,17 @@ public Converter() {
               + " the BUILD file they are ultimately loaded from.")
   public boolean incompatibleResolveSelectKeysEagerly;
 
+  @Option(
+      name = "force_starlark_stack_trace",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+      help =
+          "If --force_starlark_stack_trace=true, Starlark stace traces will always be printed from"
+              + " calls to fail(), including those normally supressed with fail(..., stack_trace ="
+              + " False)")
+  public boolean forceStarlarkStackTrace;
+
   /**
    * An interner to reduce the number of StarlarkSemantics instances. A single Blaze instance should
    * never accumulate a large number of these and being able to shortcut on object identity makes a
@@ -956,7 +967,8 @@ private void setFlags(FlagConsumer consumer) {
             .setBool(
                 StarlarkSemantics.INTERNAL_BAZEL_ONLY_UTF_8_BYTE_STRINGS,
                 internalStarlarkUtf8ByteStrings)
-            .setBool(EXPERIMENTAL_REPOSITORY_CTX_EXECUTE_WASM, repositoryCtxExecuteWasm);
+            .setBool(EXPERIMENTAL_REPOSITORY_CTX_EXECUTE_WASM, repositoryCtxExecuteWasm)
+            .setBool(StarlarkSemantics.FORCE_STARLARK_STACK_TRACE, forceStarlarkStackTrace);
   }
 
   /** Constructs a {@link StarlarkSemantics} object corresponding to this set of option values. */
@@ -1130,6 +1142,7 @@ public FlagConsumer setBool(String key, boolean ignored) {
       "-experimental_repository_ctx_execute_wasm";
   public static final String INCOMPATIBLE_RESOLVE_SELECT_KEYS_EAGERLY =
       "-incompatible_resolve_select_keys_eagerly";
+
   // non-booleans
   public static final StarlarkSemantics.Key<List<String>> INCOMPATIBLE_DISABLE_TRANSITIONS_OPTIONS =
       new StarlarkSemantics.Key<>("incompatible_disable_transitions_on", ImmutableList.of());

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

@@ -37,25 +37,39 @@ public class EvalException extends Exception {
   // stack when popping a frame. Thus an exception newly created by a
   // built-in function has no stack until it is thrown out of a function call.
   @Nullable private ImmutableList<StarlarkThread.CallStackEntry> callstack;
+  private final boolean includeStackTrace;
 
   /** Constructs an EvalException. Use {@link Starlark#errorf} if you want string formatting. */
   public EvalException(String message) {
-    this(message, /* cause= */ (Throwable) null);
+    this(message, /* cause= */ (Throwable) null, /* includeStackTrace= */ true);
   }
 
   /**
-   * Constructs an EvalException with a message and optional cause.
+   * Constructs an EvalException with a message and optional cause and bool indicating if the error
+   * should contain a stack trace.
    *
    * <p>The cause does not affect the error message, so callers should incorporate {@code
    * cause.getMessage()} into {@code message} if desired, or call {@code EvalException(Throwable)}.
    */
-  public EvalException(String message, @Nullable Throwable cause) {
+  public EvalException(String message, @Nullable Throwable cause, boolean includeStackTrace) {
     super(checkNotNull(message), cause);
+    this.includeStackTrace = includeStackTrace;
+  }
+
+  /**
+   * Constructs an EvalException with a message and optional cause and defaulting stack trace to
+   * true.
+   *
+   * <p>The cause does not affect the error message, so callers should incorporate {@code
+   * cause.getMessage()} into {@code message} if desired, or call {@code EvalException(Throwable)}.
+   */
+  public EvalException(String message, @Nullable Throwable cause) {
+    this(checkNotNull(message), cause, /* includeStackTrace= */ true);
   }
 
   /** Constructs an EvalException using the same message as the cause exception. */
   public EvalException(Throwable cause) {
-    super(getCauseMessage(cause), cause);
+    this(getCauseMessage(cause), cause, /* includeStackTrace= */ true);
   }
 
   /**
@@ -135,7 +149,7 @@ public final String getMessageWithStack() {
    * source line for each stack frame is obtained from the provided SourceReader.
    */
   public final String getMessageWithStack(SourceReader src) {
-    if (callstack != null) {
+    if (includeStackTrace && callstack != null) {
       return formatCallStack(callstack, getMessage(), src);
     }
     return getMessage();

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

@@ -869,7 +869,13 @@ public StarlarkList<String> dir(Object object, StarlarkThread thread) throws Eva
             defaultValue = "\" \"",
             named = true,
             positional = false,
-            doc = "The separator string between the objects, default is space (\" \").")
+            doc = "The separator string between the objects, default is space (\" \")."),
+        @Param(
+            name = "stack_trace",
+            doc = "If False stack trace is elided from failure for friendlier user messages",
+            defaultValue = "True",
+            positional = false,
+            named = true),
       },
       extraPositionals =
           @Param(
@@ -879,8 +885,11 @@ public StarlarkList<String> dir(Object object, StarlarkThread thread) throws Eva
                       + " default) and joined with sep (defaults to \" \"), that appear in the"
                       + " error message."),
       useStarlarkThread = true)
-  public void fail(Object msg, Object attr, String sep, Tuple args, StarlarkThread thread)
+  public void fail(
+      Object msg, Object attr, String sep, Boolean stackTrace, Tuple args, StarlarkThread thread)
       throws EvalException {
+    boolean includeStack =
+        stackTrace || thread.getSemantics().getBool(StarlarkSemantics.FORCE_STARLARK_STACK_TRACE);
     Printer printer = new Printer();
     boolean needSeparator = false;
     if (attr != Starlark.NONE) {
@@ -902,7 +911,7 @@ public void fail(Object msg, Object attr, String sep, Tuple args, StarlarkThread
       printer.debugPrint(arg, thread);
       needSeparator = true;
     }
-    throw Starlark.errorf("%s", printer.toString());
+    throw new EvalException(printer.toString(), null, includeStack);
   }
 
   @StarlarkMethod(

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

@@ -266,4 +266,7 @@ public final String toString() {
   /** Whether the Starlark interpreter uses UTF-8 byte strings instead of UTF-16 strings. */
   public static final String INTERNAL_BAZEL_ONLY_UTF_8_BYTE_STRINGS =
       "-internal_bazel_only_utf_8_byte_strings";
+
+  /** Globally Override fail(stack_trace=) to true. Flag default is false. */
+  public static final String FORCE_STARLARK_STACK_TRACE = "-force_starlark_stack_trace";
 }

src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java

@@ -139,6 +139,7 @@ private static BuildLanguageOptions buildRandomOptions(Random rand) throws Excep
         "--experimental_cc_shared_library=" + rand.nextBoolean(),
         "--experimental_repo_remote_exec=" + rand.nextBoolean(),
         "--experimental_dormant_deps=" + rand.nextBoolean(),
+        "--force_starlark_stack_trace=" + rand.nextBoolean(),
         "--incompatible_always_check_depset_elements=" + rand.nextBoolean(),
         "--incompatible_disallow_empty_glob=" + rand.nextBoolean(),
         "--incompatible_do_not_split_linking_cmdline=" + rand.nextBoolean(),
@@ -185,6 +186,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
         .setBool(BuildLanguageOptions.EXPERIMENTAL_CC_SHARED_LIBRARY, rand.nextBoolean())
         .setBool(BuildLanguageOptions.EXPERIMENTAL_REPO_REMOTE_EXEC, rand.nextBoolean())
         .setBool(BuildLanguageOptions.EXPERIMENTAL_DORMANT_DEPS, rand.nextBoolean())
+        .setBool(StarlarkSemantics.FORCE_STARLARK_STACK_TRACE, rand.nextBoolean())
         .setBool(BuildLanguageOptions.INCOMPATIBLE_ALWAYS_CHECK_DEPSET_ELEMENTS, rand.nextBoolean())
         .setBool(BuildLanguageOptions.INCOMPATIBLE_DISALLOW_EMPTY_GLOB, rand.nextBoolean())
         .setBool(BuildLanguageOptions.INCOMPATIBLE_DO_NOT_SPLIT_LINKING_CMDLINE, rand.nextBoolean())

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

@@ -42,7 +42,7 @@ class EvaluationTestCase {
    * Updates the semantics used to filter predeclared bindings, and carried by subsequently created
    * threads. Causes a new StarlarkThread and Module to be created when next needed.
    */
-  private final void setSemantics(StarlarkSemantics semantics) {
+  public final void setSemantics(StarlarkSemantics semantics) {
     this.semantics = semantics;
 
     // Re-initialize the thread and module with the new semantics when needed.

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

@@ -708,8 +708,8 @@ ev.new Scenario()
    * string and chars argument. If chars is null no argument is passed.
    */
   private void checkStrip(
-      String input, Object chars,
-      String expLeft, String expRight, String expBoth) throws Exception {
+      String input, Object chars, String expLeft, String expRight, String expBoth)
+      throws Exception {
     if (chars == null) {
       ev.new Scenario()
           .update("s", input)
@@ -739,9 +739,7 @@ public void testStrip() throws Exception {
     checkStrip(" a b c ", Starlark.NONE, "a b c ", " a b c", "a b c");
     // Default whitespace with full range of Latin-1 whitespace chars.
     String whitespace = "\u0009\n\u000B\u000C\r\u001C\u001D\u001E\u001F\u0020\u0085\u00A0";
-    checkStrip(
-        whitespace + "a" + whitespace, null,
-        "a" + whitespace, whitespace + "a", "a");
+    checkStrip(whitespace + "a" + whitespace, null, "a" + whitespace, whitespace + "a", "a");
     checkStrip(
         whitespace + "a" + whitespace, Starlark.NONE, "a" + whitespace, whitespace + "a", "a");
     // Empty cases.
@@ -762,6 +760,42 @@ ev.new Scenario()
         .testIfErrorContains("0 1 2 3", "fail(1, 2, 3, msg=0)"); // deprecated
   }
 
+  @Test
+  public void testFail_stackTrace() throws Exception {
+    final StarlarkSemantics withoutForce =
+        StarlarkSemantics.DEFAULT.toBuilder()
+            .setBool(StarlarkSemantics.FORCE_STARLARK_STACK_TRACE, false)
+            .build();
+
+    final StarlarkSemantics withForce =
+        StarlarkSemantics.DEFAULT.toBuilder()
+            .setBool(StarlarkSemantics.FORCE_STARLARK_STACK_TRACE, true)
+            .build();
+
+    EvaluationTestCase customSemanticsEv = new EvaluationTestCase();
+
+    try {
+      customSemanticsEv.setSemantics(withoutForce);
+      customSemanticsEv.eval("fail('with_a_trace')");
+    } catch (EvalException e) {
+      assertThat(e.getMessageWithStack()).contains("Traceback (most recent call last)");
+    }
+
+    try {
+      customSemanticsEv.setSemantics(withoutForce);
+      customSemanticsEv.eval("fail('without_a_trace', stack_trace=False)");
+    } catch (EvalException e) {
+      assertThat(e.getMessageWithStack()).doesNotContain("Traceback (most recent call last)");
+    }
+
+    try {
+      customSemanticsEv.setSemantics(withForce);
+      customSemanticsEv.eval("fail('without_a_trace', stack_trace=False)");
+    } catch (EvalException e) {
+      assertThat(e.getMessageWithStack()).contains("Traceback (most recent call last)");
+    }
+  }
+
   @Test
   public void testTupleCoercion() throws Exception {
     ev.new Scenario()