Report error when using read-only var in for loop

Using a read-only variable like `status` as a for loop control variable
has never worked. But without this change you simply get non-sensical
behavior that leaves you scratching your head in puzzlement. This change
replaces the non-sensical behavior with an explicit error message.

Fixes #4342

CHANGELOG.md

@@ -17,6 +17,7 @@ This section is for changes merged to the `major` branch that are not also merge
 - Local exported (`set -lx`) vars are now visible to functions (#1091).
 - `abbr` has been reimplemented to be faster. This means the old `fish_user_abbreviations` variable is ignored (#4048).
 - Setting variables is much faster meaning fish is much faster (#4200, #4341).
+- Using a read-only variable in a for loop is now an error. Note that this never worked. It simply failed to set the for loop var and thus silently produced incorrect results (#4342).
 
 ## Other significant changes
 - Command substitution output is now limited to 10 MB by default (#3822).

src/parse_execution.cpp

@@ -473,11 +473,15 @@ parse_execution_result_t parse_execution_context_t::run_for_statement(
         }
 
         const wcstring &val = argument_sequence.at(i);
-        // This is wrong. It should or in ENV_USER and test if ENV_PERM is returned.
-        // TODO: Fix this so it correctly handles read-only vars.
-        env_set_one(for_var_name, ENV_LOCAL, val);
-        fb->loop_status = LOOP_NORMAL;
+        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;
+        }
 
+        fb->loop_status = LOOP_NORMAL;
         this->run_job_list(block_contents, fb);
 
         if (this->cancellation_reason(fb) == execution_cancellation_loop_control) {
@@ -493,7 +497,6 @@ parse_execution_result_t parse_execution_context_t::run_for_statement(
     }
 
     parser->pop_block(fb);
-
     return ret;
 }
 

tests/test1.err

@@ -32,6 +32,12 @@
 ####################
 # Make sure while loops don't run forever with no-exec (#1543)
 
+####################
+# For loops with read-only vars is an error (#4342)
+fish: You cannot use read-only variable 'status' in a for loop
+for status in a b c
+    ^
+
 ####################
 # Comments allowed in between lines (#1987)
 

tests/test1.in

@@ -146,11 +146,15 @@ echo "/bin/echo pipe 10 <&10 10<&-" | source 10<&0
 echo "/bin/echo pipe 11 <&11 11<&-" | source 11<&0
 echo "/bin/echo pipe 12 <&12 12<&-" | source 12<&0
 
-
 logmsg "Make sure while loops don't run forever with no-exec (#1543)"
 echo "Checking for infinite loops in no-execute"
 echo "while true; end" | ../test/root/bin/fish --no-execute
 
+logmsg "For loops with read-only vars is an error (#4342)"
+for status in a b c
+  echo $status
+end
+
 logmsg 'Comments allowed in between lines (#1987)'
 echo before comment \
   # comment

tests/test1.out

@@ -87,6 +87,9 @@ pipe 12
 # Make sure while loops don't run forever with no-exec (#1543)
 Checking for infinite loops in no-execute
 
+####################
+# For loops with read-only vars is an error (#4342)
+
 ####################
 # Comments allowed in between lines (#1987)
 before comment after comment