code review 3989056: go/ast: reflect communication operator changes accurate...

go/ast: reflect communication operator changes accurately in ast

- go/ast: introduce SendStmt; adjust SelectStmt
- go/parser: accept new communication syntax, minor
  unrelated cleanups
- go/printer: adjustments for new ast, fewer binary
  expression precedences
- go/token: remove one binary precedence

Adjusted dependent code. gofmt -w src -misc. Ran all tests.

[gri, 2011-02-01 03:31:01]

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

I'd like you to review this change.

[rsc, 2011-02-01 12:47:13]

http://codereview.appspot.com/3989056/diff/2001/src/pkg/go/ast/ast.go
File src/pkg/go/ast/ast.go (right):

http://codereview.appspot.com/3989056/diff/2001/src/pkg/go/ast/ast.go#newcode640
src/pkg/go/ast/ast.go:640: Comm  Stmt      // send or receive statement
// send or receive statement; nil means default

?

http://codereview.appspot.com/3989056/diff/2001/src/pkg/go/ast/walk.go
File src/pkg/go/ast/walk.go (right):

http://codereview.appspot.com/3989056/diff/2001/src/pkg/go/ast/walk.go#newcod...
src/pkg/go/ast/walk.go:261: Walk(v, n.Comm)
if n.Comm != nil {
?

http://codereview.appspot.com/3989056/diff/2001/src/pkg/go/parser/parser.go
File src/pkg/go/parser/parser.go (right):

http://codereview.appspot.com/3989056/diff/2001/src/pkg/go/parser/parser.go#n...
src/pkg/go/parser/parser.go:1499: // RecvStmt without assignment
I might be reading this wrong, but it looks like at
this point it is not guaranteed that this is a receive case.
Consider:

package main

func main() {
    c := make(chan chan int, 1)
    c <- make(chan int, 1)
    select {
    case <-c <- 2:
        println("sent")
    }
}

I think the code should call p.parseExprList unconditionally
and distinguish the cases by looking at the token that follows.

[gri1, 2011-02-01 18:25:45]

PTAL

http://codereview.appspot.com/3989056/diff/2001/src/pkg/go/ast/ast.go
File src/pkg/go/ast/ast.go (right):

http://codereview.appspot.com/3989056/diff/2001/src/pkg/go/ast/ast.go#newcode640
src/pkg/go/ast/ast.go:640: Comm  Stmt      // send or receive statement
On 2011/02/01 12:47:13, rsc wrote:
> // send or receive statement; nil means default
> 
> ?

Done.

http://codereview.appspot.com/3989056/diff/2001/src/pkg/go/ast/walk.go
File src/pkg/go/ast/walk.go (right):

http://codereview.appspot.com/3989056/diff/2001/src/pkg/go/ast/walk.go#newcod...
src/pkg/go/ast/walk.go:261: Walk(v, n.Comm)
On 2011/02/01 12:47:13, rsc wrote:
> if n.Comm != nil {
> ?
of course. good catch.

http://codereview.appspot.com/3989056/diff/2001/src/pkg/go/parser/parser.go
File src/pkg/go/parser/parser.go (right):

http://codereview.appspot.com/3989056/diff/2001/src/pkg/go/parser/parser.go#n...
src/pkg/go/parser/parser.go:1499: // RecvStmt without assignment
On 2011/02/01 12:47:13, rsc wrote:
> I might be reading this wrong, but it looks like at
> this point it is not guaranteed that this is a receive case.
> Consider:
> 
> package main
> 
> func main() {
>     c := make(chan chan int, 1)
>     c <- make(chan int, 1)
>     select {
>     case <-c <- 2:
>         println("sent")
>     }
> }
> 
> I think the code should call p.parseExprList unconditionally
> and distinguish the cases by looking at the token that follows.

You are correct. This was wrong before (it parsed the code but with incorrect
precedence).

[rsc, 2011-02-01 19:13:22]

http://codereview.appspot.com/3989056/diff/14001/src/pkg/go/parser/parser.go
File src/pkg/go/parser/parser.go (right):

http://codereview.appspot.com/3989056/diff/14001/src/pkg/go/parser/parser.go#...
src/pkg/go/parser/parser.go:1522: arrow := p.expect(token.ARROW)
Again I'm not sure that I'm reading this right, but what about

case <-1+2

Obviously that can't type check but I believe it should parse
as (<-1)+2 while this code would parse it as <-(1+2).
I think that, like in the last change, this should call p.parseExprList
and then check that len(list) == 1 and list[0] is a UnaryExpr with token.ARROW.

The interpretation I am proposing is different from the spec
(the code here now agrees with the spec).  I think the spec
should be changed too.  The rationale is that otherwise the
AssignStmts in:

x = <-1+2
case x = <-1+2

have different parse trees.

Similarly

case <-1+2
case x = <-1+2

would have different parse trees for the expression.

It may never come up (unless we allow operator methods),
but we should probably be consistent nonetheless.

http://codereview.appspot.com/3989056/diff/14001/src/pkg/go/parser/parser.go#...
src/pkg/go/parser/parser.go:1535: p.errorExpected(lhs.Pos(), "receive
operation")
s/receive/send or receive/

(depends on what the user intended.
select admits both.  otherwise writing

case x < 1:

(< instead of <-) will say receive operation expected.

[gri1, 2011-02-01 21:25:38]

PTAL

http://codereview.appspot.com/3989056/diff/14001/src/pkg/go/parser/parser.go
File src/pkg/go/parser/parser.go (right):

http://codereview.appspot.com/3989056/diff/14001/src/pkg/go/parser/parser.go#...
src/pkg/go/parser/parser.go:1522: arrow := p.expect(token.ARROW)
On 2011/02/01 19:13:22, rsc wrote:
> Again I'm not sure that I'm reading this right, but what about
> 
> case <-1+2
> 
> Obviously that can't type check but I believe it should parse
> as (<-1)+2 while this code would parse it as <-(1+2).
> I think that, like in the last change, this should call p.parseExprList
> and then check that len(list) == 1 and list[0] is a UnaryExpr with
token.ARROW.
> 
> The interpretation I am proposing is different from the spec
> (the code here now agrees with the spec).  I think the spec
> should be changed too.  The rationale is that otherwise the
> AssignStmts in:
> 
> x = <-1+2
> case x = <-1+2
> 
> have different parse trees.
> 
> Similarly
> 
> case <-1+2
> case x = <-1+2
> 
> would have different parse trees for the expression.
> 
> It may never come up (unless we allow operator methods),
> but we should probably be consistent nonetheless.

Done.

http://codereview.appspot.com/3989056/diff/14001/src/pkg/go/parser/parser.go#...
src/pkg/go/parser/parser.go:1535: p.errorExpected(lhs.Pos(), "receive
operation")
On 2011/02/01 19:13:22, rsc wrote:
> s/receive/send or receive/
> 
> (depends on what the user intended.
> select admits both.  otherwise writing
> 
> case x < 1:
> 
> (< instead of <-) will say receive operation expected.
> 

Done.

[rsc, 2011-02-01 21:31:53]

LGTM

[gri, 2011-02-01 21:47:55]

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

go/ast: reflect communication operator changes accurately in ast

- go/ast: introduce SendStmt; adjust SelectStmt
- go/parser: accept new communication syntax, minor
  unrelated cleanups
- go/printer: adjustments for new ast, fewer binary
  expression precedences
- go/token: remove one binary precedence

Adjusted dependent code. gofmt -w src -misc. Ran all tests.

R=rsc, gri
CC=golang-dev
http://codereview.appspot.com/3989056

Committer: Robert Griesemer <gri@golang.org>