libcmd/repl: print error for invalid :st argument

currently, the `:st <n>` command in the debugger will simply silently
fail if the argument cannot be converted to an integer, or if it falls
outside the range of valid stack indices. this isn't too big of problem,
but it can be nicer to tell the user something went wrong, rather than
not give them any output and having them guess (esp. in the second case).

this commit adds two errors, one for each case:

  1. the argument is not actually an integer, or is outside INT_MIN/MAX
     -> "argument '%arg' is not a valid integer"

  2. the argument is an integer outside the range of stack traces
     -> "stack index must be between 0 and %max_frame, but was %arg"

Change-Id: I8109feeede79a9ad3db9ee7dc95d37e7dd19741a

doc/manual/change-authors.yml

@@ -57,6 +57,10 @@ blitz:
   display_name: Julian Stecklina
   github: blitz
 
+blokyk:
+  display_name: blokyk
+  github: blokyk
+
 cole-h:
   display_name: Cole Helbling
   github: cole-h

doc/manual/rl-next/repl-st-errors.md

@@ -0,0 +1,14 @@
+---
+synopsis: "invalid arguments to :st now print an error"
+cls: [5386]
+category: "Improvements"
+credits: [blokyk]
+---
+
+When using the debugger, the `:st` command used to traverse the call stack would
+silently fail and put the debugger in an invalid state if the argument given to
+it wasn't a valid stack frame index.
+
+This change adds an error message warning the user if the given index wasn't a
+valid frame (telling them the range of valid indices), as well as if it wasn't
+even a valid integer to begin with.

lix/libcmd/repl.cc

@@ -712,14 +712,21 @@ void NixRepl::initDebugBuiltinCommands()
     addCommand(
         "show-trace",
         [](NixRepl & repl, const std::string & arg) {
-            try {
-                repl.debugTraceIndex = stoi(arg);
-            } catch (...) {
+            int requestedTraceIdx = repl.debugTraceIndex;
+            if (arg.length() != 0) {
+                std::optional<int> maybeIdx = string2Int<int>(arg);
+                if (!maybeIdx) {
+                    throw Error("argument '%s' is not a valid integer", arg);
+                }
+                requestedTraceIdx = maybeIdx.value();
             }
 
+            size_t traceCount = 0;
             auto traces = repl.evaluator.debug->traces();
             for (const auto & [idx, i] : enumerate(traces)) {
-                if (idx == repl.debugTraceIndex) {
+                traceCount++;
+                if (idx == (size_t) requestedTraceIdx) {
+                    repl.debugTraceIndex = requestedTraceIdx;
                     std::cout << "\n" << ANSI_BLUE << idx << ANSI_NORMAL << ": ";
                     showDebugTrace(std::cout, repl.evaluator.positions, *i);
                     std::cout << std::endl;
@@ -728,6 +735,20 @@ void NixRepl::initDebugBuiltinCommands()
                     break;
                 }
             }
+
+            // if we didn't find any trace matching the user's request
+            if (repl.debugTraceIndex != (size_t) requestedTraceIdx) {
+                // note: if we get here, then the loop ran fully without matching anything,
+                // so `traceCount` is the total number of traces
+                throw Error(
+                    "stack index must be between %ld and %ld (inclusive), but was %ld",
+                    0,
+                    // decrease it by one to get the max *index*, since stacks are indexed by 0
+                    traceCount - 1,
+                    requestedTraceIdx
+                );
+            }
+
             return ProcessLineResult::PromptAgain;
         },
         {.aliases = {"st"},

tests/functional/repl_characterization/data/stacktrace_invalid_arg.test

@@ -0,0 +1,73 @@
+@args --debugger
+  nix-repl> throw "(forever?????????)"
+  error: (forever?????????)
+
+argument-less :st works fine
+  nix-repl> :st
+
+  0: error: (forever?????????)
+  «string»:1:1
+
+       1| throw "(forever?????????)"
+        | ^
+
+  Env level 0
+  static:
+
+  Env level 1
+  abort baseNameOf break builtins derivation derivationStrict dirOf false fetchGit fetchMercurial fetchTarball fetchTree fromTOML import isNull map null placeholder removeAttrs scopedImport throw toString true
+
+a non-numeric strings produces an error
+  nix-repl> :st chat
+  error: argument 'chat' is not a valid integer
+
+  nix-repl> :st bedroom community
+  error: argument 'bedroom community' is not a valid integer
+
+...even when they start with a digit
+  nix-repl> :st 6up
+  error: argument '6up' is not a valid integer
+
+...or when they're floats
+  nix-repl> :st 4.50
+  error: argument '4.50' is not a valid integer
+
+an integer outside the range produces an error
+  nix-repl> :st 23571113171923
+  error: argument '23571113171923' is not a valid integer
+
+argument-less :st is still at the same index after errors
+  nix-repl> :st 1
+
+  1: while calling a function
+  «string»:1:1
+
+       1| throw "(forever?????????)"
+        | ^
+
+  Env level 0
+  static:
+
+  Env level 1
+  abort baseNameOf break builtins derivation derivationStrict dirOf false fetchGit fetchMercurial fetchTarball fetchTree fromTOML import isNull map null placeholder removeAttrs scopedImport throw toString true
+
+  nix-repl> :st foo
+  error: argument 'foo' is not a valid integer
+
+  nix-repl> :st
+
+  1: while calling a function
+  «string»:1:1
+
+       1| throw "(forever?????????)"
+        | ^
+
+  Env level 0
+  static:
+
+  Env level 1
+  abort baseNameOf break builtins derivation derivationStrict dirOf false fetchGit fetchMercurial fetchTarball fetchTree fromTOML import isNull map null placeholder removeAttrs scopedImport throw toString true
+
+quit
+  nix-repl> :quit
+  error: (forever?????????)

tests/functional/repl_characterization/data/stacktrace_oob.test

@@ -0,0 +1,77 @@
+@args --debugger
+  nix-repl> let f = _: throw "x_x"; x = f 5; in x
+  error: x_x
+
+frames from 0 up to 4 work fine
+  nix-repl> :st 0
+
+  0: error: x_x
+  «string»:1:12
+
+       1| let f = _: throw "x_x"; x = f 5; in x
+        |            ^
+
+  Env level 0
+  static: _
+
+  Env level 1
+  static: f x
+
+  Env level 2
+  static:
+
+  Env level 3
+  abort baseNameOf break builtins derivation derivationStrict dirOf false fetchGit fetchMercurial fetchTarball fetchTree fromTOML import isNull map null placeholder removeAttrs scopedImport throw toString true
+
+  nix-repl> :st 4
+
+  4: while evaluating a 'let' expression
+  «string»:1:1
+
+       1| let f = _: throw "x_x"; x = f 5; in x
+        | ^
+
+  Env level 0
+  static: f x
+
+  Env level 1
+  static:
+
+  Env level 2
+  abort baseNameOf break builtins derivation derivationStrict dirOf false fetchGit fetchMercurial fetchTarball fetchTree fromTOML import isNull map null placeholder removeAttrs scopedImport throw toString true
+
+negative frames print an error
+  nix-repl> :st -1
+  error: stack index must be between 0 and 4, but was -1
+
+  nix-repl> :st -100
+  error: stack index must be between 0 and 4, but was -100
+
+positives frames out of bounds also print an error
+  nix-repl> :st 5
+  error: stack index must be between 0 and 4, but was 5
+
+  nix-repl> :st 100
+  error: stack index must be between 0 and 4, but was 100
+
+argument-less :st is still at the same after oob
+  nix-repl> :st
+
+  4: while evaluating a 'let' expression
+  «string»:1:1
+
+       1| let f = _: throw "x_x"; x = f 5; in x
+        | ^
+
+  Env level 0
+  static: f x
+
+  Env level 1
+  static:
+
+  Env level 2
+  abort baseNameOf break builtins derivation derivationStrict dirOf false fetchGit fetchMercurial fetchTarball fetchTree fromTOML import isNull map null placeholder removeAttrs scopedImport throw toString true
+
+quit
+  nix-repl> :quit
+  error: x_x

tests/functional/repl_characterization/repl_characterization.cc

@@ -197,5 +197,7 @@ REPL_TEST(idempotent);
 REPL_TEST(debug_frames);
 REPL_TEST(debug_ignore_try);
 REPL_TEST(debug_ignore_try_defaults);
+REPL_TEST(stacktrace_invalid_arg);
+REPL_TEST(stacktrace_oob);
 
 }; // namespace nix