Properly print leading comments and indentation in functions

Store the entire function declaration, not just its job list.
This allows us to extract the body of the function complete with any
leading comments and indents.

Fixes #5285

src/builtin_function.cpp

@@ -207,7 +207,8 @@ static int validate_function_name(int argc, const wchar_t *const *argv, wcstring
 /// Define a function. Calls into `function.cpp` to perform the heavy lifting of defining a
 /// function.
 int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args,
-                     const parsed_source_ref_t &source, tnode_t<grammar::job_list> body) {
+                     const parsed_source_ref_t &source,
+                     tnode_t<grammar::block_statement> func_node) {
     assert(source && "Missing source in builtin_function");
     // The wgetopt function expects 'function' as the first argument. Make a new wcstring_list with
     // that property. This is needed because this builtin has a different signature than the other
@@ -258,7 +259,7 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis
     props->shadow_scope = opts.shadow_scope;
     props->named_arguments = std::move(opts.named_arguments);
     props->parsed_source = source;
-    props->body_node = body;
+    props->func_node = func_node;
 
     // Populate inherit_vars.
     for (const wcstring &name : opts.inherit_vars) {

src/builtin_function.h

@@ -9,5 +9,5 @@ class parser_t;
 struct io_streams_t;
 
 int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args,
-                     const parsed_source_ref_t &source, tnode_t<grammar::job_list> body);
+                     const parsed_source_ref_t &source, tnode_t<grammar::block_statement> body);
 #endif

src/builtin_functions.cpp

@@ -223,9 +223,8 @@ static wcstring functions_def(const wcstring &name) {
             out.append(earg);
         }
     }
-
-    // More forced indentation.
-    append_format(out, L"\n    %ls", def.c_str());
+    out.push_back('\n');
+    out.append(def);
 
     // Append a newline before the 'end', unless there already is one there.
     if (!string_suffixes_string(L"\n", def)) {

src/exec.cpp

@@ -722,10 +722,12 @@ static proc_performer_t get_performer_for_process(process_t *p, job_t *job,
         }
         auto argv = move_to_sharedptr(p->get_argv_array().to_list());
         return [=](parser_t &parser) {
+            // Pull out the job list from the function.
+            tnode_t<grammar::job_list> body = props->func_node.child<1>();
             const auto &ld = parser.libdata();
             auto saved_exec_count = ld.exec_count;
             const block_t *fb = function_prepare_environment(parser, *argv, *props);
-            auto res = parser.eval_node(props->parsed_source, props->body_node, lineage);
+            auto res = parser.eval_node(props->parsed_source, body, lineage);
             function_restore_environment(parser, fb);
 
             switch (res) {

src/function.cpp

@@ -221,14 +221,23 @@ void function_remove(const wcstring &name) {
 
 bool function_get_definition(const wcstring &name, wcstring &out_definition) {
     const auto funcset = function_set.acquire();
-    if (const function_info_t *func = funcset->get_info(name)) {
-        auto props = func->props;
-        if (props && props->parsed_source) {
-            out_definition = props->body_node.get_source(props->parsed_source->src);
-        }
-        return true;
+    const function_info_t *func = funcset->get_info(name);
+    if (!func || !func->props) return false;
+    // We want to preserve comments that the AST attaches to the header (#5285).
+    // Take everything from the end of the header to the end of the body.
+    const auto &props = func->props;
+    namespace g = grammar;
+    tnode_t<g::block_header> header = props->func_node.child<0>();
+    tnode_t<g::job_list> jobs = props->func_node.child<1>();
+    auto header_src = header.source_range();
+    auto jobs_src = jobs.source_range();
+    if (header_src && jobs_src) {
+        uint32_t body_start = header_src->start + header_src->length;
+        uint32_t body_end = jobs_src->start + jobs_src->length;
+        assert(body_start <= jobs_src->start && "job list must come after header");
+        out_definition = wcstring(props->parsed_source->src, body_start, body_end - body_start);
     }
-    return false;
+    return true;
 }
 
 bool function_get_desc(const wcstring &name, wcstring &out_desc) {
@@ -304,9 +313,7 @@ int function_get_definition_lineno(const wcstring &name) {
     // return one plus the number of newlines at offsets less than the start of our function's
     // statement (which includes the header).
     // TODO: merge with line_offset_of_character_at_offset?
-    auto block_stat = func->props->body_node.try_get_parent<grammar::block_statement>();
-    assert(block_stat && "Function body is not part of block statement");
-    auto source_range = block_stat.source_range();
+    auto source_range = func->props->func_node.source_range();
     assert(source_range && "Function has no source range");
     uint32_t func_start = source_range->start;
     const wcstring &source = func->props->parsed_source->src;

src/function.h

@@ -20,8 +20,10 @@ struct function_properties_t {
     /// Parsed source containing the function.
     parsed_source_ref_t parsed_source;
 
-    /// Node containing the function body, pointing into parsed_source.
-    tnode_t<grammar::job_list> body_node;
+    /// Node containing the function statement, pointing into parsed_source.
+    /// We store block_statement, not job_list, so that comments attached to the header are
+    /// preserved.
+    tnode_t<grammar::block_statement> func_node;
 
     /// List of all named arguments for this function.
     wcstring_list_t named_arguments;

src/parse_execution.cpp

@@ -331,8 +331,8 @@ eval_result_t parse_execution_context_t::run_begin_statement(tnode_t<g::job_list
 }
 
 // Define a function.
-eval_result_t parse_execution_context_t::run_function_statement(tnode_t<g::function_header> header,
-                                                                tnode_t<g::job_list> body) {
+eval_result_t parse_execution_context_t::run_function_statement(
+    tnode_t<grammar::block_statement> statement, tnode_t<grammar::function_header> header) {
     // Get arguments.
     wcstring_list_t arguments;
     argument_node_list_t arg_nodes = header.descendants<g::argument>();
@@ -343,7 +343,7 @@ eval_result_t parse_execution_context_t::run_function_statement(tnode_t<g::funct
     }
     trace_if_enabled(*parser, L"function", arguments);
     io_streams_t streams(0);  // no limit on the amount of output from builtin_function()
-    int err = builtin_function(*parser, streams, arguments, pstree, body);
+    int err = builtin_function(*parser, streams, arguments, pstree, statement);
     parser->set_last_statuses(statuses_t::just(err));
 
     wcstring errtext = streams.err.contents();
@@ -366,7 +366,7 @@ eval_result_t parse_execution_context_t::run_block_statement(tnode_t<g::block_st
     } else if (auto header = bheader.try_get_child<g::while_header, 0>()) {
         ret = run_while_statement(header, contents, associated_block);
     } else if (auto header = bheader.try_get_child<g::function_header, 0>()) {
-        ret = run_function_statement(header, contents);
+        ret = run_function_statement(statement, header);
     } else if (auto header = bheader.try_get_child<g::begin_header, 0>()) {
         ret = run_begin_statement(contents);
     } else {

src/parse_execution.h

@@ -110,8 +110,8 @@ class parse_execution_context_t {
     eval_result_t run_while_statement(tnode_t<grammar::while_header> header,
                                       tnode_t<grammar::job_list> contents,
                                       const block_t *associated_block);
-    eval_result_t run_function_statement(tnode_t<grammar::function_header> header,
-                                         tnode_t<grammar::job_list> body);
+    eval_result_t run_function_statement(tnode_t<grammar::block_statement> statement,
+                                         tnode_t<grammar::function_header> header);
     eval_result_t run_begin_statement(tnode_t<grammar::job_list> contents);
 
     enum globspec_t { failglob, nullglob };

tests/checks/function-definition.fish

@@ -0,0 +1,35 @@
+#RUN: %fish %s
+
+function stuff --argument a b c
+    # This is a comment
+    echo stuff
+    # This is another comment
+end
+
+functions stuff
+#CHECK: # Defined in {{.*}}
+#CHECK: function stuff --argument a b c
+#CHECK:    # This is a comment
+#CHECK:    echo stuff
+#CHECK:    # This is another comment
+#CHECK: end
+
+function commenting
+
+    # line 2
+
+    # line 4
+
+    echo Bye bye says line 6
+end
+
+functions commenting
+#CHECK: # Defined in {{.*}}
+#CHECK: function commenting
+#CHECK:     
+#CHECK:     # line 2
+#CHECK:     
+#CHECK:     # line 4
+#CHECK:     
+#CHECK:     echo Bye bye says line 6
+#CHECK: end