|
|
|
go.tools/go/types: Add Info parameter to EvalNode, remove results, rename to CheckExpr, remove eval.go.
Fixes issue 7151.
Patch Set 1 #Patch Set 2 : diff -r f01dd4f7d639 https://code.google.com/p/go.tools #Patch Set 3 : diff -r f01dd4f7d639 https://code.google.com/p/go.tools #Patch Set 4 : diff -r f01dd4f7d639 https://code.google.com/p/go.tools #Patch Set 5 : diff -r f01dd4f7d639 https://code.google.com/p/go.tools #
Total comments: 19
Patch Set 6 : diff -r fd4c6a614a3b https://code.google.com/p/go.tools #
Total comments: 2
Total messages: 9
|
gmk
I shouldn't have just blown away eval_test.go. Resuscitating...
|
11 years, 11 months ago (2014年02月05日 18:40:43 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
I shouldn't have just blown away eval_test.go. Resuscitating...
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
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.
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.
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 {
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.
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.
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.
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.