libexpr/primops: make break force its argument

previously, `builtins.break` didn't force its argument, resulting in
a value wrapped with `break` being opaque to most builtins if not
also wrapped with `seq`. see [lix-project/lix#1165] for more details
on what this can break.

this tiny fix just adds a call to `forceValue` inside `prim_break`,
but unfortunately this "breaks" a few existing tests because it
changes the call stack; those tests' golden outputs have been adjusted
without modifying their intended purpose.

Fixes #1165

[lix-project/lix#1165]: https://git.lix.systems/lix-project/lix/issues/1165

Change-Id: I5fe4ee3ff28b38aaf924125b8978130812e58fef

doc/manual/rl-next/libexpr-fix-break.md

@@ -0,0 +1,14 @@
+---
+synopsis: "builtins.break doesn't break expression anymore"
+issues: [1165]
+cls: [5422]
+category: "Fixes"
+credits: [blokyk]
+---
+
+Wrapping an expression in `builtins.break` used to break some builtins like
+`map` and the `is*` functions, which could modify the execution path of code
+inadvertently, made debugging nix harder than it already is, and in some cases
+even crashed the interpreter. Now, using `break` should be completely
+transparent to whatever function receives it as an input, preventing the
+above-mentioned issues.

lix/libexpr/primops.cc

@@ -656,6 +656,7 @@ static void prim_break(EvalState & state, Value * * args, Value & v)
     }
 
     // Return the value we were passed.
+    state.forceValue(*args[0], noPos);
     v = *args[0];
 }
 

tests/functional/repl_characterization/data/regression_9917.test

@@ -35,6 +35,36 @@ This test ensures that continues don't skip opportunities to enter the debugger.
       |       ^
       3|     builtins.break { msg = "hello"; }
 
+  3: while calling a function
+  $TEST_DATA/regression_9917.nix:6:5
+
+      5|   b = builtins.trace "before outer break" (
+      6|     builtins.break a
+      |     ^
+      7|   );
+
+  4: while calling a function
+  $TEST_DATA/regression_9917.nix:5:7
+
+      4|   );
+      5|   b = builtins.trace "before outer break" (
+      |       ^
+      6|     builtins.break a
+
+  5: while evaluating a 'let' expression
+  $TEST_DATA/regression_9917.nix:1:1
+
+      1| let
+      | ^
+      2|   a = builtins.trace "before inner break" (
+
+  6: while evaluating the file '$TEST_DATA/regression_9917.nix':
+  $TEST_DATA/regression_9917.nix:1:1
+
+      1| let
+      | ^
+      2|   a = builtins.trace "before inner break" (
+
   nix-repl> :c
 
   nix-repl> msg

tests/functional/repl_characterization/data/stack_vars.test

@@ -87,18 +87,22 @@ If we :st past the frame in the backtrace with the meow in it, the meow should n
 
   nix-repl> :quit
   error:
-         … while calling the 'trace' builtin
+         … while evaluating the file '$TEST_DATA/stack_vars.nix':
-           at $TEST_DATA/stack_vars.nix:2:7:
-              1| let
-              2|   a = builtins.trace "before inner break" (
-               |       ^
-              3|     let meow' = 3; in builtins.break { msg = "hello"; }
 
-         … while calling the 'break' builtin
+         … while evaluating b
-           at $TEST_DATA/stack_vars.nix:3:23:
+           at $TEST_DATA/stack_vars.nix:9:3:
-              2|   a = builtins.trace "before inner break" (
+              8| in
-              3|     let meow' = 3; in builtins.break { msg = "hello"; }
+              9|   b
                |   ^
+             10|
+
+         … while calling the 'trace' builtin
+           at $TEST_DATA/stack_vars.nix:5:7:
               4|   );
+              5|   b = builtins.trace "before outer break" (
+               |       ^
+              6|     let meow = 2; in builtins.break a
+
+         (stack trace truncated; use '--show-trace' to show the full trace)
 
          error: breakpoint reached

tests/functional2/eval/test_debugger.py

@@ -27,3 +27,59 @@ def test_debugger_output(nix: Nix):
     assert "error: oh snap" in res.stderr_plain
     assert re.findall(r"with: .*a", res.stdout_plain)
     assert re.findall(r"static: .*x", res.stdout_plain)
+
+
+def test_transparent_break(nix: Nix):
+    """
+    Make sure that adding a call to builtins.break doesn't
+    change the result of an expression
+    """
+
+    expr = dedent("""
+        let
+          inherit (builtins)
+            attrNames
+            break
+            elem
+            functionArgs
+            head
+            isAttrs
+            isPath
+            isFunction
+            map
+            mapAttrs
+            removeAttrs
+            toJSON
+            typeOf;
+        in
+        builtins.all (b: b) [
+            ((attrNames { a = 5; }) == (attrNames (break { a = 5; })))
+            ((elem 5 [1 5]) == (elem 5 (break [1 5])))
+            ((elem (2+3) [1 (2+3)]) == (elem (2+3) (break [1 (2+3)])))
+            ((functionArgs ({ a }: 5)) == (functionArgs (break ({ a }: 5))))
+            ((head [1 2]) == (head (break [1 2])))
+            ((isAttrs { a = 5; }) == (isAttrs (break { a = 5; })))
+            ((isPath ./.) == (isPath (break ./.)))
+            ((isPath ./${".meow"}) == (isPath (break ./${".meow"})))
+            ((isFunction (x: x)) == (isFunction (break (x: x))))
+            ((map (x: x) [1 5]) == (map (x: x) (break [1 5])))
+            ((mapAttrs (n: v: v) { a = 5; }) == (mapAttrs (n: v: v) (break { a = 5; })))
+            ((removeAttrs { a = 5; b = 6; } ["a"]) == (removeAttrs (break { a = 5; b = 6; }) ["a"]))
+            ((removeAttrs { ab = 5; } [("a"+"b")]) == (removeAttrs { ab = 5; } [(break ("a"+"b"))]))
+            ((toJSON { a = 5; }) == (toJSON (break { a = 5; })))
+            ((toJSON { a = [(1+2)]; }) == (toJSON { a = break [(1+2)]; }))
+            ((typeOf { a = 5; }) == (typeOf (break { a = 5; })))
+            ((typeOf (1+2)) == (typeOf (break (1+2))))
+        ]
+    """)
+
+    res_no_dbg = nix.nix(["eval", "--expr", expr], flake=True).run().expect(0)
+    assert "true" in res_no_dbg.stdout_plain
+
+    res_with_dbg = (
+        nix.nix(["eval", "--debugger", "--expr", expr], flake=True)
+        .with_stdin(b":c\n" * 50)
+        .run()
+        .expect(0)
+    )
+    assert "true" in res_with_dbg.stdout_plain