go.tools/go/types: Add Info parameter to EvalNode, remo...

go.tools/go/types: Add Info parameter to EvalNode, remove results, rename to CheckExpr, remove eval.go.

Fixes issue 7151.

[gmk, 2014-02-05 18:40:43]

I shouldn't have just blown away eval_test.go.  Resuscitating...

[gmk, 2014-02-05 19:59:38]

On 2014/02/05 18:40:43, gmk wrote:
> I shouldn't have just blown away eval_test.go.  Resuscitating...

eval_test.go came back as checkexpr_test.go

[gmk, 2014-02-09 13:00:56]

https://codereview.appspot.com/60110044/diff/80001/go/types/object.go
File go/types/object.go (right):

https://codereview.appspot.com/60110044/diff/80001/go/types/object.go#newcode106
go/types/object.go:106: // introduced via CheckExpr)
pkg == nil for fields of unnamed struct types and methods of unnamed interfaces
types, right?  Which can happen not only in Eval/CheckExpr.

[gmk, 2014-02-09 16:41:16]

https://codereview.appspot.com/60110044/diff/80001/go/types/object.go
File go/types/object.go (right):

https://codereview.appspot.com/60110044/diff/80001/go/types/object.go#newcode106
go/types/object.go:106: // introduced via CheckExpr)
On 2014/02/09 13:00:57, gmk wrote:
> pkg == nil for fields of unnamed struct types and methods of unnamed
interfaces
> types, right?  Which can happen not only in Eval/CheckExpr.

Never mind, that's not true.

[adonovan, 2014-02-09 18:53:49]

Generally looks good, but wait for gri's review.

FYI: I didn't see the mail for this until today, even though I'm on the
reviewers list.  Was I a late addition, or is something else misconfigured?

https://codereview.appspot.com/60110044/diff/80001/cmd/vet/types.go
File cmd/vet/types.go (right):

https://codereview.appspot.com/60110044/diff/80001/cmd/vet/types.go#newcode76
cmd/vet/types.go:76: // Create a file set that looks structurally identical to
the
I wish we had versions of parser.ParseExpr that accepted a *token.FileSet
parameter.
Perhaps we could add a little utility function to go.tools/astutil that does
that.
It may be tricky because ParseExpr is not just a convenience function, i.e. it
can't be expressed only in terms of the API.

https://codereview.appspot.com/60110044/diff/80001/cmd/vet/types.go#newcode82
cmd/vet/types.go:82: err = types.CheckExpr(x, fset, nil, nil, info)
Use the smallest scope for 'err' where convenient:

if err := ...; err != nil {

https://codereview.appspot.com/60110044/diff/80001/go/pointer/pointer_test.go
File go/pointer/pointer_test.go (left):

https://codereview.appspot.com/60110044/diff/80001/go/pointer/pointer_test.go...
go/pointer/pointer_test.go:224: // Don't print err since its location is bad.
What has changed to render this comment obsolete?

https://codereview.appspot.com/60110044/diff/80001/go/pointer/testdata/mapref...
File go/pointer/testdata/mapreflect.go (left):

https://codereview.appspot.com/60110044/diff/80001/go/pointer/testdata/mapref...
go/pointer/testdata/mapreflect.go:49: // types.EvalNode won't let us refer to
non-exported types:
This comment needs to be updated; the underlying problem still exists.
(If it was solved, the correct change here would be to uncomment the assertion
in the comment, i.e remove the '#'.)

// types.CheckExpr won't let us...

https://codereview.appspot.com/60110044/diff/80001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/60110044/diff/80001/go/types/api.go#newcode49
go/types/api.go:49: // CheckExpr type-checks an expression in the context of a
package and scope.
I like this name much better than Eval.

https://codereview.appspot.com/60110044/diff/80001/go/types/api.go#newcode65
go/types/api.go:65: // s == nil || s == pkg.scope
Redundant comment; no need to state what's obvious from the control flow.

https://codereview.appspot.com/60110044/diff/80001/go/types/api.go#newcode82
go/types/api.go:82: // evaluate node
s/evaluate/type-check/

https://codereview.appspot.com/60110044/diff/80001/go/types/api.go#newcode90
go/types/api.go:90: check.delayed = nil // not needed anymore
Delete this?  check is about to become unreachable.

https://codereview.appspot.com/60110044/diff/80001/go/types/checkexpr_test.go
File go/types/checkexpr_test.go (right):

https://codereview.appspot.com/60110044/diff/80001/go/types/checkexpr_test.go...
go/types/checkexpr_test.go:33: err = CheckExpr(x, fset, pkg, scope, info)
if err := ...; err != nil {

[gmk, 2014-02-09 20:10:49]

On 2014/02/09 18:53:49, adonovan wrote:
> Generally looks good, but wait for gri's review.
> 
> FYI: I didn't see the mail for this until today, even though I'm on the
> reviewers list.  Was I a late addition, or is something else misconfigured?

You were both late additions.  I started the CL as a proof-of-concept, then
later decided to mail it.

[gmk, 2014-02-09 20:11:26]

https://codereview.appspot.com/60110044/diff/80001/cmd/vet/types.go
File cmd/vet/types.go (right):

https://codereview.appspot.com/60110044/diff/80001/cmd/vet/types.go#newcode82
cmd/vet/types.go:82: err = types.CheckExpr(x, fset, nil, nil, info)
On 2014/02/09 18:53:50, adonovan wrote:
> Use the smallest scope for 'err' where convenient:
> 
> if err := ...; err != nil {
> 

Done.

https://codereview.appspot.com/60110044/diff/80001/go/pointer/pointer_test.go
File go/pointer/pointer_test.go (left):

https://codereview.appspot.com/60110044/diff/80001/go/pointer/pointer_test.go...
go/pointer/pointer_test.go:224: // Don't print err since its location is bad.
On 2014/02/09 18:53:50, adonovan wrote:
> What has changed to render this comment obsolete?

Nothing in this changeset.  I noticed that the identical comment below didn't
make sense (err was printed), and I purposely broke some tests to check that err
could be printed here, too.  I guess the comments were just out of date.

https://codereview.appspot.com/60110044/diff/80001/go/pointer/testdata/mapref...
File go/pointer/testdata/mapreflect.go (left):

https://codereview.appspot.com/60110044/diff/80001/go/pointer/testdata/mapref...
go/pointer/testdata/mapreflect.go:49: // types.EvalNode won't let us refer to
non-exported types:
On 2014/02/09 18:53:50, adonovan wrote:
> This comment needs to be updated; the underlying problem still exists.
> (If it was solved, the correct change here would be to uncomment the assertion
> in the comment, i.e remove the '#'.)
> 
> // types.CheckExpr won't let us...

Oops!  That was a case of me blindly uncommenting, seeing the tests pass, and
feeling bliss.

I don't quite understand the comment.  CheckExpr can refer to non-exported
types.  I guess I'm missing some context.  Wow, do those pointer tests spew when
they fail!

https://codereview.appspot.com/60110044/diff/80001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/60110044/diff/80001/go/types/api.go#newcode49
go/types/api.go:49: // CheckExpr type-checks an expression in the context of a
package and scope.
On 2014/02/09 18:53:50, adonovan wrote:
> I like this name much better than Eval.

Great!

https://codereview.appspot.com/60110044/diff/80001/go/types/api.go#newcode65
go/types/api.go:65: // s == nil || s == pkg.scope
On 2014/02/09 18:53:50, adonovan wrote:
> Redundant comment; no need to state what's obvious from the control flow.

I copied this from EvalNode.  But I agree.  Done.

https://codereview.appspot.com/60110044/diff/80001/go/types/api.go#newcode82
go/types/api.go:82: // evaluate node
On 2014/02/09 18:53:50, adonovan wrote:
> s/evaluate/type-check/

Done.

https://codereview.appspot.com/60110044/diff/80001/go/types/api.go#newcode90
go/types/api.go:90: check.delayed = nil // not needed anymore
On 2014/02/09 18:53:50, adonovan wrote:
> Delete this?  check is about to become unreachable.

Done.

https://codereview.appspot.com/60110044/diff/80001/go/types/checkexpr_test.go
File go/types/checkexpr_test.go (right):

https://codereview.appspot.com/60110044/diff/80001/go/types/checkexpr_test.go...
go/types/checkexpr_test.go:33: err = CheckExpr(x, fset, pkg, scope, info)
On 2014/02/09 18:53:50, adonovan wrote:
> if err := ...; err != nil {

Done.

[gri, 2014-02-10 21:09:41]

I'm going to have to think about this for a little while.

The API is already complicated, and I don't want to expose more. After
CheckExpr, the next thing will be CheckStmt, etc. - In fact, CheckStmt makes
more sense since it subsumes expressions and (local) declarations. It would also
enable "line-by-line" type checking.

That said, providing the correctly set up context requires extra care. As is,
this is only exposing machinery but not making it easy to use.

- gri

https://codereview.appspot.com/60110044/diff/100001/cmd/vet/types.go
File cmd/vet/types.go (right):

https://codereview.appspot.com/60110044/diff/100001/cmd/vet/types.go#newcode76
cmd/vet/types.go:76: // Create a file set that looks structurally identical to
the
We really don't want to expose this to clients, exactly for reasons like this.
types.New() is easy to use. types.CheckExpr is not.

[gmk, 2014-02-18 12:59:49]

Yes, CheckStmt would be great in theory, but is there a real need for it? 
CheckExpr is definitely useful and it makes go/types complement go/parser.

Regarding ease of use:  I've addressed the issue of the FileSet below; what
else?  The Package, Scope, and Info parameters are straightforward.

https://codereview.appspot.com/60110044/diff/100001/cmd/vet/types.go
File cmd/vet/types.go (right):

https://codereview.appspot.com/60110044/diff/100001/cmd/vet/types.go#newcode76
cmd/vet/types.go:76: // Create a file set that looks structurally identical to
the
On 2014/02/10 21:09:42, gri wrote:
> We really don't want to expose this to clients, exactly for reasons like this.
> types.New() is easy to use. types.CheckExpr is not.

In some (most?) cases, clients will already have a token.FileSet and then
types.CheckExpr is not so bad.

As Alan pointed out, parser.ParseExpr really ought to accept a token.FileSet.  I
don't think we should let this limitation of go/parser affect the API of
go/types.  But to make it easier to use we should provide such a version of
parser.ParseExpr, if possible; or at least wrap up the following two lines in
something like astutil.FileSetForExpr and mention it in the doc for CheckExpr.