diff --git a/src/builtin_function.cpp b/src/builtin_function.cpp index 9576ce9dc..e865c2a4c 100644 --- a/src/builtin_function.cpp +++ b/src/builtin_function.cpp @@ -200,7 +200,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 wcstring &contents, int definition_line_offset) { + const parsed_source_ref_t &source, tnode_t body) { + 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 // builtins. @@ -257,8 +258,9 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis e.function_name = d.name; } - d.definition = contents.c_str(); - function_add(d, parser, definition_line_offset); + d.parsed_source = source; + d.body_node = body; + function_add(d, parser); // Handle wrap targets by creating the appropriate completions. for (size_t w = 0; w < opts.wrap_targets.size(); w++) { diff --git a/src/builtin_function.h b/src/builtin_function.h index f144924c7..650f7254e 100644 --- a/src/builtin_function.h +++ b/src/builtin_function.h @@ -3,10 +3,11 @@ #define FISH_BUILTIN_FUNCTION_H #include "common.h" +#include "parse_tree.h" class parser_t; struct io_streams_t; int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args, - const wcstring &contents, int definition_line_offset); + const parsed_source_ref_t &source, tnode_t body); #endif diff --git a/src/builtin_functions.cpp b/src/builtin_functions.cpp index a7e6ff3b4..c1142a646 100644 --- a/src/builtin_functions.cpp +++ b/src/builtin_functions.cpp @@ -227,7 +227,7 @@ static int report_function_metadata(const wchar_t *funcname, bool verbose, io_st path = function_get_definition_file(funcname); if (path) { autoloaded = function_is_autoloaded(funcname) ? L"autoloaded" : L"not-autoloaded"; - line_number = function_get_definition_offset(funcname); + line_number = function_get_definition_lineno(funcname); } else { path = L"stdin"; } diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 3ec041cba..408152640 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2166,9 +2166,10 @@ static void test_complete(void) { do_test(completions.at(0).completion == L"space"); // Add a function and test completing it in various ways. + // Note we're depending on function_data_t not complaining when given missing parsed_source / + // body_node. struct function_data_t func_data = {}; func_data.name = L"scuttlebutt"; - func_data.definition = L"echo gongoozle"; function_add(func_data, parser_t::principal_parser()); // Complete a function name. diff --git a/src/function.cpp b/src/function.cpp index 5efadc689..eae881746 100644 --- a/src/function.cpp +++ b/src/function.cpp @@ -10,11 +10,12 @@ #include #include +#include #include #include -#include #include #include +#include #include #include "autoload.h" @@ -117,33 +118,32 @@ static std::map snapshot_vars(const wcstring_list_t &vars) } function_info_t::function_info_t(const function_data_t &data, const wchar_t *filename, - int def_offset, bool autoload) - : definition(data.definition), + bool autoload) + : parsed_source(data.parsed_source), + body_node(data.body_node), description(data.description), definition_file(intern(filename)), - definition_offset(def_offset), named_arguments(data.named_arguments), inherit_vars(snapshot_vars(data.inherit_vars)), is_autoload(autoload), 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) - : definition(data.definition), + bool autoload) + : parsed_source(data.parsed_source), + body_node(data.body_node), description(data.description), definition_file(intern(filename)), - definition_offset(def_offset), named_arguments(data.named_arguments), inherit_vars(data.inherit_vars), is_autoload(autoload), shadow_scope(data.shadow_scope) {} -void function_add(const function_data_t &data, const parser_t &parser, int definition_line_offset) { +void function_add(const function_data_t &data, const parser_t &parser) { UNUSED(parser); ASSERT_IS_MAIN_THREAD(); CHECK(!data.name.empty(), ); //!OCLINT(multiple unary operator) - CHECK(data.definition, ); scoped_rlock locker(functions_lock); // Remove the old function. @@ -152,8 +152,8 @@ void function_add(const function_data_t &data, const parser_t &parser, int defin // Create and store a new function. const wchar_t *filename = reader_current_filename(); - const function_map_t::value_type new_pair( - data.name, function_info_t(data, filename, definition_line_offset, is_autoload)); + const function_map_t::value_type new_pair(data.name, + function_info_t(data, filename, is_autoload)); loaded_functions.insert(new_pair); // Add event handlers. @@ -223,7 +223,7 @@ bool function_get_definition(const wcstring &name, wcstring *out_definition) { scoped_rlock locker(functions_lock); const function_info_t *func = function_get(name); if (func && out_definition) { - out_definition->assign(func->definition); + out_definition->assign(func->body_node.get_source(func->parsed_source->src)); } return func != NULL; } @@ -275,7 +275,7 @@ bool function_copy(const wcstring &name, const wcstring &new_name) { // This new instance of the function shouldn't be tied to the definition file of the // original, so pass NULL filename, etc. const function_map_t::value_type new_pair(new_name, - function_info_t(iter->second, NULL, 0, false)); + function_info_t(iter->second, NULL, false)); loaded_functions.insert(new_pair); result = true; } @@ -311,10 +311,20 @@ bool function_is_autoloaded(const wcstring &name) { return func->is_autoload; } -int function_get_definition_offset(const wcstring &name) { +int function_get_definition_lineno(const wcstring &name) { scoped_rlock locker(functions_lock); const function_info_t *func = function_get(name); - return func ? func->definition_offset : -1; + if (!func) return -1; + // 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->body_node.try_get_parent(); + assert(block_stat && "Function body is not part of block statement"); + auto source_range = block_stat.source_range(); + assert(source_range && "Function has no source range"); + uint32_t func_start = source_range->start; + const wcstring &source = func->parsed_source->src; + assert(func_start <= source.size() && "function start out of bounds"); + return 1 + std::count(source.begin(), source.begin() + func_start, L'\n'); } // Setup the environment for the function. There are three components of the environment: diff --git a/src/function.h b/src/function.h index 514a3e65a..6d50107b9 100644 --- a/src/function.h +++ b/src/function.h @@ -10,6 +10,8 @@ #include "common.h" #include "env.h" #include "event.h" +#include "parse_tree.h" +#include "tnode.h" class parser_t; @@ -21,8 +23,10 @@ struct function_data_t { wcstring name; /// Description of function. wcstring description; - /// Function definition. - const wchar_t *definition; + /// Parsed source containing the function. + parsed_source_ref_t parsed_source; + /// Node containing the function body, pointing into parsed_source. + tnode_t body_node; /// List of all event handlers for this function. std::vector events; /// List of all named arguments for this function. @@ -36,14 +40,14 @@ struct function_data_t { class function_info_t { public: - /// Function definition. - const wcstring definition; + /// Parsed source containing the function. + const parsed_source_ref_t parsed_source; + /// Node containing the function body, pointing into parsed_source. + const tnode_t body_node; /// 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 @@ -55,18 +59,14 @@ class function_info_t { 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); + function_info_t(const function_data_t &data, const wchar_t *filename, bool autoload); /// Used by function_copy. - function_info_t(const function_info_t &data, const wchar_t *filename, int def_offset, - bool autoload); + function_info_t(const function_info_t &data, const wchar_t *filename, bool autoload); }; -/// Add a function. definition_line_offset is the line number of the function's definition within -/// its source file. -void function_add(const function_data_t &data, const parser_t &parser, - int definition_line_offset = 0); +/// Add a function. +void function_add(const function_data_t &data, const parser_t &parser); /// Remove the function with the specified name. void function_remove(const wcstring &name); @@ -112,7 +112,7 @@ const wchar_t *function_get_definition_file(const wcstring &name); /// /// This function does not autoload functions, it will only work on functions that have already been /// defined. -int function_get_definition_offset(const wcstring &name); +int function_get_definition_lineno(const wcstring &name); /// Returns a list of all named arguments of the specified function. wcstring_list_t function_get_named_arguments(const wcstring &name); diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 93cc240a2..a1fc4115c 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -327,7 +327,7 @@ parse_execution_result_t parse_execution_context_t::run_begin_statement( // Define a function. parse_execution_result_t parse_execution_context_t::run_function_statement( - tnode_t header, tnode_t block_end_command) { + tnode_t header, tnode_t body) { // Get arguments. wcstring_list_t arguments; argument_node_list_t arg_nodes = header.descendants(); @@ -337,30 +337,8 @@ parse_execution_result_t parse_execution_context_t::run_function_statement( if (result != parse_execution_success) { return result; } - - // The function definition extends from the end of the header to the function end. It's not - // just the range of the contents because that loses comments - see issue #1710. - assert(block_end_command.has_source()); - auto header_range = header.source_range(); - size_t contents_start = header_range->start + header_range->length; - size_t contents_end = block_end_command.source_range() - ->start; // 1 past the last character in the function definition - assert(contents_end >= contents_start); - - // Swallow whitespace at both ends. - while (contents_start < contents_end && iswspace(pstree->src.at(contents_start))) { - contents_start++; - } - while (contents_start < contents_end && iswspace(pstree->src.at(contents_end - 1))) { - contents_end--; - } - - assert(contents_end >= contents_start); - const wcstring contents_str = - wcstring(pstree->src, contents_start, contents_end - contents_start); - int definition_line_offset = this->line_offset_of_character_at_offset(contents_start); io_streams_t streams(0); // no limit on the amount of output from builtin_function() - int err = builtin_function(*parser, streams, arguments, contents_str, definition_line_offset); + int err = builtin_function(*parser, streams, arguments, pstree, body); proc_set_last_status(err); if (!streams.err.empty()) { @@ -382,8 +360,7 @@ parse_execution_result_t parse_execution_context_t::run_block_statement( } else if (auto header = bheader.try_get_child()) { ret = run_while_statement(header, contents); } else if (auto header = bheader.try_get_child()) { - tnode_t func_end = statement.child<2>(); - ret = run_function_statement(header, func_end); + ret = run_function_statement(header, contents); } 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 24a0989e8..1b7f305d7 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -102,7 +102,7 @@ class parse_execution_context_t { parse_execution_result_t run_while_statement(tnode_t statement, tnode_t contents); parse_execution_result_t run_function_statement(tnode_t header, - tnode_t block_end); + tnode_t body); parse_execution_result_t run_begin_statement(tnode_t contents); enum globspec_t { failglob, nullglob }; diff --git a/src/parser.cpp b/src/parser.cpp index 4b91d5642..6f042f10a 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -489,7 +489,7 @@ int parser_t::get_lineno() const { // If we are executing a function, we have to add in its offset. const wchar_t *function_name = is_function(); if (function_name != NULL) { - lineno += function_get_definition_offset(function_name); + lineno += function_get_definition_lineno(function_name); } } return lineno; diff --git a/src/tnode.h b/src/tnode.h index c3658f606..0aa95dc15 100644 --- a/src/tnode.h +++ b/src/tnode.h @@ -97,12 +97,11 @@ class tnode_t { uint8_t child_count() const { return nodeptr ? nodeptr->child_count : 0; } maybe_t source_range() const { - if (!has_source()) return none(); + if (nodeptr->source_start == NODE_OFFSET_INVALID) return none(); return source_range_t{nodeptr->source_start, nodeptr->source_length}; } wcstring get_source(const wcstring &str) const { - assert(has_source() && "Source missing"); return nodeptr->get_source(str); }