code review 6585063: go/ast: track position of <- for channel types

go/ast: track position of <- for channel types

This is a backward-compatible API change.

Without the correct <- position information,
certain channel types have incorrect position
information.

[gri, 2012-10-03 00:38:59]

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

I'd like you to review this change to
https://code.google.com/p/go

[iant2, 2012-10-03 00:47:34]

LGTM

On Tue, Oct 2, 2012 at 5:38 PM,  <gri@golang.org> wrote:
> Reviewers: iant,
>
> Message:
> Hello iant@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> go/ast: track position of <- for channel types
>
> This is a backward-compatible API change.
>
> Without the correct <- position information,
> certain channel types have incorrect position
> information.
>
> Please review this at http://codereview.appspot.com/6585063/
>
> Affected files:
>   M src/pkg/go/ast/ast.go
>   M src/pkg/go/parser/parser.go
>   M src/pkg/go/parser/short_test.go
>   M src/pkg/go/printer/nodes.go
>
>
> Index: src/pkg/go/ast/ast.go
> ===================================================================
> --- a/src/pkg/go/ast/ast.go
> +++ b/src/pkg/go/ast/ast.go
> @@ -407,6 +407,7 @@
>         // A ChanType node represents a channel type.
>         ChanType struct {
>                 Begin token.Pos // position of "chan" keyword or "<-"
> (whichever comes first)
> +               Arrow token.Pos // position of "<-" (noPos if there is no
> "<-")
>                 Dir   ChanDir   // channel direction
>                 Value Expr      // value type
>         }
> Index: src/pkg/go/parser/parser.go
> ===================================================================
> --- a/src/pkg/go/parser/parser.go
> +++ b/src/pkg/go/parser/parser.go
> @@ -924,20 +924,22 @@
>
>         pos := p.pos
>         dir := ast.SEND | ast.RECV
> +       var arrow token.Pos
>         if p.tok == token.CHAN {
>                 p.next()
>                 if p.tok == token.ARROW {
> +                       arrow = p.pos
>                         p.next()
>                         dir = ast.SEND
>                 }
>         } else {
> -               p.expect(token.ARROW)
> +               arrow = p.expect(token.ARROW)
>                 p.expect(token.CHAN)
>                 dir = ast.RECV
>         }
>         value := p.parseType()
>
> -       return &ast.ChanType{Begin: pos, Dir: dir, Value: value}
> +       return &ast.ChanType{Begin: pos, Arrow: arrow, Dir: dir, Value:
> value}
>  }
>
>  // If the result is an identifier, it is not resolved.
> @@ -1397,7 +1399,7 @@
>
>         case token.ARROW:
>                 // channel type or receive expression
> -               pos := p.pos
> +               arrow := p.pos
>                 p.next()
>
>                 // If the next token is token.CHAN we still don't know if it
> @@ -1421,29 +1423,25 @@
>                         // (<-type)
>
>                         // re-associate position info and <-
> -                       arrow := true
> -                       for ok && arrow {
> -                               begin := typ.Begin
> +                       dir := ast.SEND
> +                       for ok && dir == ast.SEND {
>                                 if typ.Dir == ast.RECV {
>                                         // error: (<-type) is (<-(<-chan T))
> -                                       p.errorExpected(begin, "'chan'")
> +                                       p.errorExpected(typ.Arrow, "'chan'")
>                                 }
> -                               arrow = typ.Dir == ast.SEND
> -                               typ.Begin = pos
> -                               typ.Dir = ast.RECV
> +                               arrow, typ.Begin, typ.Arrow = typ.Arrow,
> arrow, arrow
> +                               dir, typ.Dir = typ.Dir, ast.RECV
>                                 typ, ok = typ.Value.(*ast.ChanType)
> -                               // TODO(gri) ast.ChanType should store exact
> <- position
> -                               pos = begin // estimate (we don't have the
> exact position of <- for send channels)
>                         }
> -                       if arrow {
> -                               p.errorExpected(pos, "'chan'")
> +                       if dir == ast.SEND {
> +                               p.errorExpected(arrow, "channel type")
>                         }
>
>                         return x
>                 }
>
>                 // <-(expr)
> -               return &ast.UnaryExpr{OpPos: pos, Op: token.ARROW, X:
> p.checkExpr(x)}
> +               return &ast.UnaryExpr{OpPos: arrow, Op: token.ARROW, X:
> p.checkExpr(x)}
>
>         case token.MUL:
>                 // pointer type or unary "*" expression
> Index: src/pkg/go/parser/short_test.go
> ===================================================================
> --- a/src/pkg/go/parser/short_test.go
> +++ b/src/pkg/go/parser/short_test.go
> @@ -69,7 +69,7 @@
>         `package p; var a = <- /* ERROR "expected expression" */ chan int;`,
>         `package p; func f() { select { case _ <- chan /* ERROR "expected
> expression" */ int: } };`,
>         `package p; func f() { _ = (<-<- /* ERROR "expected 'chan'" */ chan
> int)(nil) };`,
> -       `package p; func f() { _ = (<-chan<-chan<-chan<-chan<-chan /* ERROR
> "expected 'chan'" */ <-int)(nil) };`,
> +       `package p; func f() { _ = (<-chan<-chan<-chan<-chan<-chan<- /*
> ERROR "expected channel type" */ int)(nil) };`,
>  }
>
>  func TestInvalid(t *testing.T) {
> Index: src/pkg/go/printer/nodes.go
> ===================================================================
> --- a/src/pkg/go/printer/nodes.go
> +++ b/src/pkg/go/printer/nodes.go
> @@ -853,9 +853,9 @@
>                 case ast.SEND | ast.RECV:
>                         p.print(token.CHAN)
>                 case ast.RECV:
> -                       p.print(token.ARROW, token.CHAN)
> +                       p.print(token.ARROW, token.CHAN) // x.Arrow and
> x.Pos() are the same
>                 case ast.SEND:
> -                       p.print(token.CHAN, token.ARROW)
> +                       p.print(token.CHAN, x.Arrow, token.ARROW)
>                 }
>                 p.print(blank)
>                 p.expr(x.Value)
>
>

[gri, 2012-10-03 00:50:43]

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

go/ast: track position of <- for channel types

This is a backward-compatible API change.

Without the correct <- position information,
certain channel types have incorrect position
information.

R=iant, iant
CC=golang-dev
http://codereview.appspot.com/6585063