From b0acc1d8909a4987dc106d56f50bd99ef452bfab Mon Sep 17 00:00:00 2001
From: Wind <WindSoilder@outlook.com>
Date: Tue, 23 Apr 2024 06:10:35 +0800
Subject: [PATCH] Avoid panic when pipe a variable to a custom command which
 have recursive call (#12491)

# Description
Fixes: #11351

And comment here is also fixed:
https://github.com/nushell/nushell/issues/11351#issuecomment-1996191537

The panic can happened if we pipe a variable to a custom command which
recursively called itself inside another block.

TBH, I think I figure out how it works to panic, but I'm not sure if
there is a potention issue if nushell don't mutate a block in such case.

# User-Facing Changes
Nan

# Tests + Formatting
Done

# After Submitting
Done

---------

Co-authored-by: Stefan Holderbach <sholderbach@users.noreply.github.com>
---
 crates/nu-parser/src/parser.rs                | 14 ++++++-
 src/tests/test_parser.rs                      | 39 +++++++++++++++++++
 .../samples/recursive_func_with_alias.nu      | 22 +++++++++++
 3 files changed, 74 insertions(+), 1 deletion(-)
 create mode 100644 tests/parsing/samples/recursive_func_with_alias.nu

diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs
index 0b71c3c63d..ff171c07be 100644
--- a/crates/nu-parser/src/parser.rs
+++ b/crates/nu-parser/src/parser.rs
@@ -6289,7 +6289,19 @@ pub fn parse(
         // panic (again, in theory, this shouldn't be possible)
         let block = working_set.get_block(block_id);
         let block_captures_empty = block.captures.is_empty();
-        if !captures.is_empty() && block_captures_empty {
+        // need to check block_id >= working_set.permanent_state.num_blocks()
+        // to avoid mutate a block that is in the permanent state.
+        // this can happened if user defines a function with recursive call
+        // and pipe a variable to the command, e.g:
+        // def px [] { if true { 42 } else { px } };    # the block px is saved in permanent state.
+        // let x = 3
+        // $x | px
+        // If we don't guard for `block_id`, it will change captures of `px`, which is
+        // already saved in permanent state
+        if !captures.is_empty()
+            && block_captures_empty
+            && block_id >= working_set.permanent_state.num_blocks()
+        {
             let block = working_set.get_block_mut(block_id);
             block.captures = captures.into_iter().map(|(var_id, _)| var_id).collect();
         }
diff --git a/src/tests/test_parser.rs b/src/tests/test_parser.rs
index 241aaccecf..830b131b6e 100644
--- a/src/tests/test_parser.rs
+++ b/src/tests/test_parser.rs
@@ -1,4 +1,5 @@
 use crate::tests::{fail_test, run_test, run_test_with_env, TestResult};
+use nu_test_support::{nu, nu_repl_code};
 use std::collections::HashMap;
 
 use super::run_test_contains;
@@ -818,3 +819,41 @@ fn record_missing_value() -> TestResult {
 fn def_requires_body_closure() -> TestResult {
     fail_test("def a [] (echo 4)", "expected definition body closure")
 }
+
+#[test]
+fn not_panic_with_recursive_call() {
+    let result = nu!(nu_repl_code(&[
+        "def px [] { if true { 3 } else { px } }",
+        "let x = 1",
+        "$x | px",
+    ]));
+    assert_eq!(result.out, "3");
+
+    let result = nu!(nu_repl_code(&[
+        "def px [n=0] { let l = $in; if $n == 0 { return false } else { $l | px ($n - 1) } }",
+        "let x = 1",
+        "$x | px"
+    ]));
+    assert_eq!(result.out, "false");
+
+    let result = nu!(nu_repl_code(&[
+        "def px [n=0] { let l = $in; if $n == 0 { return false } else { $l | px ($n - 1) } }",
+        "let x = 1",
+        "def foo [] { $x }",
+        "foo | px"
+    ]));
+    assert_eq!(result.out, "false");
+
+    let result = nu!(nu_repl_code(&[
+        "def px [n=0] { let l = $in; if $n == 0 { return false } else { $l | px ($n - 1) } }",
+        "let x = 1",
+        "do {|| $x } | px"
+    ]));
+    assert_eq!(result.out, "false");
+
+    let result = nu!(
+        cwd: "tests/parsing/samples",
+        "nu recursive_func_with_alias.nu"
+    );
+    assert!(result.status.success());
+}
diff --git a/tests/parsing/samples/recursive_func_with_alias.nu b/tests/parsing/samples/recursive_func_with_alias.nu
new file mode 100644
index 0000000000..66dc3b5f11
--- /dev/null
+++ b/tests/parsing/samples/recursive_func_with_alias.nu
@@ -0,0 +1,22 @@
+alias "orig update" = update
+
+# Update a column to have a new value if it exists.
+#
+# If the column exists with the value `null` it will be skipped.
+export def "update" [
+    field: cell-path # The name of the column to maybe update.
+    value: any # The new value to give the cell(s), or a closure to create the value.
+]: [record -> record, table -> table, list<any> -> list<any>] {
+    let input = $in
+    match ($input | describe | str replace --regex '<.*' '') {
+        record => {
+            if ($input | get -i $field) != null {
+                $input | orig update $field $value
+            } else { $input }
+        }
+        table|list => {
+            $input | each {|| update $field $value }
+        }
+        _ => { $input | orig update $field $value }
+    }
+}