diff options
| -rw-r--r-- | doc_src/function.txt | 2 | ||||
| -rw-r--r-- | share/functions/__fish_config_interactive.fish | 18 | ||||
| -rw-r--r-- | share/functions/cd.fish | 2 | ||||
| -rw-r--r-- | share/functions/history.fish | 27 | ||||
| -rw-r--r-- | src/builtin.cpp | 55 | ||||
| -rw-r--r-- | src/builtin.h | 8 | ||||
| -rw-r--r-- | src/exec.cpp | 4 | ||||
| -rw-r--r-- | src/function.cpp | 16 | ||||
| -rw-r--r-- | src/function.h | 38 | ||||
| -rw-r--r-- | src/parse_execution.cpp | 4 | ||||
| -rw-r--r-- | tests/function.err | 8 | ||||
| -rw-r--r-- | tests/function.in | 13 | ||||
| -rw-r--r-- | tests/function.out | 1 |
13 files changed, 124 insertions, 72 deletions
diff --git a/doc_src/function.txt b/doc_src/function.txt index e58d6513..65cbdca9 100644 --- a/doc_src/function.txt +++ b/doc_src/function.txt @@ -29,6 +29,8 @@ The following options are available: - `-s` or `--on-signal SIGSPEC` tells fish to run this function when the signal SIGSPEC is delivered. SIGSPEC can be a signal number, or the signal name, such as SIGHUP (or just HUP). +- `-B` or `--shadow-builtin` must be specified if the function name is the same as a builtin. Specifying this flag indicates your acknowledgement that you are wrapping or replacing the builtin command. This is a safety feature to make it harder for people to inadvertently break the shell by doing things like `function test; return 0; end`. If the function name is not currently a builtin using this flag will produce an error. If you want to write a function that provides a builtin to an older version of fish you need to add something like `builtin --names | grep -q '^cmd$'; and return` to the top of the function script (where `cmd` is the name of the builtin/function). That will keep your script from replacing the builtin with your function on the newer fish version while allowing your function to provide similar functionality on older versions of fish. + - `-S` or `--no-scope-shadowing` allows the function to access the variables of calling functions. Normally, any variables inside the function that have the same name as variables from the calling function are "shadowed", and their contents is independent of the calling function. - `-V` or `--inherit-variable NAME` snapshots the value of the variable `NAME` and defines a local variable with that same name and value when the function is executed. diff --git a/share/functions/__fish_config_interactive.fish b/share/functions/__fish_config_interactive.fish index 7b306e3d..ec891ddd 100644 --- a/share/functions/__fish_config_interactive.fish +++ b/share/functions/__fish_config_interactive.fish @@ -4,9 +4,7 @@ # This function is called by the __fish_on_interactive function, which is defined in config.fish. # function __fish_config_interactive -d "Initializations that should be performed when entering interactive mode" - - - # Make sure this function is only run once + # Make sure this function is only run once. if set -q __fish_config_interactive_done return end @@ -27,7 +25,6 @@ function __fish_config_interactive -d "Initializations that should be performed # # If we are starting up for the first time, set various defaults # - if not set -q __fish_init_1_50_0 if not set -q fish_greeting set -l line1 (printf (_ 'Welcome to fish, the friendly interactive shell') ) @@ -97,25 +94,22 @@ function __fish_config_interactive -d "Initializations that should be performed # # Directory history colors # - set -q fish_color_history_current or set -U fish_color_history_current cyan end # - # Generate man page completions if not present + # Generate man page completions if not present. # - if not test -d $userdatadir/fish/generated_completions #fish_update_completions is a function, so it can not be directly run in background. eval "$__fish_bin_dir/fish -c 'fish_update_completions > /dev/null ^/dev/null' &" end # - # Print a greeting - # fish_greeting can be a function (preferred) or a variable + # Print a greeting. + # fish_greeting can be a function (preferred) or a variable. # - if functions -q fish_greeting fish_greeting else @@ -130,7 +124,6 @@ function __fish_config_interactive -d "Initializations that should be performed # fish_color_cwd changes value. Like all event handlers, it can't be # autoloaded. # - function __fish_repaint --on-variable fish_color_cwd --description "Event handler, repaints the prompt when fish_color_cwd changes" if status --is-interactive set -e __fish_prompt_cwd @@ -149,7 +142,6 @@ function __fish_config_interactive -d "Initializations that should be performed # Completions for SysV startup scripts. These aren't bound to any # specific command, so they can't be autoloaded. # - complete -x -p "/etc/init.d/*" -a start --description 'Start service' complete -x -p "/etc/init.d/*" -a stop --description 'Stop service' complete -x -p "/etc/init.d/*" -a status --description 'Print service status' @@ -293,7 +285,7 @@ function __fish_config_interactive -d "Initializations that should be performed # Don't allow setting color other than what linux offers (see #2001) functions -e set_color - function set_color + function set_color --shadow-builtin set -l term_colors black red green yellow blue magenta cyan white normal for a in $argv if not contains -- $a $term_colors diff --git a/share/functions/cd.fish b/share/functions/cd.fish index adf0327c..fdc8b607 100644 --- a/share/functions/cd.fish +++ b/share/functions/cd.fish @@ -1,7 +1,7 @@ # # Wrap the builtin cd command to maintain directory history. # -function cd --description "Change directory" +function cd --shadow-builtin --description "Change directory" set -l MAX_DIR_HIST 25 if test (count $argv) -gt 1 diff --git a/share/functions/history.fish b/share/functions/history.fish index 0de39e30..a365ae19 100644 --- a/share/functions/history.fish +++ b/share/functions/history.fish @@ -1,16 +1,12 @@ # -#Deletes an item from history +# Wrap the builtin history command to provide additional functionality. # -function history --description "Deletes an item from history" - +function history --shadow-builtin --description "Deletes an item from history" set -l argc (count $argv) set -l prefix_args "" set -l contains_args "" - set -l cmd print - set -l search_mode none - set -l pager less if set -q PAGER set pager $PAGER @@ -46,7 +42,7 @@ function history --description "Deletes an item from history" end end else - #Execute history builtin without any argument + # Execute history builtin without any argument. if status --is-interactive builtin history | eval $pager else @@ -57,9 +53,8 @@ function history --description "Deletes an item from history" switch $cmd case print - # Print matching items - # Note this may end up passing --search twice to the builtin, - # but that's harmless + # Print matching items. Note this may end up passing --search twice to the builtin, + # but that's harmless. builtin history --search $argv case delete @@ -72,8 +67,7 @@ function history --description "Deletes an item from history" set found_items (builtin history --search --contains $contains_args) case none builtin history $argv - - #Save changes after deleting item + # Save changes after deleting item. builtin history --save return 0 end @@ -98,7 +92,8 @@ function history --description "Deletes an item from history" continue end - #Following two validations could be embedded with "and" but I find the syntax kind of weird. + # Following two validations could be embedded with "and" but I find the syntax + # kind of weird. if not string match -qr '^[0-9]+$' $i printf "Invalid input: %s\n" $i continue @@ -124,18 +119,18 @@ function history --description "Deletes an item from history" end end - #Save changes after deleting item(s) + # Save changes after deleting item(s). builtin history --save end case save - #Save changes to history file + # Save changes to history file. builtin history $argv case merge builtin history --merge case help builtin history --help case clear - # Erase the entire history + # Erase the entire history. echo "Are you sure you want to clear history ? (y/n)" read ch if test $ch = "y" diff --git a/src/builtin.cpp b/src/builtin.cpp index e5261cb4..a79f898d 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -889,6 +889,7 @@ static int builtin_generic(parser_t &parser, io_streams_t &streams, wchar_t **ar } } } + return STATUS_BUILTIN_ERROR; } @@ -919,7 +920,11 @@ static wcstring functions_def(const wcstring &name) { out.append(esc_desc); } - if (!function_get_shadows(name)) { + if (function_get_shadow_builtin(name)) { + out.append(L" --shadow-builtin"); + } + + if (!function_get_shadow_scope(name)) { out.append(L" --no-scope-shadowing"); } @@ -1460,8 +1465,8 @@ static int builtin_pwd(parser_t &parser, io_streams_t &streams, wchar_t **argv) } /// Adds a function to the function set. It calls into function.cpp to perform any heavy lifting. -int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args, - const wcstring &contents, int definition_line_offset, wcstring *out_err) { +int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args, + const wcstring &contents, int definition_line_offset, wcstring *out_err) { wgetopter_t w; assert(out_err != NULL); @@ -1484,7 +1489,8 @@ int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list wcstring_list_t named_arguments; wcstring_list_t inherit_vars; - bool shadows = true; + bool shadow_builtin = false; + bool shadow_scope = true; wcstring_list_t wrap_targets; @@ -1504,17 +1510,17 @@ int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list {L"wraps", required_argument, 0, 'w'}, {L"help", no_argument, 0, 'h'}, {L"argument-names", no_argument, 0, 'a'}, + {L"shadow-builtin", no_argument, 0, 'B'}, {L"no-scope-shadowing", no_argument, 0, 'S'}, {L"inherit-variable", required_argument, 0, 'V'}, {0, 0, 0, 0}}; - while (1 && (!res)) { + while (1 && !res) { int opt_index = 0; // The leading - here specifies RETURN_IN_ORDER. - int opt = w.wgetopt_long(argc, argv, L"-d:s:j:p:v:e:w:haSV:", long_options, &opt_index); + int opt = w.wgetopt_long(argc, argv, L"-d:s:j:p:v:e:w:haBSV:", long_options, &opt_index); if (opt == -1) break; - switch (opt) { case 0: { if (long_options[opt_index].flag != 0) break; @@ -1536,7 +1542,6 @@ int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list events.push_back(event_t::signal_event(sig)); break; } - case 'v': { if (wcsvarname(w.woptarg)) { append_format(*out_err, _(L"%ls: Invalid variable name '%ls'"), argv[0], @@ -1586,7 +1591,6 @@ int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list e.type = EVENT_JOB_ID; e.param1.job_id = job_id; } - } else { errno = 0; pid = fish_wcstoi(w.woptarg, &end, 10); @@ -1611,8 +1615,12 @@ int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list name_is_first_positional = !positionals.empty(); break; } + case 'B': { + shadow_builtin = true; + break; + } case 'S': { - shadows = 0; + shadow_scope = false; break; } case 'w': { @@ -1700,13 +1708,38 @@ int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list } if (!res) { + bool function_name_shadows_builtin = false; + wcstring_list_t builtin_names = builtin_get_names(); + for (size_t i = 0; i < builtin_names.size(); i++) { + const wchar_t *el = builtin_names.at(i).c_str(); + if (el == function_name) { + function_name_shadows_builtin = true; + break; + } + } + if (function_name_shadows_builtin && !shadow_builtin) { + append_format( + *out_err, + _(L"%ls: function name shadows a builtin so you must use '--shadow-builtin'"), + argv[0]); + res = STATUS_BUILTIN_ERROR; + } else if (!function_name_shadows_builtin && shadow_builtin) { + append_format(*out_err, _(L"%ls: function name does not shadow a builtin so you " + L"must not use '--shadow-builtin'"), + argv[0]); + res = STATUS_BUILTIN_ERROR; + } + } + + if (!res) { // Here we actually define the function! function_data_t d; d.name = function_name; if (desc) d.description = desc; d.events.swap(events); - d.shadows = shadows; + d.shadow_builtin = shadow_builtin; + d.shadow_scope = shadow_scope; d.named_arguments.swap(named_arguments); d.inherit_vars.swap(inherit_vars); diff --git a/src/builtin.h b/src/builtin.h index 0d62babe..7648649d 100644 --- a/src/builtin.h +++ b/src/builtin.h @@ -93,10 +93,10 @@ class builtin_commandline_scoped_transient_t { // Run the __fish_print_help function to obtain the help information for the specified command. wcstring builtin_help_get(parser_t &parser, const wchar_t *cmd); -// Defines a function, like builtin_function. Returns 0 on success. args should NOT contain -// 'function' as the first argument. -int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args, - const wcstring &contents, int definition_line_offset, wcstring *out_err); +// Defines a function. Returns 0 on success. args should NOT contain 'function' as the first +// argument as the parser treats it as a keyword. +int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args, + const wcstring &contents, int definition_line_offset, wcstring *out_err); // Print help for the specified builtin. If \c b is sb_err, also print the line information. void builtin_print_help(parser_t &parser, io_streams_t &streams, const wchar_t *cmd, diff --git a/src/exec.cpp b/src/exec.cpp index 24a9af05..63f2cb77 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -636,7 +636,7 @@ void exec_job(parser_t &parser, job_t *j) { wcstring def; bool function_exists = function_get_definition(func_name, &def); - bool shadows = function_get_shadows(func_name); + bool shadow_scope = function_get_shadow_scope(func_name); const std::map<wcstring, env_var_t> inherit_vars = function_get_inherit_vars(func_name); @@ -646,7 +646,7 @@ void exec_job(parser_t &parser, job_t *j) { debug(0, _(L"Unknown function '%ls'"), p->argv0()); break; } - function_block_t *newv = new function_block_t(p, func_name, shadows); + function_block_t *newv = new function_block_t(p, func_name, shadow_scope); parser.push_block(newv); // Setting variables might trigger an event handler, hence we need to unblock diff --git a/src/function.cpp b/src/function.cpp index f717a16f..54559bee 100644 --- a/src/function.cpp +++ b/src/function.cpp @@ -141,7 +141,8 @@ function_info_t::function_info_t(const function_data_t &data, const wchar_t *fil named_arguments(data.named_arguments), inherit_vars(snapshot_vars(data.inherit_vars)), is_autoload(autoload), - shadows(data.shadows) {} + shadow_builtin(data.shadow_builtin), + shadow_scope(data.shadow_scope) {} function_info_t::function_info_t(const function_info_t &data, const wchar_t *filename, int def_offset, bool autoload) @@ -152,7 +153,8 @@ function_info_t::function_info_t(const function_info_t &data, const wchar_t *fil named_arguments(data.named_arguments), inherit_vars(data.inherit_vars), is_autoload(autoload), - shadows(data.shadows) {} + shadow_builtin(data.shadow_builtin), + shadow_scope(data.shadow_scope) {} void function_add(const function_data_t &data, const parser_t &parser, int definition_line_offset) { ASSERT_IS_MAIN_THREAD(); @@ -255,10 +257,16 @@ std::map<wcstring, env_var_t> function_get_inherit_vars(const wcstring &name) { return func ? func->inherit_vars : std::map<wcstring, env_var_t>(); } -int function_get_shadows(const wcstring &name) { +int function_get_shadow_builtin(const wcstring &name) { scoped_lock lock(functions_lock); const function_info_t *func = function_get(name); - return func ? func->shadows : false; + return func ? func->shadow_builtin : false; +} + +int function_get_shadow_scope(const wcstring &name) { + scoped_lock lock(functions_lock); + const function_info_t *func = function_get(name); + return func ? func->shadow_scope : false; } bool function_get_desc(const wcstring &name, wcstring *out_desc) { diff --git a/src/function.h b/src/function.h index 04829cbe..59b37051 100644 --- a/src/function.h +++ b/src/function.h @@ -31,44 +31,41 @@ struct function_data_t { /// List of all variables that are inherited from the function definition scope. The variable /// values are snapshotted when function_add() is called. wcstring_list_t inherit_vars; - /// Set to non-zero if invoking this function shadows the variables of the underlying function. - int shadows; + /// Set to true if invoking this function shadows the variables of the underlying function. + bool shadow_scope; + /// Set to true if this function shadows a builtin. + bool shadow_builtin; }; class function_info_t { public: - /// Constructs relevant information from the function_data. - function_info_t(const function_data_t &data, const wchar_t *filename, int def_offset, - bool autoload); - - /// Used by function_copy. - function_info_t(const function_info_t &data, const wchar_t *filename, int def_offset, - bool autoload); - /// Function definition. const wcstring definition; - /// Function description. Only the description may be changed after the function is created. wcstring description; - /// File where this function was defined (intern'd string). const wchar_t *const definition_file; - /// Line where definition started. const int definition_offset; - /// List of all named arguments for this function. const wcstring_list_t named_arguments; - /// Mapping of all variables that were inherited from the function definition scope to their /// values. const std::map<wcstring, env_var_t> inherit_vars; - /// Flag for specifying that this function was automatically loaded. const bool is_autoload; - + /// Set to true if this function shadows a builtin. + const bool shadow_builtin; /// Set to true if invoking this function shadows the variables of the underlying function. - const bool shadows; + const bool shadow_scope; + + /// Constructs relevant information from the function_data. + function_info_t(const function_data_t &data, const wchar_t *filename, int def_offset, + bool autoload); + + /// Used by function_copy. + function_info_t(const function_info_t &data, const wchar_t *filename, int def_offset, + bool autoload); }; /// Initialize function data. @@ -133,8 +130,11 @@ std::map<wcstring, env_var_t> function_get_inherit_vars(const wcstring &name); /// is successful. bool function_copy(const wcstring &name, const wcstring &new_name); +/// Returns whether this function shadows a builtin of the same name. +int function_get_shadow_builtin(const wcstring &name); + /// Returns whether this function shadows variables of the underlying function. -int function_get_shadows(const wcstring &name); +int function_get_shadow_scope(const wcstring &name); /// Prepares the environment for executing a function. void function_prepare_environment(const wcstring &name, const wchar_t *const *argv, diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index cca0b8d9..7989597f 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -392,8 +392,8 @@ parse_execution_result_t parse_execution_context_t::run_function_statement( int definition_line_offset = this->line_offset_of_character_at_offset(contents_start); wcstring error_str; io_streams_t streams; - int err = define_function(*parser, streams, argument_list, contents_str, - definition_line_offset, &error_str); + int err = builtin_function(*parser, streams, argument_list, contents_str, + definition_line_offset, &error_str); proc_set_last_status(err); if (!error_str.empty()) { diff --git a/tests/function.err b/tests/function.err index e69de29b..72671ac9 100644 --- a/tests/function.err +++ b/tests/function.err @@ -0,0 +1,8 @@ +function: function name shadows a builtin so you must use '--shadow-builtin' +fish: function pwd; end + ^ +yes, it failed as expected +function: function name does not shadow a builtin so you must not use '--shadow-builtin' +fish: function not_builtin --shadow-builtin; end + ^ +yes, it failed as expected diff --git a/tests/function.in b/tests/function.in index 747bbcec..8202fb13 100644 --- a/tests/function.in +++ b/tests/function.in @@ -44,3 +44,16 @@ for i in (seq 4) echo "Function name$i not found, but should have been" end end + +# Test that we can't define a function that shadows a builtin by accident. +function pwd; end +or echo 'yes, it failed as expected' >&2 + +# Test that we can define a function that shadows a builtin if we use the +# right flag. +function pwd --shadow-builtin; end +and echo '"function pwd --shadow-builtin" worked' + +# Using --shadow-builtin for a non-builtin function name also fails. +function not_builtin --shadow-builtin; end +or echo 'yes, it failed as expected' >&2 diff --git a/tests/function.out b/tests/function.out index 5a3da619..0d12479e 100644 --- a/tests/function.out +++ b/tests/function.out @@ -22,3 +22,4 @@ Function name1 found Function name2 found Function name3 found Function name4 found +"function pwd --shadow-builtin" worked |