functions --copy: store file and lineno (#9542)

Keeps the location of original function definition, and also stores
where it was copied. `functions` and `type` show both locations,
instead of none. It also retains the line numbers in the stack trace.

CHANGELOG.rst

@@ -18,6 +18,8 @@ Scripting improvements
 ----------------------
 - ``abbr --list`` no longer escapes the abbr name, which is necessary to be able to pass it to ``abbr --erase`` (:issue:`9470`).
 - ``read`` will now print an error if told to set a read-only variable instead of silently doing nothing (:issue:`9346`).
+- ``functions`` and ``type`` now show where a function was copied and where it originally was instead of saying ``Defined interactively``.
+- Stack trace now shows line numbers for copied functions.
 
 Interactive improvements
 ------------------------

doc_src/cmds/functions.rst

@@ -34,10 +34,10 @@ The following options are available:
     Causes the specified functions to be erased. This also means that it is prevented from autoloading in the current session. Use :doc:`funcsave <funcsave>` to remove the saved copy.
 
 **-D** or **--details**
-    Reports the path name where the specified function is defined or could be autoloaded, ``stdin`` if the function was defined interactively or on the command line or by reading standard input, **-** if the function was created via :doc:`source <source>`, and ``n/a`` if the function isn't available. (Functions created via :doc:`alias <alias>` will return **-**, because ``alias`` uses ``source`` internally.) If the **--verbose** option is also specified then five lines are written:
+    Reports the path name where the specified function is defined or could be autoloaded, ``stdin`` if the function was defined interactively or on the command line or by reading standard input, **-** if the function was created via :doc:`source <source>`, and ``n/a`` if the function isn't available. (Functions created via :doc:`alias <alias>` will return **-**, because ``alias`` uses ``source`` internally. Copied functions will return where the function was copied.) If the **--verbose** option is also specified then five lines are written:
 
-    - the pathname as already described,
+    - the path name as already described,
-    - ``autoloaded``, ``not-autoloaded`` or ``n/a``,
+    - if the function was copied, the path name to where the function was originally defined, otherwise ``autoloaded``, ``not-autoloaded`` or ``n/a``,
     - the line number within the file or zero if not applicable,
     - ``scope-shadowing`` if the function shadows the vars in the calling function (the normal case if it wasn't defined with **--no-scope-shadowing**), else ``no-scope-shadowing``, or ``n/a`` if the function isn't defined,
     - the function description minimally escaped so it is a single line, or ``n/a`` if the function isn't defined or has no description.

src/builtins/functions.cpp

@@ -135,6 +135,9 @@ static int report_function_metadata(const wcstring &funcname, bool verbose, io_s
     const wchar_t *shadows_scope = L"n/a";
     wcstring description = L"n/a";
     int line_number = 0;
+    bool is_copy = false;
+    wcstring copy_path = L"n/a";
+    int copy_line_number = 0;
 
     if (auto props = function_get_props_autoload(funcname, parser)) {
         if (props->definition_file) {
@@ -144,6 +147,16 @@ static int report_function_metadata(const wcstring &funcname, bool verbose, io_s
         } else {
             path = L"stdin";
         }
+
+        is_copy = props->is_copy;
+
+        if (props->copy_definition_file) {
+            copy_path = *props->copy_definition_file;
+            copy_line_number = props->copy_definition_lineno;
+        } else {
+            copy_path = L"stdin";
+        }
+
         shadows_scope = props->shadow_scope ? L"scope-shadowing" : L"no-scope-shadowing";
         description = escape_string(props->description, ESCAPE_NO_PRINTABLES | ESCAPE_NO_QUOTED);
     }
@@ -152,12 +165,26 @@ static int report_function_metadata(const wcstring &funcname, bool verbose, io_s
         // "stdin" means it was defined interactively, "-" means it was defined via `source`.
         // Neither is useful information.
         wcstring comment;
+
         if (path == L"stdin") {
-            append_format(comment, L"# Defined interactively\n");
+            append_format(comment, L"# Defined interactively");
         } else if (path == L"-") {
-            append_format(comment, L"# Defined via `source`\n");
+            append_format(comment, L"# Defined via `source`");
         } else {
-            append_format(comment, L"# Defined in %ls @ line %d\n", path.c_str(), line_number);
+            append_format(comment, L"# Defined in %ls @ line %d", path.c_str(), line_number);
+        }
+
+        if (is_copy) {
+            if (copy_path == L"stdin") {
+                append_format(comment, L", copied interactively\n");
+            } else if (copy_path == L"-") {
+                append_format(comment, L", copied via `source`\n");
+            } else {
+                append_format(comment, L", copied in %ls @ line %d\n", copy_path.c_str(),
+                              copy_line_number);
+            }
+        } else {
+            append_format(comment, L"\n");
         }
 
         if (!streams.out_is_redirected && isatty(STDOUT_FILENO)) {
@@ -168,9 +195,10 @@ static int report_function_metadata(const wcstring &funcname, bool verbose, io_s
             streams.out.append(comment);
         }
     } else {
-        streams.out.append_format(L"%ls\n", path.c_str());
+        streams.out.append_format(L"%ls\n", is_copy ? copy_path.c_str() : path.c_str());
+
         if (verbose) {
-            streams.out.append_format(L"%ls\n", autoloaded);
+            streams.out.append_format(L"%ls\n", is_copy ? path.c_str() : autoloaded);
             streams.out.append_format(L"%d\n", line_number);
             streams.out.append_format(L"%ls\n", shadows_scope);
             streams.out.append_format(L"%ls\n", description.c_str());
@@ -331,7 +359,7 @@ maybe_t<int> builtin_functions(parser_t &parser, io_streams_t &streams, const wc
             return STATUS_CMD_ERROR;
         }
 
-        if (function_copy(current_func, new_func)) return STATUS_CMD_OK;
+        if (function_copy(current_func, new_func, parser)) return STATUS_CMD_OK;
         return STATUS_CMD_ERROR;
     }
 

src/builtins/type.cpp

@@ -133,32 +133,44 @@ maybe_t<int> builtin_type(parser_t &parser, io_streams_t &streams, const wchar_t
             res = true;
             if (!opts.query && !opts.type) {
                 auto path = func->definition_file;
+                auto copy_path = func->copy_definition_file;
+                auto final_path = func->is_copy ? copy_path : path;
+                wcstring comment;
+
+                if (!path) {
+                    append_format(comment, _(L"Defined interactively"));
+                } else if (*path == L"-") {
+                    append_format(comment, _(L"Defined via `source`"));
+                } else {
+                    append_format(comment, _(L"Defined in %ls @ line %d"), path->c_str(),
+                                  func->definition_lineno());
+                }
+
+                if (func->is_copy) {
+                    if (!copy_path) {
+                        append_format(comment, _(L", copied interactively"));
+                    } else if (*copy_path == L"-") {
+                        append_format(comment, _(L", copied via `source`"));
+                    } else {
+                        append_format(comment, _(L", copied in %ls @ line %d"), copy_path->c_str(),
+                                      func->copy_definition_lineno);
+                    }
+                }
+
                 if (opts.path) {
-                    if (path) {
+                    if (final_path) {
-                        streams.out.append(*path);
+                        streams.out.append(*final_path);
                         streams.out.append(L"\n");
                     }
                 } else if (!opts.short_output) {
                     streams.out.append_format(_(L"%ls is a function"), name);
                     streams.out.append(_(L" with definition"));
                     streams.out.append(L"\n");
-                    // Function path
+
-                    wcstring def = func->annotated_definition(name);
+                    wcstring def;
-                    if (path) {
+                    append_format(def, L"# %ls\n%ls", comment.c_str(),
-                        int line_number = func->definition_lineno();
+                                  func->annotated_definition(name).c_str());
-                        wcstring comment;
+
-                        if (*path != L"-") {
-                            append_format(comment, L"# Defined in %ls @ line %d\n", path->c_str(),
-                                          line_number);
-                        } else {
-                            append_format(comment, L"# Defined via `source`\n");
-                        }
-                        def = comment.append(def);
-                    } else {
-                        wcstring comment;
-                        append_format(comment, L"# Defined interactively\n");
-                        def = comment.append(def);
-                    }
                     if (!streams.out_is_redirected && isatty(STDOUT_FILENO)) {
                         std::vector<highlight_spec_t> colors;
                         highlight_shell(def, colors, parser.context());
@@ -168,11 +180,7 @@ maybe_t<int> builtin_type(parser_t &parser, io_streams_t &streams, const wchar_t
                     }
                 } else {
                     streams.out.append_format(_(L"%ls is a function"), name);
-                    auto path = func->definition_file;
+                    streams.out.append_format(_(L" (%ls)\n"), comment.c_str());
-                    if (path) {
-                        streams.out.append_format(_(L" (defined in %ls)"), path->c_str());
-                    }
-                    streams.out.append(L"\n");
                 }
             } else if (opts.type) {
                 streams.out.append(L"function\n");

src/function.cpp

@@ -231,7 +231,10 @@ void function_set_desc(const wcstring &name, const wcstring &desc, parser_t &par
     }
 }
 
-bool function_copy(const wcstring &name, const wcstring &new_name) {
+bool function_copy(const wcstring &name, const wcstring &new_name, parser_t &parser) {
+    auto filename = parser.current_filename();
+    auto lineno = parser.get_lineno();
+
     auto funcset = function_set.acquire();
     auto props = funcset->get_props(name);
     if (!props) {
@@ -239,11 +242,11 @@ bool function_copy(const wcstring &name, const wcstring &new_name) {
         return false;
     }
     // Copy the function's props.
-    // This new instance of the function shouldn't be tied to the definition file of the
-    // original, so clear the filename, etc.
     auto new_props = copy_props(props);
     new_props->is_autoload = false;
-    new_props->definition_file = nullptr;
+    new_props->is_copy = true;
+    new_props->copy_definition_file = filename;
+    new_props->copy_definition_lineno = lineno;
 
     // Note this will NOT overwrite an existing function with the new name.
     // TODO: rationalize if this behavior is desired.

src/function.h

@@ -46,6 +46,15 @@ struct function_properties_t {
     /// The file from which the function was created, or nullptr if not from a file.
     filename_ref_t definition_file{};
 
+    /// Whether the function was copied.
+    bool is_copy{false};
+
+    /// The file from which the function was copied, or nullptr if not from a file.
+    filename_ref_t copy_definition_file{};
+
+    /// The line number where the specified function was copied.
+    int copy_definition_lineno{};
+
     /// \return the description, localized via _.
     const wchar_t *localized_description() const;
 
@@ -95,7 +104,7 @@ wcstring_list_t function_get_names(bool get_hidden);
 
 /// Creates a new function using the same definition as the specified function. Returns true if copy
 /// is successful.
-bool function_copy(const wcstring &name, const wcstring &new_name);
+bool function_copy(const wcstring &name, const wcstring &new_name, parser_t &parser);
 
 /// Observes that fish_function_path has changed.
 void function_invalidate_path();

tests/checks/function.fish

@@ -99,12 +99,26 @@ set -l name1 (functions name1)
 set -l name1a (functions name1a)
 set -l name3 (functions name3)
 set -l name3a (functions name3a)
-# First line for the non-copied function is "# Defined in checks/function.fish" - skip it to work around #6575.
+# First two lines for the copied and non-copied functions are different. Skip it for now.
 test "$name1[3..-1]" = "$name1a[3..-1]"; and echo "1 = 1a"
 #CHECK: 1 = 1a
 test "$name3[3..-1]" = "$name3a[3..-1]"; and echo "3 = 3a"
 #CHECK: 3 = 3a
 
+# Test the first two lines.
+string join \n -- $name1[1..2]
+#CHECK: # Defined in {{(?:(?!, copied).)*}}
+#CHECK: function name1 --argument arg1 arg2
+string join \n -- $name1a[1..2]
+#CHECK: # Defined in {{.*}}, copied in {{.*}}
+#CHECK: function name1a --argument arg1 arg2
+string join \n -- $name3[1..2]
+#CHECK: # Defined in {{(?:(?!, copied).)*}}
+#CHECK: function name3 --argument arg1 arg2
+string join \n -- $name3a[1..2]
+#CHECK: # Defined in {{.*}}, copied in {{.*}}
+#CHECK: function name3a --argument arg1 arg2
+
 function test
     echo banana
 end

tests/checks/functions.fish

@@ -54,6 +54,28 @@ if test $x[5] != 'line 1\\\\n\\nline 2 & more; way more'
     echo "Unexpected output for 'functions -v -D multiline_descr': $x" >&2
 end
 
+# ==========
+# Verify that `functions --details` works as expected when given the name of a
+# function that is copied. (Prints the filename where it was copied.)
+functions -c f1 f1a
+functions -D f1a
+#CHECK: {{.*}}checks/functions.fish
+functions -Dv f1a
+#CHECK: {{.*}}checks/functions.fish
+#CHECK: {{.*}}checks/functions.fish
+#CHECK: {{\d+}}
+#CHECK: scope-shadowing
+#CHECK:
+echo "functions -c f1 f1b" | source
+functions -D f1b
+#CHECK: -
+functions -Dv f1b
+#CHECK: -
+#CHECK: {{.*}}checks/functions.fish
+#CHECK: {{\d+}}
+#CHECK: scope-shadowing
+#CHECK:
+
 # ==========
 # Verify function description setting
 function test_func_desc
@@ -106,6 +128,41 @@ functions --no-details t
 # CHECK: echo tttt;
 # CHECK: end
 
+functions -c t t2
+functions t2
+# CHECK: # Defined via `source`, copied in {{.*}}checks/functions.fish @ line {{\d+}}
+# CHECK: function t2
+# CHECK: echo tttt;
+# CHECK: end
+functions -D t2
+#CHECK: {{.*}}checks/functions.fish
+functions -Dv t2
+#CHECK: {{.*}}checks/functions.fish
+#CHECK: -
+#CHECK: {{\d+}}
+#CHECK: scope-shadowing
+#CHECK:
+
+echo "functions -c t t3" | source
+functions t3
+# CHECK: # Defined via `source`, copied via `source`
+# CHECK: function t3
+# CHECK: echo tttt;
+# CHECK: end
+functions -D t3
+#CHECK: -
+functions -Dv t3
+#CHECK: -
+#CHECK: -
+#CHECK: {{\d+}}
+#CHECK: scope-shadowing
+#CHECK:
+
+functions --no-details t2
+# CHECK: function t2
+# CHECK: echo tttt;
+# CHECK: end
+
 functions --no-details --details t
 # CHECKERR: functions: invalid option combination
 # CHECKERR:

tests/checks/status.fish

@@ -105,3 +105,24 @@ end
 # CHECK: Failed write tests {{finished|skipped}}
 # CHECKERR: write: {{.*}}
 # CHECKERR: write: {{.*}}
+
+function test-stack-trace-main
+    status stack-trace
+end
+
+function test-stack-trace-other
+    test-stack-trace-main
+end
+
+printf "%s\n" (test-stack-trace-other | string replace \t '<TAB>')[1..4]
+# CHECK: in function 'test-stack-trace-main'
+# CHECK: <TAB>called on line {{\d+}} of file {{.*}}/status.fish
+# CHECK: in function 'test-stack-trace-other'
+# CHECK: <TAB>called on line {{\d+}} of file {{.*}}/status.fish
+
+functions -c test-stack-trace-other test-stack-trace-copy
+printf "%s\n" (test-stack-trace-copy | string replace \t '<TAB>')[1..4]
+# CHECK: in function 'test-stack-trace-main'
+# CHECK: <TAB>called on line {{\d+}} of file {{.*}}/status.fish
+# CHECK: in function 'test-stack-trace-copy'
+# CHECK: <TAB>called on line {{\d+}} of file {{.*}}/status.fish

tests/checks/type.fish

@@ -61,7 +61,7 @@ type -p alias
 # CHECK: {{.*}}/alias.fish
 
 type -s alias
-# CHECK: alias is a function (defined in {{.*}}/alias.fish)
+# CHECK: alias is a function (Defined in {{.*}}/alias.fish @ line {{\d+}})
 
 function test-type
     echo this is a type test
@@ -76,3 +76,61 @@ type test-type
 
 type -p test-type
 # CHECK: {{.*}}/type.fish
+
+functions -c test-type test-type2
+type test-type2
+# CHECK: test-type2 is a function with definition
+# CHECK: # Defined in {{.*}}/type.fish @ line {{\d+}}, copied in {{.*}}/type.fish @ line {{\d+}}
+# CHECK: function test-type2
+# CHECK: echo this is a type test
+# CHECK: end
+
+type -p test-type2
+# CHECK: {{.*}}/type.fish
+
+type -s test-type2
+# CHECK: test-type2 is a function (Defined in {{.*}}/type.fish @ line {{\d+}}, copied in {{.*}}/type.fish @ line {{\d+}})
+
+echo "functions -c test-type test-type3" | source
+type test-type3
+# CHECK: test-type3 is a function with definition
+# CHECK: # Defined in {{.*}}/type.fish @ line {{\d+}}, copied via `source`
+# CHECK: function test-type3
+# CHECK: echo this is a type test
+# CHECK: end
+
+type -p test-type3
+# CHECK: -
+
+type -s test-type3
+# CHECK: test-type3 is a function (Defined in {{.*}}/type.fish @ line {{\d+}}, copied via `source`)
+
+echo "function other-test-type; echo this is a type test; end" | source
+
+functions -c other-test-type other-test-type2
+type other-test-type2
+# CHECK: other-test-type2 is a function with definition
+# CHECK: # Defined via `source`, copied in {{.*}}/type.fish @ line {{\d+}}
+# CHECK: function other-test-type2
+# CHECK: echo this is a type test;
+# CHECK: end
+
+type -p other-test-type2
+# CHECK: {{.*}}/type.fish
+
+type -s other-test-type2
+# CHECK: other-test-type2 is a function (Defined via `source`, copied in {{.*}}/type.fish @ line {{\d+}})
+
+echo "functions -c other-test-type other-test-type3" | source
+type other-test-type3
+# CHECK: other-test-type3 is a function with definition
+# CHECK: # Defined via `source`, copied via `source`
+# CHECK: function other-test-type3
+# CHECK: echo this is a type test;
+# CHECK: end
+
+type -p other-test-type3
+# CHECK: -
+
+type -s other-test-type3
+# CHECK: other-test-type3 is a function (Defined via `source`, copied via `source`)