From e5ef45e4c04b6126ea74c4010b2d8fe1c0b06cca Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Tue, 18 Mar 2014 14:14:32 -0700 Subject: [PATCH] Rewrite parser_t::test_args and parser_t::eval_args to use new parser --- builtin_complete.cpp | 15 +++++-- complete.cpp | 2 +- fish_tests.cpp | 2 +- parse_tree.cpp | 6 +-- parse_tree.h | 2 +- parse_util.cpp | 23 ++++++++-- parser.cpp | 99 +++++++++++++++++++------------------------- parser.h | 9 ++-- 8 files changed, 81 insertions(+), 77 deletions(-) diff --git a/builtin_complete.cpp b/builtin_complete.cpp index 6e395f946..c2fb66376 100644 --- a/builtin_complete.cpp +++ b/builtin_complete.cpp @@ -467,15 +467,22 @@ static int builtin_complete(parser_t &parser, wchar_t **argv) { if (comp && wcslen(comp)) { - if (parser.test_args(comp, 0, 0)) + wcstring prefix; + if (argv[0]) + { + prefix.append(argv[0]); + prefix.append(L": "); + } + + wcstring err_text; + if (parser.detect_errors_in_argument_list(comp, &err_text, prefix.c_str())) { append_format(stderr_buffer, L"%ls: Completion '%ls' contained a syntax error\n", argv[0], comp); - - parser.test_args(comp, &stderr_buffer, argv[0]); - + stderr_buffer.append(err_text); + stderr_buffer.push_back(L'\n'); res = true; } } diff --git a/complete.cpp b/complete.cpp index 648824ecf..89655c76c 100644 --- a/complete.cpp +++ b/complete.cpp @@ -1249,7 +1249,7 @@ void completer_t::complete_from_args(const wcstring &str, if (! is_autosuggest) proc_push_interactive(0); - parser.eval_args(args, possible_comp); + parser.expand_argument_list(args, possible_comp); if (! is_autosuggest) proc_pop_interactive(); diff --git a/fish_tests.cpp b/fish_tests.cpp index 8e7c09ebb..7b2c37732 100644 --- a/fish_tests.cpp +++ b/fish_tests.cpp @@ -682,7 +682,7 @@ static void test_parser() say(L"Testing eval_args"); completion_list_t comps; - parser_t::principal_parser().eval_args(L"alpha 'beta gamma' delta", comps); + parser_t::principal_parser().expand_argument_list(L"alpha 'beta gamma' delta", comps); do_test(comps.size() == 3); do_test(comps.at(0).completion == L"alpha"); do_test(comps.at(1).completion == L"beta gamma"); diff --git a/parse_tree.cpp b/parse_tree.cpp index ae8e42d02..caba268a1 100644 --- a/parse_tree.cpp +++ b/parse_tree.cpp @@ -14,7 +14,7 @@ static bool production_is_empty(const production_t *production) } /** Returns a string description of this parse error */ -wcstring parse_error_t::describe_with_prefix(const wcstring &src, const wcstring &prefix, bool skip_caret) const +wcstring parse_error_t::describe_with_prefix(const wcstring &src, const wcstring &prefix, bool is_interactive, bool skip_caret) const { wcstring result = text; if (! skip_caret && source_start < src.size() && source_start + source_length <= src.size()) @@ -44,7 +44,7 @@ wcstring parse_error_t::describe_with_prefix(const wcstring &src, const wcstring assert(source_start >= line_start); // Don't include the caret and line if we're interactive this is the first line, because then it's obvious - bool skip_caret = (get_is_interactive() && source_start == 0); + bool skip_caret = (is_interactive && source_start == 0); if (! skip_caret) { @@ -91,7 +91,7 @@ wcstring parse_error_t::describe_with_prefix(const wcstring &src, const wcstring wcstring parse_error_t::describe(const wcstring &src) const { - return this->describe_with_prefix(src, wcstring(), false); + return this->describe_with_prefix(src, wcstring(), get_is_interactive(), false); } wcstring parse_errors_description(const parse_error_list_t &errors, const wcstring &src, const wchar_t *prefix) diff --git a/parse_tree.h b/parse_tree.h index f6406be4f..99c3bf299 100644 --- a/parse_tree.h +++ b/parse_tree.h @@ -37,7 +37,7 @@ struct parse_error_t wcstring describe(const wcstring &src) const; /** Return a string describing the error, suitable for presentation to the user, with the given prefix. If skip_caret is false, the offending line with a caret is printed as well */ - wcstring describe_with_prefix(const wcstring &src, const wcstring &prefix, bool skip_caret) const; + wcstring describe_with_prefix(const wcstring &src, const wcstring &prefix, bool is_interactive, bool skip_caret) const; }; typedef std::vector parse_error_list_t; diff --git a/parse_util.cpp b/parse_util.cpp index e7c1873de..dda4f6a75 100644 --- a/parse_util.cpp +++ b/parse_util.cpp @@ -1135,12 +1135,19 @@ parser_test_error_bits_t parse_util_detect_errors_in_argument(const parse_node_t tmp.push_back(INTERNAL_SEPARATOR); tmp.append(paran_end+1); - parse_error_list_t errors; - err |= parse_util_detect_errors(subst, &errors); + parse_error_list_t subst_errors; + err |= parse_util_detect_errors(subst, &subst_errors); + + /* Our command substitution produced error offsets relative to its source. Tweak the offsets of the errors in the command substitution to account for both its offset within the string, and the offset of the node */ + size_t error_offset = (paran_begin + 1 - arg_cpy) + node.source_start; + for (size_t i=0; i < subst_errors.size(); i++) + { + subst_errors.at(i).source_start += error_offset; + } + if (out_errors != NULL) { - /* Todo: have to tweak the offsets of the errors in the command substitution */ - out_errors->insert(out_errors->end(), errors.begin(), errors.end()); + out_errors->insert(out_errors->end(), subst_errors.begin(), subst_errors.end()); } free(arg_cpy); @@ -1239,6 +1246,7 @@ parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, pars // Verify 'or' and 'and' not used inside pipelines // Verify pipes via parser_is_pipe_forbidden // Verify return only within a function + // Verify no variable expansions if (! errored) { @@ -1286,6 +1294,13 @@ parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, pars errored = append_syntax_error(&parse_errors, node, ILLEGAL_CMD_ERR_MSG, command.c_str()); } + // Check that it doesn't contain a variable + // Note this check is clumsy (it doesn't allow for escaping) but it matches what we do in parse_execution + if (command.find(L'$') != wcstring::npos) + { + errored = append_syntax_error(&parse_errors, node, ILLEGAL_CMD_ERR_MSG, command.c_str()); + } + // Check that pipes are sound if (! errored && parser_is_pipe_forbidden(command) && is_in_pipeline) { diff --git a/parser.cpp b/parser.cpp index 872f99fc4..16dcd9710 100644 --- a/parser.cpp +++ b/parser.cpp @@ -547,7 +547,7 @@ void parser_t::print_errors_stderr() } -void parser_t::eval_args(const wcstring &arg_list_src, std::vector &output_arg_list) +void parser_t::expand_argument_list(const wcstring &arg_list_src, std::vector &output_arg_list) { expand_flags_t eflags = 0; if (! show_errors) @@ -828,14 +828,15 @@ wcstring parser_t::current_line() } } - bool skip_caret = get_is_interactive() && ! is_function(); + bool is_interactive = get_is_interactive(); + bool skip_caret = is_interactive && ! is_function(); /* Use an error with empty text */ assert(source_offset >= 0); parse_error_t empty_error = {}; empty_error.source_start = source_offset; - wcstring line_info = empty_error.describe_with_prefix(context->get_source(), prefix, skip_caret); + wcstring line_info = empty_error.describe_with_prefix(context->get_source(), prefix, is_interactive, skip_caret); if (! line_info.empty()) { line_info.push_back(L'\n'); @@ -1206,69 +1207,52 @@ int parser_t::parser_test_argument(const wchar_t *arg, wcstring *out, const wcha } -int parser_t::test_args(const wchar_t * buff, wcstring *out, const wchar_t *prefix) +bool parser_t::detect_errors_in_argument_list(const wcstring &arg_list_src, wcstring *out, const wchar_t *prefix) { - int do_loop = 1; - int err = 0; + bool errored = false; + parse_error_list_t errors; - CHECK(buff, 1); - - tokenizer_t tok(buff, 0); - scoped_push tokenizer_push(¤t_tokenizer, &tok); - scoped_push tokenizer_pos_push(¤t_tokenizer_pos); - - for (; do_loop && tok_has_next(&tok); tok_next(&tok)) + /* Use empty string for the prefix if it's NULL */ + if (prefix == NULL) { - current_tokenizer_pos = tok_get_pos(&tok); - switch (tok_last_type(&tok)) + prefix = L""; + } + + /* Parse the string as an argument list */ + parse_node_tree_t tree; + if (! parse_tree_from_string(arg_list_src, parse_flag_none, &tree, &errors, symbol_argument_list)) + { + /* Failed to parse. */ + errored = true; + } + + if (! errored) + { + /* Get the root argument list */ + assert(! tree.empty()); + const parse_node_t *arg_list = &tree.at(0); + assert(arg_list->type == symbol_argument_list); + + /* Extract arguments from it */ + while (arg_list != NULL && ! errored) { - - case TOK_STRING: + const parse_node_t *arg_node = tree.next_node_in_node_list(*arg_list, symbol_argument, &arg_list); + if (arg_node != NULL) { - err |= parser_test_argument(tok_last(&tok), out, prefix, tok_get_pos(&tok)); - break; - } - - case TOK_END: - { - break; - } - - case TOK_ERROR: - { - if (out) + const wcstring arg_src = arg_node->get_source(arg_list_src); + if (parse_util_detect_errors_in_argument(*arg_node, arg_src, &errors)) { - error(SYNTAX_ERROR, - tok_get_pos(&tok), - TOK_ERR_MSG, - tok_last(&tok)); - print_errors(*out, prefix); + errored = true; } - err=1; - do_loop=0; - break; - } - - default: - { - if (out) - { - error(SYNTAX_ERROR, - tok_get_pos(&tok), - UNEXPECTED_TOKEN_ERR_MSG, - tok_get_desc(tok_last_type(&tok))); - print_errors(*out, prefix); - } - err=1; - do_loop=0; - break; } } } - error_code=0; - - return err; + if (! errors.empty() && out != NULL) + { + out->assign(errors.at(0).describe_with_prefix(arg_list_src, prefix, false /* not interactive */, false /* don't skip caret */)); + } + return errored; } // helper type used in parser::test below @@ -1290,7 +1274,8 @@ void parser_t::get_backtrace(const wcstring &src, const parse_error_list_t &erro size_t which_line = 1 + std::count(src.begin(), src.begin() + err.source_start, L'\n'); // Don't include the caret if we're interactive, this is the first line of text, and our source is at its beginning, because then it's obvious - bool skip_caret = (get_is_interactive() && which_line == 1 && err.source_start == 0); + bool is_interactive = get_is_interactive(); + bool skip_caret = (is_interactive && which_line == 1 && err.source_start == 0); wcstring prefix; @@ -1304,7 +1289,7 @@ void parser_t::get_backtrace(const wcstring &src, const parse_error_list_t &erro prefix = L"fish: "; } - const wcstring description = err.describe_with_prefix(src, prefix, skip_caret); + const wcstring description = err.describe_with_prefix(src, prefix, is_interactive, skip_caret); if (! description.empty()) { output->append(description); diff --git a/parser.h b/parser.h index 3e4a431f3..15de1e98d 100644 --- a/parser.h +++ b/parser.h @@ -345,7 +345,7 @@ public: \param arg_src String to evaluate as an argument list \param output List to insert output into */ - void eval_args(const wcstring &arg_src, std::vector &output); + void expand_argument_list(const wcstring &arg_src, std::vector &output); /** Sets the current evaluation error. This function should only be used by libraries that are called by @@ -438,12 +438,9 @@ public: void get_backtrace(const wcstring &src, const parse_error_list_t &errors, wcstring *output) const; /** - Test if the specified string can be parsed as an argument list, - e.g. sent to eval_args. The result has the first bit set if the - string contains errors, and the second bit is set if the string - contains an unclosed block. + Detect errors in the specified string when parsed as an argument list. Returns true if an error occurred. */ - int test_args(const wchar_t * buff, wcstring *out, const wchar_t *prefix); + bool detect_errors_in_argument_list(const wcstring &arg_list_src, wcstring *out_err, const wchar_t *prefix); /** Tell the parser that the specified function may not be run if not