From 992c864f263dddb04fdb9fcf15b287d7a4eccdf5 Mon Sep 17 00:00:00 2001
From: Johannes Altmanninger <aclopte@gmail.com>
Date: Wed, 8 Jan 2020 09:03:34 +0100
Subject: [PATCH] Don't overwrite unrelated variables with for-loop-variables

for-loops that were not inside a function could overwrite global
and universal variables with the loop variable.  Avoid this by making
for-loop-variables local variables in their enclosing scope.

This means that if someone does:

    set a global
    for a in local; end
    echo $a

The local $a will shadow the global one (but not be visible in child
scopes). Which is surprising, but less dangerous than the previous
behavior.

The detection whether the loop is running inside a function was failing
inside command substitutions. Remove this special handling of functions
alltogether, it's not needed anymore.

Fixes #6480
---
 src/parse_execution.cpp | 31 ++++++++++++-------------------
 src/parse_execution.h   |  1 -
 tests/checks/for.fish   | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 20 deletions(-)
 create mode 100644 tests/checks/for.fish

diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp
index 638044cac..f53e8909d 100644
--- a/src/parse_execution.cpp
+++ b/src/parse_execution.cpp
@@ -376,15 +376,6 @@ eval_result_t parse_execution_context_t::run_block_statement(tnode_t<g::block_st
     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() == block_type_t::top && parent->is_function_call());
-    return is_within_function_call;
-}
-
 eval_result_t parse_execution_context_t::run_for_statement(
     tnode_t<grammar::for_header> header, tnode_t<grammar::job_list> block_contents) {
     // Get the variable name: `for var_name in ...`. We expand the variable name. It better result
@@ -404,17 +395,19 @@ eval_result_t parse_execution_context_t::run_for_statement(
         return ret;
     }
 
-    auto &vars = parser->vars();
-    auto var = vars.get(for_var_name, ENV_LOCAL);
-    if (!var && !is_function_context()) var = vars.get(for_var_name, ENV_DEFAULT);
-    if (!var || var->read_only()) {
-        int retval = parser->vars().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 eval_result_t::error;
-        }
+    auto var = parser->vars().get(for_var_name, ENV_DEFAULT);
+    if (var && var->read_only()) {
+        report_error(var_name_node, L"You cannot use read-only variable '%ls' in a for loop",
+                     for_var_name.c_str());
+        return eval_result_t::error;
     }
+    int retval;
+    if (var) {
+        retval = parser->vars().set(for_var_name, ENV_LOCAL | ENV_USER, var->as_list());
+    } else {
+        retval = parser->vars().set_empty(for_var_name, ENV_LOCAL | ENV_USER);
+    }
+    assert(retval == ENV_OK);
 
     if (!valid_var_name(for_var_name)) {
         report_error(var_name_node, BUILTIN_ERR_VARNAME, L"for", for_var_name.c_str());
diff --git a/src/parse_execution.h b/src/parse_execution.h
index 887c44d42..ea4be2f63 100644
--- a/src/parse_execution.h
+++ b/src/parse_execution.h
@@ -66,7 +66,6 @@ class parse_execution_context_t {
     wcstring get_source(const parse_node_t &node) const;
     tnode_t<grammar::plain_statement> infinite_recursive_statement_in_job_list(
         tnode_t<grammar::job_list> job_list, wcstring *out_func_name) const;
-    bool is_function_context() const;
 
     // Expand a command which may contain variables, producing an expand command and possibly
     // arguments. Prints an error message on error.
diff --git a/tests/checks/for.fish b/tests/checks/for.fish
new file mode 100644
index 000000000..590aefeb1
--- /dev/null
+++ b/tests/checks/for.fish
@@ -0,0 +1,34 @@
+# RUN: %fish %s
+
+# A for-loop-variable is a local variable in the enclosing scope.
+set -g i global
+# implicit set -l i $i
+for i in local
+end
+set -ql i && echo $i
+# CHECK: local
+
+# The loop variable is initialized with any previous value.
+set -g j global
+for j in
+end
+set -ql j && echo $j
+# CHECK: global
+
+# Loop variables exist only locally in the enclosing local scope.
+# They do not modify other local/global/universal variables.
+set -g k global
+begin
+    for k in local1
+        echo $k
+        # CHECK: local1
+        for k in local2
+        end
+        echo $k
+        # CHECK: local2
+    end
+    echo $k
+    # CHECK: local1
+end
+echo $k
+# CHECK: global