code review 7058060: go/types: Moving from *ast.Objects to types.Objects (st...

go/types: Moving from *ast.Objects to types.Objects (step 1).

The existing type checker was relying on augmenting ast.Object
fields (empty interfaces) for its purposes. While this worked
for some time now, it has become increasingly brittle. Also,
the need for package information for Fields and Methods would
have required a new field in each ast.Object. Rather than making
them bigger and the code even more subtle, in this CL we are moving
away from ast.Objects.

The types packge now defines its own objects for different
language entities (Const, Var, TypeName, Func), and they
implement the types.Object interface. Imported packages
create a Package object which holds the exported entities
in a types.Scope of types.Objects.

For type-checking, the current package is still using ast.Objects
to make this transition manageable. In a next step, the type-
checker will also use types.Objects instead, which opens the door
door to resolving ASTs entirely by the type checker. As a result,
the AST and type checker become less entangled, and ASTs can be
manipulated "by hand" or programmatically w/o having to worry
about scope and object invariants that are very hard to maintain.

(As a consequence, a future parser can do less work, and a
future AST will not need to define objects and scopes anymore.
Also, object resolution which is now split across the parser,
the ast, (ast.NewPackage), and even the type checker (for composite
literal keys) can be done in a single place which will be simpler
and more efficient.)

Change details:
- Check now takes a []*ast.File instead of a map[string]*ast.File.
It's easier to handle (I deleted code at all use sites) and does
not suffer from undefined order (which is a pain for testing).
- ast.Object.Data is now a *types.Package rather then an *ast.Scope
if the object is a package (obj.Kind == ast.Pkg). Eventually this
will go away altogether.
- Instead of an ast.Importer, Check now uses a types.Importer
(which returns a *types.Package).
- types.NamedType has two object fields (Obj Object and obj *ast.Object);
eventually there will be only Obj. The *ast.Object is needed during
this transition since a NamedType may refer to either an imported
(using types.Object) or locally defined (using *ast.Object) type.
- ast.NewPackage is not used anymore - there's a local copy for
package-level resolution of imports.
- struct fields now take the package origin into account.
- The GcImporter is now returning a *types.Package. It cannot be
used with ast.NewPackage anymore. If that functionality is still
used, a copy of the old GcImporter should be made locally (note
that GcImporter was part of exp/types and it's API was not frozen).
- dot-imports are not handled for the time being (this will come back).

[gri, 2013-01-11 01:08:51]

Hello adonovan@google.com (cc: golang-dev@googlegroups.com),

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

[gri, 2013-01-11 01:15:45]

PTAL. Fixed various types in CL desc.

On Thu, Jan 10, 2013 at 5:08 PM,  <gri@golang.org> wrote:
> Reviewers: adonovan,
>
> Message:
> Hello adonovan@google.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> go/types: Moving from *ast.Objects to types.Objects (step 1).
>
> The existing type checker was relying on augmenting ast.Object
> fields (empty interfaces) for its purposes. While this worked
> for some time now, it has become increasingly brittle. Also,
> the need for package information for Fields and Methods would
> have required a new field in each ast.Object. Rather than making
> them bigger and the code even more subtle, in this CL we are moving
> away from ast.Objects.
>
> The types packge now defines its own objects for different
> language entities (Const, Var, TypeName, Func), and they
> implement the types.Object interface. Imported packages
> create a Package object which holds the exported entities
> in a types.Scope of types.Objects.
>
> For type-checking, the current package is still using ast.Objects
> to make this transition manageable. In a next step, the type-
> checker will also use types.Objects instead, which opens the door
> door to resolving ASTs entirely by the type checker. As a result,
> the AST and type checker become less entangled, and ASTs can be
> manipulated "by hand" or programmatically w/o having to worry
> about scope and object invariants that are very hard to maintain.
>
> (As a consequence, a future parser will have to less work, and a
> future AST will not need to define objects and scopes anymore.
> Also, object resolution which is not split across the parser,
> the ast, (ast.NewPackage), and even the type checker (for composite
> literal keys) can be done in a single place which will be simpler
> and more efficient.)
>
> Change details:
> - Check now takes a []*ast.File instead of map[string]*ast.File.
> It's easier to handle (I deleted code at all use sites) and does
> suffer from undefined order (which is a pain for testing).
> - ast.Object.Data is now a *types.Package rather then an *ast.Scope
> if the object is a package (obj.Kind == ast.Pkg). Eventially this
> will go away altogether.
> - Instead of an ast.Importer, Check now uses a types.Importer
> (which returns *types.Packages).
> - types.NamedType has two object fields (Obj Object and obj
> *ast.Object);
> eventually there will be only Obj. The *ast.Object is needed during
> this transition since a NamedType may refer to either an imported
> (using types.Object) or locally defined (using *ast.Object) type.
> - ast.NewPackage is not used anymore - there's a local copy for
> package-level resolution of imports.
> - struct fields now take the package origin into account
> (missing: the same checks for methods)
> - The GcImporter is now returning a types.Package. It cannot be
> used with ast.NewPackage anymore. If that functionality is still
> used, a copy of the old GcImporter should be made locally (note
> that GcImporter was part of exp/types and it's API was not frozen).
> - dot-imports are not handled anymore for the time being
>
> Please review this at https://codereview.appspot.com/7058060/
>
> Affected files:
>   M src/pkg/exp/gotype/gotype.go
>   M src/pkg/go/ast/scope.go
>   M src/pkg/go/types/api.go
>   M src/pkg/go/types/check.go
>   M src/pkg/go/types/check_test.go
>   M src/pkg/go/types/conversions.go
>   M src/pkg/go/types/errors.go
>   M src/pkg/go/types/expr.go
>   M src/pkg/go/types/gcimporter.go
>   M src/pkg/go/types/gcimporter_test.go
>   A src/pkg/go/types/objects.go
>   M src/pkg/go/types/operand.go
>   M src/pkg/go/types/predicates.go
>   A src/pkg/go/types/resolve.go
>   M src/pkg/go/types/resolver_test.go
>   M src/pkg/go/types/types.go
>   M src/pkg/go/types/types_test.go
>   M src/pkg/go/types/universe.go
>
>

[adonovan, 2013-01-11 19:39:26]

Very brief comments so far:

- The implementsObject unexported field of types.Var makes composite literals
using positional parameters impossible; a slight nuisance.
- Object.name() needs to be exported since clients need it, but the identifier
Name is taken by the field. GetName()
- Q. is the Pkg field of Field and Method non-nil always, or only if the field
is un-exported?  It might be slightly more convenient if sometimes nil.
- Perhaps this would be useful:
    type Id struct { Pkg *types.Package; Name string }
  func (Id) String()
  If Pkg is nil when Name is unexported, then this 
  is a valid map key type.  You could embed it in
  types.Method and types.Field.

- Methods don't implement types.Object.   I think they should.

- I assume that all implementations of types.Object returned by the typechecker
are canonical, i.e. ptr equivalent.  This needs to be stated.
- NamedType.obj is private, so I can't create them in my client, but I do need
to (e.g. in order to fake out the reflect package.)  Please choose a capitalized
name.
- types.Check() must return the *types.Package too since there's no way to get
at it from the client---yet it is needed so that I can perform the
unexported-name equivalence check when I encounter an expression 'expr.name'.
- Check should not discard the Imports map; this was returned in the
ast.Package.Imports, but since it has a new types it will need to be in the
types.Package that Check should be returning too.

[gri, 2013-01-11 19:57:22]

On Fri, Jan 11, 2013 at 11:39 AM,  <adonovan@google.com> wrote:
> - The implementsObject unexported field of types.Var makes composite
> literals using positional parameters impossible; a slight nuisance.
> - Object.name() needs to be exported since clients need it, but the
> identifier Name is taken by the field. GetName()

will fix

> - Q. is the Pkg field of Field and Method non-nil always, or only if the
> field is un-exported?  It might be slightly more convenient if sometimes
> nil.

trivial to change

> - Perhaps this would be useful:
>     type Id struct { Pkg *types.Package; Name string }
>   func (Id) String()
>   If Pkg is nil when Name is unexported, then this
>   is a valid map key type.  You could embed it in
>   types.Method and types.Field.

yes - there's already a TODO

>
> - Methods don't implement types.Object.   I think they should.

why? (they are not in a scope - alternative is to put them into a
scope for both interfaces and named types)

>
> - I assume that all implementations of types.Object returned by the
> typechecker are canonical, i.e. ptr equivalent.  This needs to be
> stated.

as before

> - NamedType.obj is private, so I can't create them in my client, but I
> do need to (e.g. in order to fake out the reflect package.)  Please
> choose a capitalized name.

ok

> - types.Check() must return the *types.Package too since there's no way
> to get at it from the client---yet it is needed so that I can perform
> the unexported-name equivalence check when I encounter an expression
> 'expr.name'.

done

> - Check should not discard the Imports map; this was returned in the
> ast.Package.Imports, but since it has a new types it will need to be in
> the types.Package that Check should be returning too.

done

[gri, 2013-01-11 21:34:34]

Hello adonovan@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[adonovan, 2013-01-11 21:48:05]

LGTM

Still cleaning up the debris on my end though. :)

[gri, 2013-01-11 21:53:44]

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

go/types: Moving from *ast.Objects to types.Objects (step 1).

The existing type checker was relying on augmenting ast.Object
fields (empty interfaces) for its purposes. While this worked
for some time now, it has become increasingly brittle. Also,
the need for package information for Fields and Methods would
have required a new field in each ast.Object. Rather than making
them bigger and the code even more subtle, in this CL we are moving
away from ast.Objects.

The types packge now defines its own objects for different
language entities (Const, Var, TypeName, Func), and they
implement the types.Object interface. Imported packages
create a Package object which holds the exported entities
in a types.Scope of types.Objects.

For type-checking, the current package is still using ast.Objects
to make this transition manageable. In a next step, the type-
checker will also use types.Objects instead, which opens the door
door to resolving ASTs entirely by the type checker. As a result,
the AST and type checker become less entangled, and ASTs can be
manipulated "by hand" or programmatically w/o having to worry
about scope and object invariants that are very hard to maintain.

(As a consequence, a future parser can do less work, and a
future AST will not need to define objects and scopes anymore.
Also, object resolution which is now split across the parser,
the ast, (ast.NewPackage), and even the type checker (for composite
literal keys) can be done in a single place which will be simpler
and more efficient.)

Change details:
- Check now takes a []*ast.File instead of a map[string]*ast.File.
It's easier to handle (I deleted code at all use sites) and does
not suffer from undefined order (which is a pain for testing).
- ast.Object.Data is now a *types.Package rather then an *ast.Scope
if the object is a package (obj.Kind == ast.Pkg). Eventually this
will go away altogether.
- Instead of an ast.Importer, Check now uses a types.Importer
(which returns a *types.Package).
- types.NamedType has two object fields (Obj Object and obj *ast.Object);
eventually there will be only Obj. The *ast.Object is needed during
this transition since a NamedType may refer to either an imported
(using types.Object) or locally defined (using *ast.Object) type.
- ast.NewPackage is not used anymore - there's a local copy for
package-level resolution of imports.
- struct fields now take the package origin into account.
- The GcImporter is now returning a *types.Package. It cannot be
used with ast.NewPackage anymore. If that functionality is still
used, a copy of the old GcImporter should be made locally (note
that GcImporter was part of exp/types and it's API was not frozen).
- dot-imports are not handled for the time being (this will come back).

R=adonovan
CC=golang-dev
https://codereview.appspot.com/7058060

[adonovan, 2013-01-11 22:00:03]

https://codereview.appspot.com/7058060/diff/31057/src/pkg/go/types/types.go
File src/pkg/go/types/types.go (right):

https://codereview.appspot.com/7058060/diff/31057/src/pkg/go/types/types.go#n...
src/pkg/go/types/types.go:95: type QualifiedName struct {
Would be good to document the (logical) equivalence relation here, and explain
that Pkg is never nil.