Simplify function_info_t and function_data_t

Work towards cleaning up function definition. Migrate inherit_vars into
props and capture their values at the point of definition.

src/builtin_function.cpp

@@ -253,17 +253,26 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis
     }
 
     // We have what we need to actually define the function.
+    auto props = std::make_shared<function_properties_t>();
+    props->shadow_scope = opts.shadow_scope;
+    props->named_arguments = std::move(opts.named_arguments);
+
+    // Populate inherit_vars.
+    for (const wcstring &name : opts.inherit_vars) {
+        if (auto var = parser.vars().get(name)) {
+            props->inherit_vars[name] = var->as_list();
+        }
+    }
+
+    props->parsed_source = source;
+    props->body_node = body;
+
     function_data_t d;
     d.name = function_name;
-    if (!opts.description.empty()) d.description = opts.description;
+    d.description = opts.description;
-    // d.description = opts.description;
+    d.props = props;
     d.events = std::move(opts.events);
-    d.props.shadow_scope = opts.shadow_scope;
-    d.props.named_arguments = std::move(opts.named_arguments);
-    d.inherit_vars = std::move(opts.inherit_vars);
 
-    d.props.parsed_source = source;
-    d.props.body_node = body;
     function_add(std::move(d), parser);
 
     // Handle wrap targets by creating the appropriate completions.

src/builtin_functions.cpp

@@ -213,15 +213,11 @@ static wcstring functions_def(const wcstring &name) {
     }
 
     // Output any inherited variables as `set -l` lines.
-    std::map<wcstring, env_var_t> inherit_vars = function_get_inherit_vars(name);
+    for (const auto &kv : props->inherit_vars) {
-    for (const auto &kv : inherit_vars) {
-        wcstring_list_t lst;
-        kv.second.to_list(lst);
-
         // We don't know what indentation style the function uses,
         // so we do what fish_indent would.
         append_format(out, L"\n    set -l %ls", kv.first.c_str());
-        for (const auto &arg : lst) {
+        for (const auto &arg : kv.second) {
             wcstring earg = escape_string(arg, ESCAPE_ALL);
             out.push_back(L' ');
             out.append(earg);

src/exec.cpp

@@ -799,9 +799,8 @@ static block_t *function_prepare_environment(parser_t &parser, const process_t *
         idx++;
     }
 
-    std::map<wcstring, env_var_t> inherit_vars = function_get_inherit_vars(func_name);
+    for (const auto &kv : props.inherit_vars) {
-    for (const auto &kv : inherit_vars) {
+        vars.set(kv.first, ENV_LOCAL | ENV_USER, kv.second);
-        vars.set(kv.first, ENV_LOCAL | ENV_USER, kv.second.as_list());
     }
 
     vars.set_argv(std::move(argv));

src/function.cpp

@@ -34,23 +34,16 @@
 class function_info_t {
    public:
     /// Immutable properties of the function.
-    std::shared_ptr<const function_properties_t> props;
+    function_properties_ref_t props;
     /// Function description. This 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;
-    /// 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;
 
-    /// Constructs relevant information from the function_data.
+    function_info_t(function_properties_ref_t props, wcstring desc, const wchar_t *def_file,
-    function_info_t(function_data_t data, const environment_t &vars, const wchar_t *filename,
                     bool autoload);
-
-    /// Used by function_copy.
-    function_info_t(const function_info_t &data, const wchar_t *filename, bool autoload);
 };
 
 /// Type wrapping up the set of all functions.
@@ -148,30 +141,11 @@ static void autoload_names(std::unordered_set<wcstring> &names, int get_hidden)
     }
 }
 
-static std::map<wcstring, env_var_t> snapshot_vars(const wcstring_list_t &vars,
+function_info_t::function_info_t(function_properties_ref_t props, wcstring desc,
-                                                   const environment_t &src) {
+                                 const wchar_t *def_file, bool autoload)
-    std::map<wcstring, env_var_t> result;
+    : props(std::move(props)),
-    for (const wcstring &name : vars) {
+      description(std::move(desc)),
-        auto var = src.get(name);
+      definition_file(intern(def_file)),
-        if (var) result[name] = std::move(*var);
-    }
-    return result;
-}
-
-function_info_t::function_info_t(function_data_t data, const environment_t &vars,
-                                 const wchar_t *filename, bool autoload)
-    : props(std::make_shared<const function_properties_t>(std::move(data.props))),
-      description(std::move(data.description)),
-      definition_file(intern(filename)),
-      inherit_vars(snapshot_vars(data.inherit_vars, vars)),
-      is_autoload(autoload) {}
-
-function_info_t::function_info_t(const function_info_t &data, const wchar_t *filename,
-                                 bool autoload)
-    : props(data.props),
-      description(data.description),
-      definition_file(intern(filename)),
-      inherit_vars(data.inherit_vars),
       is_autoload(autoload) {}
 
 void function_add(const function_data_t &data, const parser_t &parser) {
@@ -191,8 +165,8 @@ void function_add(const function_data_t &data, const parser_t &parser) {
 
     // Create and store a new function.
     const wchar_t *filename = parser.libdata().current_filename;
-    auto ins = funcset->funcs.emplace(data.name,
+    auto ins = funcset->funcs.emplace(
-                                      function_info_t(data, parser.vars(), filename, is_autoload));
+        data.name, function_info_t(data.props, data.description, filename, is_autoload));
     assert(ins.second && "Function should not already be present in the table");
     (void)ins;
 
@@ -261,12 +235,6 @@ bool function_get_definition(const wcstring &name, wcstring &out_definition) {
     return false;
 }
 
-std::map<wcstring, env_var_t> function_get_inherit_vars(const wcstring &name) {
-    const auto funcset = function_set.acquire();
-    const function_info_t *func = funcset->get_info(name);
-    return func ? func->inherit_vars : std::map<wcstring, env_var_t>();
-}
-
 bool function_get_desc(const wcstring &name, wcstring &out_desc) {
     const auto funcset = function_set.acquire();
     const function_info_t *func = funcset->get_info(name);
@@ -294,12 +262,14 @@ bool function_copy(const wcstring &name, const wcstring &new_name) {
         // No such function.
         return false;
     }
+    const function_info_t &src_func = iter->second;
 
     // This new instance of the function shouldn't be tied to the definition file of the
     // original, so pass NULL filename, etc.
     // Note this will NOT overwrite an existing function with the new name.
     // TODO: rationalize if this behavior is desired.
-    funcset->funcs.emplace(new_name, function_info_t(iter->second, nullptr, false));
+    funcset->funcs.emplace(new_name,
+                           function_info_t(src_func.props, src_func.description, nullptr, false));
     return true;
 }
 

src/function.h

@@ -26,10 +26,16 @@ struct function_properties_t {
     /// List of all named arguments for this function.
     wcstring_list_t named_arguments;
 
+    /// Mapping of all variables that were inherited from the function definition scope to their
+    /// values.
+    std::map<wcstring, wcstring_list_t> inherit_vars;
+
     /// Set to true if invoking this function shadows the variables of the underlying function.
     bool shadow_scope{true};
 };
 
+using function_properties_ref_t = std::shared_ptr<const function_properties_t>;
+
 /// Structure describing a function. This is used by the parser to store data on a function while
 /// parsing it. It is not used internally to store functions, the function_info_t structure
 /// is used for that purpose. Parhaps this should be merged with function_info_t.
@@ -38,11 +44,8 @@ struct function_data_t {
     wcstring name;
     /// Description of function.
     wcstring description;
-    /// 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;
     /// Function's metadata fields
-    function_properties_t props;
+    function_properties_ref_t props;
     /// List of all event handlers for this function.
     std::vector<event_description_t> events;
 };
@@ -54,7 +57,7 @@ void function_add(const function_data_t &data, const parser_t &parser);
 void function_remove(const wcstring &name);
 
 /// Returns the properties for a function, or nullptr if none. This does not trigger autoloading.
-std::shared_ptr<const function_properties_t> function_get_properties(const wcstring &name);
+function_properties_ref_t function_get_properties(const wcstring &name);
 
 /// Returns by reference the definition of the function with the name \c name. Returns true if
 /// successful, false if no function with the given name exists.
@@ -100,10 +103,6 @@ const wchar_t *function_get_definition_file(const wcstring &name);
 /// This does not trigger autoloading.
 int function_get_definition_lineno(const wcstring &name);
 
-/// Returns a mapping of all variables of the specified function that were inherited from the scope
-/// of the function definition to their values.
-std::map<wcstring, env_var_t> function_get_inherit_vars(const wcstring &name);
-
 /// 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);