code review 7429047: go/types: fixed a few failure checks

go/types: fixed a few failure checks

More robustness in case of incorrect programs.

Fixes issue 4962.

[gri, 2013-03-02 00:38:34]

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

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

[dsymonds, 2013-03-02 01:18:17]

LGTM, though I don't know this code. It looks plausible.

It presumably fixes issue 4962; that's what I care about.

[gri, 2013-03-02 01:30:18]

It does fix issue 4962.

The culprit was this missing check:

        case *ast.IndexExpr:
                check.expr(x, e.X, nil, iota)
+               if x.mode == invalid {
+                       goto Error
+               }

after which it continued with some random x.typ which you saw. In turn, I
went through the code and checked for similar situations.

There's a TODO to streamline error handling, but this it not the CL.

Thanks for the bug.
- gri


On Fri, Mar 1, 2013 at 5:18 PM, David Symonds <dsymonds@golang.org> wrote:

> LGTM, though I don't know this code. It looks plausible.
>
> It presumably fixes issue 4962; that's what I care about.
>




On Fri, Mar 1, 2013 at 4:38 PM, <gri@golang.org> wrote:

> Reviewers: dsymonds,
>
> Message:
> Hello dsymonds@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> go/types: fixed a few failure checks
>
> More robustness in case of incorrect programs.
>
> Fixes issue 4962.
>
> Please review this at
https://codereview.appspot.**com/7429047/<https://codereview.appspot.com/7429...
>
> Affected files:
>   M src/pkg/go/types/expr.go
>   M src/pkg/go/types/stmt.go
>
>
> Index: src/pkg/go/types/expr.go
> ==============================**==============================**=======
> --- a/src/pkg/go/types/expr.go
> +++ b/src/pkg/go/types/expr.go
> @@ -550,6 +550,15 @@
>         check.expr(x, lhs, nil, iota)
>         check.expr(&y, rhs, nil, iota)
>
> +       if x.mode == invalid {
> +               return
> +       }
> +       if y.mode == invalid {
> +               x.mode = invalid
> +               x.expr = y.expr
> +               return
> +       }
> +
>         if isShift(op) {
>                 check.shift(x, &y, op)
>                 return
> @@ -1089,6 +1098,9 @@
>
>         case *ast.IndexExpr:
>                 check.expr(x, e.X, nil, iota)
> +               if x.mode == invalid {
> +                       goto Error
> +               }
>
>                 valid := false
>                 length := int64(-1) // valid if >= 0
> @@ -1130,9 +1142,9 @@
>                 case *Map:
>                         var key operand
>                         check.expr(&key, e.Index, nil, iota)
> -                       if key.mode == invalid || !check.assignment(&key,
> typ.Key) {
> -                               if x.mode != invalid {
> -                                       check.invalidOp(x.pos(), "cannot
> use %s as map index of type %s", &key, typ.Key)
> +                       if !check.assignment(&key, typ.Key) {
> +                               if key.mode != invalid {
> +                                       check.invalidOp(key.pos(), "cannot
> use %s as map index of type %s", &key, typ.Key)
>                                 }
>                                 goto Error
>                         }
> @@ -1157,6 +1169,9 @@
>
>         case *ast.SliceExpr:
>                 check.expr(x, e.X, nil, iota)
> +               if x.mode == invalid {
> +                       goto Error
> +               }
>
>                 valid := false
>                 length := int64(-1) // valid if >= 0
> @@ -1367,10 +1382,19 @@
>
>         case *ast.UnaryExpr:
>                 check.expr(x, e.X, nil, iota)
> +               if x.mode == invalid {
> +                       goto Error
> +               }
>                 check.unary(x, e.Op)
> +               if x.mode == invalid {
> +                       goto Error
> +               }
>
>         case *ast.BinaryExpr:
>                 check.binary(x, e.X, e.Y, e.Op, iota)
> +               if x.mode == invalid {
> +                       goto Error
> +               }
>
>         case *ast.KeyValueExpr:
>                 // key:value expressions are handled in composite literals
> @@ -1423,7 +1447,9 @@
>                 x.typ = &Chan{Dir: e.Dir, Elt: check.typ(e.Value, true)}
>
>         default:
> -               check.dump("e = %s", e)
> +               if debug {
> +                       check.dump("expr = %v (%T)", e, e)
> +               }
>                 unreachable()
>         }
>
> Index: src/pkg/go/types/stmt.go
> ==============================**==============================**=======
> --- a/src/pkg/go/types/stmt.go
> +++ b/src/pkg/go/types/stmt.go
> @@ -18,6 +18,10 @@
>  // TODO(gri) This latter behavior is for historic reasons and complicates
>  // callers. Needs to be cleaned up.
>  func (check *checker) assignment(x *operand, to Type) bool {
> +       if x.mode == invalid {
> +               return false
> +       }
> +
>         if t, ok := x.typ.(*Result); ok {
>                 // TODO(gri) elsewhere we use "assignment count mismatch"
> (consolidate)
>                 check.errorf(x.pos(), "%d-valued expression %s used as
> single value", len(t.Values), x)
>
>
>

[gri, 2013-03-02 01:39:52]

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

go/types: fixed a few failure checks

More robustness in case of incorrect programs.

Fixes issue 4962.

R=dsymonds
CC=golang-dev
https://codereview.appspot.com/7429047