From 905766fca2df84979a43962468f4de8c62339f56 Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Fri, 8 Sep 2017 21:14:26 -0700 Subject: [PATCH] Hoist `for` loop control var to enclosing scope (#4376) * Hoist `for` loop control var to enclosing scope It should be possible to reference the last value assigned to a `for` loop control var when the loop terminates. This makes it easier to detect if we broke out of the loop among other things. This change makes fish `for` loops behave like most other shells. Fixes #1935 * Remove redundant line --- CHANGELOG.md | 1 + doc_src/for.txt | 17 +++++++- src/env.cpp | 2 + src/env.h | 2 + src/parse_execution.cpp | 29 +++++++++---- src/parse_execution.h | 1 + tests/argparse.err | 6 +-- tests/argparse.in | 91 ++++++++++++++++++++++++++--------------- tests/test1.err | 3 ++ tests/test1.in | 29 +++++++++++++ tests/test1.out | 30 ++++++++++++++ 11 files changed, 166 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5251aed42..01aca162a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ This section is for changes merged to the `major` branch that are not also merge - `.` command no longer exists -- use `source` (#4294). - `read` now requires at least one var name (#4220). - `set x[1] x[2] a b` is no longer valid syntax (#4236). +- For loop control variables are no longer local to the for block (#1935). ## Notable fixes and improvements - `read` has a new `--delimiter` option as a better alternative to the `IFS` variable (#4256). diff --git a/doc_src/for.txt b/doc_src/for.txt index affb58c62..508a3ac2d 100644 --- a/doc_src/for.txt +++ b/doc_src/for.txt @@ -7,7 +7,7 @@ for VARNAME in [VALUES...]; COMMANDS...; end \subsection for-description Description -`for` is a loop construct. It will perform the commands specified by `COMMANDS` multiple times. On each iteration, the local variable specified by `VARNAME` is assigned a new value from `VALUES`. If `VALUES` is empty, `COMMANDS` will not be executed at all. +`for` is a loop construct. It will perform the commands specified by `COMMANDS` multiple times. On each iteration, the local variable specified by `VARNAME` is assigned a new value from `VALUES`. If `VALUES` is empty, `COMMANDS` will not be executed at all. The `VARNAME` is visible when the loop terminates and will contain the last value assigned to it. If `VARNAME` does not already exist it will be set in the local scope. For our purposes if the `for` block is inside a function there must be a local variable with the same name. If the `for` block is not nested inside a function then global and universal variables of the same name will be used if they exist. \subsection for-example Example @@ -19,3 +19,18 @@ foo bar baz \endfish + +\subsection for-notes Notes + +The `VARNAME` was local to the for block in releases prior to 3.0.0. This means that if you did something like this: + +\fish +for var in a b c + if break_from_loop + break + end +end +echo $var +\endfish + +The last value assigned to `var` when the loop terminated would not be available outside the loop. What `echo $var` would write depended on what it was set to before the loop was run. Likely nothing. diff --git a/src/env.cpp b/src/env.cpp index 085631461..a854621f4 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -322,6 +322,8 @@ static bool is_read_only(const wcstring &key) { return env_read_only.find(key) != env_read_only.end(); } +bool env_var_t::read_only() const { return is_read_only(name); } + /// Table of variables whose value is dynamically calculated, such as umask, status, etc. static const_string_set_t env_electric; diff --git a/src/env.h b/src/env.h index b74a0a3f4..552480069 100644 --- a/src/env.h +++ b/src/env.h @@ -95,6 +95,7 @@ class env_var_t { env_var_t() : name(), vals(), exportv(false) {} bool empty(void) const { return vals.empty() || (vals.size() == 1 && vals[0].empty()); }; + bool read_only(void) const; bool matches_string(const wcstring &str) { if (vals.size() > 1) return false; @@ -110,6 +111,7 @@ class env_var_t { void set_vals(wcstring_list_t v) { vals = std::move(v); } env_var_t &operator=(const env_var_t &var) { + this->name = var.name; this->vals = var.vals; this->exportv = var.exportv; return *this; diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 07bde79fc..3a607d153 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -33,6 +33,7 @@ #include "expand.h" #include "function.h" #include "io.h" +#include "maybe.h" #include "parse_constants.h" #include "parse_execution.h" #include "parse_tree.h" @@ -441,6 +442,15 @@ parse_execution_result_t parse_execution_context_t::run_block_statement( return ret; } +/// Return true if the current execution context is within a function block, else false. +bool parse_execution_context_t::is_function_context() const { + const block_t *current = parser->block_at_index(0); + const block_t *parent = parser->block_at_index(1); + bool is_within_function_call = + (current && parent && current->type() == TOP && parent->type() == FUNCTION_CALL); + return is_within_function_call; +} + parse_execution_result_t parse_execution_context_t::run_for_statement( const parse_node_t &header, const parse_node_t &block_contents) { assert(header.type == symbol_for_header); @@ -462,6 +472,17 @@ parse_execution_result_t parse_execution_context_t::run_for_statement( return ret; } + auto var = env_get(for_var_name, ENV_LOCAL); + if (!var && !is_function_context()) var = env_get(for_var_name, ENV_DEFAULT); + if (!var || var->read_only()) { + int retval = env_set_empty(for_var_name, ENV_LOCAL | ENV_USER); + if (retval != ENV_OK) { + report_error(var_name_node, L"You cannot use read-only variable '%ls' in a for loop", + for_var_name.c_str()); + return parse_execution_errored; + } + } + for_block_t *fb = parser->push_block(); // Now drive the for loop. @@ -473,13 +494,7 @@ parse_execution_result_t parse_execution_context_t::run_for_statement( } const wcstring &val = argument_sequence.at(i); - int retval = env_set_one(for_var_name, ENV_LOCAL | ENV_USER, val); - if (retval != ENV_OK) { - report_error(var_name_node, L"You cannot use read-only variable '%ls' in a for loop", - for_var_name.c_str()); - ret = parse_execution_errored; - break; - } + int retval = env_set_one(for_var_name, ENV_DEFAULT | ENV_USER, val); fb->loop_status = LOOP_NORMAL; this->run_job_list(block_contents, fb); diff --git a/src/parse_execution.h b/src/parse_execution.h index 7e6867fd7..194f188b7 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -76,6 +76,7 @@ class parse_execution_context_t { node_offset_t get_offset(const parse_node_t &node) const; const parse_node_t *infinite_recursive_statement_in_job_list(const parse_node_t &job_list, wcstring *out_func_name) const; + bool is_function_context() const; /// Indicates whether a job is a simple block (one block, no redirections). bool job_is_simple_block(const parse_node_t &node) const; diff --git a/tests/argparse.err b/tests/argparse.err index 52550515f..5c8457310 100644 --- a/tests/argparse.err +++ b/tests/argparse.err @@ -32,9 +32,9 @@ argparse: Implicit int short flag '#' does not allow modifiers like '=' #################### # Invalid arg in the face of a "#-val" spec argparse: Unknown option '-x' -Standard input (line 38): -argparse '#-val' -- abc -x def -^ +Standard input (line 41): + argparse '#-val' -- abc -x def + ^ #################### # Defining a short flag more than once diff --git a/tests/argparse.in b/tests/argparse.in index 32a60e3ca..2361605a5 100644 --- a/tests/argparse.in +++ b/tests/argparse.in @@ -32,28 +32,42 @@ begin end logmsg Invalid \"#-val\" spec -argparse '#-val=' -- abc -x def +begin + argparse '#-val=' -- abc -x def +end logmsg Invalid arg in the face of a \"#-val\" spec -argparse '#-val' -- abc -x def +begin + argparse '#-val' -- abc -x def +end logmsg Defining a short flag more than once -argparse 's/short' 'x/xray' 's/long' -- -s -x --long +begin + argparse 's/short' 'x/xray' 's/long' -- -s -x --long +end logmsg Defining a long flag more than once -argparse 's/short' 'x/xray' 'l/short' -- -s -x --long +begin + argparse 's/short' 'x/xray' 'l/short' -- -s -x --long +end logmsg Defining an implicit int flag more than once -argparse '#-val' 'x/xray' 'v#val' -- -s -x --long +begin + argparse '#-val' 'x/xray' 'v#val' -- -s -x --long +end logmsg Defining an implicit int flag with modifiers -argparse 'v#val=' -- +begin + argparse 'v#val=' -- +end ########## # Now verify that validly formed invocations work as expected. logmsg No args -argparse h/help -- +begin + argparse h/help -- +end logmsg One arg and no matching flags begin @@ -80,47 +94,56 @@ begin end logmsg Implicit int flags work -for v in (set -l -n); set -e $v; end -argparse '#-val' -- abc -123 def -set -l -for v in (set -l -n); set -e $v; end -argparse 'v/verbose' '#-val' 't/token=' -- -123 a1 --token woohoo --234 -v a2 --verbose -set -l +begin + argparse '#-val' -- abc -123 def + set -l +end +begin + argparse 'v/verbose' '#-val' 't/token=' -- -123 a1 --token woohoo --234 -v a2 --verbose + set -l +end logmsg Should be set to 987 -for v in (set -l -n); set -e $v; end -argparse 'm#max' -- argle -987 bargle -set -l +begin + argparse 'm#max' -- argle -987 bargle + set -l +end logmsg Should be set to 765 -for v in (set -l -n); set -e $v; end -argparse 'm#max' -- argle -987 bargle --max 765 -set -l +begin + argparse 'm#max' -- argle -987 bargle --max 765 + set -l +end logmsg Bool short flag only -for v in (set -l -n); set -e $v; end -argparse 'C' 'v' -- -C -v arg1 -v arg2 -set -l +begin + argparse 'C' 'v' -- -C -v arg1 -v arg2 + set -l +end logmsg Value taking short flag only -for v in (set -l -n); set -e $v; end -argparse 'x=' 'v/verbose' -- --verbose arg1 -v -x arg2 -set -l +begin + argparse 'x=' 'v/verbose' -- --verbose arg1 -v -x arg2 + set -l +end logmsg Implicit int short flag only -for v in (set -l -n); set -e $v; end -argparse 'x#' 'v/verbose' -- -v -v argle -v -x 321 bargle -set -l +begin + argparse 'x#' 'v/verbose' -- -v -v argle -v -x 321 bargle + set -l +end logmsg Implicit int short flag only with custom validation passes -for v in (set -l -n); set -e $v; end -argparse 'x#!_validate_int --max 500' 'v/verbose' -- -v -v -x 499 -v -set -l +begin + argparse 'x#!_validate_int --max 500' 'v/verbose' -- -v -v -x 499 -v + set -l +end logmsg Implicit int short flag only with custom validation fails -for v in (set -l -n); set -e $v; end -argparse 'x#!_validate_int --min 500' 'v/verbose' -- -v -v -x 499 -v -set -l +begin + argparse 'x#!_validate_int --min 500' 'v/verbose' -- -v -v -x 499 -v + set -l +end ########## # Verify that flag value validation works. diff --git a/tests/test1.err b/tests/test1.err index 0e22043ce..5e9cda705 100644 --- a/tests/test1.err +++ b/tests/test1.err @@ -38,6 +38,9 @@ fish: You cannot use read-only variable 'status' in a for loop for status in a b c ^ +#################### +# For loop control vars available outside the for block + #################### # Comments allowed in between lines (#1987) diff --git a/tests/test1.in b/tests/test1.in index 9c1f08e60..d14c11836 100644 --- a/tests/test1.in +++ b/tests/test1.in @@ -155,6 +155,35 @@ for status in a b c echo $status end +logmsg For loop control vars available outside the for block +begin + set -l loop_var initial-value + for loop_var in a b c + # do nothing + end + set --show loop_var +end + +set -g loop_var global_val +function loop_test + for loop_var in a b c + if test $loop_var = b + break + end + end + set --show loop_var +end +loop_test +set --show loop_var + +begin + set -l loop_var + for loop_var in aa bb cc + end + set --show loop_var +end +set --show loop_var + logmsg 'Comments allowed in between lines (#1987)' echo before comment \ # comment diff --git a/tests/test1.out b/tests/test1.out index ae06e55a6..292238753 100644 --- a/tests/test1.out +++ b/tests/test1.out @@ -90,6 +90,36 @@ Checking for infinite loops in no-execute #################### # For loops with read-only vars is an error (#4342) +#################### +# For loop control vars available outside the for block +$loop_var: set in local scope, unexported, with 1 elements +$loop_var[1]: length=1 value=|c| +$loop_var: not set in global scope +$loop_var: not set in universal scope + +$loop_var: set in local scope, unexported, with 1 elements +$loop_var[1]: length=1 value=|b| +$loop_var: set in global scope, unexported, with 1 elements +$loop_var[1]: length=10 value=|global_val| +$loop_var: not set in universal scope + +$loop_var: not set in local scope +$loop_var: set in global scope, unexported, with 1 elements +$loop_var[1]: length=10 value=|global_val| +$loop_var: not set in universal scope + +$loop_var: set in local scope, unexported, with 1 elements +$loop_var[1]: length=2 value=|cc| +$loop_var: set in global scope, unexported, with 1 elements +$loop_var[1]: length=10 value=|global_val| +$loop_var: not set in universal scope + +$loop_var: not set in local scope +$loop_var: set in global scope, unexported, with 1 elements +$loop_var[1]: length=10 value=|global_val| +$loop_var: not set in universal scope + + #################### # Comments allowed in between lines (#1987) before comment after comment