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

@@ -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).

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.

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;
 

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;

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<for_block_t>();
 
     // 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);
+        int retval = env_set_one(for_var_name, ENV_DEFAULT | 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;
-        }
 
         fb->loop_status = LOOP_NORMAL;
         this->run_job_list(block_contents, fb);

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;

tests/argparse.err

@@ -32,7 +32,7 @@ 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): 
+Standard input (line 41): 
     argparse '#-val' -- abc -x def
     ^
 

tests/argparse.in

@@ -32,28 +32,42 @@ begin
 end
 
 logmsg Invalid \"#-val\" spec
+begin
     argparse '#-val=' -- abc -x def
+end
 
 logmsg Invalid arg in the face of a \"#-val\" spec
+begin
     argparse '#-val' -- abc -x def
+end
 
 logmsg Defining a short flag more than once
+begin
     argparse 's/short' 'x/xray' 's/long' -- -s -x --long
+end
 
 logmsg Defining a long flag more than once
+begin
     argparse 's/short' 'x/xray' 'l/short' -- -s -x --long
+end
 
 logmsg Defining an implicit int flag more than once
+begin
     argparse '#-val' 'x/xray' 'v#val' -- -s -x --long
+end
 
 logmsg Defining an implicit int flag with modifiers
+begin
     argparse 'v#val=' --
+end
 
 ##########
 # Now verify that validly formed invocations work as expected.
 
 logmsg No args
+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
+begin
     argparse '#-val' -- abc -123 def
     set -l
-for v in (set -l -n); set -e $v; end
+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
+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
+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
+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
+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
+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
+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
+begin
     argparse 'x#!_validate_int --min 500' 'v/verbose' -- -v -v -x 499 -v
     set -l
+end
 
 ##########
 # Verify that flag value validation works.

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)
 

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

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