From e10a12c0f218e9d7073712b24584dfccee6b1e97 Mon Sep 17 00:00:00 2001
From: Peter Ammon <rf@fishshell.com>
Date: Sun, 1 Jun 2025 14:52:20 -0700
Subject: [PATCH] Rationalize certain edge cases for function execution

This concerns edge cases when executing a function. Historically, when we parse
fish script, we identify early whether or not it's a function; but only record
the function's name and not its properties (i.e. not its source).

This means that the function can change between parsing and execution. Example:

    function foo; echo alpha; end
    foo (function foo; echo beta; end)

This has historically output "beta" because the function is replaced as part of
its own arguments.

Worse is if the function is deleted:

    function foo; echo alpha; end
    foo (functions --erase foo)

This outputs an error but in an awkward place; that's OK since it's very rare.

Let's codify this behavior since someone might be depending on it.
---
 src/exec.rs                | 12 ++++++++----
 src/proc.rs                |  5 ++++-
 tests/checks/function.fish | 12 ++++++++++++
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/src/exec.rs b/src/exec.rs
index 431e35cf3..81bc490c2 100644
--- a/src/exec.rs
+++ b/src/exec.rs
@@ -965,11 +965,15 @@ fn function_restore_environment(parser: &Parser, block: BlockId) {
     Option<&mut OutputStream>,
 ) -> ProcStatus;
 
-// Return a function which may be to run the given process \p.
-// May return an empty std::function in the rare case that the to-be called fish function no longer
-// exists. This is just a dumb artifact of the fact that we only capture the functions name, not its
+// Return a function which may be to run the given process 'p'.
+// May return None in the rare case that the to-be called fish function no longer
+// exists. This is just an artifact of the fact that we only capture the functions name, not its
 // properties, when creating the job; thus a race could delete the function before we fetch its
-// properties.
+// properties. Note this is user visible. An example:
+//
+//    function foo; echo hi; end
+//    foo (functions --erase foo)
+//
 fn get_performer_for_process(
     p: &Process,
     job: &Job,
diff --git a/src/proc.rs b/src/proc.rs
index 15e18530c..d4aa5e46c 100644
--- a/src/proc.rs
+++ b/src/proc.rs
@@ -51,9 +51,12 @@ pub enum ProcessType {
     /// A regular external command.
     #[default]
     External,
-    /// A builtin command.
+    /// A builtin command. The builtin name is stored in `argv[0]`.
     Builtin,
     /// A shellscript function.
+    /// The function name is stored in `argv[0]`.
+    /// Note we don't capture the function body here, because the
+    /// function body may change as part of argument expansion.
     Function,
     /// A block of commands, represented as a node.
     /// This is always either block, ifs, or switchs, never boolean or decorated.
diff --git a/tests/checks/function.fish b/tests/checks/function.fish
index b1277dae6..2e30e08c7 100644
--- a/tests/checks/function.fish
+++ b/tests/checks/function.fish
@@ -174,4 +174,16 @@ functions -q foo
 echo exists $status
 # CHECK: exists 1
 
+# If a function is changed as part of its own arguments, then we see the change.
+# This codifies historic behavior.
+function foo; echo before; end
+foo (function foo; echo after; end)
+# CHECK: after
+
+# If a function is deleted as part of its own arguments, then we see the change.
+# This codifies historic behavior.
+function foo; echo before; end
+foo (functions --erase foo)
+# CHECKERR: error: Unknown function 'foo'
+
 exit 0