code review 68920044: go/parser: report error if ParseExpr argument contains ...

go/parser: report error if ParseExpr argument contains extra tokens

This partly addresses issue 6099 where a gofmt rewrite is behaving
unexpectedly because the provided rewrite term is not a valid expression
but is silently consumed anyway.

[gri, 2014-02-26 17:37:49]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-02-26 17:48:38]

LGTM



On Wed, Feb 26, 2014 at 9:37 AM, <gri@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> go/parser: report error if ParseExpr argument contains extra tokens
>
> This partly addresses issue 6099 where a gofmt rewrite is behaving
> unexpectedly because the provided rewrite term is not a valid expression
> but is silently consumed anyway.
>
> Please review this at https://codereview.appspot.com/68920044/
>
> Affected files (+18, -4 lines):
>   M src/pkg/go/parser/interface.go
>   M src/pkg/go/parser/parser_test.go
>
>
> Index: src/pkg/go/parser/interface.go
> ===================================================================
> --- a/src/pkg/go/parser/interface.go
> +++ b/src/pkg/go/parser/interface.go
> @@ -182,6 +182,13 @@
>         p.closeScope()
>         assert(p.topScope == nil, "unbalanced scopes")
>
> +       // If a semicolon was inserted, consume it;
> +       // report an error if there's more tokens.
> +       if p.tok == token.SEMICOLON {
> +               p.next()
> +       }
> +       p.expect(token.EOF)
> +
>         if p.errors.Len() > 0 {
>                 p.errors.Sort()
>                 return nil, p.errors.Err()
> Index: src/pkg/go/parser/parser_test.go
> ===================================================================
> --- a/src/pkg/go/parser/parser_test.go
> +++ b/src/pkg/go/parser/parser_test.go
> @@ -78,7 +78,7 @@
>         }
>         // sanity check
>         if _, ok := x.(*ast.BinaryExpr); !ok {
> -               t.Errorf("ParseExpr(%s): got %T, expected
> *ast.BinaryExpr", src, x)
> +               t.Errorf("ParseExpr(%s): got %T, want *ast.BinaryExpr",
> src, x)
>         }
>
>         // a valid type expression
> @@ -89,17 +89,24 @@
>         }
>         // sanity check
>         if _, ok := x.(*ast.StructType); !ok {
> -               t.Errorf("ParseExpr(%s): got %T, expected
> *ast.StructType", src, x)
> +               t.Errorf("ParseExpr(%s): got %T, want *ast.StructType",
> src, x)
>         }
>
>         // an invalid expression
>         src = "a + *"
>         _, err = ParseExpr(src)
>         if err == nil {
> -               t.Fatalf("ParseExpr(%s): %v", src, err)
> +               t.Fatalf("ParseExpr(%s): got no error", src)
>         }
>
> -       // it must not crash
> +       // a valid expression followed by extra tokens is invalid
> +       src = "a[i] := x"
> +       _, err = ParseExpr(src)
> +       if err == nil {
> +               t.Fatalf("ParseExpr(%s): got no error", src)
> +       }
> +
> +       // ParseExpr must not crash
>         for _, src := range valids {
>                 ParseExpr(src)
>         }
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[gri, 2014-02-26 17:54:05]

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

go/parser: report error if ParseExpr argument contains extra tokens

This partly addresses issue 6099 where a gofmt rewrite is behaving
unexpectedly because the provided rewrite term is not a valid expression
but is silently consumed anyway.

LGTM=bradfitz
R=golang-codereviews, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/68920044

[gobot, 2014-02-26 23:56:13]

This CL appears to have broken the linux-arm-cheney-panda builder.