Skip to content

Commit

Permalink
go/parser: fix incorrect resolution of receiver type parameters
Browse files Browse the repository at this point in the history 
Declare receiver type parameters in the function scope, but don't
resolve them (for now), as ast.Object.Decl is not documented to hold
*ast.Idents. This avoids incorrect resolution of identifiers to names
outside the function scope.

Also make tracing and error reporting more consistent.

For #50956

Change-Id: I8cd61dd25f4c0f6b974221599b00e23d8da206a1
Reviewed-on: https://go-review.googlesource.com/c/go/+/382247
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
  • Loading branch information
@findleyr
findleyr committed Feb 7, 2022
1 parent 7db75b3 commit 911c78f
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 16 deletions.
93 changes: 78 additions & 15 deletions src/go/parser/resolver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"
"go/ast"
"go/token"
"strings"
)

const debugResolve = false
Expand All @@ -24,6 +25,7 @@ func resolveFile(file *ast.File, handle *token.File, declErr func(token.Pos, str
declErr: declErr,
topScope: pkgScope,
pkgScope: pkgScope,
depth: 1,
}

for _, decl := range file.Decls {
Expand All @@ -45,7 +47,7 @@ func resolveFile(file *ast.File, handle *token.File, declErr func(token.Pos, str
i++
} else if debugResolve {
pos := ident.Obj.Decl.(interface{ Pos() token.Pos }).Pos()
r.dump("resolved %s@%v to package object %v", ident.Name, ident.Pos(), pos)
r.trace("resolved %s@%v to package object %v", ident.Name, ident.Pos(), pos)
}
}
file.Scope = r.pkgScope
Expand All @@ -60,15 +62,16 @@ type resolver struct {
pkgScope *ast.Scope // pkgScope.Outer == nil
topScope *ast.Scope // top-most scope; may be pkgScope
unresolved []*ast.Ident // unresolved identifiers
depth int // scope depth

// Label scopes
// (maintained by open/close LabelScope)
labelScope *ast.Scope // label scope for current function
targetStack [][]*ast.Ident // stack of unresolved labels
}

func (r *resolver) dump(format string, args ...any) {
fmt.Println(">>> " + r.sprintf(format, args...))
func (r *resolver) trace(format string, args ...any) {
fmt.Println(strings.Repeat(". ", r.depth) + r.sprintf(format, args...))
}

func (r *resolver) sprintf(format string, args ...any) string {
Expand All @@ -83,14 +86,16 @@ func (r *resolver) sprintf(format string, args ...any) string {

func (r *resolver) openScope(pos token.Pos) {
if debugResolve {
r.dump("opening scope @%v", pos)
r.trace("opening scope @%v", pos)
r.depth++
}
r.topScope = ast.NewScope(r.topScope)
}

func (r *resolver) closeScope() {
if debugResolve {
r.dump("closing scope")
r.depth--
r.trace("closing scope")
}
r.topScope = r.topScope.Outer
}
Expand All @@ -117,21 +122,27 @@ func (r *resolver) closeLabelScope() {

func (r *resolver) declare(decl, data any, scope *ast.Scope, kind ast.ObjKind, idents ...*ast.Ident) {
for _, ident := range idents {
assert(ident.Obj == nil, "identifier already declared or resolved")
if ident.Obj != nil {
panic(fmt.Sprintf("%v: identifier %s already declared or resolved", ident.Pos(), ident.Name))
}
obj := ast.NewObj(kind, ident.Name)
// remember the corresponding declaration for redeclaration
// errors and global variable resolution/typechecking phase
obj.Decl = decl
obj.Data = data
ident.Obj = obj
// Identifiers (for receiver type parameters) are written to the scope, but
// never set as the resolved object. See issue #50956.
if _, ok := decl.(*ast.Ident); !ok {
ident.Obj = obj
}
if ident.Name != "_" {
if debugResolve {
r.dump("declaring %s@%v", ident.Name, ident.Pos())
r.trace("declaring %s@%v", ident.Name, ident.Pos())
}
if alt := scope.Insert(obj); alt != nil && r.declErr != nil {
prevDecl := ""
if pos := alt.Pos(); pos.IsValid() {
prevDecl = fmt.Sprintf("\n\tprevious declaration at %s", r.handle.Position(pos))
prevDecl = r.sprintf("\n\tprevious declaration at %v", pos)
}
r.declErr(ident.Pos(), fmt.Sprintf("%s redeclared in this block%s", ident.Name, prevDecl))
}
Expand All @@ -153,7 +164,7 @@ func (r *resolver) shortVarDecl(decl *ast.AssignStmt) {
ident.Obj = obj
if ident.Name != "_" {
if debugResolve {
r.dump("declaring %s@%v", ident.Name, ident.Pos())
r.trace("declaring %s@%v", ident.Name, ident.Pos())
}
if alt := r.topScope.Insert(obj); alt != nil {
ident.Obj = alt // redeclaration
Expand All @@ -180,7 +191,7 @@ var unresolved = new(ast.Object)
//
func (r *resolver) resolve(ident *ast.Ident, collectUnresolved bool) {
if ident.Obj != nil {
panic(fmt.Sprintf("%s: identifier %s already declared or resolved", r.handle.Position(ident.Pos()), ident.Name))
panic(r.sprintf("%v: identifier %s already declared or resolved", ident.Pos(), ident.Name))
}
// '_' should never refer to existing declarations, because it has special
// handling in the spec.
Expand All @@ -189,8 +200,15 @@ func (r *resolver) resolve(ident *ast.Ident, collectUnresolved bool) {
}
for s := r.topScope; s != nil; s = s.Outer {
if obj := s.Lookup(ident.Name); obj != nil {
if debugResolve {
r.trace("resolved %v:%s to %v", ident.Pos(), ident.Name, obj)
}
assert(obj.Name != "", "obj with no name")
ident.Obj = obj
// Identifiers (for receiver type parameters) are written to the scope,
// but never set as the resolved object. See issue #50956.
if _, ok := obj.Decl.(*ast.Ident); !ok {
ident.Obj = obj
}
return
}
}
Expand Down Expand Up @@ -227,7 +245,7 @@ func (r *resolver) walkStmts(list []ast.Stmt) {

func (r *resolver) Visit(node ast.Node) ast.Visitor {
if debugResolve && node != nil {
r.dump("node %T@%v", node, node.Pos())
r.trace("node %T@%v", node, node.Pos())
}

switch n := node.(type) {
Expand Down Expand Up @@ -461,8 +479,7 @@ func (r *resolver) Visit(node ast.Node) ast.Visitor {
r.openScope(n.Pos())
defer r.closeScope()

// Resolve the receiver first, without declaring.
r.resolveList(n.Recv)
r.walkRecv(n.Recv)

// Type parameters are walked normally: they can reference each other, and
// can be referenced by normal parameters.
Expand Down Expand Up @@ -519,6 +536,52 @@ func (r *resolver) declareList(list *ast.FieldList, kind ast.ObjKind) {
}
}

func (r *resolver) walkRecv(recv *ast.FieldList) {
// If our receiver has receiver type parameters, we must declare them before
// trying to resolve the rest of the receiver, and avoid re-resolving the
// type parameter identifiers.
if recv == nil || len(recv.List) == 0 {
return // nothing to do
}
typ := recv.List[0].Type
if ptr, ok := typ.(*ast.StarExpr); ok {
typ = ptr.X
}

var declareExprs []ast.Expr // exprs to declare
var resolveExprs []ast.Expr // exprs to resolve
switch typ := typ.(type) {
case *ast.IndexExpr:
declareExprs = []ast.Expr{typ.Index}
resolveExprs = append(resolveExprs, typ.X)
case *ast.IndexListExpr:
declareExprs = typ.Indices
resolveExprs = append(resolveExprs, typ.X)
default:
resolveExprs = append(resolveExprs, typ)
}
for _, expr := range declareExprs {
if id, _ := expr.(*ast.Ident); id != nil {
r.declare(expr, nil, r.topScope, ast.Typ, id)
} else {
// The receiver type parameter expression is invalid, but try to resolve
// it anyway for consistency.
resolveExprs = append(resolveExprs, expr)
}
}
for _, expr := range resolveExprs {
if expr != nil {
ast.Walk(r, expr)
}
}
// The receiver is invalid, but try to resolve it anyway for consistency.
for _, f := range recv.List[1:] {
if f.Type != nil {
ast.Walk(r, f.Type)
}
}
}

func (r *resolver) walkFieldList(list *ast.FieldList, kind ast.ObjKind) {
if list == nil {
return
Expand Down
4 changes: 4 additions & 0 deletions src/go/parser/short_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,8 @@ var invalidNoTParamErrs = []string{
`package p; func (T) _[ /* ERROR "expected '\(', found '\['" */ A, B any](a A) B`,
`package p; func (T) _[ /* ERROR "expected '\(', found '\['" */ A, B C](a A) B`,
`package p; func (T) _[ /* ERROR "expected '\(', found '\['" */ A, B C[A, B]](a A) B`,

`package p; func(*T[ /* ERROR "missing ',' in parameter list" */ e, e]) _()`,
}

// invalidTParamErrs holds invalid source code examples annotated with the
Expand All @@ -255,6 +257,8 @@ var invalidTParamErrs = []string{
`package p; func (T) _[ /* ERROR "must have no type parameters" */ A, B any](a A) B`,
`package p; func (T) _[ /* ERROR "must have no type parameters" */ A, B C](a A) B`,
`package p; func (T) _[ /* ERROR "must have no type parameters" */ A, B C[A, B]](a A) B`,

`package p; func(*T[e, e /* ERROR "e redeclared" */ ]) _()`,
}

func TestInvalid(t *testing.T) {
Expand Down
10 changes: 9 additions & 1 deletion src/go/parser/testdata/resolution/typeparams.go2
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,15 @@ func Add /* =@AddDecl */[T /* =@T */ Addable /* @Addable */](l /* =@l */, r /* =

type Receiver /* =@Receiver */[P /* =@P */ any] struct {}

type RP /* =@RP1 */ struct{}

// TODO(rFindley): make a decision on how/whether to resolve identifiers that
// refer to receiver type parameters, as is the case for the 'P' result
// parameter below.
func (r /* =@recv */ Receiver /* @Receiver */ [P]) m() P {}
//
// For now, we ensure that types are not incorrectly resolved when receiver
// type parameters are in scope.
func (r /* =@recv */ Receiver /* @Receiver */ [RP]) m(RP) RP {}

func f /* =@f */[T1 /* =@T1 */ interface{~[]T2 /* @T2 */}, T2 /* =@T2 */ any](
x /* =@x */ T1 /* @T1 */, T1 /* =@T1_duplicate */ y, // Note that this is a bug:
Expand All @@ -41,3 +46,6 @@ func f /* =@f */[T1 /* =@T1 */ interface{~[]T2 /* @T2 */}, T2 /* =@T2 */ any](
T1 /* @T1 */ := 0
var t1var /* =@t1var */ T1 /* @T1 */
}

// From issue #39634
func(*ph1[e, e])h(d)

0 comments on commit 911c78f

Please sign in to comment.