code review 4271061: go/parser: resolve identifiers properly

go/parser: resolve identifiers properly

Correctly distinguish between lhs and rhs identifiers
and resolve/declare them accordingly.

Collect field and method names in respective scopes
(will be available after some minor AST API changes).

Also collect imports since it's useful to have that
list directly w/o having to re-traverse the AST
(will also be available after some minor AST API changes).

No external API changes in this CL.

[gri, 2011-03-23 22:36:39]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[rog, 2011-03-24 08:55:47]

LGTM

http://codereview.appspot.com/4271061/diff/9001/src/pkg/go/parser/interface.go
File src/pkg/go/parser/interface.go (right):

http://codereview.appspot.com/4271061/diff/9001/src/pkg/go/parser/interface.g...
src/pkg/go/parser/interface.go:72: x := p.parseRhs()
it would be useful if ParseExpr were to return the unresolved symbols in an
expression. similarly for ParseStmtList and ParseDeclList below.

[rsc, 2011-03-24 13:41:39]

> it would be useful if ParseExpr were to return the unresolved symbols in
> an expression. similarly for ParseStmtList and ParseDeclList below.

I think you'd want that to be a separate function, not additional
return values.  Also, in a standalone expression (func exprs
don't count) aren't all the symbols unresolved?

Russ

[rog, 2011-03-24 13:43:10]

On 24 March 2011 13:41, Russ Cox <rsc@golang.org> wrote:
>> it would be useful if ParseExpr were to return the unresolved symbols in
>> an expression. similarly for ParseStmtList and ParseDeclList below.
>
> I think you'd want that to be a separate function, not additional
> return values.  Also, in a standalone expression (func exprs
> don't count) aren't all the symbols unresolved?

no, because you can have function literals.

[rsc, 2011-03-24 13:50:23]

LGTM

It would be nice if a final pass rewrote the Obj pointers from unresolved to
nil, so that client code can test for unresolved by == nil.

As is I believe client code must use Name == Name && Obj == Obj in order to make
sure an unresolved x is not treated the same as an unresolved y.  That would be
cleaner as Obj != nil && Obj == Obj.

http://codereview.appspot.com/4271061/diff/9001/src/pkg/go/parser/parser.go
File src/pkg/go/parser/parser.go (right):

http://codereview.appspot.com/4271061/diff/9001/src/pkg/go/parser/parser.go#n...
src/pkg/go/parser/parser.go:146: panic("internal error: identifier already
declared or resolved")
internal -> go/parser: internal

http://codereview.appspot.com/4271061/diff/9001/src/pkg/go/parser/parser.go#n...
src/pkg/go/parser/parser.go:174: panic("internal error: identifier already
declared or resolved")
same

http://codereview.appspot.com/4271061/diff/9001/src/pkg/go/parser/parser.go#n...
src/pkg/go/parser/parser.go:207: panic("internal error: identifier already
declared or resolved")
same

[rog, 2011-03-24 14:13:55]

On 24 March 2011 13:50,  <rsc@golang.org> wrote:
> LGTM
>
> It would be nice if a final pass rewrote the Obj pointers from
> unresolved to nil, so that client code can test for unresolved by ==
> nil.

i think it does. see line 2246:
       ident.Obj = p.pkgScope.Lookup(ident.Name) // also removes
unresolved sentinel

[rsc, 2011-03-24 14:16:34]

LGTM

Thanks Roger.

[gri, 2011-03-24 18:45:36]

On Thu, Mar 24, 2011 at 6:50 AM,  <rsc@golang.org> wrote:
> LGTM

Thanks for the reviews.

>
> It would be nice if a final pass rewrote the Obj pointers from
> unresolved to nil, so that client code can test for unresolved by ==
> nil.

As Roger points out, this should already happen.

>
> As is I believe client code must use Name == Name && Obj == Obj in order
> to make sure an unresolved x is not treated the same as an unresolved y.
>  That would be cleaner as Obj != nil && Obj == Obj.

I'm not sure I understand: As it is, if Obj != nil, two identifier
denote the same object if the Object ptrs are the same (at least
that's how it should be). Two identifiers that are not resolved may or
may not denote the same object even if they have the same name. But I
assume you are referring to something else?


>
>
> http://codereview.appspot.com/4271061/diff/9001/src/pkg/go/parser/parser.go
> File src/pkg/go/parser/parser.go (right):
>
>
http://codereview.appspot.com/4271061/diff/9001/src/pkg/go/parser/parser.go#n...
> src/pkg/go/parser/parser.go:146: panic("internal error: identifier
> already declared or resolved")
> internal -> go/parser: internal

fixed all those by introducing asserts


>
>
http://codereview.appspot.com/4271061/diff/9001/src/pkg/go/parser/parser.go#n...
> src/pkg/go/parser/parser.go:174: panic("internal error: identifier
> already declared or resolved")
> same
>
>
http://codereview.appspot.com/4271061/diff/9001/src/pkg/go/parser/parser.go#n...
> src/pkg/go/parser/parser.go:207: panic("internal error: identifier
> already declared or resolved")
> same
>
> http://codereview.appspot.com/4271061/
>

Regarding Roger's suggestion of having access to unresolved
identifiers after ParseExpr, ParseDecl, etc. Perhaps there should be
some functionality like that, but I am not super happy with the
current API for those functions anyway. For them to be useful, I think
one may also want to be able to provide some context (with declared
identifiers) before they are called. I'm aware of the issue; I'll
tackle it once we have the other pieces in place.

- Robert

[gri, 2011-03-24 18:46:00]

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

go/parser: resolve identifiers properly

Correctly distinguish between lhs and rhs identifiers
and resolve/declare them accordingly.

Collect field and method names in respective scopes
(will be available after some minor AST API changes).

Also collect imports since it's useful to have that
list directly w/o having to re-traverse the AST
(will also be available after some minor AST API changes).

No external API changes in this CL.

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

[rsc, 2011-03-26 16:10:24]

>> As is I believe client code must use Name == Name && Obj == Obj in order
>> to make sure an unresolved x is not treated the same as an unresolved y.
>>  That would be cleaner as Obj != nil && Obj == Obj.
>
> I'm not sure I understand: As it is, if Obj != nil, two identifier
> denote the same object if the Object ptrs are the same (at least
> that's how it should be).

I was missing the part in the code where it rewrites
Obj == unresolved to Obj == nil.  Roger pointed it out
for me.

> Two identifiers that are not resolved may or
> may not denote the same object even if they have the same name.

After resolving all the names that can be resolved within
a single file, all that is left is unresolved references that
will end up referring to an imported name or a top-level name
declared in another file.  There can only be one of those
in a valid package, so I believe it *is* true that two identifiers
with Obj == nil and the same name do always refer to the
same object.

Russ

[gri, 2011-03-26 16:16:56]

On Sat, Mar 26, 2011 at 9:10 AM, Russ Cox <rsc@golang.org> wrote:
>>> As is I believe client code must use Name == Name && Obj == Obj in order
>>> to make sure an unresolved x is not treated the same as an unresolved y.
>>>  That would be cleaner as Obj != nil && Obj == Obj.
>>
>> I'm not sure I understand: As it is, if Obj != nil, two identifier
>> denote the same object if the Object ptrs are the same (at least
>> that's how it should be).
>
> I was missing the part in the code where it rewrites
> Obj == unresolved to Obj == nil.  Roger pointed it out
> for me.

yes, I saw that.
>
>> Two identifiers that are not resolved may or
>> may not denote the same object even if they have the same name.
>
> After resolving all the names that can be resolved within
> a single file, all that is left is unresolved references that
> will end up referring to an imported name or a top-level name
> declared in another file.  There can only be one of those
> in a valid package, so I believe it *is* true that two identifiers
> with Obj == nil and the same name do always refer to the
> same object.

good point. yes.
- robert

>
> Russ
>