go/types: add Checker.walkDecl to simplify checking declarations

Handling ast.GenDecls while typechecking is repetitive, and a source of
diffs compared to typechecking using the cmd/compile/internal/syntax
package, which unpacks declaration groups into individual declarations.

Refactor to extract the logic for walking declarations.  This introduces
a new AST abstraction: types.decl, which comes at some minor performance
cost. However, if we are to fully abstract the AST we will be paying
this cost anyway, and benchmarking suggests that the cost is negligible.

Change-Id: If73c30c3d08053ccf7bf21ef886f0452fdbf142e
Reviewed-on: https://go-review.googlesource.com/c/go/+/256298
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Trust: Robert Findley <rfindley@google.com>

src/go/types/decl.go

@@ -381,6 +381,76 @@ func firstInSrc(path []Object) int {
 	return fst
 }
 
+type (
+	decl interface {
+		node() ast.Node
+	}
+
+	importDecl struct{ spec *ast.ImportSpec }
+	constDecl  struct {
+		spec *ast.ValueSpec
+		iota int
+		typ  ast.Expr
+		init []ast.Expr
+	}
+	varDecl  struct{ spec *ast.ValueSpec }
+	typeDecl struct{ spec *ast.TypeSpec }
+	funcDecl struct{ decl *ast.FuncDecl }
+)
+
+func (d importDecl) node() ast.Node { return d.spec }
+func (d constDecl) node() ast.Node  { return d.spec }
+func (d varDecl) node() ast.Node    { return d.spec }
+func (d typeDecl) node() ast.Node   { return d.spec }
+func (d funcDecl) node() ast.Node   { return d.decl }
+
+func (check *Checker) walkDecls(decls []ast.Decl, f func(decl)) {
+	for _, d := range decls {
+		check.walkDecl(d, f)
+	}
+}
+
+func (check *Checker) walkDecl(d ast.Decl, f func(decl)) {
+	switch d := d.(type) {
+	case *ast.BadDecl:
+		// ignore
+	case *ast.GenDecl:
+		var last *ast.ValueSpec // last ValueSpec with type or init exprs seen
+		for iota, s := range d.Specs {
+			switch s := s.(type) {
+			case *ast.ImportSpec:
+				f(importDecl{s})
+			case *ast.ValueSpec:
+				switch d.Tok {
+				case token.CONST:
+					// determine which initialization expressions to use
+					switch {
+					case s.Type != nil || len(s.Values) > 0:
+						last = s
+					case last == nil:
+						last = new(ast.ValueSpec) // make sure last exists
+					}
+					check.arityMatch(s, last)
+					f(constDecl{spec: s, iota: iota, init: last.Values, typ: last.Type})
+				case token.VAR:
+					check.arityMatch(s, nil)
+					f(varDecl{s})
+				default:
+					check.invalidAST(s.Pos(), "invalid token %s", d.Tok)
+				}
+			case *ast.TypeSpec:
+				f(typeDecl{s})
+			default:
+				check.invalidAST(s.Pos(), "unknown ast.Spec node %T", s)
+			}
+		}
+	case *ast.FuncDecl:
+		f(funcDecl{d})
+	default:
+		check.invalidAST(d.Pos(), "unknown ast.Decl node %T", d)
+	}
+}
+
 func (check *Checker) constDecl(obj *Const, typ, init ast.Expr) {
 	assert(obj.typ == nil)
 
@@ -664,46 +734,28 @@ func (check *Checker) funcDecl(obj *Func, decl *declInfo) {
 	}
 }
 
-func (check *Checker) declStmt(decl ast.Decl) {
+func (check *Checker) declStmt(d ast.Decl) {
 	pkg := check.pkg
 
-	switch d := decl.(type) {
+	check.walkDecl(d, func(d decl) {
-	case *ast.BadDecl:
+		switch d := d.(type) {
-		// ignore
+		case constDecl:
-
-	case *ast.GenDecl:
-		var last *ast.ValueSpec // last ValueSpec with type or init exprs seen
-		for iota, spec := range d.Specs {
-			switch s := spec.(type) {
-			case *ast.ValueSpec:
-				switch d.Tok {
-				case token.CONST:
 			top := len(check.delayed)
 
-					// determine which init exprs to use
-					switch {
-					case s.Type != nil || len(s.Values) > 0:
-						last = s
-					case last == nil:
-						last = new(ast.ValueSpec) // make sure last exists
-					}
-
 			// declare all constants
-					lhs := make([]*Const, len(s.Names))
+			lhs := make([]*Const, len(d.spec.Names))
-					for i, name := range s.Names {
+			for i, name := range d.spec.Names {
-						obj := NewConst(name.Pos(), pkg, name.Name, nil, constant.MakeInt64(int64(iota)))
+				obj := NewConst(name.Pos(), pkg, name.Name, nil, constant.MakeInt64(int64(d.iota)))
 				lhs[i] = obj
 
 				var init ast.Expr
-						if i < len(last.Values) {
+				if i < len(d.init) {
-							init = last.Values[i]
+					init = d.init[i]
 				}
 
-						check.constDecl(obj, last.Type, init)
+				check.constDecl(obj, d.typ, init)
 			}
 
-					check.arityMatch(s, last)
-
 			// process function literals in init expressions before scope changes
 			check.processDelayed(top)
 
@@ -711,16 +763,16 @@ func (check *Checker) declStmt(decl ast.Decl) {
 			// inside a function begins at the end of the ConstSpec or VarSpec
 			// (ShortVarDecl for short variable declarations) and ends at the
 			// end of the innermost containing block."
-					scopePos := s.End()
+			scopePos := d.spec.End()
-					for i, name := range s.Names {
+			for i, name := range d.spec.Names {
 				check.declare(check.scope, name, lhs[i], scopePos)
 			}
 
-				case token.VAR:
+		case varDecl:
 			top := len(check.delayed)
 
-					lhs0 := make([]*Var, len(s.Names))
+			lhs0 := make([]*Var, len(d.spec.Names))
-					for i, name := range s.Names {
+			for i, name := range d.spec.Names {
 				lhs0[i] = NewVar(name.Pos(), pkg, name.Name, nil)
 			}
 
@@ -728,21 +780,21 @@ func (check *Checker) declStmt(decl ast.Decl) {
 			for i, obj := range lhs0 {
 				var lhs []*Var
 				var init ast.Expr
-						switch len(s.Values) {
+				switch len(d.spec.Values) {
-						case len(s.Names):
+				case len(d.spec.Names):
 					// lhs and rhs match
-							init = s.Values[i]
+					init = d.spec.Values[i]
 				case 1:
 					// rhs is expected to be a multi-valued expression
 					lhs = lhs0
-							init = s.Values[0]
+					init = d.spec.Values[0]
 				default:
-							if i < len(s.Values) {
+					if i < len(d.spec.Values) {
-								init = s.Values[i]
+						init = d.spec.Values[i]
 					}
 				}
-						check.varDecl(obj, lhs, s.Type, init)
+				check.varDecl(obj, lhs, d.spec.Type, init)
-						if len(s.Values) == 1 {
+				if len(d.spec.Values) == 1 {
 					// If we have a single lhs variable we are done either way.
 					// If we have a single rhs expression, it must be a multi-
 					// valued expression, in which case handling the first lhs
@@ -757,40 +809,30 @@ func (check *Checker) declStmt(decl ast.Decl) {
 				}
 			}
 
-					check.arityMatch(s, nil)
-
 			// process function literals in init expressions before scope changes
 			check.processDelayed(top)
 
 			// declare all variables
 			// (only at this point are the variable scopes (parents) set)
-					scopePos := s.End() // see constant declarations
+			scopePos := d.spec.End() // see constant declarations
-					for i, name := range s.Names {
+			for i, name := range d.spec.Names {
 				// see constant declarations
 				check.declare(check.scope, name, lhs0[i], scopePos)
 			}
 
-				default:
+		case typeDecl:
-					check.invalidAST(s.Pos(), "invalid token %s", d.Tok)
+			obj := NewTypeName(d.spec.Name.Pos(), pkg, d.spec.Name.Name, nil)
-				}
-
-			case *ast.TypeSpec:
-				obj := NewTypeName(s.Name.Pos(), pkg, s.Name.Name, nil)
 			// spec: "The scope of a type identifier declared inside a function
 			// begins at the identifier in the TypeSpec and ends at the end of
 			// the innermost containing block."
-				scopePos := s.Name.Pos()
+			scopePos := d.spec.Name.Pos()
-				check.declare(check.scope, s.Name, obj, scopePos)
+			check.declare(check.scope, d.spec.Name, obj, scopePos)
 			// mark and unmark type before calling typeDecl; its type is still nil (see Checker.objDecl)
 			obj.setColor(grey + color(check.push(obj)))
-				check.typeDecl(obj, s.Type, nil, s.Assign.IsValid())
+			check.typeDecl(obj, d.spec.Type, nil, d.spec.Assign.IsValid())
 			check.pop().setColor(black)
 		default:
-				check.invalidAST(s.Pos(), "const, type, or var declaration expected")
+			check.invalidAST(d.node().Pos(), "unknown ast.Decl node %T", d.node())
-			}
-		}
-
-	default:
-		check.invalidAST(d.Pos(), "unknown ast.Decl node %T", d)
 		}
+	})
 }

src/go/types/resolver.go

@@ -235,26 +235,19 @@ func (check *Checker) collectObjects() {
 		// we get "." as the directory which is what we would want.
 		fileDir := dir(check.fset.Position(file.Name.Pos()).Filename)
 
-		for _, decl := range file.Decls {
+		check.walkDecls(file.Decls, func(d decl) {
-			switch d := decl.(type) {
+			switch d := d.(type) {
-			case *ast.BadDecl:
+			case importDecl:
-				// ignore
-
-			case *ast.GenDecl:
-				var last *ast.ValueSpec // last ValueSpec with type or init exprs seen
-				for iota, spec := range d.Specs {
-					switch s := spec.(type) {
-					case *ast.ImportSpec:
 				// import package
-						path, err := validatedImportPath(s.Path.Value)
+				path, err := validatedImportPath(d.spec.Path.Value)
 				if err != nil {
-							check.errorf(s.Path.Pos(), "invalid import path (%s)", err)
+					check.errorf(d.spec.Path.Pos(), "invalid import path (%s)", err)
-							continue
+					return
 				}
 
-						imp := check.importPackage(s.Path.Pos(), path, fileDir)
+				imp := check.importPackage(d.spec.Path.Pos(), path, fileDir)
 				if imp == nil {
-							continue
+					return
 				}
 
 				// add package to list of explicit imports
@@ -267,25 +260,25 @@ func (check *Checker) collectObjects() {
 
 				// local name overrides imported package name
 				name := imp.name
-						if s.Name != nil {
+				if d.spec.Name != nil {
-							name = s.Name.Name
+					name = d.spec.Name.Name
 					if path == "C" {
 						// match cmd/compile (not prescribed by spec)
-								check.errorf(s.Name.Pos(), `cannot rename import "C"`)
+						check.errorf(d.spec.Name.Pos(), `cannot rename import "C"`)
-								continue
+						return
 					}
 					if name == "init" {
-								check.errorf(s.Name.Pos(), "cannot declare init - must be func")
+						check.errorf(d.spec.Name.Pos(), "cannot declare init - must be func")
-								continue
+						return
 					}
 				}
 
-						obj := NewPkgName(s.Pos(), pkg, name, imp)
+				obj := NewPkgName(d.spec.Pos(), pkg, name, imp)
-						if s.Name != nil {
+				if d.spec.Name != nil {
 					// in a dot-import, the dot represents the package
-							check.recordDef(s.Name, obj)
+					check.recordDef(d.spec.Name, obj)
 				} else {
-							check.recordImplicit(s, obj)
+					check.recordImplicit(d.spec, obj)
 				}
 
 				if path == "C" {
@@ -306,108 +299,83 @@ func (check *Checker) collectObjects() {
 							// the object may be imported into more than one file scope
 							// concurrently. See issue #32154.)
 							if alt := fileScope.Insert(obj); alt != nil {
-										check.errorf(s.Name.Pos(), "%s redeclared in this block", obj.Name())
+								check.errorf(d.spec.Name.Pos(), "%s redeclared in this block", obj.Name())
 								check.reportAltDecl(alt)
 							}
 						}
 					}
 					// add position to set of dot-import positions for this file
 					// (this is only needed for "imported but not used" errors)
-							check.addUnusedDotImport(fileScope, imp, s.Pos())
+					check.addUnusedDotImport(fileScope, imp, d.spec.Pos())
 				} else {
 					// declare imported package object in file scope
 					// (no need to provide s.Name since we called check.recordDef earlier)
 					check.declare(fileScope, nil, obj, token.NoPos)
 				}
-
+			case constDecl:
-					case *ast.ValueSpec:
-						switch d.Tok {
-						case token.CONST:
-							// determine which initialization expressions to use
-							switch {
-							case s.Type != nil || len(s.Values) > 0:
-								last = s
-							case last == nil:
-								last = new(ast.ValueSpec) // make sure last exists
-							}
-
 				// declare all constants
-							for i, name := range s.Names {
+				for i, name := range d.spec.Names {
-								obj := NewConst(name.Pos(), pkg, name.Name, nil, constant.MakeInt64(int64(iota)))
+					obj := NewConst(name.Pos(), pkg, name.Name, nil, constant.MakeInt64(int64(d.iota)))
 
 					var init ast.Expr
-								if i < len(last.Values) {
+					if i < len(d.init) {
-									init = last.Values[i]
+						init = d.init[i]
 					}
 
-								d := &declInfo{file: fileScope, typ: last.Type, init: init}
+					d := &declInfo{file: fileScope, typ: d.typ, init: init}
 					check.declarePkgObj(name, obj, d)
 				}
 
-							check.arityMatch(s, last)
+			case varDecl:
-
+				lhs := make([]*Var, len(d.spec.Names))
-						case token.VAR:
-							lhs := make([]*Var, len(s.Names))
 				// If there's exactly one rhs initializer, use
 				// the same declInfo d1 for all lhs variables
 				// so that each lhs variable depends on the same
 				// rhs initializer (n:1 var declaration).
 				var d1 *declInfo
-							if len(s.Values) == 1 {
+				if len(d.spec.Values) == 1 {
 					// The lhs elements are only set up after the for loop below,
 					// but that's ok because declareVar only collects the declInfo
 					// for a later phase.
-								d1 = &declInfo{file: fileScope, lhs: lhs, typ: s.Type, init: s.Values[0]}
+					d1 = &declInfo{file: fileScope, lhs: lhs, typ: d.spec.Type, init: d.spec.Values[0]}
 				}
 
 				// declare all variables
-							for i, name := range s.Names {
+				for i, name := range d.spec.Names {
 					obj := NewVar(name.Pos(), pkg, name.Name, nil)
 					lhs[i] = obj
 
-								d := d1
+					di := d1
-								if d == nil {
+					if di == nil {
 						// individual assignments
 						var init ast.Expr
-									if i < len(s.Values) {
+						if i < len(d.spec.Values) {
-										init = s.Values[i]
+							init = d.spec.Values[i]
 						}
-									d = &declInfo{file: fileScope, typ: s.Type, init: init}
+						di = &declInfo{file: fileScope, typ: d.spec.Type, init: init}
 					}
 
-								check.declarePkgObj(name, obj, d)
+					check.declarePkgObj(name, obj, di)
 				}
-
+			case typeDecl:
-							check.arityMatch(s, nil)
+				obj := NewTypeName(d.spec.Name.Pos(), pkg, d.spec.Name.Name, nil)
-
+				check.declarePkgObj(d.spec.Name, obj, &declInfo{file: fileScope, typ: d.spec.Type, alias: d.spec.Assign.IsValid()})
-						default:
+			case funcDecl:
-							check.invalidAST(s.Pos(), "invalid token %s", d.Tok)
+				info := &declInfo{file: fileScope, fdecl: d.decl}
-						}
+				name := d.decl.Name.Name
-
+				obj := NewFunc(d.decl.Name.Pos(), pkg, name, nil)
-					case *ast.TypeSpec:
+				if d.decl.Recv == nil {
-						obj := NewTypeName(s.Name.Pos(), pkg, s.Name.Name, nil)
-						check.declarePkgObj(s.Name, obj, &declInfo{file: fileScope, typ: s.Type, alias: s.Assign.IsValid()})
-
-					default:
-						check.invalidAST(s.Pos(), "unknown ast.Spec node %T", s)
-					}
-				}
-
-			case *ast.FuncDecl:
-				name := d.Name.Name
-				obj := NewFunc(d.Name.Pos(), pkg, name, nil)
-				if d.Recv == nil {
 					// regular function
 					if name == "init" {
 						// don't declare init functions in the package scope - they are invisible
 						obj.parent = pkg.scope
-						check.recordDef(d.Name, obj)
+						check.recordDef(d.decl.Name, obj)
 						// init functions must have a body
-						if d.Body == nil {
+						if d.decl.Body == nil {
 							check.softErrorf(obj.pos, "missing function body")
 						}
 					} else {
-						check.declare(pkg.scope, d.Name, obj, token.NoPos)
+						check.declare(pkg.scope, d.decl.Name, obj, token.NoPos)
 					}
 				} else {
 					// method
@@ -417,20 +385,16 @@ func (check *Checker) collectObjects() {
 					if name != "_" {
 						methods = append(methods, obj)
 					}
-					check.recordDef(d.Name, obj)
+					check.recordDef(d.decl.Name, obj)
 				}
-				info := &declInfo{file: fileScope, fdecl: d}
 				// Methods are not package-level objects but we still track them in the
 				// object map so that we can handle them like regular functions (if the
 				// receiver is invalid); also we need their fdecl info when associating
 				// them with their receiver base type, below.
 				check.objMap[obj] = info
 				obj.setOrder(uint32(len(check.objMap)))
-
-			default:
-				check.invalidAST(d.Pos(), "unknown ast.Decl node %T", d)
-			}
 			}
+		})
 	}
 
 	// verify that objects in package and file scopes have different names