From a6074219124a304af56ddd570fcbb1ee8338c0e6 Mon Sep 17 00:00:00 2001 From: esdmr Date: Mon, 13 Feb 2023 19:29:28 +0330 Subject: [PATCH] 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 | 2 ++ doc_src/cmds/functions.rst | 6 ++-- src/builtins/functions.cpp | 40 +++++++++++++++++++++---- src/builtins/type.cpp | 56 +++++++++++++++++++--------------- src/function.cpp | 11 ++++--- src/function.h | 11 ++++++- tests/checks/function.fish | 16 +++++++++- tests/checks/functions.fish | 57 +++++++++++++++++++++++++++++++++++ tests/checks/status.fish | 21 +++++++++++++ tests/checks/type.fish | 60 ++++++++++++++++++++++++++++++++++++- 10 files changed, 240 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 1f1406848..219c24481 100644 --- a/CHANGELOG.rst +++ b/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 ------------------------ diff --git a/doc_src/cmds/functions.rst b/doc_src/cmds/functions.rst index b2ff9368b..269f302a3 100644 --- a/doc_src/cmds/functions.rst +++ b/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 ` 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 `, and ``n/a`` if the function isn't available. (Functions created via :doc:`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 `, and ``n/a`` if the function isn't available. (Functions created via :doc:`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, - - ``autoloaded``, ``not-autoloaded`` or ``n/a``, + - the path name as already described, + - 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. diff --git a/src/builtins/functions.cpp b/src/builtins/functions.cpp index 499f036e5..cb3f77e06 100644 --- a/src/builtins/functions.cpp +++ b/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 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; } diff --git a/src/builtins/type.cpp b/src/builtins/type.cpp index 6e9d09bd1..d10c714f9 100644 --- a/src/builtins/type.cpp +++ b/src/builtins/type.cpp @@ -133,32 +133,44 @@ maybe_t 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) { - streams.out.append(*path); + if (final_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); - if (path) { - int line_number = func->definition_lineno(); - 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); - } + + wcstring def; + append_format(def, L"# %ls\n%ls", comment.c_str(), + func->annotated_definition(name).c_str()); + if (!streams.out_is_redirected && isatty(STDOUT_FILENO)) { std::vector colors; highlight_shell(def, colors, parser.context()); @@ -168,11 +180,7 @@ maybe_t 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; - if (path) { - streams.out.append_format(_(L" (defined in %ls)"), path->c_str()); - } - streams.out.append(L"\n"); + streams.out.append_format(_(L" (%ls)\n"), comment.c_str()); } } else if (opts.type) { streams.out.append(L"function\n"); diff --git a/src/function.cpp b/src/function.cpp index b64d57bc0..48bab6b4e 100644 --- a/src/function.cpp +++ b/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. diff --git a/src/function.h b/src/function.h index cd1dc1316..54b02931e 100644 --- a/src/function.h +++ b/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(); diff --git a/tests/checks/function.fish b/tests/checks/function.fish index fd926c4cb..c4be11e71 100644 --- a/tests/checks/function.fish +++ b/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 diff --git a/tests/checks/functions.fish b/tests/checks/functions.fish index 7021ea0fe..06ea0967a 100644 --- a/tests/checks/functions.fish +++ b/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: diff --git a/tests/checks/status.fish b/tests/checks/status.fish index 913cd99a6..94aa2d1e4 100644 --- a/tests/checks/status.fish +++ b/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 '')[1..4] +# CHECK: in function 'test-stack-trace-main' +# CHECK: called on line {{\d+}} of file {{.*}}/status.fish +# CHECK: in function 'test-stack-trace-other' +# CHECK: 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 '')[1..4] +# CHECK: in function 'test-stack-trace-main' +# CHECK: called on line {{\d+}} of file {{.*}}/status.fish +# CHECK: in function 'test-stack-trace-copy' +# CHECK: called on line {{\d+}} of file {{.*}}/status.fish diff --git a/tests/checks/type.fish b/tests/checks/type.fish index 85a2d142a..cf479c49d 100644 --- a/tests/checks/type.fish +++ b/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`)