code review 6846131: exp/types: checking of type switches and range clauses

exp/types: checking of type switches and range clauses

Also:
- better handling of type assertions
- implemented built-in error type
- first cut at handling variadic function signatures
- several bug fixes

[gri, 2012-11-30 04:06:23]

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

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

[rog, 2012-11-30 12:14:35]

https://codereview.appspot.com/6846131/diff/7002/src/pkg/exp/types/stmt.go
File src/pkg/exp/types/stmt.go (right):

https://codereview.appspot.com/6846131/diff/7002/src/pkg/exp/types/stmt.go#ne...
src/pkg/exp/types/stmt.go:531: lhs.Type = typ
doesn't this end up with the AST holding invalid types in all clauses but the
last one?

i suspect that if i typechecked this code and looked at the Object pointed to by
the t argument to foo, i'd find it was string.

  switch t := interface{}(5).(type) {
  case int:
       foo(t)
  case string:
       bar(t)
  }

[gri, 2012-11-30 17:55:54]

PTAL.

https://codereview.appspot.com/6846131/diff/7002/src/pkg/exp/types/stmt.go
File src/pkg/exp/types/stmt.go (right):

https://codereview.appspot.com/6846131/diff/7002/src/pkg/exp/types/stmt.go#ne...
src/pkg/exp/types/stmt.go:531: lhs.Type = typ
On 2012/11/30 12:14:35, rog wrote:
> doesn't this end up with the AST holding invalid types in all clauses but the
> last one?
> 
> i suspect that if i typechecked this code and looked at the Object pointed to
by
> the t argument to foo, i'd find it was string.
> 
>   switch t := interface{}(5).(type) {
>   case int:
>        foo(t)
>   case string:
>        bar(t)
>   }

That's correct - I was thinking about that last night. At the moment there's
only one t object associated with all identifiers t. One could change that at
the cost of an extra walk. However, the types associated with expressions as
reported with the function f passed into types.Check is still correct (test
needs to be added).

I'm going to set the type to nil for now so that it cannot be used by mistake,
and added a comment.

In hindsight it would be better to not augment the AST at all (it leads to a
cumbersome split between work done by the parser and work done here), and
instead do it all in the type checker. It will be cleaner and lead to a leaner
AST as well. It would also not raise questions as to what happens with scopes,
objects, etc. if one manipulates the AST manually. It's best to rely on the
results reported by the type checker if one is interested in the types. This
will permit cleaning up the AST in a next phase (i.e., moving augmentation out).

[gri, 2012-12-01 01:24:41]

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

Please take another look.

[gri, 2012-12-03 03:54:13]

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

Please take another look.

[gri, 2012-12-05 06:47:08]

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

Please take another look.

[rsc, 2012-12-06 06:24:26]

LGTM

[gri, 2012-12-06 17:21:50]

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

exp/types: checking of type switches and range clauses

Also:
- better handling of type assertions
- implemented built-in error type
- first cut at handling variadic function signatures
- several bug fixes

R=rsc, rogpeppe
CC=golang-dev
https://codereview.appspot.com/6846131