code review 51380044: go.tools/go/types: allow package references with types.New

go.tools/go/types: allow package references with types.New Also: Remove unnecessary recv from internal function (cleanup). Fixes issue 6259.

[gri, 2014-01-17 04:17:17]

Hello adonovan@google.com, r@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.tools

[adonovan, 2014-01-17 04:22:21]

On 2014/01/17 04:17:17, gri wrote: > Hello mailto:adonovan@google.com, mailto:r@golang.org (cc: > mailto:golang-codereviews@googlegroups.com), > > I'd like you to review this change to > https://code.google.com/p/go.tools What's the motivating use-case? The code looks fine but New is starting to have a lot of complexity with no way to configure it. e.g. multiple calls to New will not amortize the work of imports, and there's no way to control the importer except via the global variable. Let me think about this tomorrow.

[gri, 2014-01-17 04:26:21]

On 2014/01/17 04:22:21, adonovan wrote: > On 2014/01/17 04:17:17, gri wrote: > > Hello mailto:adonovan@google.com, mailto:r@golang.org (cc: > > mailto:golang-codereviews@googlegroups.com), > > > > I'd like you to review this change to > > https://code.google.com/p/go.tools > > What's the motivating use-case? > > The code looks fine but New is starting to have a lot of complexity with no way > to configure it. e.g. multiple calls to New will not amortize the work of > imports, and there's no way to control the importer except via the global > variable. > > Let me think about this tomorrow. The motivating use case is issue 6259: It makes sense to want to create types built from types declared in other packages. For instance, one may want to check if some type implements io.Writer, and this is a way to get to the io.Writer type easily. As an optimization one could amortize the imports by keeping the pkgMap around (under a lock). Future imports of the same package would be very efficient. This would be trivial to do. Not sure it's worth it. I'm going to add a comment about the costliness of New.

[gri, 2014-01-17 04:33:58]

Hello adonovan@google.com, r@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.

[adonovan, 2014-01-17 18:38:21]

https://codereview.appspot.com/51380044/diff/140001/go/types/eval.go File go/types/eval.go (right): https://codereview.appspot.com/51380044/diff/140001/go/types/eval.go#newcode41 go/types/eval.go:41: var scope *Scope I think New is doing too much. Why not provide a separate utility function of type func(imports...string) (*Package, *Scope) ? Users that want it can call it (and then Eval), but there are many other ways to create a package and its scope: from a set of pre-parsed files; from a non-Default importer; using an inner scope, etc. https://codereview.appspot.com/51380044/diff/140001/go/types/eval.go#newcode49 go/types/eval.go:49: pkgMap := make(map[string]*Package) This map is arguably the central piece of state in a multi-package type checking session, since it is the namespace that gives import paths meaning. Creating one to throw away seems like a recipe for problems. e.g. the following code will panic, won't it? t1 := New("*bytes.Buffer", "bytes") t2 := New("*bytes.Buffer", "bytes") if types.IsIdentical(t1, t2) { panic("oops") }

[adonovan, 2014-01-17 18:40:00]

On 2014/01/17 18:38:21, adonovan wrote: > https://codereview.appspot.com/51380044/diff/140001/go/types/eval.go > File go/types/eval.go (right): > > https://codereview.appspot.com/51380044/diff/140001/go/types/eval.go#newcode41 > go/types/eval.go:41: var scope *Scope > I think New is doing too much. Why not provide a separate utility function of > type > func(imports...string) (*Package, *Scope) > ? Users that want it can call it (and then Eval), but there are many other ways > to create a package and its scope: from a set of pre-parsed files; from a > non-Default importer; using an inner scope, etc. > > https://codereview.appspot.com/51380044/diff/140001/go/types/eval.go#newcode49 > go/types/eval.go:49: pkgMap := make(map[string]*Package) > This map is arguably the central piece of state in a multi-package type checking > session, since it is the namespace that gives import paths meaning. Creating > one to throw away seems like a recipe for problems. > > e.g. the following code will panic, won't it? > > t1 := New("*bytes.Buffer", "bytes") > t2 := New("*bytes.Buffer", "bytes") > if types.IsIdentical(t1, t2) { > panic("oops") > } I meant: !types.IsIdentical.

[gri, 2014-01-17 18:50:13]

ok - thinking some more https://codereview.appspot.com/51380044/diff/140001/go/types/eval.go File go/types/eval.go (right): https://codereview.appspot.com/51380044/diff/140001/go/types/eval.go#newcode41 go/types/eval.go:41: var scope *Scope On 2014/01/17 18:38:21, adonovan wrote: > I think New is doing too much. Why not provide a separate utility function of > type > func(imports...string) (*Package, *Scope) > ? Users that want it can call it (and then Eval), but there are many other ways > to create a package and its scope: from a set of pre-parsed files; from a > non-Default importer; using an inner scope, etc. This might be an option. https://codereview.appspot.com/51380044/diff/140001/go/types/eval.go#newcode49 go/types/eval.go:49: pkgMap := make(map[string]*Package) On 2014/01/17 18:38:21, adonovan wrote: > This map is arguably the central piece of state in a multi-package type checking > session, since it is the namespace that gives import paths meaning. Creating > one to throw away seems like a recipe for problems. > > e.g. the following code will panic, won't it? > > t1 := New("*bytes.Buffer", "bytes") > t2 := New("*bytes.Buffer", "bytes") > if types.IsIdentical(t1, t2) { > panic("oops") > } Yes, it would fail (!IsIdentical) since the base types are named types that are defined by different objects. That check could be fixed though...

[adonovan, 2014-03-24 16:54:12]

On 2014/01/17 18:50:13, gri wrote: > ok - thinking some more > > https://codereview.appspot.com/51380044/diff/140001/go/types/eval.go > File go/types/eval.go (right): > > https://codereview.appspot.com/51380044/diff/140001/go/types/eval.go#newcode41 > go/types/eval.go:41: var scope *Scope > On 2014/01/17 18:38:21, adonovan wrote: > > I think New is doing too much. Why not provide a separate utility function of > > type > > func(imports...string) (*Package, *Scope) > > ? Users that want it can call it (and then Eval), but there are many other > ways > > to create a package and its scope: from a set of pre-parsed files; from a > > non-Default importer; using an inner scope, etc. > > This might be an option. > > https://codereview.appspot.com/51380044/diff/140001/go/types/eval.go#newcode49 > go/types/eval.go:49: pkgMap := make(map[string]*Package) > On 2014/01/17 18:38:21, adonovan wrote: > > This map is arguably the central piece of state in a multi-package type > checking > > session, since it is the namespace that gives import paths meaning. Creating > > one to throw away seems like a recipe for problems. > > > > e.g. the following code will panic, won't it? > > > > t1 := New("*bytes.Buffer", "bytes") > > t2 := New("*bytes.Buffer", "bytes") > > if types.IsIdentical(t1, t2) { > > panic("oops") > > } > > Yes, it would fail (!IsIdentical) since the base types are named types that are > defined by different objects. That check could be fixed though... Revert this CL?

[gobot, 2014-12-19 05:18:02]

R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/51380044 is best) in the description in your new CL. Thanks very much.