inconsistency of invalid identifier error messages

inconsistency of invalid identifier error messages

[Oliver Kiddle]

The following seems somewhat inconsistent. Compare the error messages
and note the difference in whether the error is printed before or after
the edits.

vared -c a-b
vared -c ''
vared -- -a
vared -- -
read a-b
unset a-b
unset 'a[b'

I think unset should be producing an error in both cases.  It's fair
that it quitely succeeds if you unset an already unset parameter but an
invalid parameter name is different.

Any preference on what error message to standardise on. We currently
have:
not an identifier: -
not an identifier: `-a'
invalid parameter name: a-b
a[b: invalid parameter name

Also, any views on whether errors should be printed before or after or
whether it is correct that read consumes the input regardless while vared
immediately prints the error message? Bash and Ksh have the opposite
behaviour in this regard.

The patch below makes the first two (vared) cases consistent with the
third one. Any thoughts on how to best fix the last vared case (editing
$-) and what to do about unset? With this patch, I'm also assuming that
the first error condition should have been calling unqueue_signals().

Oliver

Index: Src/Zle/zle_main.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_main.c,v
retrieving revision 1.120
diff -u -r1.120 zle_main.c
--- Src/Zle/zle_main.c	12 Nov 2008 12:59:07 -0000	1.120
+++ Src/Zle/zle_main.c	12 Nov 2008 13:17:55 -0000
@@ -1492,11 +1492,11 @@
 	unqueue_signals();
 	zwarnnam(name, "no such variable: %s", args[0]);
 	return 1;
+    } else if (*s || s == args[0]) {
+	unqueue_signals();
+	zwarnnam(name, "not an identifier: %s", args[0]);
+	return 1;
     } else if (v) {
-	if (*s) {
-	    zwarnnam(name, "not an identifier: `%s'", args[0]);
-	    return 1;
-	}
 	if (v->isarr) {
 	    /* Array: check for separators and quote them. */
 	    char **arr = getarrvalue(v), **aptr, **tmparr, **tptr;
@@ -1549,10 +1549,6 @@
 	    s = ztrdup(getstrvalue(v));
 	}
 	unqueue_signals();
-    } else if (*s) {
-	unqueue_signals();
-	zwarnnam(name, "invalid parameter name: %s", args[0]);
-	return 1;
     } else {
 	unqueue_signals();
 	s = ztrdup(s);

Re: inconsistency of invalid identifier error messages

[Peter Stephenson]

On Wed, 12 Nov 2008 14:50:22 +0100
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> The following seems somewhat inconsistent. Compare the error messages
> and note the difference in whether the error is printed before or after
> the edits.
> 
> vared -c a-b
> vared -c ''
> vared -- -a
> vared -- -
> read a-b
> unset a-b
> unset 'a[b'
> 
> I think unset should be producing an error in both cases.  It's fair
> that it quitely succeeds if you unset an already unset parameter but an
> invalid parameter name is different.

Presumably this is connect to parsing subscripts?  unset 'a[b]' is not an
error if a is an associative array (even if the element b doesn't exist),
though 'a[b' obviously is.

> Any preference on what error message to standardise on. We currently
> have:
> invalid parameter name: a-b

I think this one is probably clearest.  ("Parameter" isn't a very good word
for a variable, but that's too widespread to do anything about.)

> Also, any views on whether errors should be printed before or after or
> whether it is correct that read consumes the input regardless while vared
> immediately prints the error message? Bash and Ksh have the opposite
> behaviour in this regard.

I would think having the error as soon as possible is good.

> The patch below makes the first two (vared) cases consistent with the
> third one. Any thoughts on how to best fix the last vared case (editing
> $-) and what to do about unset?

Hmmm... "-" *is* an identifier if $- is a parameter.  Maybe the test
needs fixing.  However, the parameter is readonly, and readonly specials
can't be made writeable, so a simple local fix would be to test if the
parameter existed despite the failure of the identifier test and report an
error if it was readonly.  Same with "?".

-- 
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: inconsistency of invalid identifier error messages

[Stephane Chazelas]

On Wed, Nov 12, 2008 at 02:12:13PM +0000, Peter Stephenson wrote:
[...]
> Hmmm... "-" *is* an identifier if $- is a parameter.  Maybe the test
> needs fixing.  However, the parameter is readonly, and readonly specials
> can't be made writeable, so a simple local fix would be to test if the
> parameter existed despite the failure of the identifier test and report an
> error if it was readonly.  Same with "?".
[...]

And "#", "!", "$"?

What about "@" and "*"? You can do read 'argv[*]' but not read
'*'.

Also note that one can do 1=12, or "read 1" but not (( 1 = 12 ))
(though you can do (( argv[1] = 12 )) but it's probably a good
thing.

-- 
Stéphane

Re: inconsistency of invalid identifier error messages

[Oliver Kiddle]

Peter wrote:
> Presumably this is connect to parsing subscripts?  unset 'a[b]' is not an
> error if a is an associative array (even if the element b doesn't exist),
> though 'a[b' obviously is.

Yes. For unset 'a[b]' to not be an error is entirely consistent with
unset b not being an error when there is no b variable.

What I think should change is that unset 'a-b' should produce an error.

> > invalid parameter name: a-b
>
> I think this one is probably clearest.  ("Parameter" isn't a very good word
> for a variable, but that's too widespread to do anything about.)
> 
We've also got:
% vared not-here
vared: no such variable: not-here

I actually think there would be some sense to trying to use variable for
the documentation and error messages even if the internals and some
things like the zsh/parameter module can't be changed. Similarly the
documentation could prefer declare to typeset.

> Hmmm... "-" *is* an identifier if $- is a parameter.  Maybe the test

Exactly. So the ideal error message would be one complaining that `-' is
readonly.

Oliver