Migrate a function's description into its immutable properties

No functional change here.

src/builtin_function.cpp

@@ -271,6 +271,7 @@ maybe_t<int> builtin_function(parser_t &parser, io_streams_t &streams,
     props->named_arguments = std::move(opts.named_arguments);
     props->parsed_source = source;
     props->func_node = &func_node;
+    props->description = opts.description;
     props->definition_file = parser.libdata().current_filename;
 
     // Populate inherit_vars.
@@ -281,7 +282,7 @@ maybe_t<int> builtin_function(parser_t &parser, io_streams_t &streams,
     }
 
     // Add the function itself.
-    function_add(function_name, opts.description, props);
+    function_add(function_name, props);
 
     // Handle wrap targets by creating the appropriate completions.
     for (const wcstring &wt : opts.wrap_targets) {

src/fish_tests.cpp

@@ -3271,7 +3271,7 @@ static void test_complete() {
 #endif
 
     // Add a function and test completing it in various ways.
-    function_add(L"scuttlebutt", {}, func_props);
+    function_add(L"scuttlebutt", func_props);
 
     // Complete a function name.
     completions = do_complete(L"echo (scuttlebut", {});
@@ -3361,7 +3361,7 @@ static void test_complete() {
 
     // Test abbreviations.
     auto &pvars = parser_t::principal_parser().vars();
-    function_add(L"testabbrsonetwothreefour", {}, func_props);
+    function_add(L"testabbrsonetwothreefour", func_props);
     int ret = pvars.set_one(L"_fish_abbr_testabbrsonetwothreezero", ENV_LOCAL, L"expansion");
     completions = complete(L"testabbrsonetwothree", {}, parser->context());
     do_test(ret == 0);

src/function.cpp

@@ -38,10 +38,7 @@ class function_info_t {
    public:
     /// Immutable properties of the function.
     function_properties_ref_t props;
-    /// Function description. This may be changed after the function is created.
+    explicit function_info_t(function_properties_ref_t props);
-    wcstring description;
-
-    function_info_t(function_properties_ref_t props, wcstring desc);
 };
 
 /// Type wrapping up the set of all functions.
@@ -145,11 +142,9 @@ static void autoload_names(std::unordered_set<wcstring> &names, int get_hidden)
     }
 }
 
-function_info_t::function_info_t(function_properties_ref_t props, wcstring desc)
+function_info_t::function_info_t(function_properties_ref_t props) : props(std::move(props)) {}
-    : props(std::move(props)), description(std::move(desc)) {}
 
-void function_add(wcstring name, wcstring description,
+void function_add(wcstring name, std::shared_ptr<function_properties_t> props) {
-                  std::shared_ptr<function_properties_t> props) {
     ASSERT_IS_MAIN_THREAD();
     assert(props && "Null props");
     auto funcset = function_set.acquire();
@@ -166,13 +161,12 @@ void function_add(wcstring name, wcstring description,
     props->is_autoload = funcset->autoloader.autoload_in_progress(name);
 
     // Create and store a new function.
-    auto ins = funcset->funcs.emplace(std::move(name),
+    auto ins = funcset->funcs.emplace(std::move(name), function_info_t(std::move(props)));
-                                      function_info_t(std::move(props), std::move(description)));
     assert(ins.second && "Function should not already be present in the table");
     (void)ins;
 }
 
-std::shared_ptr<const function_properties_t> function_get_properties(const wcstring &name) {
+function_properties_ref_t function_get_properties(const wcstring &name) {
     if (parser_keywords_is_reserved(name)) return nullptr;
     auto funcset = function_set.acquire();
     if (auto info = funcset->get_info(name)) {
@@ -240,10 +234,8 @@ bool function_get_definition(const wcstring &name, wcstring &out_definition) {
 }
 
 bool function_get_desc(const wcstring &name, wcstring &out_desc) {
-    const auto funcset = function_set.acquire();
+    if (auto props = function_get_properties(name)) {
-    const function_info_t *func = funcset->get_info(name);
+        out_desc = _(props->description.c_str());
-    if (func && !func->description.empty()) {
-        out_desc = _(func->description.c_str());
         return true;
     }
     return false;
@@ -255,7 +247,11 @@ void function_set_desc(const wcstring &name, const wcstring &desc, parser_t &par
     auto funcset = function_set.acquire();
     auto iter = funcset->funcs.find(name);
     if (iter != funcset->funcs.end()) {
-        iter->second.description = desc;
+        // Note the description is immutable, as it may be accessed on another thread, so we copy
+        // the properties to modify it.
+        auto new_props = std::make_shared<function_properties_t>(*iter->second.props);
+        new_props->description = desc;
+        iter->second.props = new_props;
     }
 }
 
@@ -277,7 +273,7 @@ bool function_copy(const wcstring &name, const wcstring &new_name) {
 
     // 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(new_props, src_func.description));
+    funcset->funcs.emplace(new_name, function_info_t(new_props));
     return true;
 }
 

src/function.h

@@ -31,6 +31,9 @@ struct function_properties_t {
     /// List of all named arguments for this function.
     wcstring_list_t named_arguments;
 
+    /// Description of the function.
+    wcstring description;
+
     /// Mapping of all variables that were inherited from the function definition scope to their
     /// values.
     std::map<wcstring, wcstring_list_t> inherit_vars;
@@ -49,8 +52,7 @@ struct function_properties_t {
 using function_properties_ref_t = std::shared_ptr<const function_properties_t>;
 
 /// Add a function. This may mutate \p props to set is_autoload.
-void function_add(wcstring name, wcstring description,
+void function_add(wcstring name, std::shared_ptr<function_properties_t> props);
-                  std::shared_ptr<function_properties_t> props);
 
 /// Remove the function with the specified name.
 void function_remove(const wcstring &name);