code review 189080: Steps towards tracking scopes for identifiers.

Steps towards tracking scopes for identifiers.

	- Identifiers refer now to the language entity (Object)
	  that they denote. At the moment this is at best an
	  approximation.

	- Initial data structures for language entities (Objects)
          and expression types (Type) independent of the actual
	  type notations.

	- Initial support for declaring and looking up identifiers.

	- Updated various dependent files and added support functions.

	- Extensively tested to avoid breakage. This is an AST change.

[gri, 2010-01-15 02:20:52]

Hello rsc (cc: golang-dev@googlegroups.com, rog),

I'd like you to review this change.

[rsc, 2010-01-15 06:19:54]

I'm a little worried about doing too much in the parser.
I'd like to preserve the property that you can parse a single
package or file without having to look up anything about its
imports, and some of what's promised in the new interface here
suggests that the parser is going to start looking at metadata
about or source code for imported packages.

I don't believe the parser or the AST should:
  * know what the size of a float is
  * know the set of predefined identifiers
  * resolve selectors or qualified identifiers
  * have a concept of type beyond the syntax in type declarations
  * evaluate constants

That functionality can be written as a new package go/typecheck
(go/typer?), invoked when needed but not required.

As a client of go/ast and go/parser, I would want the result of scoping
to be that each *Ident corresponding to an unqualified identifier
would contain a pointer to an *Obj that contained this information:

	type Obj struct {
		Decl Decl
	}

All identifiers referring to the same declared identifier would 
have the same (==) *Obj pointer, including the identifier in the
declaration, so that if Decl leads to a ValueSpec, the specific
*Ident in Names can be found by looking for the one with
the same *Obj.

Identifiers that refer to a name undeclared in the file being parsed
would still get a *Obj, one that would have a nil Decl and be 
recorded in the top-level identifier map so that a client could
provide them later.

Concretely, suppose that scoping is implemented as a separate
pass over the AST, so as to avoid changing all of the many 
parsing entry points:

package ast
func Scope(n Node, top *Block)

If presented with

1	package p
2
3	type T int
4	
5	func f() {
6		x := y
7		y := x + 1
8	}

and an empty top *ast.Block, Scope would create six *Obj:
for T, int, f, x (line 6), y (line 6), and y (line 7).
The four *Obj for T, f, x (line 6), and y (line 7) would have
Decl fields referring to the corresponding declarations.
The *Ident for the x on line 7 would have as its *Obj the one
created for the x on line 6.  So far nothing surprising.

The scoping function would record the four *Obj for
T, int, f, and y (line 6) in top, but the *Obj for int and
y (line 6) would have nil Decls: they refer to names not declared
in this file, so the declaration is as yet unknown.

Now suppose we call scope on this source code and the same
top we just used for the last file:

10	package p
11
12	type U int
13
14	var y T = x

The scoping function would look for *Obj for U, int, y, T, and x.
It would find the *Obj for int, y, and T left there by the previous
call and reuse those.  Then it would need to create *Obj for U and x
and add those to the *ast.Block.

In this way each file in the package can be considered incrementally,
and the process can stop at any time and have useful data.
After all files have been considered, the accumulated top *ast.Block
can be consulted to look for *Obj with nil Decls.  Those are undeclared
identifiers (like the x on line 14) or maybe names of imported packages
or predeclared identifiers (like int).  Whatever they are, once the caller
resolves one by setting obj.Decl, every reference in the program
to that top-level identifier is now corrected.  Imports, if they are handled,
simply add the package name to the top *ast.Block, or in the case of
import ".", add all the identifiers.  In both cases, the processing would
simply check for and reuse *Obj in top with nil Decls. 

It's important that the predeclared identifiers are not actually 
predeclared in the *ast.Block before processing any files.  If line
12 above had said type int struct{}, the reference to int on line 3 
in the other file would need to refer to that declaration, not the
predeclared int.

Cgo would use this to scope the input files and then when it processes
the AST, instead of assuming that every *Ident with Value == "C"
refers to the import "C", it could check that the *Ident has Value == "C"
and the *Obj has Decl == nil, so that if there is a local C it won't get
confused.  Cgo would not bother with full type checking or looking
at imported packages, just scoping.

And of course a separate package could add type information as 
another pass.

Russ

http://codereview.appspot.com/189080/diff/1049/67
File src/pkg/go/ast/ast.go (right):

http://codereview.appspot.com/189080/diff/1049/67#newcode121
src/pkg/go/ast/ast.go:121: Obj            *Object // denoted object
Should there be a way to express that the scoping has not
yet been computed?

It might work to add Name string in place of Value
and let obj == nil mean that the reference is unknown.
It seems like that would be a useful way to make sure
that there's only one Object per declaration instead
of one Object per identifier instance.

Regardless, I suggest, for now, leaving Value *Ident here,
even if it duplicates information, so that clients don't have
to be edited until the new interface has settled.

http://codereview.appspot.com/189080/diff/1049/67#newcode376
src/pkg/go/ast/ast.go:376: // TODO(gri): remove eventually in favor of the more
evocative
I don't believe converting x.Value -> x.Value() is any cheaper
than converting directly from x.Value -> x.Name(), but as I
mentioned above, I think that should wait until the new code
is a bit firmer.

http://codereview.appspot.com/189080/diff/1049/70
File src/pkg/go/ast/scope.go (left):

http://codereview.appspot.com/189080/diff/1049/70#oldcode13
src/pkg/go/ast/scope.go:13: type Scope struct {
The language spec now calls this concept a block.
Should this be Block?

http://codereview.appspot.com/189080/diff/1049/70
File src/pkg/go/ast/scope.go (right):

http://codereview.appspot.com/189080/diff/1049/70#newcode89
src/pkg/go/ast/scope.go:89: Entries map[string]*Object
Objects?  Avoids needing to introduce the term Entry too.

[rog, 2010-01-15 15:11:29]

2010/1/15  <rsc@golang.org>:
> I'm a little worried about doing too much in the parser.
> I'd like to preserve the property that you can parse a single
> package or file without having to look up anything about its
> imports, and some of what's promised in the new interface here
> suggests that the parser is going to start looking at metadata
> about or source code for imported packages.

i agree. i think it's possible to do quite a lot without doing a full
type check - you can often get a long way with just local
information, even if it isn't complete.

> As a client of go/ast and go/parser, I would want the result of scoping
> to be that each *Ident corresponding to an unqualified identifier
> would contain a pointer to an *Obj that contained this information:
>
>        type Obj struct {
>                Decl Decl
>        }
>
> All identifiers referring to the same declared identifier would
> have the same (==) *Obj pointer, including the identifier in the
> declaration, so that if Decl leads to a ValueSpec, the specific
> *Ident in Names can be found by looking for the one with
> the same *Obj.
>
> Identifiers that refer to a name undeclared in the file being parsed
> would still get a *Obj, one that would have a nil Decl and be
> recorded in the top-level identifier map so that a client could
> provide them later.

i like this approach (it's actually mostly straightforward to plug it
into the way i did things, but see below)

> In this way each file in the package can be considered incrementally,
> and the process can stop at any time and have useful data.
> After all files have been considered, the accumulated top *ast.Block
> can be consulted to look for *Obj with nil Decls.  Those are undeclared
> identifiers (like the x on line 14) or maybe names of imported packages
> or predeclared identifiers (like int).  Whatever they are, once the
> caller
> resolves one by setting obj.Decl, every reference in the program
> to that top-level identifier is now corrected.  Imports, if they are
> handled,
> simply add the package name to the top *ast.Block, or in the case of
> import ".", add all the identifiers.  In both cases, the processing
> would
> simply check for and reuse *Obj in top with nil Decls.
>
> It's important that the predeclared identifiers are not actually
> predeclared in the *ast.Block before processing any files.  If line
> 12 above had said type int struct{}, the reference to int on line 3
> in the other file would need to refer to that declaration, not the
> predeclared int.

i think there's a problem here. what happens if you have
a package like this?

package foo
import f "go/ast"
type Foo1 struct {A *f.File; B *Foo2;}

package foo
import f "container/list"
type Foo2 struct {A *f.List;}


the "f" identifier in both files would refer to the same object,
AFAICS, so a client processing Foo1 can't know that each f
refers to a different package.
you could include the top level block in each identifier, but
that starts to move towards where i was, with a scope in each identifier.

i still think that a lot of pain would be avoided by automatically deriving
the package name from the package path when necessary.

[rsc, 2010-01-15 17:22:00]

> i think there's a problem here. what happens if you have
> a package like this?
>
> package foo
> import f "go/ast"
> type Foo1 struct {A *f.File; B *Foo2;}
>
> package foo
> import f "container/list"
> type Foo2 struct {A *f.List;}
>
> the "f" identifier in both files would refer to the same object,
> AFAICS, so a client processing Foo1 can't know that each f
> refers to a different package.

i glossed over this.  when processing the first file
you'd end up with an undefined f in the top block,
and then you'd apply the imports by resolving that f
to the package object, and then before moving to the
next file you'd delete it from the block.  package names
cut across scopes differently than any other name.

russ

[rog, 2010-01-15 17:40:39]

2010/1/15 Russ Cox <rsc@golang.org>:
> i glossed over this.  when processing the first file
> you'd end up with an undefined f in the top block,
> and then you'd apply the imports by resolving that f
> to the package object, and then before moving to the
> next file you'd delete it from the block.  package names
> cut across scopes differently than any other name.

presumably that means you couldn't have something
like the current ParseDir, which processes several
files in the same package, but without resolving any imports
along the way.

[rsc, 2010-01-15 18:03:06]

On Fri, Jan 15, 2010 at 09:40, roger peppe <rogpeppe@gmail.com> wrote:
> 2010/1/15 Russ Cox <rsc@golang.org>:
>> i glossed over this.  when processing the first file
>> you'd end up with an undefined f in the top block,
>> and then you'd apply the imports by resolving that f
>> to the package object, and then before moving to the
>> next file you'd delete it from the block.  package names
>> cut across scopes differently than any other name.
>
> presumably that means you couldn't have something
> like the current ParseDir, which processes several
> files in the same package, but without resolving any imports
> along the way.

You could, but it would have to make a different top block for
each file and then merge between them separately.  A good
reason that scoping should be separate from the parsers proper.

Russ

[rog, 2010-01-15 18:09:56]

2010/1/15 Russ Cox <rsc@golang.org>:
> You could, but it would have to make a different top block for
> each file and then merge between them separately.  A good
> reason that scoping should be separate from the parsers proper.

how do you merge two files, if all the global symbols in each file are
now separated because they've been entered in different tables?
AFAICS you'd have to go back and rewrite all the *ast.Idents in
the parse tree.

[rsc, 2010-01-15 18:28:37]

> how do you merge two files, if all the global symbols in each file are
> now separated because they've been entered in different tables?
> AFAICS you'd have to go back and rewrite all the *ast.Idents in
> the parse tree.

it's certainly more work than doing it right from the start.  ;-)

russ

[gri, 2010-01-15 18:37:55]

On Thu, Jan 14, 2010 at 10:19 PM, <rsc@golang.org> wrote:

> I'm a little worried about doing too much in the parser.
> I'd like to preserve the property that you can parse a single
> package or file without having to look up anything about its
> imports, and some of what's promised in the new interface here
> suggests that the parser is going to start looking at metadata
> about or source code for imported packages.
>

That is incorrect. The new interface so far simply provides a link to the
declaration. There may be more information attached to it eventually, but
that doesn't mean that it has to be done while parsing or even as part of
the parsing process.

>
> I don't believe the parser or the AST should:
>  * know what the size of a float is
>

Whether the type representation remains as it is now or not, remains to be
seen. This is work in progress and this particular code is not yet used.


>  * know the set of predefined identifiers
>

This will eventually be relevant for resolving declarations. It is not
correct at the moment but also not used.


>  * resolve selectors or qualified identifiers
>

The code does not do this nor is it implied that the parser will be doing
this. It cannot if parsing of any source text should remain possible. But a
separate phase can.


>  * have a concept of type beyond the syntax in type declarations
>

I disagree. Eventually I want more type information in the AST.


>  * evaluate constants
>

I don't see any code that does that...

>
> That functionality can be written as a new package go/typecheck
> (go/typer?), invoked when needed but not required.
>

That is the plan.

>
> As a client of go/ast and go/parser, I would want the result of scoping
> to be that each *Ident corresponding to an unqualified identifier
> would contain a pointer to an *Obj that contained this information:
>
>        type Obj struct {
>                Decl Decl
>        }
>

That is exactly what you get. Each Ident has a pointer to an Object which
contains information about the relevant declaration if found. Representation
is not exactly the one you are suggesting, but the effect is the same.

>
> All identifiers referring to the same declared identifier would
> have the same (==) *Obj pointer, including the identifier in the
> declaration, so that if Decl leads to a ValueSpec, the specific
> *Ident in Names can be found by looking for the one with
> the same *Obj.
>

That is exactly as it is now. The backward link to the Decl is not there yet
because I am not using it yet.

>
> Identifiers that refer to a name undeclared in the file being parsed
> would still get a *Obj, one that would have a nil Decl and be
> recorded in the top-level identifier map so that a client could
> provide them later.
>

Yes, also exactly as planned.

>
> Concretely, suppose that scoping is implemented as a separate
> pass over the AST, so as to avoid changing all of the many
> parsing entry points:
>
> package ast
> func Scope(n Node, top *Block)
>
> If presented with
>
> 1       package p
> 2
> 3       type T int
> 4
> 5       func f() {
> 6               x := y
> 7               y := x + 1
> 8       }
>
> and an empty top *ast.Block, Scope would create six *Obj:
> for T, int, f, x (line 6), y (line 6), and y (line 7).
>

Happens now.


> The four *Obj for T, f, x (line 6), and y (line 7) would have
> Decl fields referring to the corresponding declarations.
> The *Ident for the x on line 7 would have as its *Obj the one
> created for the x on line 6.  So far nothing surprising.
>

Exactly.

>
> The scoping function would record the four *Obj for
> T, int, f, and y (line 6) in top, but the *Obj for int and
> y (line 6) would have nil Decls: they refer to names not declared
> in this file, so the declaration is as yet unknown.
>

Sure. As you may notice, the Universe scope is not linked in yet... I am
fully aware of this issue.

>
> Now suppose we call scope on this source code and the same
> top we just used for the last file:
>
> 10      package p
> 11
> 12      type U int
> 13
> 14      var y T = x
>
> The scoping function would look for *Obj for U, int, y, T, and x.
> It would find the *Obj for int, y, and T left there by the previous
> call and reuse those.  Then it would need to create *Obj for U and x
> and add those to the *ast.Block.
>

Sure.

>
> In this way each file in the package can be considered incrementally,
> and the process can stop at any time and have useful data.
> After all files have been considered, the accumulated top *ast.Block
> can be consulted to look for *Obj with nil Decls.  Those are undeclared
> identifiers (like the x on line 14) or maybe names of imported packages
> or predeclared identifiers (like int).  Whatever they are, once the

caller
> resolves one by setting obj.Decl, every reference in the program
> to that top-level identifier is now corrected.  Imports, if they are
> handled,
> simply add the package name to the top *ast.Block, or in the case of
> import ".", add all the identifiers.  In both cases, the processing
> would
> simply check for and reuse *Obj in top with nil Decls.
>

Sure.

>
> It's important that the predeclared identifiers are not actually
> predeclared in the *ast.Block before processing any files.  If line
> 12 above had said type int struct{}, the reference to int on line 3
> in the other file would need to refer to that declaration, not the
> predeclared int.
>

See above.

>
> Cgo would use this to scope the input files and then when it processes
> the AST, instead of assuming that every *Ident with Value == "C"
> refers to the import "C", it could check that the *Ident has Value ==
> "C"
> and the *Obj has Decl == nil, so that if there is a local C it won't get
> confused.  Cgo would not bother with full type checking or looking
> at imported packages, just scoping.
>
> And of course a separate package could add type information as
> another pass.
>

I think you are running in open doors. Details on the remaining feedback in
a separate mail.

>
> Russ
>
>
>
> http://codereview.appspot.com/189080/diff/1049/67
> File src/pkg/go/ast/ast.go (right):
>
> http://codereview.appspot.com/189080/diff/1049/67#newcode121
> src/pkg/go/ast/ast.go:121: Obj            *Object // denoted object
> Should there be a way to express that the scoping has not
> yet been computed?
>
> It might work to add Name string in place of Value
> and let obj == nil mean that the reference is unknown.
> It seems like that would be a useful way to make sure
> that there's only one Object per declaration instead
> of one Object per identifier instance.
>
> Regardless, I suggest, for now, leaving Value *Ident here,
> even if it duplicates information, so that clients don't have
> to be edited until the new interface has settled.
>
> http://codereview.appspot.com/189080/diff/1049/67#newcode376
> src/pkg/go/ast/ast.go:376: // TODO(gri): remove eventually in favor of
> the more evocative
> I don't believe converting x.Value -> x.Value() is any cheaper
> than converting directly from x.Value -> x.Name(), but as I
> mentioned above, I think that should wait until the new code
> is a bit firmer.
>
> http://codereview.appspot.com/189080/diff/1049/70
> File src/pkg/go/ast/scope.go (left):
>
> http://codereview.appspot.com/189080/diff/1049/70#oldcode13
> src/pkg/go/ast/scope.go:13: type Scope struct {
> The language spec now calls this concept a block.
> Should this be Block?
>
> http://codereview.appspot.com/189080/diff/1049/70
> File src/pkg/go/ast/scope.go (right):
>
> http://codereview.appspot.com/189080/diff/1049/70#newcode89
> src/pkg/go/ast/scope.go:89: Entries map[string]*Object
> Objects?  Avoids needing to introduce the term Entry too.
>
>
> http://codereview.appspot.com/189080
>

[gri, 2010-01-15 18:59:31]

http://codereview.appspot.com/189080/diff/1049/67
File src/pkg/go/ast/ast.go (right):

http://codereview.appspot.com/189080/diff/1049/67#newcode121
src/pkg/go/ast/ast.go:121: Obj            *Object // denoted object
On 2010/01/15 06:19:54, rsc wrote:
> Should there be a way to express that the scoping has not
> yet been computed?
> 
> It might work to add Name string in place of Value
> and let obj == nil mean that the reference is unknown.
> It seems like that would be a useful way to make sure
> that there's only one Object per declaration instead
> of one Object per identifier instance.
> 
> Regardless, I suggest, for now, leaving Value *Ident here,
> even if it duplicates information, so that clients don't have
> to be edited until the new interface has settled.
> 

Currently that is done with an Object that has an Err Kind. That is likely to
change in some way, but I need something that compiles and runs.

Clients have to change anyway when adding the Object field, even if Value is
still here because we use composite literals to create Idents in many places
(which is actually bad practice from an OO view point).

The accessor function solves this problem.

http://codereview.appspot.com/189080/diff/1049/67#newcode376
src/pkg/go/ast/ast.go:376: // TODO(gri): remove eventually in favor of the more
evocative
On 2010/01/15 06:19:54, rsc wrote:
> I don't believe converting x.Value -> x.Value() is any cheaper
> than converting directly from x.Value -> x.Name(), but as I
> mentioned above, I think that should wait until the new code
> is a bit firmer.
> 

Sure, but the conversion is done.

http://codereview.appspot.com/189080/diff/1049/70
File src/pkg/go/ast/scope.go (left):

http://codereview.appspot.com/189080/diff/1049/70#oldcode13
src/pkg/go/ast/scope.go:13: type Scope struct {
On 2010/01/15 06:19:54, rsc wrote:
> The language spec now calls this concept a block.
> Should this be Block?
> 

I thought about that. I prefer scope simply because it's not just a syntactic
construct. Block is a syntactic construct. Different objects have different
scopes, not all of them match the syntactic block.

http://codereview.appspot.com/189080/diff/1049/70
File src/pkg/go/ast/scope.go (right):

http://codereview.appspot.com/189080/diff/1049/70#newcode89
src/pkg/go/ast/scope.go:89: Entries map[string]*Object
On 2010/01/15 06:19:54, rsc wrote:
> Objects?  Avoids needing to introduce the term Entry too.
> 
Sure.

[gri, 2010-01-15 19:41:09]

PTAL.

I moved the type stuff out for now as it is not needed yet.
- Robert

On Fri, Jan 15, 2010 at 10:59 AM, <gri@golang.org> wrote:

>
> http://codereview.appspot.com/189080/diff/1049/67
> File src/pkg/go/ast/ast.go (right):
>
> http://codereview.appspot.com/189080/diff/1049/67#newcode121
> src/pkg/go/ast/ast.go:121: Obj            *Object // denoted object
> On 2010/01/15 06:19:54, rsc wrote:
>
>> Should there be a way to express that the scoping has not
>> yet been computed?
>>
>
>  It might work to add Name string in place of Value
>> and let obj == nil mean that the reference is unknown.
>> It seems like that would be a useful way to make sure
>> that there's only one Object per declaration instead
>> of one Object per identifier instance.
>>
>
>  Regardless, I suggest, for now, leaving Value *Ident here,
>> even if it duplicates information, so that clients don't have
>> to be edited until the new interface has settled.
>>
>
>
> Currently that is done with an Object that has an Err Kind. That is
> likely to change in some way, but I need something that compiles and
> runs.
>
> Clients have to change anyway when adding the Object field, even if
> Value is still here because we use composite literals to create Idents
> in many places (which is actually bad practice from an OO view point).
>
> The accessor function solves this problem.
>
>
> http://codereview.appspot.com/189080/diff/1049/67#newcode376
> src/pkg/go/ast/ast.go:376: // TODO(gri): remove eventually in favor of
> the more evocative
> On 2010/01/15 06:19:54, rsc wrote:
>
>> I don't believe converting x.Value -> x.Value() is any cheaper
>> than converting directly from x.Value -> x.Name(), but as I
>> mentioned above, I think that should wait until the new code
>> is a bit firmer.
>>
>
>
> Sure, but the conversion is done.
>
>
> http://codereview.appspot.com/189080/diff/1049/70
> File src/pkg/go/ast/scope.go (left):
>
> http://codereview.appspot.com/189080/diff/1049/70#oldcode13
> src/pkg/go/ast/scope.go:13: type Scope struct {
> On 2010/01/15 06:19:54, rsc wrote:
>
>> The language spec now calls this concept a block.
>> Should this be Block?
>>
>
>
> I thought about that. I prefer scope simply because it's not just a
> syntactic construct. Block is a syntactic construct. Different objects
> have different scopes, not all of them match the syntactic block.
>
>
> http://codereview.appspot.com/189080/diff/1049/70
> File src/pkg/go/ast/scope.go (right):
>
> http://codereview.appspot.com/189080/diff/1049/70#newcode89
> src/pkg/go/ast/scope.go:89: Entries map[string]*Object
> On 2010/01/15 06:19:54, rsc wrote:
>
>> Objects?  Avoids needing to introduce the term Entry too.
>>
>
> Sure.
>
>
> http://codereview.appspot.com/189080
>

[rsc, 2010-01-15 19:49:51]

http://codereview.appspot.com/189080/diff/204/207
File src/cmd/cgo/main.go (right):

http://codereview.appspot.com/189080/diff/204/207#newcode103
src/cmd/cgo/main.go:103: *cref.Expr = &ast.Ident{Position: pos, Obj:
ast.NewObj(ast.Err, pos, "_C_"+cref.Name)}
Change to call ast.NewIdent and then override pos?
Would be nice not to have any external users of NewObj yet.

http://codereview.appspot.com/189080/diff/204/220
File src/pkg/go/ast/ast.go (right):

http://codereview.appspot.com/189080/diff/204/220#newcode378
src/pkg/go/ast/ast.go:378: func (id *Ident) Value() string { return id.Obj.Name
}
Drop this.  Already rewriting every reference to id.Value;
might as well rewrite directly to Name().

http://codereview.appspot.com/189080/diff/204/222
File src/pkg/go/ast/scope.go (right):

http://codereview.appspot.com/189080/diff/204/222#newcode9
src/pkg/go/ast/scope.go:9: type Kind int
ObjectKind?

[gri, 2010-01-15 21:15:36]

http://codereview.appspot.com/189080/diff/204/207
File src/cmd/cgo/main.go (right):

http://codereview.appspot.com/189080/diff/204/207#newcode103
src/cmd/cgo/main.go:103: *cref.Expr = &ast.Ident{Position: pos, Obj:
ast.NewObj(ast.Err, pos, "_C_"+cref.Name)}
On 2010/01/15 19:49:51, rsc wrote:
> Change to call ast.NewIdent and then override pos?
> Would be nice not to have any external users of NewObj yet.
> 

If NewObj changes in its interface it's easy to find all places that are
affected. Patching pos after using NewIdent also requires knowledge of Object,
so it's no better, and possibly worse because it might compile even though some
fields are not set up that should be. Leave as is.

http://codereview.appspot.com/189080/diff/204/220
File src/pkg/go/ast/ast.go (right):

http://codereview.appspot.com/189080/diff/204/220#newcode378
src/pkg/go/ast/ast.go:378: func (id *Ident) Value() string { return id.Obj.Name
}
On 2010/01/15 19:49:51, rsc wrote:
> Drop this.  Already rewriting every reference to id.Value;
> might as well rewrite directly to Name().
> 

Done.

http://codereview.appspot.com/189080/diff/204/222
File src/pkg/go/ast/scope.go (right):

http://codereview.appspot.com/189080/diff/204/222#newcode9
src/pkg/go/ast/scope.go:9: type Kind int
On 2010/01/15 19:49:51, rsc wrote:
> ObjectKind?
> 
> 

Done.

[rsc, 2010-01-15 21:17:22]

LGTM

> If NewObj changes in its interface it's easy to find all places that are
> affected. Patching pos after using NewIdent also requires knowledge of
> Object, so it's no better, and possibly worse because it might compile
> even though some fields are not set up that should be. Leave as is.

No it doesn't.

*cref.Expr = NewIdent(...)
(*cref.Expr).Position = pos

No reference to Obj at all.

[gri, 2010-01-15 21:25:42]

On Fri, Jan 15, 2010 at 1:17 PM, Russ Cox <rsc@golang.org> wrote:

> LGTM
>

Thanks.

>
> > If NewObj changes in its interface it's easy to find all places that are
> > affected. Patching pos after using NewIdent also requires knowledge of
> > Object, so it's no better, and possibly worse because it might compile
> > even though some fields are not set up that should be. Leave as is.
>
> No it doesn't.
>
> *cref.Expr = NewIdent(...)
> (*cref.Expr).Position = pos
>
> No reference to Obj at all.
>

Yes it does:

*cref.Expr = NewIdent(...)
(*cref.Expr).Position = pos
(*cref.Expr).Obj.Pos = pos

if one wants to set up the object consistently. Admittedly it doesn't matter
in this case because the Pos value is not used, but for consistency it is
certainly better to setup a data structure using a function so there is one
place we can ensure any invariants.

[gri, 2010-01-15 21:27:48]

*** Submitted as http://code.google.com/p/go/source/detail?r=1f84afedd4db ***

	Steps towards tracking scopes for identifiers.

	- Identifiers refer now to the language entity (Object)
	  that they denote. At the moment this is at best an
	  approximation.

	- Initial data structures for language entities (Objects)
          and expression types (Type) independent of the actual
	  type notations.

	- Initial support for declaring and looking up identifiers.

	- Updated various dependent files and added support functions.

	- Extensively tested to avoid breakage. This is an AST change.

R=rsc
CC=golang-dev, rog
http://codereview.appspot.com/189080