code review 4428057: spec: narrow syntax for expression and select statements

spec: narrow syntax for expression and select statements

This is not a language change, it simply expresses the
accepted cases explicitly in the respective productions.

[gri, 2011-04-20 23:30:21]

Hello rsc, r, iant (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-04-21 12:09:43]

LGTM

But only if go/parser does not implement this rule.
I think we should be able to gofmt programs that
have top-level expressions.  Also I don't think we should
have different error messages for the unused top-level
expressions:

    len(x)
    x+1

but that's almost unavoidable if the parser handles
one but the type checker handles the other (and it must).

On the same note, I am a little worried that having this
in the grammar will encourage other parser implementers
to implement the rule, which will limit the generality of
their parsers.  But it does look correct.

Russ

[gri, 2011-04-21 17:31:07]

On Thu, Apr 21, 2011 at 5:09 AM, Russ Cox <rsc@golang.org> wrote:
> LGTM
>
> But only if go/parser does not implement this rule.
> I think we should be able to gofmt programs that
> have top-level expressions.  Also I don't think we should

What is your rationale for it? If the grammar has this restriction,
the parser should enforce it, I would think.
Maybe the grammar should not have this restriction, but I think it is
easily syntactically enforced.


> have different error messages for the unused top-level
> expressions:
>
>    len(x)
>    x+1
>
> but that's almost unavoidable if the parser handles
> one but the type checker handles the other (and it must).

There is a difference between legal grammar (assuming we want to put
above rule in) and the unused rule in my mind. The reason why I
started making this change is because I want to get a handle on the
unused rule so that it can be written down in the spec. Fixing the
grammar reduces the problem a little bit. As it is, 6g and gccgo have
different ideas about what is used and what is not. We put the unused
rule to a large part so that we would see potential errors with :=
redeclarations. It seems that 6g goes a step further and applies it to
built-in function calls w/ no side-effect. I'm not sure that's
necessary, especially if we can come up with a simpler rule.

>
> On the same note, I am a little worried that having this
> in the grammar will encourage other parser implementers
> to implement the rule, which will limit the generality of
> their parsers.  But it does look correct.

Are you worried about the exported ParseStmtList not being useful
enough anymore (say for an interpreter), or is it some other concern?
- gri

>
> Russ
>

[rsc, 2011-04-21 19:04:04]

> What is your rationale for it? If the grammar has this restriction,
> the parser should enforce it, I would think.
> Maybe the grammar should not have this restriction, but I think it is
> easily syntactically enforced.

Yes, it is easily syntactically enforced.  But the higher up you
give the error, the more useful you can make it, and the more
uniform you can make the errors.  This is a semantic rule
(can't compute something without side effects and not use it),
and even though it can be partially expressed syntactically
that doesn't mean it's the right place to implement the restriction.
Delaying the rejection means you can identify more problems
in a single pass through the compiler.

Consider this program:

    package main

    func g(int)
    func f() (int, int)

    func main() {
        x = f()  // oops, forgot that f returns two arguments
        println(x)
        g-(x)  // oops, meant g(-x)
    }

I think everyone can look at this program and see what the
Go AST would be for it.  If the parser accepts it, then the
type checker can tell the programmer about both errors.
If the parser rejects it early, you only find out about one.

To take another example,
    x, y, z = m[k]
is definitely an invalid program, because a map index assignment
cannot have 3 expressions on the lhs.  But it's not a syntax error
(I think the parser did reject it once but I asked for a change),
for much the same reasons as above.  We all still know what
the AST looks like, and if we let it go through, a tool can
point out more problems (or process) the program instead of
failing.  Also, in this case, because the parser can't possibly reject
    x, y, z = f()  // oops, f only returns two values
and you'd want to have a compiler or other tool print the same
error message in both cases, it makes sense to leave the
check to the higher levels.

Back to the current case, especially since I believe the same
error message is going to apply to a statement-level len(x)
as applies to +x (and the parser can't reasonably reject len(x)),
I think the parser should let both through and let higher level
processing take care of them.

> Are you worried about the exported ParseStmtList not being useful
> enough anymore (say for an interpreter), or is it some other concern?

I'm worried about generality in general more than specifics.
I don't see any benefit to the parser excluding side effect-free
expression statements since it feels like a semantic concern
and since higher level code is going to have to deal with some
other cases anyway.

That said, the case you brought up (for exp/eval) is a good
specific reason not to restrict the parser.

Russ

[gri, 2011-04-22 00:35:51]

On Thu, Apr 21, 2011 at 12:04 PM, Russ Cox <rsc@golang.org> wrote:
>> What is your rationale for it? If the grammar has this restriction,
>> the parser should enforce it, I would think.
>> Maybe the grammar should not have this restriction, but I think it is
>> easily syntactically enforced.
>
> Yes, it is easily syntactically enforced.  But the higher up you
> give the error, the more useful you can make it, and the more

sure, I am fully aware of that

At some point it's a judgement call where to best put an error.
There's other error messages in the parser that could be delivered
better and later. The rule I have tried to follow is simple: if it's
in the syntax, the parser should complain. That's the only reason for
the checks in the parser.


> uniform you can make the errors.  This is a semantic rule
> (can't compute something without side effects and not use it),
> and even though it can be partially expressed syntactically
> that doesn't mean it's the right place to implement the restriction.

Originally it was not a semantic rule. It was a syntactic rule with a
semantic implication. The "is used" rule came later. But that's not
the point, really.

> Delaying the rejection means you can identify more problems
> in a single pass through the compiler.

Sure. But that was not the primary design goal of all these packages.
The goal for the parser was to reject anything that is not
syntactically correct.

>
> Consider this program:
>
>    package main
>
>    func g(int)
>    func f() (int, int)
>
>    func main() {
>        x = f()  // oops, forgot that f returns two arguments
>        println(x)
>        g-(x)  // oops, meant g(-x)
>    }
>
> I think everyone can look at this program and see what the
> Go AST would be for it.  If the parser accepts it, then the
> type checker can tell the programmer about both errors.
> If the parser rejects it early, you only find out about one.
>
> To take another example,
>    x, y, z = m[k]
> is definitely an invalid program, because a map index assignment
> cannot have 3 expressions on the lhs.  But it's not a syntax error
> (I think the parser did reject it once but I asked for a change),
> for much the same reasons as above.  We all still know what
> the AST looks like, and if we let it go through, a tool can
> point out more problems (or process) the program instead of
> failing.  Also, in this case, because the parser can't possibly reject
>    x, y, z = f()  // oops, f only returns two values
> and you'd want to have a compiler or other tool print the same
> error message in both cases, it makes sense to leave the
> check to the higher levels.

I am not disputing this in any way.

>
> Back to the current case, especially since I believe the same
> error message is going to apply to a statement-level len(x)
> as applies to +x (and the parser can't reasonably reject len(x)),
> I think the parser should let both through and let higher level
> processing take care of them.

We don't have a spec rule about what "is used" means yet. gccgo and gc
have different answers. But we have a trivial syntactic rule.

>
>> Are you worried about the exported ParseStmtList not being useful
>> enough anymore (say for an interpreter), or is it some other concern?
>
> I'm worried about generality in general more than specifics.
> I don't see any benefit to the parser excluding side effect-free
> expression statements since it feels like a semantic concern
> and since higher level code is going to have to deal with some
> other cases anyway.

Ok. Perhaps the syntax should not restrict the kinds of expressions
permitted in a statement (like before), and the explanation should not
even mention calls and receive operations but instead refer to a more
general is-used rule. I would be fine with that.

I am going to let this rest until we have an is-used rule that we
believe and that can be implemented.

- Robert


>
> That said, the case you brought up (for exp/eval) is a good
> specific reason not to restrict the parser.
>
> Russ
>

[gri, 2011-04-28 21:54:40]

Hello rsc, r, iant (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2011-04-28 22:19:30]

LGTM

[gri, 2011-04-29 16:49:26]

*** Submitted as http://code.google.com/p/go/source/detail?r=19e540fc7d7d ***

spec: narrow syntax for expression and select statements

This is not a language change, it simply expresses the
accepted cases explicitly in the respective productions.

R=rsc, r, iant
CC=golang-dev
http://codereview.appspot.com/4428057

[rsc, 2011-04-29 17:38:14]

This is a language change.

package main

import "fmt"

func main() {
	c := make(chan int)
	select {
	case x := (<-c):
		fmt.Println(x)
	}
}

is now invalid but is accepted by 6g at least.

[gri, 2011-04-29 18:07:04]

Maybe it's a language change; but more likely the wording in the spec
was not precise enough (hence the CL):

For instance, gccgo doesn't accept your example (but takes it w/o the
parentheses).

gccgo test.go
test.go:8:18: error: expected '<-'
test.go:8:18: error: expected colon
test.go:8:23: error: expected ';' or '}' or newline

I am fine with the original syntax, with extra words making this clear
(and perhaps an example).

Rob?
- Robert

On Fri, Apr 29, 2011 at 10:38 AM, Russ Cox <rsc@golang.org> wrote:
> This is a language change.
>
> package main
>
> import "fmt"
>
> func main() {
>        c := make(chan int)
>        select {
>        case x := (<-c):
>                fmt.Println(x)
>        }
> }
>
> is now invalid but is accepted by 6g at least.
>

[rsc, 2011-04-29 18:08:45]

On Fri, Apr 29, 2011 at 14:07, Robert Griesemer <gri@golang.org> wrote:
> Maybe it's a language change; but more likely the wording in the spec
> was not precise enough (hence the CL):
>
> For instance, gccgo doesn't accept your example (but takes it w/o the
> parentheses).
>
> gccgo test.go
> test.go:8:18: error: expected '<-'
> test.go:8:18: error: expected colon
> test.go:8:23: error: expected ';' or '}' or newline
>
> I am fine with the original syntax, with extra words making this clear
> (and perhaps an example).

gofmt doesn't take it either.  i will fix 6g

[gri, 2011-04-29 18:38:52]

Wait, though. The CL also implies that the expression statement

(<-c) // as opposed to: <-c

is not legal anymore. gccgo accepts that. I'm not sure that two
(expression statements, select) have to be in sync, but I am starting
to think that perhaps they should. That is, the current CL should be
reverted in favor of the old wording with some examples for
clarification.

For instance, for assignments, the LHS must be of a certain form, but
we don't express that syntactically, either (and ()'s are ok).

Anyway, I think it doesn't much matter, but consistency would be good.
Preferences?
- Robert

On Fri, Apr 29, 2011 at 11:08 AM, Russ Cox <rsc@golang.org> wrote:
> On Fri, Apr 29, 2011 at 14:07, Robert Griesemer <gri@golang.org> wrote:
>> Maybe it's a language change; but more likely the wording in the spec
>> was not precise enough (hence the CL):
>>
>> For instance, gccgo doesn't accept your example (but takes it w/o the
>> parentheses).
>>
>> gccgo test.go
>> test.go:8:18: error: expected '<-'
>> test.go:8:18: error: expected colon
>> test.go:8:23: error: expected ';' or '}' or newline
>>
>> I am fine with the original syntax, with extra words making this clear
>> (and perhaps an example).
>
> gofmt doesn't take it either.  i will fix 6g
>

[rsc, 2011-04-29 18:49:23]

(f()) at top level has the same problem.

Another problem with the new grammar is that
as an expression <-f() means <-(f()) but the new
grammar says that at top level <-f() means (<-f)().

I agree: revert the CL.  You can use hg undo 4428057.

Russ