From 31d6abb17720dce4df2d2b54a1d1ca75c203200a Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Thu, 28 Oct 2021 16:28:54 +0200 Subject: [PATCH] Don't fire variable set event before entering a for-loop Since #4376, for-loops would set the loop variable outside, so it stays valid. They did this by doing the equivalent of ```fish set -l foo $foo for foo in 1 2 3 ``` And that first imaginary `set -l` would also fire a set-event. Since there's no use for it and the variable isn't actually set, we remove it. Fixes #8384. --- src/parse_execution.cpp | 7 +++---- tests/checks/for.fish | 11 +++++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 2fae5d4b5..af4cf3c16 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -458,18 +458,17 @@ end_execution_reason_t parse_execution_context_t::run_for_statement( for_var_name.c_str()); } - // We fire the same event over and over again, just construct it once. - event_t evt = event_t::variable_set(for_var_name); auto &vars = parser->vars(); int retval; retval = vars.set(for_var_name, ENV_LOCAL | ENV_USER, var ? var->as_list() : wcstring_list_t{}); assert(retval == ENV_OK); - // TODO: For historical reasons we fire here as well, I'm not sure that makes sense? - event_fire(*parser, evt); trace_if_enabled(*parser, L"for", arguments); block_t *fb = parser->push_block(block_t::for_block()); + // We fire the same event over and over again, just construct it once. + event_t evt = event_t::variable_set(for_var_name); + // Now drive the for loop. for (const wcstring &val : arguments) { if (auto reason = check_end_execution()) { diff --git a/tests/checks/for.fish b/tests/checks/for.fish index 590aefeb1..139ab1ff9 100644 --- a/tests/checks/for.fish +++ b/tests/checks/for.fish @@ -32,3 +32,14 @@ begin end echo $k # CHECK: global + +function foo --on-variable foo + echo foo set +end + +for foo in 1 2 3 + true +end +# CHECK: foo set +# CHECK: foo set +# CHECK: foo set