From 62302ee17242ccc19b343874dce0f52f5233d976 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Fri, 3 Jan 2020 14:40:28 -0800 Subject: [PATCH] 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 | 5 ++-- src/builtin_function.h | 2 +- src/builtin_functions.cpp | 5 ++-- src/exec.cpp | 4 ++- src/function.cpp | 27 +++++++++++++-------- src/function.h | 6 +++-- src/parse_execution.cpp | 8 +++--- src/parse_execution.h | 4 +-- tests/checks/function-definition.fish | 35 +++++++++++++++++++++++++++ 9 files changed, 71 insertions(+), 25 deletions(-) create mode 100644 tests/checks/function-definition.fish diff --git a/src/builtin_function.cpp b/src/builtin_function.cpp index b1a6655f5..cc4bf816c 100644 --- a/src/builtin_function.cpp +++ b/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 body) { + const parsed_source_ref_t &source, + tnode_t 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) { diff --git a/src/builtin_function.h b/src/builtin_function.h index 650f7254e..be69f8485 100644 --- a/src/builtin_function.h +++ b/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 body); + const parsed_source_ref_t &source, tnode_t body); #endif diff --git a/src/builtin_functions.cpp b/src/builtin_functions.cpp index f189bd3e2..c3f5404c0 100644 --- a/src/builtin_functions.cpp +++ b/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)) { diff --git a/src/exec.cpp b/src/exec.cpp index 351a67a18..523cc87dd 100644 --- a/src/exec.cpp +++ b/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 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) { diff --git a/src/function.cpp b/src/function.cpp index 283d79d89..56e5fb2c5 100644 --- a/src/function.cpp +++ b/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 header = props->func_node.child<0>(); + tnode_t 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(); - 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; diff --git a/src/function.h b/src/function.h index b777f7539..3f612efab 100644 --- a/src/function.h +++ b/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 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 func_node; /// List of all named arguments for this function. wcstring_list_t named_arguments; diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 6fe1b8409..638044cac 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -331,8 +331,8 @@ eval_result_t parse_execution_context_t::run_begin_statement(tnode_t header, - tnode_t body) { +eval_result_t parse_execution_context_t::run_function_statement( + tnode_t statement, tnode_t header) { // Get arguments. wcstring_list_t arguments; argument_node_list_t arg_nodes = header.descendants(); @@ -343,7 +343,7 @@ eval_result_t parse_execution_context_t::run_function_statement(tnode_tset_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()) { ret = run_while_statement(header, contents, associated_block); } else if (auto header = bheader.try_get_child()) { - ret = run_function_statement(header, contents); + ret = run_function_statement(statement, header); } else if (auto header = bheader.try_get_child()) { ret = run_begin_statement(contents); } else { diff --git a/src/parse_execution.h b/src/parse_execution.h index 52a8ff2f1..887c44d42 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -110,8 +110,8 @@ class parse_execution_context_t { eval_result_t run_while_statement(tnode_t header, tnode_t contents, const block_t *associated_block); - eval_result_t run_function_statement(tnode_t header, - tnode_t body); + eval_result_t run_function_statement(tnode_t statement, + tnode_t header); eval_result_t run_begin_statement(tnode_t contents); enum globspec_t { failglob, nullglob }; diff --git a/tests/checks/function-definition.fish b/tests/checks/function-definition.fish new file mode 100644 index 000000000..9ee933f05 --- /dev/null +++ b/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