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.