add `function --shadow-builtin` flag

It's currently too easy for someone to bork their shell by doing something
like `function test; return 0; end`. That's obviously a silly, contrived,
example but the point is that novice users who learn about functions are
prone to do something like that without realizing it will bork the shell. Even
expert users who know about the `test` builtin might forget that, say, `pwd`
is a builtin.

This change adds a `--shadow-builtin` flag that must be specified to
indicate you know what you're doing.

Fixes #3000

6 files changed, 83 insertions, 42 deletions

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()) {