From 0bda853dc73c937d0731180df747d3721da48b79 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Thu, 28 Mar 2019 19:55:23 -0500 Subject: [PATCH 1/3] Add detection of `eval` to the parser While `eval` is still a function, this paves the way for changing that in the future, and lets the proc/exec functions detect when an eval is used to allow/disallow certain behaviors and optimizations. --- src/complete.cpp | 8 ++++++++ src/exec.cpp | 1 + src/parse_constants.h | 9 ++++++--- src/parse_grammar.h | 2 ++ src/parse_productions.cpp | 4 ++++ src/proc.h | 2 ++ 6 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/complete.cpp b/src/complete.cpp index d4091855d..c892f4482 100644 --- a/src/complete.cpp +++ b/src/complete.cpp @@ -1465,6 +1465,14 @@ void completer_t::perform() { use_abbr = false; break; } + case parse_statement_decoration_eval: { + use_command = true; + use_function = true; + use_builtin = true; + use_implicit_cd = false; + use_abbr = false; + break; + } } if (cmd_node.location_in_or_at_end_of_source_range(pos)) { diff --git a/src/exec.cpp b/src/exec.cpp index be1eec6b6..a6441798e 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -944,6 +944,7 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr< // Execute the process. p->check_generations_before_launch(); switch (p->type) { + case process_type_t::eval: /* so long as `eval` is a function */ case process_type_t::function: case process_type_t::block_node: { // Allow buffering unless this is a deferred run. If deferred, then processes after us diff --git a/src/parse_constants.h b/src/parse_constants.h index 53eda65c4..2f86f05fd 100644 --- a/src/parse_constants.h +++ b/src/parse_constants.h @@ -97,7 +97,6 @@ const enum_map token_enum_map[] = { // // IMPORTANT: These enums must start at zero. enum parse_keyword_t { - parse_keyword_none = 0, parse_keyword_and, parse_keyword_begin, parse_keyword_builtin, @@ -105,13 +104,15 @@ enum parse_keyword_t { parse_keyword_command, parse_keyword_else, parse_keyword_end, + parse_keyword_eval, + parse_keyword_exclam, parse_keyword_exec, parse_keyword_for, parse_keyword_function, parse_keyword_if, parse_keyword_in, + parse_keyword_none, parse_keyword_not, - parse_keyword_exclam, parse_keyword_or, parse_keyword_switch, parse_keyword_while, @@ -125,6 +126,7 @@ const enum_map keyword_enum_map[] = {{parse_keyword_exclam, L"! {parse_keyword_command, L"command"}, {parse_keyword_else, L"else"}, {parse_keyword_end, L"end"}, + {parse_keyword_eval, L"eval"}, {parse_keyword_exec, L"exec"}, {parse_keyword_for, L"for"}, {parse_keyword_function, L"function"}, @@ -144,7 +146,8 @@ enum parse_statement_decoration_t { parse_statement_decoration_none, parse_statement_decoration_command, parse_statement_decoration_builtin, - parse_statement_decoration_exec + parse_statement_decoration_exec, + parse_statement_decoration_eval, }; // Boolean statement types, stored in node tag. diff --git a/src/parse_grammar.h b/src/parse_grammar.h index b6b1cb2ee..03e163ae1 100644 --- a/src/parse_grammar.h +++ b/src/parse_grammar.h @@ -334,6 +334,8 @@ DEF_ALT(decorated_statement) { using cmds = seq, plain_statement>; using builtins = seq, plain_statement>; using execs = seq, plain_statement>; + // using evals = seq, plain_statement>; + using evals = single; /* so long as `eval` is a function */ ALT_BODY(decorated_statement, plains, cmds, builtins, execs); }; diff --git a/src/parse_productions.cpp b/src/parse_productions.cpp index 4718d826a..2e60f3b46 100644 --- a/src/parse_productions.cpp +++ b/src/parse_productions.cpp @@ -314,6 +314,10 @@ RESOLVE(decorated_statement) { *out_tag = parse_statement_decoration_exec; return production_for(); } + case parse_keyword_eval: { + *out_tag = parse_statement_decoration_eval; + return production_for(); + } default: { *out_tag = parse_statement_decoration_none; return production_for(); diff --git a/src/proc.h b/src/proc.h index c39e1a123..abca58465 100644 --- a/src/proc.h +++ b/src/proc.h @@ -36,6 +36,8 @@ enum class process_type_t { block_node, /// The exec builtin. exec, + /// A literal evaluation + eval, }; enum class job_control_t { From 2fe2169065c1146d495830bc4748ecca68094ba0 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Wed, 10 Apr 2019 22:38:19 -0500 Subject: [PATCH 2/3] Make `eval` a decorator `eval` has always been implemented as a function, which was always a bit of a hack that caused some issues such as triggering the creation of a new scope. This turns `eval` into a decorator. The scoping issues with eval prevented it from being usable to actually implement other shell components in fish script, such as the problems described in #4442, which should now no longer be the case. Closes #4443. --- share/functions/eval.fish | 70 +-------------------------------------- src/complete.cpp | 4 +-- src/exec.cpp | 26 ++++++++++++++- src/parse_execution.cpp | 38 ++++++++++++--------- src/parse_grammar.h | 13 ++++++-- src/proc.cpp | 3 ++ 6 files changed, 64 insertions(+), 90 deletions(-) diff --git a/share/functions/eval.fish b/share/functions/eval.fish index 20910e944..5798655fc 100644 --- a/share/functions/eval.fish +++ b/share/functions/eval.fish @@ -1,71 +1,3 @@ -function eval -S -d "Evaluate parameters as a command" - # keep a copy of the previous $status and use restore_status - # to preserve the status in case the block that is evaluated - # does not modify the status itself. - set -l status_copy $status - function __fish_restore_status - return $argv[1] - end +function eval - if not set -q argv[2] - # like most builtins, we only check for -h/--help - # if we only have a single argument - switch "$argv[1]" - case -h --help - __fish_print_help eval - return 0 - end - end - - if not string length -q -- $argv - # If the argument is empty, eval should return 0 for compatibility with other shells. - # See #5692. - return 0 - end - - # If we are in an interactive shell, eval should enable full - # job control since it should behave like the real code was - # executed. If we don't do this, commands that expect to be - # used interactively, like less, wont work using eval. - - set -l mode - if status --is-interactive-job-control - set mode interactive - else - if status --is-full-job-control - set mode full - else - set mode none - end - end - if status --is-interactive - status --job-control full - end - __fish_restore_status $status_copy - - # To eval 'foo', we construct a block "begin ; foo; end <&3 3<&-" - # Note the redirections are also within the quotes. - # - # We then pipe this to 'source 3<&0’. - # - # You might expect that the dup2(3, stdin) should overwrite stdin, - # and therefore prevent 'source' from reading the piped-in block. This doesn't happen - # because when you pipe to a builtin, we don't overwrite stdin with the read end - # of the block; instead we set a separate fd in a variable 'builtin_stdin', which is - # what it reads from. So builtins are magic in that, in pipes, their stdin - # is not fd 0. - # - # ‘source’ does not apply the redirections to itself. Instead it saves them and passes - # them as block-level redirections to parser.eval(). Ultimately the eval’d code sees - # the following redirections (in the following order): - # dup2 0 -> 3 - # dup2 pipe -> 0 - # dup2 3 -> 0 - # where the pipe is the pipe we get from piping ‘echo’ to ‘source’. Thus the redirection - # effectively makes stdin fd0, instead of the thing that was piped to ‘source’ - echo "begin; $argv "\n" ;end <&3 3<&-" | source 3<&0 - set -l res $status - - status --job-control $mode - return $res end diff --git a/src/complete.cpp b/src/complete.cpp index c892f4482..ddec89ca2 100644 --- a/src/complete.cpp +++ b/src/complete.cpp @@ -1469,8 +1469,8 @@ void completer_t::perform() { use_command = true; use_function = true; use_builtin = true; - use_implicit_cd = false; - use_abbr = false; + use_implicit_cd = true; + use_abbr = true; break; } } diff --git a/src/exec.cpp b/src/exec.cpp index a6441798e..3ff9a5d74 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -6,6 +6,7 @@ #include #include +#include #ifdef HAVE_SIGINFO_H #include #endif @@ -903,6 +904,7 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr< // The IO chain for this process. io_chain_t process_net_io_chain = j->block_io_chain(); + auto cached_status = proc_get_last_status(); if (pipes.write.valid()) { process_net_io_chain.push_back(std::make_shared( @@ -944,7 +946,6 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr< // Execute the process. p->check_generations_before_launch(); switch (p->type) { - case process_type_t::eval: /* so long as `eval` is a function */ case process_type_t::function: case process_type_t::block_node: { // Allow buffering unless this is a deferred run. If deferred, then processes after us @@ -982,6 +983,29 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr< "Aborting."); break; } + + case process_type_t::eval: { + // int eval(const wcstring &cmd, const io_chain_t &io, enum block_type_t block_type); + bool has_args = false; + wcstring new_cmd; + for (const wchar_t * const* arg = p->get_argv() + 1; *arg != nullptr; ++arg) { + has_args = true; + new_cmd += L' '; + new_cmd += *arg; + } + + // `eval` is not supposed to error or do anything at all if no arguments are provided, + // or if it is used to execute a function that wouldn't have changed the status code + // (e.g. an empty function) if it were executed normally. + j->processes[0]->completed = true; + p->status = proc_status_t::from_exit_code(cached_status); + + if (has_args) { + parser.eval(new_cmd.c_str(), process_net_io_chain, block_type_t::TOP); + p->status = proc_status_t::from_exit_code(proc_get_last_status()); + } + break; + } } return true; } diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 06ed5bf00..51bf0f3cf 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -151,22 +151,30 @@ process_type_t parse_execution_context_t::process_type_for_command( // etc). enum parse_statement_decoration_t decoration = get_decoration(statement); - if (decoration == parse_statement_decoration_exec) { - // Always exec. - process_type = process_type_t::exec; - } else if (decoration == parse_statement_decoration_command) { - // Always a command. - process_type = process_type_t::external; - } else if (decoration == parse_statement_decoration_builtin) { - // What happens if this builtin is not valid? - process_type = process_type_t::builtin; - } else if (function_exists(cmd)) { - process_type = process_type_t::function; - } else if (builtin_exists(cmd)) { - process_type = process_type_t::builtin; - } else { - process_type = process_type_t::external; + switch (decoration) { + case parse_statement_decoration_exec: + process_type = process_type_t::exec; + break; + case parse_statement_decoration_command: + process_type = process_type_t::external; + break; + case parse_statement_decoration_builtin: + process_type = process_type_t::builtin; + break; + case parse_statement_decoration_eval: + process_type = process_type_t::eval; + break; + case parse_statement_decoration_none: + if (function_exists(cmd)) { + process_type = process_type_t::function; + } else if (builtin_exists(cmd)) { + process_type = process_type_t::builtin; + } else { + process_type = process_type_t::external; + } + break; } + return process_type; } diff --git a/src/parse_grammar.h b/src/parse_grammar.h index 03e163ae1..5a3c5f0ce 100644 --- a/src/parse_grammar.h +++ b/src/parse_grammar.h @@ -334,9 +334,16 @@ DEF_ALT(decorated_statement) { using cmds = seq, plain_statement>; using builtins = seq, plain_statement>; using execs = seq, plain_statement>; - // using evals = seq, plain_statement>; - using evals = single; /* so long as `eval` is a function */ - ALT_BODY(decorated_statement, plains, cmds, builtins, execs); + // Ideally, `evals` should be defined as `seq, + // arguments_or_redirections_list`, but other parts of the code have the logic hard coded to + // search for a process at the head of a statement, and bug out if we do that. + // We also can't define `evals` as a `seq, plain_statement>` because + // `expand.cpp` hard-codes its "command substitution at the head of a statement is not allowed" + // check without any way of telling it to perform the substitution anyway. Our solution is to + // create an empty function called `eval` that never actually gets executed and convert a + // decorated statement `eval ...` into a plain statement `eval ...` + using evals = seq; + ALT_BODY(decorated_statement, plains, cmds, builtins, execs, evals); }; DEF(plain_statement) diff --git a/src/proc.cpp b/src/proc.cpp index c4777764b..4d64bbaa2 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -477,6 +477,9 @@ static bool process_clean_after_marking(bool allow_interactive) { static std::vector> erase_list; const bool only_one_job = jobs().size() == 1; for (const auto &j : jobs()) { + if (!j->is_constructed()) { + continue; + } // If we are reaping only jobs who do not need status messages sent to the console, do not // consider reaping jobs that need status messages. if ((!j->get_flag(job_flag_t::SKIP_NOTIFICATION)) && (!interactive) && From da20d197b4b6eacb3a47bf119c163887380d98c3 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Thu, 11 Apr 2019 00:37:22 -0500 Subject: [PATCH 3/3] Add regression test for eval scope (#4443) --- tests/eval_scope.err | 0 tests/eval_scope.in | 3 +++ tests/eval_scope.out | 1 + 3 files changed, 4 insertions(+) create mode 100644 tests/eval_scope.err create mode 100644 tests/eval_scope.in create mode 100644 tests/eval_scope.out diff --git a/tests/eval_scope.err b/tests/eval_scope.err new file mode 100644 index 000000000..e69de29bb diff --git a/tests/eval_scope.in b/tests/eval_scope.in new file mode 100644 index 000000000..b5b89b7c0 --- /dev/null +++ b/tests/eval_scope.in @@ -0,0 +1,3 @@ +# Regression test for issue #4443 +eval set -l previously_undefined foo +echo $previously_undefined diff --git a/tests/eval_scope.out b/tests/eval_scope.out new file mode 100644 index 000000000..257cc5642 --- /dev/null +++ b/tests/eval_scope.out @@ -0,0 +1 @@ +foo