Properly handle environments with unset IFS

[sudonym1]

From the bash man page:

Word Splitting
...snip...
The shell treats each character of IFS as a delimiter, and splits the
results of the other expansions into words using these characters as
field terminators.  If IFS is unset, or its value is exactly
<space><tab><newline>, the  default, then  sequences of <space>, <tab>,
and <newline> at the beginning and end of the results of the previous
expansions are ignored, and any sequence of IFS characters not at the
beginning or end serves to delimit words.  If IFS has a value other than
the default, then sequences of the whitespace characters space, tab, and
newline are ignored at the beginning and end of the word, as long as the
whitespace character is in the value of IFS (an IFS whitespace
character).  Any character  in  IFS that is not IFS whitespace, along
with any adjacent IFS whitespace characters, delimits a field.  A
sequence of IFS whitespace characters is also treated as a delimiter.
If the value of IFS is null, no word splitting occurs.

Per this paragraph, IFS can have one of three states:

1. IFS is set to some value, eg $' \t\n'
2. IFS is unset, in which case it behaves like $' \t\n'
3. IFS is null, in this case no splitting occurs.

Prior to this commit, the current state of IFS was stored into a local
variable ifs. Later, IFS is restored by setting IFS=$ifs. If the
user starts with either a set IFS or a null IFS, then this behavior
is correct. If the user starts with an unset IFS, then IFS=$ifs
results in a NULL IFS, which is not the original state. Most of the
time a null IFS is not desired, and indeed it breaks some things
inside bash-completions. eg

unset IFS
local ifs=$IFS IFS=$' \t\n'
restore=$(shopt -po noglob)
IFS=$ifs
$restore

Above the line IFS=$ifs puts us in a NULL IFS state, which is not
where we started. The line $restore fails with a confusing error
message -bash: set -o noglob: command not found.

In addition, this pattern does not make good use of bash shadowing
rules. When a local IFS shadows the global IFS, all that is needed
to restore the global IFS is to unset the local one.

With this commit, all instances of local ifs=$IFS have been removed.

[akinomyoga]

Thank you for the PR! I think we should finally merge this PR. There are two things that should be addressed.

1. unset -v

We are making it explicit whether the unset target is a variable or a function by specifying the option -v or -f. In this case, we want to use unset -v IFS.

2. dynamic unset

unset in the same scope as the variable doesn't recover the original value but just makes it unset state (local unset).

$ function func { declare -p IFS; local IFS=b; declare -p IFS; unset -v IFS; declare -p IFS; }
$ IFS=a; func
declare -- IFS="a"
declare -- IFS="b"
declare -- IFS

In order to remove the local variable of the current scope and to make the variable of the previous context visible (dynamic unset), one needs to let unset find the variable through dynamic scoping, i.e., to call unset in a function:

$ function _comp_unlocal { unset -v "$@"; }
$ function func { declare -p IFS; local IFS=b; declare -p IFS; _comp_unlocal IFS; declare -p IFS; }
$ IFS=a; func
declare -- IFS="a"
declare -- IFS="b"
declare -- IFS="a"

In Bash 5.0+, one additionally needs to care about shopt -s localvar_unset in the above _comp_unlocal. You can add the function _comp_unlocal before the existing function _upvar since they are both basic utilities and utilize the behavior of dynamic unset.

[sudonym1]

Mind blown on that _comp_unlocal function. Let me see if I understand why it works:

1. func declares some local variable.
2. _comp_unlocal inherits the local version of the variable.
3. _comp_unlocal unsets that variable.
4. We leave the scope of _comp_unlocal and bash sees the variable as unset, and so unshadows the global variable.

Is this correct understanding? Is there somewhere in the docs I can read more?

[akinomyoga]

Ah, I think I first should have explained that the variable scoping in Bash is dynamic scoping like Perl's local, Emacs Lisp's let, PostScript's begin, etc. The dynamic scoping is explained in Bash Reference Manual §3.3/¶11:

The shell uses dynamic scoping to control a variable’s visibility within functions. With dynamic scoping, visible variables and their values are a result of the sequence of function calls that caused execution to reach the current function. The value of a variable that a function sees depends on its value within its caller, if any, whether that caller is the "global" scope or another shell function. This is also the value that a local variable declaration "shadows", and the value that is restored when the function returns.

Also, it should be noted that we need to differentiate three different situations for a variable in a specific scope: 1) a value is set, 2) the variable is unset, and 3) the variable does not exist. Case 2 is different from case 1 with an empty value '' and also from case 3 where the variable of the same name of the previous context is visible.

Then the behavior of unset is explained in ¶14 in the same section:

The unset builtin also acts using the same dynamic scope: if a variable is local to the current scope, unset will unset it; otherwise the unset will refer to the variable found in any calling scope as described above. If a variable at the current local scope is unset, it will remain so until it is reset in that scope or until the function returns. Once the function returns, any instance of the variable at a previous scope will become visible. If the unset acts on a variable at a previous scope, any instance of a variable with that name that had been shadowed will become visible.

In short, when the unset just finds the variable in the local scope (i.e., the same scope as the call of unset), unset performs "localvar-unset" which makes the variable of the local scope the unset state (case 2). When the unset finds the variable using dynamic scoping, unset performs "dynamic-unset" which removes the variable (case 3) so that the variable in the second previous scope becomes visible.

[sudonym1]

I misunderstood the key sentence: If the unset acts on a variable at a previous scope, any instance of a variable with that name that had been shadowed will become visible.. Thank you for clarifying.

Looking at the source code, there is more explanation:

  /* If we're unsetting a local variable and we're still executing inside
     the function, just mark the variable as invisible.  The function
     eventually called by pop_var_context() will clean it up later.  This
     must be done so that if the variable is subsequently assigned a new
     value inside the function, the `local' attribute is still present.
     We also need to add it back into the correct hash table. */
  if (old_var && local_p (old_var) &&
	(old_var->context == variable_context || (localvar_unset && old_var->context < variable_context)))
    {
         // keep the variable around
    } else {
        // actually get rid of it

So how stable is this? Would it be better to just handle the unset global IFS case explicitly?

[akinomyoga]

So how stable is this?

AFAIK, the behavior is consistent at least from Bash 3.0 except for the recent addition of shopt -s localvar_unset (Bash 5.0). I have now checked the behavior of Bash 2.* and it seems the behavior doesn't change at least since Bash 2.0 (25 years ago). I haven't checked Bash 1.* since I don't have working binaries of Bash 1.*.

You can find a long discussion at bug-bash/2018-02/#65 bug-bash/2018-03/#0, which introduced localvar_unset and localvar_inherit IIRC.

Would it be better to just handle the unset global IFS case explicitly?

Could you describe in more detail about what you mean by "the unset global IFS case"?

[sudonym1]

Sounds stable enough!

[sudonym1]

Updated. Is there a better place to slap that unit test?

[sudonym1]

Oops. Messed up the version check. Fixed.

[akinomyoga]

Thank you for updating! Could you see the following comments?

[akinomyoga]

Just suggestions of cosmetic changes

[sudonym1]

Ok, I think I have all of the feedback folded in. I held off on the _comp_shopt_restore stuff, as that makes more sense to me as a separate commit.

[akinomyoga]

Thank you! Now it LGTM. Let's run the CI test now.

Can I reopen these two unresolved conversations although they should be addressed in separate PRs? This is just because I wanted to later work on them and would like to keep attention to them. Otherwise, I can possibly forget them.

[akinomyoga]

@sudonym1 It seems the pre-commit hook fails. Can you set pre-commit up and fix the issues? The way to set it up is a bit non-trivial but you can reference the comment at #683 (comment).

[sudonym1]

Oops. Went and fixed the lints before I saw your comment. Will recheck with the tool you mention.

[sudonym1]

Ok, my environment doesn't support docker ATM so I ran the shfmt tool manually. Everything seems to pass now.

[akinomyoga]

OK, Thanks! I rerun the CI test.

[sudonym1]

Can I reopen these two unresolved conversations although they should be addressed in separate PRs?

Of course!

[akinomyoga]

Somehow the test in fedoradev failed but it did pass in the previous time. The difference between the previous time is just formatting made by shfmt 87d1672...f241252, so I don't think that change generated the error. Another error in debian10, true, none in the previous test is caused by the timeout we often see. Now I think this PR is ready to merge.

@scop Do you have any comments?

[scop]

No other comments besides impressive work, thanks! Tests seem flaky at the moment, I've kicked off a re-run, feel free to merge at your discretion whether they fail or not if the failures seem unrelated.

[sudonym1]

is caused by the timeout we often see

Is there a ticket for this? I wouldn't mind taking a look this upcoming weekend.

[akinomyoga]

Thanks, I have merged it.

Is there a ticket for this? I wouldn't mind taking a look this upcoming weekend.

Thank you! I don't remember any. I have quickly searched, but there seems none explicitly mentioning this issue. #136 and #137 seem to be the timeouts caused by the test case itself. I'm not sure whether the cause of #139 is the test case itself or our test framework. There is a report #430 saying that the test fails or is flaky in ARM architecture with high loads but there is no further information. #573 is related to refactoring the test suite. I couldn't find any issue explicitly mentioning the timeout failure.

[scop]

Timeouts are a quite general failure mode in the test suite.

Sometimes they occur when tests fail to process expected/unexpected output or the lack of it, kind of masking the root cause. These are fairly reproducible and straightforward to fix.

Sometimes they occur due to something else, perhaps a command actually stalling every now and then, this would be hard to reproduce and fix.

But the most "interesting" thing to me (and the one I guess we're referring to) here is that I think we may still have something wrong in our test suite that triggers this in unknown situations that is not caused by the things mentioned here. Would be awesome to track these down and fix this category of issues. I believe enabling parallel tests provokes the failures more often -- at least it does provoke some failures more. Parallel running happens by default in local dockerized test runs, and can be enabled on the host without docker e.g. like

$ PYTESTFLAGS="--numprocesses=auto --dist=loadfile" make check

[sudonym1]

It seems I have been getting (un)lucky. I've run the fedora docker container ~10 times with parallel pytests, and no failures so far. I will try a different approach. It could be these issues are related to the performance characteristics of the host computer.

[akinomyoga]

I guess the problem is caused by some race conditions or the synchronization problem of input and output pipes in the communications between pexpect and the Bash process. There is already some sleep that I guess is related.

Actually, I'm not sure if the patterns specified here covers all the possible cases of the Bash outputs. What happens when some garbage outputs remained in the stdout before writing something to stdin (though I'm not even sure whether that can happen because we are checking MAGIC_MARK between every roundtrip)? The accepted patterns in the current testing code are

• /\r\n<PROMPT><COMMAND>.*<MAGIC_MARK>/
• /^<MAGIC_MARK>/
• /^([^\r]+)<MAGIC_MARK>$/
• /^([^\r].*)<MAGIC_MARK>$/

I feel we should just accept [\s\S]*<MAGIC_MARK> and then examine the extracted outputs, or at least we can add another fallback pattern in the above list in order to check what pexpect actually captured.

[akinomyoga]

Maybe we can create a new issue or a Draft PR for this timeout issue of the test suite.