Regression in braces completion

Regression in braces completion

[Vin Shelton]

I just noticed a problem with completion using braces that has
apparently been around since at least June.  Here's the recipe to
reproduce:

zsh -f
$ touch abcdef abcdefg abcghi
$ ls abc{d<TAB>  ==>
$ ls abc{def

as it should.

autoload -U compinit
compinit

$ ls abc{d<TAB>  ==>
$ ls abcdef{

which is not correct.

This is under cygwin on windows.  I can reproduce this problem with a
zsh built on 2008-06-17, but a zsh built on 2008-01-02 does not
exhibit this problem.

Let me know if you need any more info to reproduce this problem.

  - Vin

Re: Regression in braces completion

[Bart Schaefer]

On Oct 11, 10:32pm, Vin Shelton wrote:
}
} $ ls abc{d<TAB>  ==>
} $ ls abcdef{
} 
} which is not correct.

Hmm.  Looking at _complete_debug output, "abc{d" has already become
"abcdef" before the first completer is even called.  In fact, it has
become that before _setup is even called.  Here we are, barely into
_main_complete:

: _main_complete:49:if; [[ automenu-unambiguous == tab* ]]
: _main_complete:59:if; [[ -z '' ]]
: _main_complete:60:then if; [[ -o equals ]]
: _main_complete:60:then if cmdand; compset -P 1 '='
: _main_complete:62:then elif; [[ abcdef != */* && a == \~ ]]
: _main_complete:70; _setup default

That's $PREFIX on line 62 and the curly brace is already missing.

Re: Regression in braces completion

[Bart Schaefer]

On Oct 11,  9:32pm, Bart Schaefer wrote:
}
} On Oct 11, 10:32pm, Vin Shelton wrote:
} }
} } $ ls abc{d<TAB>  ==>
} } $ ls abcdef{
} } 
} } which is not correct.
} 
} : _main_complete:62:then elif; [[ abcdef != */* && a == \~ ]]
} 
} That's $PREFIX on line 62 and the curly brace is already missing.

Turns out that's a red herring, it was that way in the older version
as well.

As nearly as I can track it down, the problem is with:

	* 25130 slightly tweaked for typos:
	Completion/Unix/Type/_path_files: changes to use -U flag
	to compadd so that spelling corrections in non-final
	path segments are accepted.

If I remove the -U option from all the calls to compadd in _path_files,
the old behavior is restored.

(That took *way* longer to track down than it should have, because I
didn't realize just how *many* calls to "compadd -U" there are, and I
missed the crucial one the first time and ended up chasing a different
red herring for an hour.  Oy.)

Removing the -U *only* from the compadd on line 634 of _path_files is
sufficient to fix this particular example, but I wonder if it should be
removed from all three of the compadd calls in the "we are listing, or
something" branch (see comment line 605), or if it needs to be omitted
based on some other condition ... or if we need to know when spelling
correction has been done so we can only add -U when we have to, or some
even more complicated set of conditions.

Re: Regression in braces completion

[Peter Stephenson]

On Sun, 12 Oct 2008 00:10:51 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> (That took *way* longer to track down than it should have, because I
> didn't realize just how *many* calls to "compadd -U" there are, and I
> missed the crucial one the first time and ended up chasing a different
> red herring for an hour.  Oy.)

I don't know what they're all doing, either.

> Removing the -U *only* from the compadd on line 634 of _path_files is
> sufficient to fix this particular example, but I wonder if it should be
> removed from all three of the compadd calls in the "we are listing, or
> something" branch (see comment line 605), or if it needs to be omitted
> based on some other condition ... or if we need to know when spelling
> correction has been done so we can only add -U when we have to, or some
> even more complicated set of conditions.

There will be an underlying reason why this particular case inserts the
brace in the wrong place.  It will take many hours of work to find out.

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: Regression in braces completion

[Bart Schaefer]

On Oct 12,  8:42pm, Peter Stephenson wrote:
}
} There will be an underlying reason why this particular case inserts the
} brace in the wrong place.  It will take many hours of work to find out.

It's not really "this particular case".  The brace ends up in the
wrong place in the following examples too:

schaefer<508> ls abc{g,d<TAB>
schaefer<508> ls abcdef{g,

schaefer<516> cd ..
schaefer<517> ls dta/abc{d<TAB>
schaefer<517> ls dta/abcdef{

Seems to happen any time there's a longer unambiguous prefix possible.

In order to find the possible completions of the rightmost expansion
of the brace expression, the C code removes everything between the
brace and the comma, inclusive, and then asks the completer (which
happens to be _path_files) for the matches.  It's not expecting the
completer to modify the command line as a side-effect, but that's
what happens when "compadd -U" is called.

Is there a reason you think it's deeper than that?

This one is interesting, though:

schaefer<518> ls dat/abc{d<TAB>
schaefer<518> ls dta/abc{def/
Completing corrections
abcdef/   abcdefg/
Completing original
dat/abcd

Note that it's wrong about what the "original" string is, and sort of
wrong about what the corrections are, because really the correction is
"dat" --> "dta" ... but in this case the brace is in the right place,
and menu completion cycles through:

schaefer<518> ls dta/abc{def/
schaefer<518> ls dta/abc{defg/
schaefer<518> ls dat/abc{d

Looking at _complete_debug output, this seems in part to be because
with the misspelt "dat" in front, "dta/abcghi" remains a candidate
match until later in the process.

It helps, when testing this stuff, to remove _expand and _oldlist
from the completer style.

-- 

Re: Regression in braces completion

[Peter Stephenson]

On Sun, 12 Oct 2008 15:47:19 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> In order to find the possible completions of the rightmost expansion
> of the brace expression, the C code removes everything between the
> brace and the comma, inclusive, and then asks the completer (which
> happens to be _path_files) for the matches.  It's not expecting the
> completer to modify the command line as a side-effect, but that's
> what happens when "compadd -U" is called.

No, that's not how "compadd -U" works.  It simply changes the rules of how
completions added by the command are treated.  The actual modification of
the command line happens later in any case.  It's very roughly like this:

- completion system kicks off a set of functions
- somewhere inside those compadd is called
- within the C code some internal variables are updated to add lists of
things grouped together in horribly obscure ways, storing the fact that
"-U" was present
- eventually the completion function exits
- the code in compresult.c uses the previous results to decide how to
manipulate the command line.

As the compresult code is called a lot later than the "compadd" and there's
no clear grouping of variables used by it, it's extremely difficult to pin
changes in the result onto changes in the command that added the
completion, particularly since the whole thing is sensitive (via what's
visible as compstate etc.) to context (although different context shouldn't
be an issue here).  One of the things that's taken account of at various
places along the line, including in the compresult code that adds ambiguous
or unambiguous completions, is whether -U was present (except that it's
called something entirely different internally that I've forgotten).

It's certainly possible that the code that takes account of braces in the
case of compadd -U isn't present because it's never been tested before, but
that would be bug: the C code takes pains to hide the existence of the
braces from the rest of the system, so there's no way you can treat this as
a special case anywhere else, and if you did it would arbitrarily limit the
completions you could do on expressions with braces.

pws

Re: Regression in braces completion

[Bart Schaefer]

On Oct 13, 10:04am, Peter Stephenson wrote:
} Subject: Re: Regression in braces completion
}
} It's certainly possible that the code that takes account of braces in
} the case of compadd -U isn't present because it's never been tested
} before, but that would be bug: the C code takes pains to hide the
} existence of the braces from the rest of the system

The direct effect of -U (if the doc is remotely descriptive of the code)
is to cause matching of the returned words against the command line to
be skipped, so perhaps the code that handles braces is embedded in the
compresult code that performs matching.

The usual indirect effect of -U is that the the word on the command
line is thrown away and replaced entirely with the result from compadd,
which does look like what's happening here.  Maybe all that's been lost
is the position at which to re-insert the brace, but I'm betting that
this comment above cline_str() is significant:

/* This builds the unambiguous string. If ins is one, it is immediately
 * inserted into the line. Otherwise csp is used to return the relative
 * cursor position in the string returned and posl contains all 
 * positions with missing or ambiguous characters. If ins is two, csp
 * and posl contain real command line positions (including braces). */

Comparing walkthroughs of cline_str() when ins==1 for the correct case
(-U removed from compadd) and the incorrect case, the difference is at
line 240, where in the correct case li==3 and in the incorrect li==0.
The value for li comes from l->prefix->llen, which must be the result
of the call to cut_cline() on line 175.

I've run out of time to spend on this, but I suspect that the effect of
the -U option is to cause cut_cline() to decide that nothing that was
already on the line is worth keeping, when really in this case we want
to keep the first three characters up to the brace.

However, I'm not sure the C code is actually wrong here.  Postulate a
completer that given "abcd" as the thing to match, returns "zyxw" and
uses compadd -U to say "accept this as a completion even though it does
not look like what you started with."  Now call that completer for a
line with "abc{d".  Is the right answer really "zyx{w" ?

} so there's no way you can treat this as a special case anywhere else

I wasn't suggesting treating this as a special case, I was suggesting
that _path_files treat a spelling correction in the path prefix as a
special case.  That's the stated reason why _path_files is using -U.

In fact _path_files probably ought to bail out in the case where it has 
corrected the path prefix and make the user confirm that the correction
is, well, correct, before it goes on to guessing about stuff further
along in the path.  The trouble most likely is that we're trying to do
too many things in one go.

Of course if we change that, someone will complain that what used to
need only one tab now needs two, but this may be a case of lesser of
evils.  Or there may be an entirely other way of solving this, such as
adding the entire paths including the correction as separate matches
so that the point of ambiguity is much farther to the left.  In any
case if you agree that the C code behavior for -U is correct, then the
right fix is something that eliminates the -U from _path_files by using
another approach for the original auto-correction problem.

Re: Regression in braces completion

[Peter Stephenson]

Bart Schaefer wrote:
> However, I'm not sure the C code is actually wrong here.  Postulate a
> completer that given "abcd" as the thing to match, returns "zyxw" and
> uses compadd -U to say "accept this as a completion even though it does
> not look like what you started with."  Now call that completer for a
> line with "abc{d".  Is the right answer really "zyx{w" ?

Yes, if the C code has (as it does) stripped all the stuff to do with
the parts of the brace expansion it doesn't think you want to see away
before you ever got to look at the word to be completed.  Otherwise you
get a meaningless hybrid---you're adding back braces you've never had
any information about in the first place.

(I can see how you could argue it differently: you can infer the braces
by looking at the raw command line and tell the completion system that
you've decided you don't like what's done and decide to do it yourself.
However, that's overloading -U with multiple meanings: not just "use my
completion as it is in your results table" but "forget everything you've
done so far and insert this the way I want".  I'm not convinced it even
works---the system is now thoroughly confused about what it's actually
completing, so working out what it's replacing with your string isn't
easy.  What -U does and doesn't do is already rather obscure, so this is
all probably moot.)

I think to do it consistently with the other interpretation, you would
need completion internally to ignore the presence of braces right from
the start and allow the function system to decide whether to handle
them.  That's not going to happen any time soon---though it's actually
possible it's a more elegant solution.

> I wasn't suggesting treating this as a special case, I was suggesting
> that _path_files treat a spelling correction in the path prefix as a
> special case.  That's the stated reason why _path_files is using -U.

As you previously noted, there are already far too many ways of running
compadd within _path_files as it is.  I'm not at all happy about
doubling the number to get sets with and without -U --- which would
leave spelling correction broken in the case where braces are present
(perhaps depending which way the pinball decides to roll down
_path_files in that case, but I presume it would still miss the
flippers in the same way).

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: Regression in braces completion

[Bart Schaefer]

On Oct 13,  5:27pm, Peter Stephenson wrote:
}
} However, that's overloading -U with multiple meanings: not just "use my
} completion as it is in your results table" but "forget everything you've
} done so far and insert this the way I want".

I think if you look around at the other uses of -U in the existing
completion functions, you'll find that it's employed in the latter
way in a significant number of cases.  In other words, it's too late,
-U has already become overloaded.

-- 

PATCH (?) Re: Regression in braces completion

[Bart Schaefer]

On Oct 13,  5:27pm, Peter Stephenson wrote:
}
} > I wasn't suggesting treating this as a special case, I was suggesting
} > that _path_files treat a spelling correction in the path prefix as a
} > special case.  That's the stated reason why _path_files is using -U.
} 
} As you previously noted, there are already far too many ways of running
} compadd within _path_files as it is.  I'm not at all happy about
} doubling the number to get sets with and without -U --- which would
} leave spelling correction broken in the case where braces are present

Somebody tell me what's wrong with this.  Doesn't seem to break spelling
correction in the path prefix when braces are present, and doesn't seem
to break braces when spelling correction isn't present, and doesn't add
any new calls to compadd; just changes whether the ones that are there
get -U passed in.

What am I missing?


--- ../zsh-forge/current/Completion/Unix/Type/_path_files	2008-09-02 20:09:04.000000000 -0700
+++ Completion/Unix/Type/_path_files	2008-10-13 21:31:53.000000000 -0700
@@ -7,7 +7,7 @@
 local tmp1 tmp2 tmp3 tmp4 i orig eorig pre suf tpre tsuf opre osuf cpre
 local pats haspats ignore pfx pfxsfx sopt gopt opt sdirs ignpar cfopt listsfx
 local nm=$compstate[nmatches] menu matcher mopts sort mid accex fake
-local listfiles listopts tmpdisp origtmp1
+local listfiles listopts tmpdisp origtmp1 Uopt
 integer npathcheck
 local -a match mbegin mend
 
@@ -209,7 +209,7 @@
 [[ $compstate[insert] = (*menu|[0-9]*) || -n "$_comp_correct" ||
    ( -n "$compstate[pattern_match]" &&
      "${orig#\~}" != (|*[^\\])[][*?#~^\|\<\>]* ) ]] && menu=yes
-[[ -n "$_comp_correct" ]] && cfopt=-
+[[ -n "$_comp_correct" ]] && cfopt=- Uopt=-U
 
 # Now let's have a closer look at the string to complete.
 
@@ -612,7 +612,7 @@
 	    # back up the path.
 	    tmp1=("${(@)tmp1%%/*}")
 	    _list_files tmp1 "$prepath$realpath$testpath"
-	    compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \
+	    compadd $Uopt -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \
 	            -s "/${tmp3#*/}$ISUFFIX" \
 	            -W "$prepath$realpath$testpath" \
 		    "$pfxsfx[@]" \
@@ -622,7 +622,7 @@
 	    # Same with a non-empty suffix
 	    tmp1=("${(@)^tmp1%%/*}/${tmp3#*/}")
 	    _list_files tmp1 "$prepath$realpath$testpath"
-	    compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \
+	    compadd $Uopt -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \
 	            -s "$ISUFFIX" \
 	            -W "$prepath$realpath$testpath" \
 		    "$pfxsfx[@]" \
@@ -631,7 +631,7 @@
           fi
 	else
 	  _list_files tmp1 "$prepath$realpath$testpath"
-	  compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \
+	  compadd $Uopt -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \
 	          -s "$ISUFFIX" \
 	          -W "$prepath$realpath$testpath" \
 		   "$pfxsfx[@]" \
@@ -661,7 +661,7 @@
           fi
         else
 	  _list_files tmp1 "$prepath$realpath$testpath"
-	  compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \
+	  compadd $Uopt -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \
 	          -s "$ISUFFIX" \
                   -W "$prepath$realpath$testpath" \
 		  "$pfxsfx[@]" \
@@ -730,7 +730,7 @@
       compquote tmp4 tmp2 tmp1
       for i in "$tmp1[@]"; do
 	_list_files tmp2 "$prepath$realpath${mid%/*/}"
-        compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp3/" \
+        compadd $Uopt -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp3/" \
 	        -s "/$tmp4$i$ISUFFIX" \
                 -W "$prepath$realpath${mid%/*/}/" \
 	        "$pfxsfx[@]" $listopts - "$tmp2"
@@ -761,7 +761,7 @@
       else
 	# Not a pattern match
 	_list_files tmp1 "$prepath$realpath$testpath"
-        compadd -U -Qf -p "$IPREFIX$linepath$tmp4" \
+        compadd $Uopt -Qf -p "$IPREFIX$linepath$tmp4" \
 	        -s "$ISUFFIX" \
 	        -W "$prepath$realpath$testpath" \
 	        "$pfxsfx[@]" "$mopts[@]" $listopts -a tmp1

Re: PATCH (?) Re: Regression in braces completion

[Peter Stephenson]

Bart Schaefer wrote:
> Somebody tell me what's wrong with this.  Doesn't seem to break spelling
> correction in the path prefix when braces are present, and doesn't seem
> to break braces when spelling correction isn't present, and doesn't add
> any new calls to compadd; just changes whether the ones that are there
> get -U passed in.
> 
> What am I missing?

Presumably we won't know for months until somebody tries the completion
in question, but if this seems to fix all the obvious problems feel free
to commit it, since I wasn't going to have much time to look at
compresult.c anyway.  (It might, for example, break spelling correction
where the prefix itself is in braces, but I don't feel particularly
moved to experiment.)

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: PATCH (?) Re: Regression in braces completion

[Vin Shelton]

On Tue, Oct 14, 2008 at 4:45 AM, Peter Stephenson <pws@csr.com> wrote:
> Bart Schaefer wrote:
>> Somebody tell me what's wrong with this.  Doesn't seem to break spelling
>> correction in the path prefix when braces are present, and doesn't seem
>> to break braces when spelling correction isn't present, and doesn't add
>> any new calls to compadd; just changes whether the ones that are there
>> get -U passed in.
>>
>> What am I missing?
>
> Presumably we won't know for months until somebody tries the completion
> in question, but if this seems to fix all the obvious problems feel free
> to commit it, since I wasn't going to have much time to look at
> compresult.c anyway.  (It might, for example, break spelling correction
> where the prefix itself is in braces, but I don't feel particularly
> moved to experiment.)
>

Bart -

+1

Your patch worked for my case, but you probably already guessed that.

  - Vin