diff --git a/CHANGELOG.md b/CHANGELOG.md index a205f2895..fef28d3df 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index b243ef466..07bde79fc 100644 --- a/src/parse_execution.cpp +++ b/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; } diff --git a/tests/test1.err b/tests/test1.err index fff85a472..0e22043ce 100644 --- a/tests/test1.err +++ b/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) diff --git a/tests/test1.in b/tests/test1.in index b002b48f1..9c1f08e60 100644 --- a/tests/test1.in +++ b/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 diff --git a/tests/test1.out b/tests/test1.out index 64a5dbc30..ae06e55a6 100644 --- a/tests/test1.out +++ b/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