resolve: move resolver types to resolver package (google#211)

The job of the resolver is to compute a binding for
each identifer, the local and free variables for each function,
and the locals and globals for each module. As a space optimization and
for convenience, this information is saved in the syntax tree rather
than in a side table.

We have a choice between declaring these resolver data structures
in the syntax package or the resolve package. Putting them in the syntax
package, as we did prior to this change, allowed each Identifier, File,
DefStmt, and LambdaExpr to depend directly on the resolver types,
even though those types didn't really belong there.
This change moves these types to the resolver package where they belong.

The dependencies on the types are broken by using an interface{} in each case.
(The values are are all pointers, so this does not induce new allocation.)

Also, this change eliminates syntax.Function, which was a false abstraction
of DefStmt and LambdaExpr. The two syntax nodes are now fully concrete;
it is in the resolver that their shared concept of Function belongs.

This is a breaking API change, but it should affect very few clients
and be trivial to fix.

internal/compile/compile.go

@@ -985,7 +985,7 @@ func (fcomp *fcomp) setPos(pos syntax.Position) {
 // set emits code to store the top-of-stack value
 // to the specified local, cell, or global variable.
 func (fcomp *fcomp) set(id *syntax.Ident) {
-	bind := id.Binding
+	bind := id.Binding.(*resolve.Binding)
 	switch bind.Scope {
 	case resolve.Local:
 		fcomp.emit1(SETLOCAL, uint32(bind.Index))
@@ -1002,7 +1002,7 @@ func (fcomp *fcomp) set(id *syntax.Ident) {
 
 // lookup emits code to push the value of the specified variable.
 func (fcomp *fcomp) lookup(id *syntax.Ident) {
-	bind := id.Binding
+	bind := id.Binding.(*resolve.Binding)
 	if bind.Scope != resolve.Universal { // (universal lookup can't fail)
 		fcomp.setPos(id.NamePos)
 	}
@@ -1149,7 +1149,7 @@ func (fcomp *fcomp) stmt(stmt syntax.Stmt) {
 		}
 
 	case *syntax.DefStmt:
-		fcomp.function(stmt.Def, stmt.Name.Name, &stmt.Function)
+		fcomp.function(stmt.Function.(*resolve.Function))
 		fcomp.set(stmt.Name)
 
 	case *syntax.ForStmt:
@@ -1428,7 +1428,7 @@ func (fcomp *fcomp) expr(e syntax.Expr) {
 		fcomp.call(e)
 
 	case *syntax.LambdaExpr:
-		fcomp.function(e.Lambda, "lambda", &e.Function)
+		fcomp.function(e.Function.(*resolve.Function))
 
 	default:
 		start, _ := e.Span()
@@ -1777,9 +1777,9 @@ func (fcomp *fcomp) comprehension(comp *syntax.Comprehension, clauseIndex int) {
 	log.Fatalf("%s: unexpected comprehension clause %T", start, clause)
 }
 
-func (fcomp *fcomp) function(pos syntax.Position, name string, f *syntax.Function) {
+func (fcomp *fcomp) function(f *resolve.Function) {
 	// Evaluation of the defaults may fail, so record the position.
-	fcomp.setPos(pos)
+	fcomp.setPos(f.Pos)
 
 	// To reduce allocation, we emit a combined tuple
 	// for the defaults and the freevars.
@@ -1821,7 +1821,7 @@ func (fcomp *fcomp) function(pos syntax.Position, name string, f *syntax.Functio
 
 	fcomp.emit1(MAKETUPLE, uint32(ndefaults+len(f.FreeVars)))
 
-	funcode := fcomp.pcomp.function(name, pos, f.Body, f.Locals, f.FreeVars)
+	funcode := fcomp.pcomp.function(f.Name, f.Pos, f.Body, f.Locals, f.FreeVars)
 
 	if debug {
 		// TODO(adonovan): do compilations sequentially not as a tree,

resolve/binding.go

@@ -0,0 +1,74 @@
+// Copyright 2019 The Bazel Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package resolve
+
+import "go.starlark.net/syntax"
+
+// This file defines resolver data types saved in the syntax tree.
+// We cannot guarantee API stability for these types
+// as they are closely tied to the implementation.
+
+// A Binding contains resolver information about an identifer.
+// The resolver populates the Binding field of each syntax.Identifier.
+// The Binding ties together all identifiers that denote the same variable.
+type Binding struct {
+	Scope Scope
+
+	// Index records the index into the enclosing
+	// - {DefStmt,File}.Locals, if Scope==Local
+	// - DefStmt.FreeVars,      if Scope==Free
+	// - File.Globals,          if Scope==Global.
+	// It is zero if Scope is Predeclared, Universal, or Undefined.
+	Index int
+
+	First *syntax.Ident // first binding use (iff Scope==Local/Free/Global)
+}
+
+// The Scope of Binding indicates what kind of scope it has.
+type Scope uint8
+
+const (
+	Undefined   Scope = iota // name is not defined
+	Local                    // name is local to its function or file
+	Cell                     // name is function-local but shared with a nested function
+	Free                     // name is cell of some enclosing function
+	Global                   // name is global to module
+	Predeclared              // name is predeclared for this module (e.g. glob)
+	Universal                // name is universal (e.g. len)
+)
+
+var scopeNames = [...]string{
+	Undefined:   "undefined",
+	Local:       "local",
+	Cell:        "cell",
+	Free:        "free",
+	Global:      "global",
+	Predeclared: "predeclared",
+	Universal:   "universal",
+}
+
+func (scope Scope) String() string { return scopeNames[scope] }
+
+// A Module contains resolver information about a file.
+// The resolver populates the Module field of each syntax.File.
+type Module struct {
+	Locals  []*Binding // the file's (comprehension-)local variables
+	Globals []*Binding // the file's global variables
+}
+
+// A Function contains resolver information about a named or anonymous function.
+// The resolver populates the Function field of each syntax.DefStmt and syntax.LambdaExpr.
+type Function struct {
+	Pos    syntax.Position // of DEF or LAMBDA
+	Name   string          // name of def, or "lambda"
+	Params []syntax.Expr   // param = ident | ident=expr | * | *ident | **ident
+	Body   []syntax.Stmt   // contains synthetic 'return expr' for lambda
+
+	HasVarargs      bool       // whether params includes *args (convenience)
+	HasKwargs       bool       // whether params includes **kwargs (convenience)
+	NumKwonlyParams int        // number of keyword-only optional parameters
+	Locals          []*Binding // this function's local/cell variables, parameters first
+	FreeVars        []*Binding // enclosing cells to capture in closure
+}

resolve/resolve.go

@@ -108,7 +108,8 @@ var (
 	LoadBindsGlobally   = false // load creates global not file-local bindings (deprecated)
 )
 
-// File resolves the specified file.
+// File resolves the specified file and records information about the
+// module in file.Module.
 //
 // The isPredeclared and isUniversal predicates report whether a name is
 // a pre-declared identifier (visible in the current module) or a
@@ -131,8 +132,10 @@ func File(file *syntax.File, isPredeclared, isUniversal func(name string) bool)
 	// Function bodies may contain forward references to later global declarations.
 	r.resolveNonLocalUses(r.env)
 
-	file.Locals = r.moduleLocals
-	file.Globals = r.moduleGlobals
+	file.Module = &Module{
+		Locals:  r.moduleLocals,
+		Globals: r.moduleGlobals,
+	}
 
 	if len(r.errors) > 0 {
 		return r.errors
@@ -180,20 +183,6 @@ func newResolver(isPredeclared, isUniversal func(name string) bool) *resolver {
 	}
 }
 
-// Declare aliases for types and constants that logically belong here.
-
-type Binding = syntax.Binding
-
-const (
-	Undefined   = syntax.UndefinedScope
-	Local       = syntax.LocalScope
-	Cell        = syntax.CellScope
-	Free        = syntax.FreeScope
-	Global      = syntax.GlobalScope
-	Predeclared = syntax.PredeclaredScope
-	Universal   = syntax.UniversalScope
-)
-
 type resolver struct {
 	// env is the current local environment:
 	// a linked list of blocks, innermost first.
@@ -244,7 +233,7 @@ type block struct {
 	parent *block // nil for file block
 
 	// In the file (root) block, both these fields are nil.
-	function *syntax.Function      // only for function blocks
+	function *Function             // only for function blocks
 	comp     *syntax.Comprehension // only for comprehension blocks
 
 	// bindings maps a name to its binding.
@@ -272,7 +261,7 @@ func (b *block) bind(name string, bind *Binding) {
 
 func (b *block) String() string {
 	if b.function != nil {
-		return "function block at " + fmt.Sprint(b.function.Span())
+		return "function block at " + fmt.Sprint(b.function.Pos)
 	}
 	if b.comp != nil {
 		return "comprehension block at " + fmt.Sprint(b.comp.Span())
@@ -502,7 +491,14 @@ func (r *resolver) stmt(stmt syntax.Stmt) {
 			r.errorf(stmt.Def, doesnt+"support nested def")
 		}
 		r.bind(stmt.Name)
-		r.function(stmt.Def, stmt.Name.Name, &stmt.Function)
+		fn := &Function{
+			Name:   stmt.Name.Name,
+			Pos:    stmt.Def,
+			Params: stmt.Params,
+			Body:   stmt.Body,
+		}
+		stmt.Function = fn
+		r.function(fn, stmt.Def)
 
 	case *syntax.ForStmt:
 		if !AllowGlobalReassign && r.container().function == nil {
@@ -773,7 +769,14 @@ func (r *resolver) expr(e syntax.Expr) {
 		if !AllowLambda {
 			r.errorf(e.Lambda, doesnt+"support lambda")
 		}
-		r.function(e.Lambda, "lambda", &e.Function)
+		fn := &Function{
+			Name:   "lambda",
+			Pos:    e.Lambda,
+			Params: e.Params,
+			Body:   []syntax.Stmt{&syntax.ReturnStmt{Result: e.Body}},
+		}
+		e.Function = fn
+		r.function(fn, e.Lambda)
 
 	case *syntax.ParenExpr:
 		r.expr(e.X)
@@ -783,7 +786,7 @@ func (r *resolver) expr(e syntax.Expr) {
 	}
 }
 
-func (r *resolver) function(pos syntax.Position, name string, function *syntax.Function) {
+func (r *resolver) function(function *Function, pos syntax.Position) {
 	// Resolve defaults in enclosing environment.
 	for _, param := range function.Params {
 		if binary, ok := param.(*syntax.BinaryExpr); ok {
@@ -944,7 +947,7 @@ func (r *resolver) lookupLexical(use use, env *block) (bind *Binding) {
 			}
 			if debug {
 				fmt.Printf("creating freevar %v in function at %s: %s\n",
-					len(env.function.FreeVars), fmt.Sprint(env.function.Span()), use.id.Name)
+					len(env.function.FreeVars), env.function.Pos, use.id.Name)
 			}
 		}
 

resolve/resolve_test.go

@@ -59,7 +59,7 @@ func TestDefVarargsAndKwargsSet(t *testing.T) {
 	if err := resolve.File(file, isPredeclared, isUniversal); err != nil {
 		t.Fatal(err)
 	}
-	fn := file.Stmts[0].(*syntax.DefStmt)
+	fn := file.Stmts[0].(*syntax.DefStmt).Function.(*resolve.Function)
 	if !fn.HasVarargs {
 		t.Error("HasVarargs not set")
 	}
@@ -78,7 +78,7 @@ func TestLambdaVarargsAndKwargsSet(t *testing.T) {
 	if err := resolve.File(file, isPredeclared, isUniversal); err != nil {
 		t.Fatal(err)
 	}
-	lam := file.Stmts[0].(*syntax.AssignStmt).RHS.(*syntax.LambdaExpr)
+	lam := file.Stmts[0].(*syntax.AssignStmt).RHS.(*syntax.LambdaExpr).Function.(*resolve.Function)
 	if !lam.HasVarargs {
 		t.Error("HasVarargs not set")
 	}

starlark/eval.go

@@ -329,7 +329,8 @@ func FileProgram(f *syntax.File, isPredeclared func(string) bool) (*Program, err
 		pos = syntax.MakePosition(&f.Path, 1, 1)
 	}
 
-	compiled := compile.File(f.Stmts, pos, "<toplevel>", f.Locals, f.Globals)
+	module := f.Module.(*resolve.Module)
+	compiled := compile.File(f.Stmts, pos, "<toplevel>", module.Locals, module.Globals)
 
 	return &Program{compiled}, nil
 }

starlarkstruct/struct_test.go

@@ -9,8 +9,8 @@ import (
 	"path/filepath"
 	"testing"
 
-	"go.starlark.net/starlark"
 	"go.starlark.net/resolve"
+	"go.starlark.net/starlark"
 	"go.starlark.net/starlarkstruct"
 	"go.starlark.net/starlarktest"
 )
@@ -67,7 +67,7 @@ func (sym *symbol) Name() string          { return sym.name }
 func (sym *symbol) String() string        { return sym.name }
 func (sym *symbol) Type() string          { return "symbol" }
 func (sym *symbol) Freeze()               {} // immutable
-func (sym *symbol) Truth() starlark.Bool   { return starlark.True }
+func (sym *symbol) Truth() starlark.Bool  { return starlark.True }
 func (sym *symbol) Hash() (uint32, error) { return 0, fmt.Errorf("unhashable: %s", sym.Type()) }
 
 func (sym *symbol) CallInternal(thread *starlark.Thread, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) {

syntax/parse.go

@@ -164,13 +164,10 @@ func (p *parser) parseDefStmt() Stmt {
 	p.consume(COLON)
 	body := p.parseSuite()
 	return &DefStmt{
-		Def:  defpos,
-		Name: id,
-		Function: Function{
-			StartPos: defpos,
-			Params:   params,
-			Body:     body,
-		},
+		Def:    defpos,
+		Name:   id,
+		Params: params,
+		Body:   body,
 	}
 }
 
@@ -569,11 +566,8 @@ func (p *parser) parseLambda(allowCond bool) Expr {
 
 	return &LambdaExpr{
 		Lambda: lambda,
-		Function: Function{
-			StartPos: lambda,
-			Params:   params,
-			Body:     []Stmt{&ReturnStmt{Result: body}},
-		},
+		Params: params,
+		Body:   body,
 	}
 }