Functions to store nodes

Prior to this fix, functions stored a string representation of their
contents. Switch them to storing a parsed source reference and the
tnode of the contents. This is part of an effort to avoid reparsing
a function's contents every time it executes.

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<grammar::job_list> 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++) {

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<grammar::job_list> body);
 #endif

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";
         }

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.

src/function.cpp

@@ -10,11 +10,12 @@
 #include <stddef.h>
 #include <wchar.h>
 
+#include <algorithm>
 #include <map>
 #include <memory>
-#include <unordered_set>
 #include <string>
 #include <unordered_map>
+#include <unordered_set>
 #include <utility>
 
 #include "autoload.h"
@@ -117,33 +118,32 @@ static std::map<wcstring, env_var_t> 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<grammar::block_statement>();
+    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:

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<grammar::job_list> body_node;
     /// List of all event handlers for this function.
     std::vector<event_t> 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<grammar::job_list> 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);

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<g::function_header> header, tnode_t<g::end_command> block_end_command) {
+    tnode_t<g::function_header> header, tnode_t<g::job_list> body) {
     // Get arguments.
     wcstring_list_t arguments;
     argument_node_list_t arg_nodes = header.descendants<g::argument>();
@@ -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<g::while_header, 0>()) {
         ret = run_while_statement(header, contents);
     } else if (auto header = bheader.try_get_child<g::function_header, 0>()) {
-        tnode_t<g::end_command> 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<g::begin_header, 0>()) {
         ret = run_begin_statement(contents);
     } else {

src/parse_execution.h

@@ -102,7 +102,7 @@ class parse_execution_context_t {
     parse_execution_result_t run_while_statement(tnode_t<grammar::while_header> statement,
                                                  tnode_t<grammar::job_list> contents);
     parse_execution_result_t run_function_statement(tnode_t<grammar::function_header> header,
-                                                    tnode_t<grammar::end_command> block_end);
+                                                    tnode_t<grammar::job_list> body);
     parse_execution_result_t run_begin_statement(tnode_t<grammar::job_list> contents);
 
     enum globspec_t { failglob, nullglob };

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;

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_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);
     }