syntax: parameterize API over FileOptions, avoid globals (#477)

This change eliminates the need for client applications
to set global variables to control dialect options,
as globals are error-prone and concurrency hostile.

All relevant API functions Foo now have a variant FooOptions
that takes an explicit FileOptions; the original function
accesses the legacy options by reading the global variables.

Fixes #435

internal/compile/codegen_test.go

@@ -64,7 +64,7 @@ func TestPlusFolding(t *testing.T) {
 			t.Errorf("#%d: %v", i, err)
 			continue
 		}
-		got := disassemble(Expr(expr, "<expr>", locals).Toplevel)
+		got := disassemble(Expr(syntax.LegacyFileOptions(), expr, "<expr>", locals).Toplevel)
 		if test.want != got {
 			t.Errorf("expression <<%s>> generated <<%s>>, want <<%s>>",
 				test.src, got, test.want)

internal/compile/compile.go

@@ -23,7 +23,6 @@
 //
 // Operands, logically uint32s, are encoded using little-endian 7-bit
 // varints, the top bit indicating that more bytes follow.
-//
 package compile // import "go.starlark.net/internal/compile"
 
 import (
@@ -47,7 +46,7 @@ var Disassemble = false
 const debug = false // make code generation verbose, for debugging the compiler
 
 // Increment this to force recompilation of saved bytecode files.
-const Version = 13
+const Version = 14
 
 type Opcode uint8
 
@@ -317,6 +316,7 @@ type Program struct {
 	Functions []*Funcode
 	Globals   []Binding // for error messages and tracing
 	Toplevel  *Funcode  // module initialization function
+	Recursion bool      // disable recursion check for functions in this file
 }
 
 // The type of a bytes literal value, to distinguish from text string.
@@ -486,17 +486,20 @@ func bindings(bindings []*resolve.Binding) []Binding {
 }
 
 // Expr compiles an expression to a program whose toplevel function evaluates it.
-func Expr(expr syntax.Expr, name string, locals []*resolve.Binding) *Program {
+// The options must be consistent with those used when parsing expr.
+func Expr(opts *syntax.FileOptions, expr syntax.Expr, name string, locals []*resolve.Binding) *Program {
 	pos := syntax.Start(expr)
 	stmts := []syntax.Stmt{&syntax.ReturnStmt{Result: expr}}
-	return File(stmts, pos, name, locals, nil)
+	return File(opts, stmts, pos, name, locals, nil)
 }
 
 // File compiles the statements of a file into a program.
-func File(stmts []syntax.Stmt, pos syntax.Position, name string, locals, globals []*resolve.Binding) *Program {
+// The options must be consistent with those used when parsing stmts.
+func File(opts *syntax.FileOptions, stmts []syntax.Stmt, pos syntax.Position, name string, locals, globals []*resolve.Binding) *Program {
 	pcomp := &pcomp{
 		prog: &Program{
-			Globals: bindings(globals),
+			Globals:   bindings(globals),
+			Recursion: opts.Recursion,
 		},
 		names:     make(map[string]uint32),
 		constants: make(map[interface{}]uint32),

internal/compile/serial.go

@@ -25,6 +25,7 @@ package compile
 //	toplevel	Funcode
 //	numfuncs	varint
 //	funcs		[]Funcode
+//	recursion	varint (0 or 1)
 //	<strings>	[]byte		# concatenation of all referenced strings
 //	EOF
 //
@@ -130,6 +131,7 @@ func (prog *Program) Encode() []byte {
 	for _, fn := range prog.Functions {
 		e.function(fn)
 	}
+	e.int(b2i(prog.Recursion))
 
 	// Patch in the offset of the string data section.
 	binary.LittleEndian.PutUint32(e.p[4:8], uint32(len(e.p)))
@@ -270,6 +272,7 @@ func DecodeProgram(data []byte) (_ *Program, err error) {
 	for i := range funcs {
 		funcs[i] = d.function()
 	}
+	recursion := d.int() != 0
 
 	prog := &Program{
 		Loads:     loads,
@@ -278,6 +281,7 @@ func DecodeProgram(data []byte) (_ *Program, err error) {
 		Globals:   globals,
 		Functions: funcs,
 		Toplevel:  toplevel,
+		Recursion: recursion,
 	}
 	toplevel.Prog = prog
 	for _, f := range funcs {

lib/proto/cmd/star2proto/star2proto.go

@@ -40,8 +40,10 @@ var (
 
 // Starlark dialect flags
 func init() {
-	flag.BoolVar(&resolve.AllowFloat, "fp", true, "allow floating-point numbers")
 	flag.BoolVar(&resolve.AllowSet, "set", resolve.AllowSet, "allow set data type")
+
+	// obsolete, no effect:
+	flag.BoolVar(&resolve.AllowFloat, "fp", true, "allow floating-point numbers")
 	flag.BoolVar(&resolve.AllowLambda, "lambda", resolve.AllowLambda, "allow lambda expressions")
 	flag.BoolVar(&resolve.AllowNestedDef, "nesteddef", resolve.AllowNestedDef, "allow nested def statements")
 }

repl/repl.go

@@ -20,21 +20,25 @@ import (
 	"os/signal"
 
 	"github.com/chzyer/readline"
-	"go.starlark.net/resolve"
 	"go.starlark.net/starlark"
 	"go.starlark.net/syntax"
 )
 
 var interrupted = make(chan os.Signal, 1)
 
-// REPL executes a read, eval, print loop.
+// REPL calls [REPLOptions] using [syntax.LegacyFileOptions].
+// Deprecated: relies on legacy global variables.
+func REPL(thread *starlark.Thread, globals starlark.StringDict) {
+	REPLOptions(syntax.LegacyFileOptions(), thread, globals)
+}
+
+// REPLOptions executes a read, eval, print loop.
 //
 // Before evaluating each expression, it sets the Starlark thread local
 // variable named "context" to a context.Context that is cancelled by a
 // SIGINT (Control-C). Client-supplied global functions may use this
 // context to make long-running operations interruptable.
-//
-func REPL(thread *starlark.Thread, globals starlark.StringDict) {
+func REPLOptions(opts *syntax.FileOptions, thread *starlark.Thread, globals starlark.StringDict) {
 	signal.Notify(interrupted, os.Interrupt)
 	defer signal.Stop(interrupted)
 
@@ -45,7 +49,7 @@ func REPL(thread *starlark.Thread, globals starlark.StringDict) {
 	}
 	defer rl.Close()
 	for {
-		if err := rep(rl, thread, globals); err != nil {
+		if err := rep(opts, rl, thread, globals); err != nil {
 			if err == readline.ErrInterrupt {
 				fmt.Println(err)
 				continue
@@ -59,7 +63,7 @@ func REPL(thread *starlark.Thread, globals starlark.StringDict) {
 //
 // It returns an error (possibly readline.ErrInterrupt)
 // only if readline failed. Starlark errors are printed.
-func rep(rl *readline.Instance, thread *starlark.Thread, globals starlark.StringDict) error {
+func rep(opts *syntax.FileOptions, rl *readline.Instance, thread *starlark.Thread, globals starlark.StringDict) error {
 	// Each item gets its own context,
 	// which is cancelled by a SIGINT.
 	//
@@ -93,8 +97,14 @@ func rep(rl *readline.Instance, thread *starlark.Thread, globals starlark.String
 		return []byte(line + "\n"), nil
 	}
 
+	// Treat load bindings as global (like they used to be) in the REPL.
+	// Fixes github.com/google/starlark-go/issues/224.
+	opts2 := *opts
+	opts2.LoadBindsGlobally = true
+	opts = &opts2
+
 	// parse
-	f, err := syntax.ParseCompoundStmt("<stdin>", readline)
+	f, err := opts.ParseCompoundStmt("<stdin>", readline)
 	if err != nil {
 		if eof {
 			return io.EOF
@@ -103,16 +113,9 @@ func rep(rl *readline.Instance, thread *starlark.Thread, globals starlark.String
 		return nil
 	}
 
-	// Treat load bindings as global (like they used to be) in the REPL.
-	// This is a workaround for github.com/google/starlark-go/issues/224.
-	// TODO(adonovan): not safe wrt concurrent interpreters.
-	// Come up with a more principled solution (or plumb options everywhere).
-	defer func(prev bool) { resolve.LoadBindsGlobally = prev }(resolve.LoadBindsGlobally)
-	resolve.LoadBindsGlobally = true
-
 	if expr := soleExpr(f); expr != nil {
 		// eval
-		v, err := starlark.EvalExpr(thread, expr, globals)
+		v, err := starlark.EvalExprOptions(f.Options, thread, expr, globals)
 		if err != nil {
 			PrintError(err)
 			return nil
@@ -149,10 +152,16 @@ func PrintError(err error) {
 	}
 }
 
-// MakeLoad returns a simple sequential implementation of module loading
-// suitable for use in the REPL.
-// Each function returned by MakeLoad accesses a distinct private cache.
+// MakeLoad calls [MakeLoadOptions] using [syntax.LegacyFileOptions].
+// Deprecated: relies on legacy global variables.
 func MakeLoad() func(thread *starlark.Thread, module string) (starlark.StringDict, error) {
+	return MakeLoadOptions(syntax.LegacyFileOptions())
+}
+
+// MakeLoadOptions returns a simple sequential implementation of module loading
+// suitable for use in the REPL.
+// Each function returned by MakeLoadOptions accesses a distinct private cache.
+func MakeLoadOptions(opts *syntax.FileOptions) func(thread *starlark.Thread, module string) (starlark.StringDict, error) {
 	type entry struct {
 		globals starlark.StringDict
 		err     error
@@ -173,7 +182,7 @@ func MakeLoad() func(thread *starlark.Thread, module string) (starlark.StringDic
 
 			// Load it.
 			thread := &starlark.Thread{Name: "exec " + module, Load: thread.Load}
-			globals, err := starlark.ExecFile(thread, module, nil, nil)
+			globals, err := starlark.ExecFileOptions(opts, thread, module, nil, nil)
 			e = &entry{globals, err}
 
 			// Update the cache.

resolve/resolve.go

@@ -97,6 +97,9 @@ const doesnt = "this Starlark dialect does not "
 // global options
 // These features are either not standard Starlark (yet), or deprecated
 // features of the BUILD language, so we put them behind flags.
+//
+// Deprecated: use an explicit [syntax.FileOptions] argument instead,
+// as it avoids all the usual problems of global variables.
 var (
 	AllowSet            = false // allow the 'set' built-in
 	AllowGlobalReassign = false // allow reassignment to top-level names; also, allow if/for/while at top-level
@@ -130,7 +133,7 @@ func File(file *syntax.File, isPredeclared, isUniversal func(name string) bool)
 // REPLChunk is a generalization of the File function that supports a
 // non-empty initial global block, as occurs in a REPL.
 func REPLChunk(file *syntax.File, isGlobal, isPredeclared, isUniversal func(name string) bool) error {
-	r := newResolver(isGlobal, isPredeclared, isUniversal)
+	r := newResolver(file.Options, isGlobal, isPredeclared, isUniversal)
 	r.stmts(file.Stmts)
 
 	r.env.resolveLocalUses()
@@ -151,12 +154,18 @@ func REPLChunk(file *syntax.File, isGlobal, isPredeclared, isUniversal func(name
 	return nil
 }
 
-// Expr resolves the specified expression.
+// Expr calls [ExprOptions] using [syntax.LegacyFileOptions].
+// Deprecated: relies on legacy global variables.
+func Expr(expr syntax.Expr, isPredeclared, isUniversal func(name string) bool) ([]*Binding, error) {
+	return ExprOptions(syntax.LegacyFileOptions(), expr, isPredeclared, isUniversal)
+}
+
+// ExprOptions resolves the specified expression.
 // It returns the local variables bound within the expression.
 //
-// The isPredeclared and isUniversal predicates behave as for the File function.
-func Expr(expr syntax.Expr, isPredeclared, isUniversal func(name string) bool) ([]*Binding, error) {
-	r := newResolver(nil, isPredeclared, isUniversal)
+// The isPredeclared and isUniversal predicates behave as for the File function
+func ExprOptions(opts *syntax.FileOptions, expr syntax.Expr, isPredeclared, isUniversal func(name string) bool) ([]*Binding, error) {
+	r := newResolver(opts, nil, isPredeclared, isUniversal)
 	r.expr(expr)
 	r.env.resolveLocalUses()
 	r.resolveNonLocalUses(r.env) // globals & universals
@@ -179,9 +188,10 @@ type Error struct {
 
 func (e Error) Error() string { return e.Pos.String() + ": " + e.Msg }
 
-func newResolver(isGlobal, isPredeclared, isUniversal func(name string) bool) *resolver {
+func newResolver(options *syntax.FileOptions, isGlobal, isPredeclared, isUniversal func(name string) bool) *resolver {
 	file := new(block)
 	return &resolver{
+		options:       options,
 		file:          file,
 		env:           file,
 		isGlobal:      isGlobal,
@@ -193,6 +203,8 @@ func newResolver(isGlobal, isPredeclared, isUniversal func(name string) bool) *r
 }
 
 type resolver struct {
+	options *syntax.FileOptions
+
 	// env is the current local environment:
 	// a linked list of blocks, innermost first.
 	// The tail of the list is the file block.
@@ -314,7 +326,7 @@ func (r *resolver) bind(id *syntax.Ident) bool {
 				r.moduleGlobals = append(r.moduleGlobals, bind)
 			}
 		}
-		if ok && !AllowGlobalReassign {
+		if ok && !r.options.GlobalReassign {
 			r.errorf(id.NamePos, "cannot reassign %s %s declared at %s",
 				bind.Scope, id.Name, bind.First.NamePos)
 		}
@@ -382,7 +394,7 @@ func (r *resolver) use(id *syntax.Ident) {
 	// We will piggyback support for the legacy semantics on the
 	// AllowGlobalReassign flag, which is loosely related and also
 	// required for Bazel.
-	if AllowGlobalReassign && r.env == r.file {
+	if r.options.GlobalReassign && r.env == r.file {
 		r.useToplevel(use)
 		return
 	}
@@ -420,7 +432,7 @@ func (r *resolver) useToplevel(use use) (bind *Binding) {
 		r.predeclared[id.Name] = bind // save it
 	} else if r.isUniversal(id.Name) {
 		// use of universal name
-		if !AllowSet && id.Name == "set" {
+		if !r.options.Set && id.Name == "set" {
 			r.errorf(id.NamePos, doesnt+"support sets")
 		}
 		bind = &Binding{Scope: Universal}
@@ -493,7 +505,7 @@ func (r *resolver) stmt(stmt syntax.Stmt) {
 		}
 
 	case *syntax.IfStmt:
-		if !AllowGlobalReassign && r.container().function == nil {
+		if !r.options.GlobalReassign && r.container().function == nil {
 			r.errorf(stmt.If, "if statement not within a function")
 		}
 		r.expr(stmt.Cond)
@@ -519,7 +531,7 @@ func (r *resolver) stmt(stmt syntax.Stmt) {
 		r.function(fn, stmt.Def)
 
 	case *syntax.ForStmt:
-		if !AllowGlobalReassign && r.container().function == nil {
+		if !r.options.GlobalReassign && r.container().function == nil {
 			r.errorf(stmt.For, "for loop not within a function")
 		}
 		r.expr(stmt.X)
@@ -530,10 +542,10 @@ func (r *resolver) stmt(stmt syntax.Stmt) {
 		r.loops--
 
 	case *syntax.WhileStmt:
-		if !AllowRecursion {
+		if !r.options.While {
 			r.errorf(stmt.While, doesnt+"support while loops")
 		}
-		if !AllowGlobalReassign && r.container().function == nil {
+		if !r.options.GlobalReassign && r.container().function == nil {
 			r.errorf(stmt.While, "while loop not within a function")
 		}
 		r.expr(stmt.Cond)
@@ -569,9 +581,9 @@ func (r *resolver) stmt(stmt syntax.Stmt) {
 			}
 
 			id := stmt.To[i]
-			if LoadBindsGlobally {
+			if r.options.LoadBindsGlobally {
 				r.bind(id)
-			} else if r.bindLocal(id) && !AllowGlobalReassign {
+			} else if r.bindLocal(id) && !r.options.GlobalReassign {
 				// "Global" in AllowGlobalReassign is a misnomer for "toplevel".
 				// Sadly we can't report the previous declaration
 				// as id.Binding may not be set yet.

resolve/resolve_test.go

@@ -14,30 +14,38 @@ import (
 	"go.starlark.net/syntax"
 )
 
-func setOptions(src string) {
-	resolve.AllowGlobalReassign = option(src, "globalreassign")
-	resolve.AllowRecursion = option(src, "recursion")
-	resolve.AllowSet = option(src, "set")
-	resolve.LoadBindsGlobally = option(src, "loadbindsglobally")
+// A test may enable non-standard options by containing (e.g.) "option:recursion".
+func getOptions(src string) *syntax.FileOptions {
+	// TODO(adonovan): use new fine-grained names.
+	// And share with eval_test.go
+	allowGlobalReassign := option(src, "globalreassign")
+	recursion := option(src, "recursion")
+	return &syntax.FileOptions{
+		Set:               option(src, "set"),
+		While:             allowGlobalReassign || recursion,
+		TopLevelControl:   allowGlobalReassign,
+		GlobalReassign:    allowGlobalReassign,
+		LoadBindsGlobally: option(src, "loadbindsglobally"),
+		Recursion:         recursion,
+	}
 }
 
 func option(chunk, name string) bool {
 	return strings.Contains(chunk, "option:"+name)
 }
 
 func TestResolve(t *testing.T) {
-	defer setOptions("")
 	filename := starlarktest.DataFile("resolve", "testdata/resolve.star")
 	for _, chunk := range chunkedfile.Read(filename, t) {
-		f, err := syntax.Parse(filename, chunk.Source, 0)
+		// A chunk may set options by containing e.g. "option:recursion".
+		opts := getOptions(chunk.Source)
+
+		f, err := opts.Parse(filename, chunk.Source, 0)
 		if err != nil {
 			t.Error(err)
 			continue
 		}
 
-		// A chunk may set options by containing e.g. "option:recursion".
-		setOptions(chunk.Source)
-
 		if err := resolve.File(f, isPredeclared, isUniversal); err != nil {
 			for _, err := range err.(resolve.ErrorList) {
 				chunk.GotError(int(err.Pos.Line), err.Msg)