revert the --shadow-builtin flag

Implementing the --shadow-builtin flag has proven to be highly controversial.
Revert the introduction of that flag to the `function` command. If someone
shoots themselves in the foot by redefining a builtin as a function that's
their problem and not our responsibility to protect them from doing so.

Fixes #3319

doc_src/function.txt

@@ -29,8 +29,6 @@ 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.

share/functions/__fish_config_interactive.fish

@@ -310,7 +310,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 --shadow-builtin
+        function set_color
             set -l term_colors black red green yellow blue magenta cyan white normal
             for a in $argv
                 if not contains -- $a $term_colors

share/functions/cd.fish

@@ -1,7 +1,7 @@
 #
 # Wrap the builtin cd command to maintain directory history.
 #
-function cd --shadow-builtin --description "Change directory"
+function cd --description "Change directory"
     set -l MAX_DIR_HIST 25
 
     if test (count $argv) -gt 1

share/functions/history.fish

@@ -1,7 +1,7 @@
 #
 # Wrap the builtin history command to provide additional functionality.
 #
-function history --shadow-builtin --description "display or manipulate interactive command history"
+function history --description "display or manipulate interactive command history"
     set -l cmd
     set -l search_mode
     set -l with_time
@@ -40,7 +40,7 @@ function history --shadow-builtin --description "display or manipulate interacti
     end
 
     if not set -q cmd[1]
-        set cmd search  # default to "search" if the user didn't explicitly specify a command
+        set cmd search # default to "search" if the user didn't explicitly specify a command
     else if set -q cmd[2]
         printf (_ "You cannot specify multiple commands: %s\n") "$cmd"
         return 1
@@ -60,7 +60,7 @@ function history --shadow-builtin --description "display or manipulate interacti
                 builtin history --search $search_mode $with_time -- $argv
             end
 
-        case delete  # Interactively delete history
+        case delete # Interactively delete history
             # TODO: Fix this to deal with history entries that have multiple lines.
             if not set -q argv[1]
                 printf (_ "You must specify at least one search term when deleting entries") >&2
@@ -105,8 +105,8 @@ function history --shadow-builtin --description "display or manipulate interacti
 
                 for i in (string split " " -- $choice)
                     if test -z "$i"
-                    or not string match -qr '^[1-9][0-9]*$' -- $i
+                        or not string match -qr '^[1-9][0-9]*$' -- $i
-                    or test $i -gt $found_items_count
+                        or test $i -gt $found_items_count
                         printf "Ignoring invalid history entry ID \"%s\"\n" $i
                         continue
                     end
@@ -130,7 +130,7 @@ function history --shadow-builtin --description "display or manipulate interacti
             # Erase the entire history.
             read --local --prompt "echo 'Are you sure you want to clear history? (y/n) '" choice
             if test "$choice" = "y"
-            or test "$choice" = "yes"
+                or test "$choice" = "yes"
                 builtin history --clear -- $argv
                 and echo "History cleared!"
             end

src/builtin.cpp

@@ -920,10 +920,6 @@ static wcstring functions_def(const wcstring &name) {
         out.append(esc_desc);
     }
 
-    if (function_get_shadow_builtin(name)) {
-        out.append(L" --shadow-builtin");
-    }
-
     if (!function_get_shadow_scope(name)) {
         out.append(L" --no-scope-shadowing");
     }
@@ -1492,17 +1488,13 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis
     // Hackish const_cast matches the one in builtin_run.
     const null_terminated_array_t<wchar_t> argv_array(args);
     wchar_t **argv = const_cast<wchar_t **>(argv_array.get());
-
     int argc = builtin_count_args(argv);
     int res = STATUS_BUILTIN_OK;
     wchar_t *desc = 0;
     std::vector<event_t> events;
-
     bool has_named_arguments = false;
     wcstring_list_t named_arguments;
     wcstring_list_t inherit_vars;
-
-    bool shadow_builtin = false;
     bool shadow_scope = true;
 
     wcstring_list_t wrap_targets;
@@ -1523,7 +1515,6 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis
                                            {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}};
@@ -1532,7 +1523,7 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis
         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:haBSV:", long_options, &opt_index);
+        int opt = w.wgetopt_long(argc, argv, L"-d:s:j:p:v:e:w:haSV:", long_options, &opt_index);
         if (opt == -1) break;
         switch (opt) {
             case 0: {
@@ -1628,10 +1619,6 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis
                 name_is_first_positional = !positionals.empty();
                 break;
             }
-            case 'B': {
-                shadow_builtin = true;
-                break;
-            }
             case 'S': {
                 shadow_scope = false;
                 break;
@@ -1720,30 +1707,6 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis
             }
         }
 
-        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;
@@ -1751,7 +1714,6 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis
             d.name = function_name;
             if (desc) d.description = desc;
             d.events.swap(events);
-            d.shadow_builtin = shadow_builtin;
             d.shadow_scope = shadow_scope;
             d.named_arguments.swap(named_arguments);
             d.inherit_vars.swap(inherit_vars);

src/exec.cpp

@@ -633,7 +633,6 @@ void exec_job(parser_t &parser, job_t *j) {
                 const wcstring func_name = p->argv0();
                 wcstring def;
                 bool function_exists = function_get_definition(func_name, &def);
-
                 bool shadow_scope = function_get_shadow_scope(func_name);
                 const std::map<wcstring, env_var_t> inherit_vars =
                     function_get_inherit_vars(func_name);

src/function.cpp

@@ -144,7 +144,6 @@ 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),
-      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,
@@ -156,7 +155,6 @@ 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),
-      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) {
@@ -260,12 +258,6 @@ 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_shadow_builtin(const wcstring &name) {
-    scoped_lock locker(functions_lock);
-    const function_info_t *func = function_get(name);
-    return func ? func->shadow_builtin : false;
-}
-
 int function_get_shadow_scope(const wcstring &name) {
     scoped_lock locker(functions_lock);
     const function_info_t *func = function_get(name);

src/function.h

@@ -32,8 +32,6 @@ struct function_data_t {
     wcstring_list_t inherit_vars;
     /// 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 {
@@ -53,8 +51,6 @@ class function_info_t {
     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 shadow_scope;
 
@@ -129,9 +125,6 @@ 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_shadow_scope(const wcstring &name);
 

tests/function.err

@@ -1,8 +0,0 @@
-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

tests/function.in

@@ -44,16 +44,3 @@ 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

tests/function.out

@@ -22,4 +22,3 @@ Function name1 found
 Function name2 found
 Function name3 found
 Function name4 found
-"function pwd --shadow-builtin" worked