[PATCH] don't exit shell on [[ -o invalid@option ]]

[PATCH] don't exit shell on [[ -o invalid@option ]]

[Martijn Dekker]

[-- Attachment #1: Type: text/plain, Size: 744 bytes --]

Does it make sense for [[ -o invalid@option ]] to exit the shell with an
error message?

Other shells with '[[' (bash, ksh93 and pdksh/mksh variants) quietly
return an unsuccessful status for a non-existent shell option. That
behaviour makes more sense to me because of:

(1) backwards compatibility: a script that uses '[[' to test if a shell
option introduced in a recent zsh is set, would still work on an older
zsh that doesn't have that shell option.

(2) cross-shell compatibility: treating a non-existent option as not set
would make it easier to write a script that works on bash, ksh93, and
pdksh/mksh as well as zsh.

The attached patch brings zsh in line with those other shells. Up to you
to decide what to do with it...

- Martijn

[-- Attachment #2: dont-exit-on-testing-invalid-option.patch --]
[-- Type: text/plain, Size: 504 bytes --]

diff --git a/Src/cond.c b/Src/cond.c
index b9a47ce..5eb59b2 100644
--- a/Src/cond.c
+++ b/Src/cond.c
@@ -492,7 +492,7 @@ dolstat(char *s)
 
 
 /*
- * optison returns evalcond-friendly statuses (true, false, error).
+ * optison returns evalcond-friendly statuses (true, false).
  */
 
 /**/
@@ -506,8 +506,7 @@ optison(char *name, char *s)
     else
 	i = optlookup(s);
     if (!i) {
-	zwarnnam(name, "no such option: %s", s);
-	return 2;
+	return 1;
     } else if(i < 0)
 	return !unset(-i);
     else

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Eric Cook]

On 11/10/2017 01:24 PM, Martijn Dekker wrote:

> The attached patch brings zsh in line with those other shells. Up to you
> to decide what to do with it...
> 
> - Martijn
> 

The passive aggression is real

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Martijn Dekker]

Op 10-11-17 om 18:53 schreef Eric Cook:
> On 11/10/2017 01:24 PM, Martijn Dekker wrote:
> 
>> The attached patch brings zsh in line with those other shells. Up to you
>> to decide what to do with it...
> 
> The passive aggression is real

No passive aggression was intended, I apologise if it came across as such.

- Martijn

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Bart Schaefer]

On Nov 10,  6:24pm, Martijn Dekker wrote:
}
} Does it make sense for [[ -o invalid@option ]] to exit the shell with an
} error message?

I have the impression that this was discussed on the list before, some
time ago, and the conclusion was that it was useful to be able to tell
[[ -o an_option_typo ]] apart from [[ -o an_unset_option ]].

In other words, if the option doesn't exist at all, then for any code
depending on the result of the test, both the true and false branches
may very well be wrong.

It's easy enough to write backwards-compatible code by simply adding a
subshell around the test, if that's what's intended.

We could certainly suppress the error in emulation modes given that's
what other shells do.  Further, I'm not strongly invested in the current
behavior even for native mode, but we should consider the ramifications.

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Peter Stephenson]

On Fri, 10 Nov 2017 14:37:17 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> We could certainly suppress the error in emulation modes given that's
> what other shells do.  Further, I'm not strongly invested in the current
> behavior even for native mode, but we should consider the ramifications.

It's not clear if it matters in practice rather than theory, no.  The
test failing is probably usually good enough.

We could attach it to POSIXBUILTINS as that does control some aspects of
reutrn / exit behaviour, and [[ ... ]] behaves like a specially parsed
builtin.

pws

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Martijn Dekker]

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

Op 11-11-17 om 12:45 schreef Peter Stephenson:
> On Fri, 10 Nov 2017 14:37:17 -0800
> Bart Schaefer <schaefer@brasslantern.com> wrote:
>> We could certainly suppress the error in emulation modes given that's
>> what other shells do.  Further, I'm not strongly invested in the current
>> behavior even for native mode, but we should consider the ramifications.
> 
> It's not clear if it matters in practice rather than theory, no.  The
> test failing is probably usually good enough.
> 
> We could attach it to POSIXBUILTINS as that does control some aspects of
> reutrn / exit behaviour, and [[ ... ]] behaves like a specially parsed
> builtin.

Something like this?

Seems a bit wrong as this is not POSIX at all. Also, POSIXBUILTINS makes
'set -o' exit the shell on an invalid option, which is the opposite
effect. But I can't find a better shell option either.

- M.

[-- Attachment #2: dont-exit-on-testing-invalid-option.patch --]
[-- Type: text/plain, Size: 898 bytes --]

diff --git a/Doc/Zsh/cond.yo b/Doc/Zsh/cond.yo
index e08fc0d..0ea5fc1 100644
--- a/Doc/Zsh/cond.yo
+++ b/Doc/Zsh/cond.yo
@@ -45,6 +45,8 @@ item(tt(-o) var(option))(
 true if option named var(option) is on.  var(option)
 may be a single character, in which case it is a single letter option name.
 (See noderef(Specifying Options).)
+If the var(option) does not exist, the shell exits with an error message,
+unless the shell option tt(POSIX_BUILTINS) is set.
 )
 item(tt(-p) var(file))(
 true if var(file) exists and is a FIFO special file (named pipe).
diff --git a/Src/cond.c b/Src/cond.c
index b9a47ce..814f4b0 100644
--- a/Src/cond.c
+++ b/Src/cond.c
@@ -506,6 +506,8 @@ optison(char *name, char *s)
     else
 	i = optlookup(s);
     if (!i) {
+	if (isset(POSIXBUILTINS))
+	   return 1; /* act like bash and *ksh */
 	zwarnnam(name, "no such option: %s", s);
 	return 2;
     } else if(i < 0)

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Bart Schaefer]

On Nov 11,  7:01pm, Martijn Dekker wrote:
}
} Op 11-11-17 om 12:45 schreef Peter Stephenson:
} > On Fri, 10 Nov 2017 14:37:17 -0800
} > Bart Schaefer <schaefer@brasslantern.com> wrote:
} >> We could certainly suppress the error in emulation modes
} > 
} > We could attach it to POSIXBUILTINS as that does control some aspects
} 
} Seems a bit wrong as this is not POSIX at all.

I was thinking more along the lines of tying it to EMULATION(EMULATE_SH)
rather directly to a given option bit.  So you get it if the shell is
started as sh/ksh/etc., but you can't switch it on if started as zsh.

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Peter Stephenson]

On Sat, 11 Nov 2017 15:19:05 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Nov 11,  7:01pm, Martijn Dekker wrote:
> }
> } Op 11-11-17 om 12:45 schreef Peter Stephenson:
> } > On Fri, 10 Nov 2017 14:37:17 -0800
> } > Bart Schaefer <schaefer@brasslantern.com> wrote:
> } >> We could certainly suppress the error in emulation modes
> } > 
> } > We could attach it to POSIXBUILTINS as that does control some aspects
> } 
> } Seems a bit wrong as this is not POSIX at all.
> 
> I was thinking more along the lines of tying it to EMULATION(EMULATE_SH)
> rather directly to a given option bit.  So you get it if the shell is
> started as sh/ksh/etc., but you can't switch it on if started as zsh.

This is tending to hide the knob under the sticky out bit at the top where
the logo is attached (as it were).

Maybe we should just accept the original patch and note the
incimpatibility.  It's a non-issue for properly written shell code
anyway.

pws

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Daniel Shahaf]

Sorry for the late response; family reasons.

Peter Stephenson wrote on Sun, Nov 12, 2017 at 19:56:57 +0000:
> On Sat, 11 Nov 2017 15:19:05 -0800
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> > I was thinking more along the lines of tying it to EMULATION(EMULATE_SH)
> > rather directly to a given option bit.  So you get it if the shell is
> > started as sh/ksh/etc., but you can't switch it on if started as zsh.
> 
> This is tending to hide the knob under the sticky out bit at the top where
> the logo is attached (as it were).
> 
> Maybe we should just accept the original patch and note the
> incimpatibility.  It's a non-issue for properly written shell code
> anyway.

I wasn't sold by the OP's reasoning:

Martijn Dekker wrote on Fri, Nov 10, 2017 at 18:24:29 +0000:
> Does it make sense for [[ -o invalid@option ]] to exit the shell with an
> error message?
> 

Yes: "invalid@option" is neither set nor unset; it does not exist.

> Other shells with '[[' (bash, ksh93 and pdksh/mksh variants) quietly
> return an unsuccessful status for a non-existent shell option. That
> behaviour makes more sense to me because of:
> 
> (1) backwards compatibility: a script that uses '[[' to test if a shell
> option introduced in a recent zsh is set, would still work on an older
> zsh that doesn't have that shell option.
> 

That use-case is addressable by probing ${options[schroedingerscat]} or
`set -o | grep`.

As Bart hinted, the proposed change is backwards *incompatible*: it
makes [[ -o invalid@option ]] return 1 where currently it returns 2.
(User code might be relying on the distinction between $? == 1 and
$? == 2.)

Moreover, if cross-version compatibility is the goal, why is it a good
thing to lump "This shell does not have INVALID_OPTION" and "This shell
has INVALID_OPTION and it's unset"?  It's easy to imagine a situation in
which that'd be a bug: if INVALID_OPTION was added in zsh version N, is
set by default, and a plugin that was developed against version N is
installed by a user running version N-1.  With the current code that
situation would result in a (proper) warning.

I suppose we could plug the latter concern somewhat if we decided that
whenever we add a new option, it'd be unset by default.

> (2) cross-shell compatibility: treating a non-existent option as not set
> would make it easier to write a script that works on bash, ksh93, and
> pdksh/mksh as well as zsh.

Devil's advocate, but why can't people just do, today,

    if [[ -o INVALID_OPTION ]] 2>/dev/null; then

or

    if [[ ${options[invalidoption]:-off} == off ]]; then

?

Cheers,

Daniel

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Martijn Dekker]

Op 14-11-17 om 14:26 schreef Daniel Shahaf:
> As Bart hinted, the proposed change is backwards *incompatible*: it
> makes [[ -o invalid@option ]] return 1 where currently it returns 2.

Actually, it exits/aborts the shell with status 2. This is an essential
difference from simply returning status 2.

> Moreover, if cross-version compatibility is the goal, why is it a good
> thing to lump "This shell does not have INVALID_OPTION" and "This shell
> has INVALID_OPTION and it's unset"?

Because
(a) that's how all other [[ -o ... ]] implementations work,
(b) that's what the current documentation in zshmisc.1 says that [[ -o
...] does, and
(c) the canonical way to check if an option exists
   if (set +o someoption) 2>/dev/null; then ...
works fine on all shells.

Re (b), zshmisc.1 simply says that [[ -o ... ]] checks if the option is
on. It says nothing about checking if the option exists, much less
aborting the shell if it doesn't.

> It's easy to imagine a situation in
> which that'd be a bug: if INVALID_OPTION was added in zsh version N, is
> set by default, and a plugin that was developed against version N is
> installed by a user running version N-1.

I don't see how that would introduce a bug. If a new option NEW_OPTION
is introduced in zsh version N, default on, then [[ -o NEW_OPTION ]] can
be used to run code dependent on that new option only if that new option
is on. That code will then never be run on version N-1, which is what is
expected.

> With the current code that
> situation would result in a (proper) warning.

A mere warning would be fine, but what actually happens is that the
shell aborts with an error message.

> Devil's advocate, but why can't people just do, today,
> 
>     if [[ -o INVALID_OPTION ]] 2>/dev/null; then

Because, on zsh (unlike bash and *ksh) that will cause the shell to
exit, as in, the program aborts. Not only that, adding 2>/dev/null will
cause the program to abort silently, because the error message is
suppressed.

You can of course do

    if ([[ -o INVALID_OPTION ]]) 2>/dev/null; then

but that comes at the cost of forking a subshell, so that had better not
be within a loop with many iterations.

>     if [[ ${options[invalidoption]:-off} == off ]]; then

That's fine if the script needs to work on zsh only. Not very intuitive,
though.

- Martijn

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Daniel Shahaf]

Martijn Dekker wrote on Tue, Nov 14, 2017 at 15:22:35 +0200:
> Op 14-11-17 om 14:26 schreef Daniel Shahaf:
> > As Bart hinted, the proposed change is backwards *incompatible*: it
> > makes [[ -o invalid@option ]] return 1 where currently it returns 2.
> 
> Actually, it exits/aborts the shell with status 2. This is an essential
> difference from simply returning status 2.
> 

Good point, I'd overlooked that.

> > Moreover, if cross-version compatibility is the goal, why is it a good
> > thing to lump "This shell does not have INVALID_OPTION" and "This shell
> > has INVALID_OPTION and it's unset"?
> 
> Because
> (a) that's how all other [[ -o ... ]] implementations work,
> (b) that's what the current documentation in zshmisc.1 says that [[ -o
> ...] does, and
> (c) the canonical way to check if an option exists
>    if (set +o someoption) 2>/dev/null; then ...
> works fine on all shells.
> 
> Re (b), zshmisc.1 simply says that [[ -o ... ]] checks if the option is
> on. It says nothing about checking if the option exists, much less
> aborting the shell if it doesn't.

(a) That's a fair point, but I'm not convinced it swings the balance.
(I agree that being compatible with bash/ksh is a plus.)

(b) zshmisc.1 says "true if the named option var(option) is on".  It
does not say that it's valid for var(option) not to be an option name at
all.  Therefore, «[[ -o not_an_option ]]» is undefined behaviour; the
documentation does NOT promise that it would behave identically to [[ -o
an_unset_option ]].  In other words, the current behaviour is consistent
with the documentation.

(c) I don't follow your argument.  Okay, so «(set +o option)» works,
what does that have to do with the behaviour of [[ -o not_an_option ]]?

> > It's easy to imagine a situation in
> > which that'd be a bug: if INVALID_OPTION was added in zsh version N, is
> > set by default, and a plugin that was developed against version N is
> > installed by a user running version N-1.
> 
> I don't see how that would introduce a bug. If a new option NEW_OPTION
> is introduced in zsh version N, default on, then [[ -o NEW_OPTION ]] can
> be used to run code dependent on that new option only if that new option
> is on. That code will then never be run on version N-1, which is what is
> expected.
> 

You're considering the "if" branch, I'm considering the "else" branch:

    if [[ -o NEW_OPTION ]]; then lorem; else ipsum; fi

"lorem" will only run on zsh vN when the option is set, but "ipsum" will
run under vN with the option explicitly unset by the user AND under
vN-1.  Since the option is on by default, vN-1 should take the "then"
branch.

> > With the current code that
> > situation would result in a (proper) warning.
> 
> A mere warning would be fine, but what actually happens is that the
> shell aborts with an error message.
> 

Okay, so how about if we demoted the fatal error to a warning?  Like
this:

    % [[ -o not_an_option ]] || echo This gets run
    zsh: [[: no such option: not_an_option
    This gets run
    % 

> > Devil's advocate, but why can't people just do, today,
> > 
> >     if [[ -o INVALID_OPTION ]] 2>/dev/null; then
> 
> Because, on zsh (unlike bash and *ksh) that will cause the shell to
> exit, as in, the program aborts. Not only that, adding 2>/dev/null will
> cause the program to abort silently, because the error message is
> suppressed.
> 

If the fatal error were a warning, the 2> redirection would hide [['s
warning but the program would continue.

> You can of course do
> 
>     if ([[ -o INVALID_OPTION ]]) 2>/dev/null; then
> 
> but that comes at the cost of forking a subshell, so that had better not
> be within a loop with many iterations.
> 
> >     if [[ ${options[invalidoption]:-off} == off ]]; then
> 
> That's fine if the script needs to work on zsh only. Not very intuitive,
> though.

The «:-off» is just there in case 'set -u' is in effect.  If that's not
a concern, the code would be «[[ ${options[invalidoption]} == on ]]»,
which may be somewhat more bearable.

Cheers,

Daniel

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Martijn Dekker]

Op 15-11-17 om 01:52 schreef Daniel Shahaf:
> Martijn Dekker wrote on Tue, Nov 14, 2017 at 15:22:35 +0200:
>> Because
>> (a) that's how all other [[ -o ... ]] implementations work,
>> (b) that's what the current documentation in zshmisc.1 says that [[ -o
>> ...] does, and
>> (c) the canonical way to check if an option exists
>>    if (set +o someoption) 2>/dev/null; then ...
>> works fine on all shells.
>>
>> Re (b), zshmisc.1 simply says that [[ -o ... ]] checks if the option is
>> on. It says nothing about checking if the option exists, much less
>> aborting the shell if it doesn't.
> 
> (a) That's a fair point, but I'm not convinced it swings the balance.
> (I agree that being compatible with bash/ksh is a plus.)

I would note additionally that '[[' is a feature originally copied from
ksh, so to me it does not seem like a good idea to differ from ksh
behaviour without strong reasons.

> (b) zshmisc.1 says "true if the named option var(option) is on".  It
> does not say that it's valid for var(option) not to be an option name at
> all.  Therefore, «[[ -o not_an_option ]]» is undefined behaviour; the
> documentation does NOT promise that it would behave identically to [[ -o
> an_unset_option ]].  In other words, the current behaviour is consistent
> with the documentation.

I don't follow that reasoning. By definition, a non-existent option
cannot be on, so the condition "true if the named option var(option) is
on" is always false for a non-existent option. Had it said "false if the
named option var(option) is off", your point would have made sense to me.

In either case, not documenting that the shell aborts on a non-existent
option is definitely an omission. In fact that omission makes me wonder
if that behaviour was intended in the first place.

> (c) I don't follow your argument.  Okay, so «(set +o option)» works,
> what does that have to do with the behaviour of [[ -o not_an_option ]]?

Nothing, except that there is already a well-known way of checking if an
option exists, so it's not really necessary for [[ -o ... ]] to have
that functionality, particularly if it comes at the expense of not being
compatible with ksh and bash.

>>> It's easy to imagine a situation in
>>> which that'd be a bug: if INVALID_OPTION was added in zsh version N, is
>>> set by default, and a plugin that was developed against version N is
>>> installed by a user running version N-1.
>>
>> I don't see how that would introduce a bug. If a new option NEW_OPTION
>> is introduced in zsh version N, default on, then [[ -o NEW_OPTION ]] can
>> be used to run code dependent on that new option only if that new option
>> is on. That code will then never be run on version N-1, which is what is
>> expected.
> 
> You're considering the "if" branch, I'm considering the "else" branch:
> 
>     if [[ -o NEW_OPTION ]]; then lorem; else ipsum; fi
> 
> "lorem" will only run on zsh vN when the option is set, but "ipsum" will
> run under vN with the option explicitly unset by the user AND under
> vN-1.  Since the option is on by default, vN-1 should take the "then"
> branch.

Fair point. However, zsh makes this easy to work around with its dynamic
'no-' prefix for all options.

    if [[ -o noNEW_OPTION ]]; then ipsum; else lorem; fi

[...]
> Okay, so how about if we demoted the fatal error to a warning?  Like
> this:
> 
>     % [[ -o not_an_option ]] || echo This gets run
>     zsh: [[: no such option: not_an_option
>     This gets run
>     % 

I think that would be fine, as well as returning status 2 along with it.

I do think the warning (and the status 2) should be suppressed if
POSIX_IDENTIFIERS or some other emulation-relevant shell option is
active, so that 'emulate ksh' and 'emulate sh' reproduce ksh behaviour.

Thanks,

- Martijn

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Daniel Shahaf]

Martijn Dekker wrote on Thu, 16 Nov 2017 02:52 +0200:
> Op 15-11-17 om 01:52 schreef Daniel Shahaf:
> > Okay, so how about if we demoted the fatal error to a warning?  Like
> > this:
> > 
> >     % [[ -o not_an_option ]] || echo This gets run
> >     zsh: [[: no such option: not_an_option
> >     This gets run
> >     % 
> 
> I think that would be fine, as well as returning status 2 along with it.
> 
> I do think the warning (and the status 2) should be suppressed if
> POSIX_IDENTIFIERS or some other emulation-relevant shell option is
> active, so that 'emulate ksh' and 'emulate sh' reproduce ksh behaviour.

How about the following.

It uses status 3 because status 2 currently means "syntax error in [["
and I didn't want to overload that; and it uses POSIX_BUILTINS because
that seemed more closely related than POSIX_IDENTIFIERS.

Consider it a work in progress, i.e., particulars are still malleable.
There are good arguments in favour of all sides here… (incumbent
behaviour, Martijn's patch, this patch)

Cheers,

Daniel

diff --git a/Doc/Zsh/cond.yo b/Doc/Zsh/cond.yo
index e08fc0d36..4ca132a26 100644
--- a/Doc/Zsh/cond.yo
+++ b/Doc/Zsh/cond.yo
@@ -45,6 +45,10 @@ item(tt(-o) var(option))(
 true if option named var(option) is on.  var(option)
 may be a single character, in which case it is a single letter option name.
 (See noderef(Specifying Options).)
+
+When no option named var(option) exists, and the tt(POSIX_BUILTINS) option
+hasn't been set, return 3 with a warning.  If that option is set, return 1
+with no warning.
 )
 item(tt(-p) var(file))(
 true if var(file) exists and is a FIFO special file (named pipe).
diff --git a/Doc/Zsh/options.yo b/Doc/Zsh/options.yo
index 28155d796..d043cf398 100644
--- a/Doc/Zsh/options.yo
+++ b/Doc/Zsh/options.yo
@@ -2169,6 +2169,9 @@ command found in the path.
 Furthermore, the tt(getopts) builtin behaves in a POSIX-compatible
 fashion in that the associated variable tt(OPTIND) is not made
 local to functions.
+
+Moreover, the warning and special exit code from
+tt([[ -o )var(non_existent_option)tt( ]]) are suppressed.
 )
 pindex(POSIX_IDENTIFIERS)
 pindex(NO_POSIX_IDENTIFIERS)
diff --git a/Src/cond.c b/Src/cond.c
index b9a47cea5..9f13e07d7 100644
--- a/Src/cond.c
+++ b/Src/cond.c
@@ -61,7 +61,8 @@ static void cond_subst(char **strp, int glob_ok)
  * of functionality.
  *
  * Return status is the final shell status, i.e. 0 for true,
- * 1 for false and 2 for error.
+ * 1 for false, 2 for syntax error, 3 for "option in tested in
+ * -o does not exist".
  */
 
 /**/
@@ -86,10 +87,10 @@ case COND_NOT:
 	if (tracingcond)
 	    fprintf(xtrerr, " %s", condstr[ctype]);
 	ret = evalcond(state, fromtest);
-	if (ret == 2)
-	    return ret;
-	else
+	if (ret == 0 || ret == 1)
 	    return !ret;
+	else
+	    return ret;
     case COND_AND:
 	if (!(ret = evalcond(state, fromtest))) {
 	    if (tracingcond)
@@ -100,7 +101,8 @@ case COND_AND:
 	    return ret;
 	}
     case COND_OR:
-	if ((ret = evalcond(state, fromtest)) == 1) {
+	ret = evalcond(state, fromtest);
+	if (ret == 1 || ret == 3) {
 	    if (tracingcond)
 		fprintf(xtrerr, " %s", condstr[ctype]);
 	    goto rec;
@@ -506,8 +508,12 @@ optison(char *name, char *s)
     else
 	i = optlookup(s);
     if (!i) {
-	zwarnnam(name, "no such option: %s", s);
-	return 2;
+	if (isset(POSIXBUILTINS))
+	    return 1;
+	else {
+	    zwarnnam(name, "no such option: %s", s);
+	    return 3;
+	}
     } else if(i < 0)
 	return !unset(-i);
     else
diff --git a/Test/C02cond.ztst b/Test/C02cond.ztst
index 38525016c..04e1ca8f2 100644
--- a/Test/C02cond.ztst
+++ b/Test/C02cond.ztst
@@ -440,6 +440,23 @@ F:Failures in these cases do not indicate a problem in the shell.
 >  [[ 'a' == 'b' || 'b' = 'c' || 'c' != 'd' ]]
 >}
 
+  (setopt posixbuiltins; eval '[[ -o invalidoption ]]; echo set: $?'; echo "no warning" >&2)
+  (unsetopt posixbuiltins; [[ -o invalidoption ]]; echo unset: $?)
+  [[ -o invalidoption || -n nonempty ]]; echo "in disjunction, true: $?"
+  [[ -o invalidoption || -z nonempty ]]; echo "in disjunction, false: $?"
+  [[ ! -o invalidoption ]]; echo "negated: $?"
+0:-o invalidoption
+>set: 1
+?no warning
+>unset: 3
+?(eval):2: no such option: invalidoption
+>in disjunction, true: 0
+?(eval):3: no such option: invalidoption
+>in disjunction, false: 1
+?(eval):4: no such option: invalidoption
+>negated: 3
+?(eval):5: no such option: invalidoption
+
 %clean
   # This works around a bug in rm -f in some versions of Cygwin
   chmod 644 unmodish

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Martijn Dekker]

Op 18-11-17 om 20:22 schreef Daniel Shahaf:
> How about the following.

I, for one, am happy with it.

Thanks,

- M.

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Bart Schaefer]

On Sat, Nov 18, 2017 at 10:22 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> How about the following.
>
> It uses status 3 because status 2 currently means "syntax error in [["
> and I didn't want to overload that; and it uses POSIX_BUILTINS because
> that seemed more closely related than POSIX_IDENTIFIERS.

For reasons I can't entirely define, it bothers me that an operation
to test the setting of options changes its behavior based on the
setting of an option.

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Peter Stephenson]

On Sun, 19 Nov 2017 11:41:57 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Sat, Nov 18, 2017 at 10:22 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > How about the following.
> >
> > It uses status 3 because status 2 currently means "syntax error in [["
> > and I didn't want to overload that; and it uses POSIX_BUILTINS because
> > that seemed more closely related than POSIX_IDENTIFIERS.
> 
> For reasons I can't entirely define, it bothers me that an operation
> to test the setting of options changes its behavior based on the
> setting of an option.

I'm actually more worried if this *isn't* an option, given it's (in a
more touchy feely sense) optional behaviour.  Having something of that
kind based on emulation seems a bit gross.  But we do have gross syntax
changes based on emulation, it's true, such as variables that never
appear.

pws

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Daniel Shahaf]

Peter Stephenson wrote on Sun, 19 Nov 2017 19:53 +0000:
> On Sun, 19 Nov 2017 11:41:57 -0800
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> > On Sat, Nov 18, 2017 at 10:22 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > >
> > > How about the following.
> > >
> > > It uses status 3 because status 2 currently means "syntax error in [["
> > > and I didn't want to overload that; and it uses POSIX_BUILTINS because
> > > that seemed more closely related than POSIX_IDENTIFIERS.
> > 
> > For reasons I can't entirely define, it bothers me that an operation
> > to test the setting of options changes its behavior based on the
> > setting of an option.
> 

Are you concerned about self-reference?  I.e., that code meant to run
under any options (example: z-sy-h) wouldn't be able to test the
settedness of an option using [[ -o foo ]] because the behaviour of
that command depends on whether foo is set?  (That's basically the
Liar Paradox)

I don't think that's a problem in this case, but I'm listening...

> I'm actually more worried if this *isn't* an option, given it's (in a
> more touchy feely sense) optional behaviour.  Having something of that
> kind based on emulation seems a bit gross.  But we do have gross syntax
> changes based on emulation, it's true, such as variables that never
> appear.

I'm quite swamped this week (in a good way) but when I get a chance I'll
finish the patch and push.  (Needs more tests and a README blurb, at least)

Cheers,

Daniel

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Daniel Shahaf]

Daniel Shahaf wrote on Mon, Nov 20, 2017 at 01:22:41 +0000:
> I'm quite swamped this week (in a good way) but when I get a chance I'll
> finish the patch and push.  (Needs more tests and a README blurb, at least)

Interdiff:

diff --git a/README b/README
index 6fad1d516..59abf29b2 100644
--- a/README
+++ b/README
@@ -54,6 +54,18 @@ foo=([aeiou]\=vowel)
 This is only required for array values contained within parentheses;
 command line expansion for normal arguments has not changed.
 
+3) The syntax
+
+[[ -o foo ]]
+
+where foo is not the name of a shell option (with optional underscores
+and optional "no" prefix) used to be treated as a syntax error, i.e.,
+the enclosing command line or file were aborted.  It now emits a warning
+and returns a non-zero exit code.  For further details, see the
+documentation of the -o switch in the chapter "Conditional Expressions"
+in the zshmisc(1) manual.
+
+
 Incompatibilities between 5.3.1 and 5.4.2
 -----------------------------------------
 
diff --git a/Test/C02cond.ztst b/Test/C02cond.ztst
index 04e1ca8f2..4ffb07dd4 100644
--- a/Test/C02cond.ztst
+++ b/Test/C02cond.ztst
@@ -440,14 +440,15 @@ F:Failures in these cases do not indicate a problem in the shell.
 >  [[ 'a' == 'b' || 'b' = 'c' || 'c' != 'd' ]]
 >}
 
-  (setopt posixbuiltins; eval '[[ -o invalidoption ]]; echo set: $?'; echo "no warning" >&2)
+  (setopt posixbuiltins; [[ -o invalidoption ]]; echo set: $?; echo "line 1: no warning" >&2)
   (unsetopt posixbuiltins; [[ -o invalidoption ]]; echo unset: $?)
   [[ -o invalidoption || -n nonempty ]]; echo "in disjunction, true: $?"
   [[ -o invalidoption || -z nonempty ]]; echo "in disjunction, false: $?"
   [[ ! -o invalidoption ]]; echo "negated: $?"
+  [[ -o invalidoption && -n nonempty ]] || echo "in conjunction: $?"
 0:-o invalidoption
 >set: 1
-?no warning
+?line 1: no warning
 >unset: 3
 ?(eval):2: no such option: invalidoption
 >in disjunction, true: 0
@@ -456,6 +457,8 @@ F:Failures in these cases do not indicate a problem in the shell.
 ?(eval):4: no such option: invalidoption
 >negated: 3
 ?(eval):5: no such option: invalidoption
+>in conjunction: 3
+?(eval):6: no such option: invalidoption
 
 %clean
   # This works around a bug in rm -f in some versions of Cygwin

Re: [PATCH] don't exit shell on [[ -o invalid@option ]]

[Mikael Magnusson]

On Fri, Nov 24, 2017 at 10:50 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Daniel Shahaf wrote on Mon, Nov 20, 2017 at 01:22:41 +0000:
>> I'm quite swamped this week (in a good way) but when I get a chance I'll
>> finish the patch and push.  (Needs more tests and a README blurb, at least)
>
> Interdiff:
>
> diff --git a/README b/README
> index 6fad1d516..59abf29b2 100644
> --- a/README
> +++ b/README
> @@ -54,6 +54,18 @@ foo=([aeiou]\=vowel)
>  This is only required for array values contained within parentheses;
>  command line expansion for normal arguments has not changed.
>
> +3) The syntax
> +
> +[[ -o foo ]]
> +
> +where foo is not the name of a shell option (with optional underscores
> +and optional "no" prefix) used to be treated as a syntax error, i.e.,
> +the enclosing command line or file were aborted.  It now emits a warning
> +and returns a non-zero exit code.  For further details, see the
> +documentation of the -o switch in the chapter "Conditional Expressions"
> +in the zshmisc(1) manual.

I don't know if it's also worth pointing out that you can do
{ [[ -o invalid_option ]] } always { TRY_BLOCK_ERROR=0 }
since 4.2.1 or so? (It will suppress the syntax error but preserve the
return code).

-- 
Mikael Magnusson