From f669cc580a78d228fc2417f715c7476f152c9103 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 15 Jun 2023 11:48:59 -0400 Subject: [PATCH] syntax: parameterize API over FileOptions, avoid globals 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 | 2 +- internal/compile/compile.go | 15 ++-- internal/compile/serial.go | 4 + lib/proto/cmd/star2proto/star2proto.go | 4 +- repl/repl.go | 47 +++++----- resolve/resolve.go | 42 +++++---- resolve/resolve_test.go | 28 +++--- starlark/bench_test.go | 12 ++- starlark/eval.go | 81 ++++++++++++------ starlark/eval_test.go | 25 +++--- starlark/interp.go | 7 +- syntax/options.go | 63 ++++++++++++++ syntax/parse.go | 113 +++++++++++++++---------- syntax/syntax.go | 6 +- 14 files changed, 306 insertions(+), 143 deletions(-) create mode 100644 syntax/options.go diff --git a/internal/compile/codegen_test.go b/internal/compile/codegen_test.go index f67204ff..c5954c45 100644 --- a/internal/compile/codegen_test.go +++ b/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, "", locals).Toplevel) + got := disassemble(Expr(syntax.LegacyFileOptions(), expr, "", locals).Toplevel) if test.want != got { t.Errorf("expression <<%s>> generated <<%s>>, want <<%s>>", test.src, got, test.want) diff --git a/internal/compile/compile.go b/internal/compile/compile.go index 888d95c5..ecf689f0 100644 --- a/internal/compile/compile.go +++ b/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), diff --git a/internal/compile/serial.go b/internal/compile/serial.go index adadabfc..4d71738c 100644 --- a/internal/compile/serial.go +++ b/internal/compile/serial.go @@ -25,6 +25,7 @@ package compile // toplevel Funcode // numfuncs varint // funcs []Funcode +// recursion varint (0 or 1) // []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 { diff --git a/lib/proto/cmd/star2proto/star2proto.go b/lib/proto/cmd/star2proto/star2proto.go index 24b5a0e4..791f2072 100644 --- a/lib/proto/cmd/star2proto/star2proto.go +++ b/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") } diff --git a/repl/repl.go b/repl/repl.go index 94f8947e..6bb7f560 100644 --- a/repl/repl.go +++ b/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("", readline) + f, err := opts.ParseCompoundStmt("", 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. diff --git a/resolve/resolve.go b/resolve/resolve.go index 09b9acde..8c780d8c 100644 --- a/resolve/resolve.go +++ b/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. diff --git a/resolve/resolve_test.go b/resolve/resolve_test.go index 23bee215..95a9445a 100644 --- a/resolve/resolve_test.go +++ b/resolve/resolve_test.go @@ -14,11 +14,20 @@ 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 { @@ -26,18 +35,17 @@ func option(chunk, name string) bool { } 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) diff --git a/starlark/bench_test.go b/starlark/bench_test.go index e860df7a..46be8900 100644 --- a/starlark/bench_test.go +++ b/starlark/bench_test.go @@ -18,8 +18,6 @@ import ( ) func BenchmarkStarlark(b *testing.B) { - defer setOptions("") - starlark.Universe["json"] = json.Module testdata := starlarktest.DataFile("starlark", ".") @@ -36,10 +34,10 @@ func BenchmarkStarlark(b *testing.B) { b.Error(err) continue } - setOptions(string(src)) + opts := getOptions(string(src)) // Evaluate the file once. - globals, err := starlark.ExecFile(thread, filename, src, nil) + globals, err := starlark.ExecFileOptions(opts, thread, filename, src, nil) if err != nil { reportEvalError(b, err) } @@ -63,9 +61,9 @@ func BenchmarkStarlark(b *testing.B) { // It provides b.n, the number of iterations that must be executed by the function, // which is typically of the form: // -// def bench_foo(b): -// for _ in range(b.n): -// ...work... +// def bench_foo(b): +// for _ in range(b.n): +// ...work... // // It also provides stop, start, and restart methods to stop the clock in case // there is significant set-up work that should not count against the measured diff --git a/starlark/eval.go b/starlark/eval.go index 949cb934..706c6249 100644 --- a/starlark/eval.go +++ b/starlark/eval.go @@ -325,7 +325,13 @@ func (prog *Program) Write(out io.Writer) error { return err } -// ExecFile parses, resolves, and executes a Starlark file in the +// ExecFile calls [ExecFileOptions] using [syntax.LegacyFileOptions]. +// Deprecated: relies on legacy global variables. +func ExecFile(thread *Thread, filename string, src interface{}, predeclared StringDict) (StringDict, error) { + return ExecFileOptions(syntax.LegacyFileOptions(), thread, filename, src, predeclared) +} + +// ExecFileOptions parses, resolves, and executes a Starlark file in the // specified global environment, which may be modified during execution. // // Thread is the state associated with the Starlark thread. @@ -340,11 +346,11 @@ func (prog *Program) Write(out io.Writer) error { // Execution does not modify this dictionary, though it may mutate // its values. // -// If ExecFile fails during evaluation, it returns an *EvalError +// If ExecFileOptions fails during evaluation, it returns an *EvalError // containing a backtrace. -func ExecFile(thread *Thread, filename string, src interface{}, predeclared StringDict) (StringDict, error) { +func ExecFileOptions(opts *syntax.FileOptions, thread *Thread, filename string, src interface{}, predeclared StringDict) (StringDict, error) { // Parse, resolve, and compile a Starlark source file. - _, mod, err := SourceProgram(filename, src, predeclared.Has) + _, mod, err := SourceProgramOptions(opts, filename, src, predeclared.Has) if err != nil { return nil, err } @@ -354,7 +360,13 @@ func ExecFile(thread *Thread, filename string, src interface{}, predeclared Stri return g, err } -// SourceProgram produces a new program by parsing, resolving, +// SourceProgram calls [SourceProgramOptions] using [syntax.LegacyFileOptions]. +// Deprecated: relies on legacy global variables. +func SourceProgram(filename string, src interface{}, isPredeclared func(string) bool) (*syntax.File, *Program, error) { + return SourceProgramOptions(syntax.LegacyFileOptions(), filename, src, isPredeclared) +} + +// SourceProgramOptions produces a new program by parsing, resolving, // and compiling a Starlark source file. // On success, it returns the parsed file and the compiled program. // The filename and src parameters are as for syntax.Parse. @@ -363,8 +375,8 @@ func ExecFile(thread *Thread, filename string, src interface{}, predeclared Stri // a pre-declared identifier of the current module. // Its typical value is predeclared.Has, // where predeclared is a StringDict of pre-declared values. -func SourceProgram(filename string, src interface{}, isPredeclared func(string) bool) (*syntax.File, *Program, error) { - f, err := syntax.Parse(filename, src, 0) +func SourceProgramOptions(opts *syntax.FileOptions, filename string, src interface{}, isPredeclared func(string) bool) (*syntax.File, *Program, error) { + f, err := opts.Parse(filename, src, 0) if err != nil { return nil, nil, err } @@ -396,7 +408,7 @@ func FileProgram(f *syntax.File, isPredeclared func(string) bool) (*Program, err } module := f.Module.(*resolve.Module) - compiled := compile.File(f.Stmts, pos, "", module.Locals, module.Globals) + compiled := compile.File(f.Options, f.Stmts, pos, "", module.Locals, module.Globals) return &Program{compiled}, nil } @@ -453,7 +465,7 @@ func ExecREPLChunk(f *syntax.File, thread *Thread, globals StringDict) error { } module := f.Module.(*resolve.Module) - compiled := compile.File(f.Stmts, pos, "", module.Locals, module.Globals) + compiled := compile.File(f.Options, f.Stmts, pos, "", module.Locals, module.Globals) prog := &Program{compiled} // -- variant of Program.Init -- @@ -512,7 +524,13 @@ func makeToplevelFunction(prog *compile.Program, predeclared StringDict) *Functi } } -// Eval parses, resolves, and evaluates an expression within the +// Eval calls [EvalOptions] using [syntax.LegacyFileOptions]. +// Deprecated: relies on legacy global variables. +func Eval(thread *Thread, filename string, src interface{}, env StringDict) (Value, error) { + return EvalOptions(syntax.LegacyFileOptions(), thread, filename, src, env) +} + +// EvalOptions parses, resolves, and evaluates an expression within the // specified (predeclared) environment. // // Evaluation cannot mutate the environment dictionary itself, @@ -520,58 +538,71 @@ func makeToplevelFunction(prog *compile.Program, predeclared StringDict) *Functi // // The filename and src parameters are as for syntax.Parse. // -// If Eval fails during evaluation, it returns an *EvalError +// If EvalOptions fails during evaluation, it returns an *EvalError // containing a backtrace. -func Eval(thread *Thread, filename string, src interface{}, env StringDict) (Value, error) { - expr, err := syntax.ParseExpr(filename, src, 0) +func EvalOptions(opts *syntax.FileOptions, thread *Thread, filename string, src interface{}, env StringDict) (Value, error) { + expr, err := opts.ParseExpr(filename, src, 0) if err != nil { return nil, err } - f, err := makeExprFunc(expr, env) + f, err := makeExprFunc(opts, expr, env) if err != nil { return nil, err } return Call(thread, f, nil, nil) } -// EvalExpr resolves and evaluates an expression within the +// EvalExpr calls [EvalExprOptions] using [syntax.LegacyFileOptions]. +// Deprecated: relies on legacy global variables. +func EvalExpr(thread *Thread, expr syntax.Expr, env StringDict) (Value, error) { + return EvalExprOptions(syntax.LegacyFileOptions(), thread, expr, env) +} + +// EvalExprOptions resolves and evaluates an expression within the // specified (predeclared) environment. // Evaluating a comma-separated list of expressions yields a tuple value. // // Resolving an expression mutates it. -// Do not call EvalExpr more than once for the same expression. +// Do not call EvalExprOptions more than once for the same expression. // // Evaluation cannot mutate the environment dictionary itself, // though it may modify variables reachable from the dictionary. // -// If Eval fails during evaluation, it returns an *EvalError +// If EvalExprOptions fails during evaluation, it returns an *EvalError // containing a backtrace. -func EvalExpr(thread *Thread, expr syntax.Expr, env StringDict) (Value, error) { - fn, err := makeExprFunc(expr, env) +func EvalExprOptions(opts *syntax.FileOptions, thread *Thread, expr syntax.Expr, env StringDict) (Value, error) { + fn, err := makeExprFunc(opts, expr, env) if err != nil { return nil, err } return Call(thread, fn, nil, nil) } +// ExprFunc calls [ExprFuncOptions] using [syntax.LegacyFileOptions]. +// Deprecated: relies on legacy global variables. +func ExprFunc(filename string, src interface{}, env StringDict) (*Function, error) { + return ExprFuncOptions(syntax.LegacyFileOptions(), filename, src, env) +} + // ExprFunc returns a no-argument function // that evaluates the expression whose source is src. -func ExprFunc(filename string, src interface{}, env StringDict) (*Function, error) { - expr, err := syntax.ParseExpr(filename, src, 0) +func ExprFuncOptions(options *syntax.FileOptions, filename string, src interface{}, env StringDict) (*Function, error) { + expr, err := options.ParseExpr(filename, src, 0) if err != nil { return nil, err } - return makeExprFunc(expr, env) + return makeExprFunc(options, expr, env) } // makeExprFunc returns a no-argument function whose body is expr. -func makeExprFunc(expr syntax.Expr, env StringDict) (*Function, error) { - locals, err := resolve.Expr(expr, env.Has, Universe.Has) +// The options must be consistent with those used when parsing expr. +func makeExprFunc(opts *syntax.FileOptions, expr syntax.Expr, env StringDict) (*Function, error) { + locals, err := resolve.ExprOptions(opts, expr, env.Has, Universe.Has) if err != nil { return nil, err } - return makeToplevelFunction(compile.Expr(expr, "", locals), env), nil + return makeToplevelFunction(compile.Expr(opts, expr, "", locals), env), nil } // The following functions are primitive operations of the byte code interpreter. diff --git a/starlark/eval_test.go b/starlark/eval_test.go index 9ffd1794..4b3c5768 100644 --- a/starlark/eval_test.go +++ b/starlark/eval_test.go @@ -20,7 +20,6 @@ import ( starlarkmath "go.starlark.net/lib/math" "go.starlark.net/lib/proto" "go.starlark.net/lib/time" - "go.starlark.net/resolve" "go.starlark.net/starlark" "go.starlark.net/starlarkstruct" "go.starlark.net/starlarktest" @@ -31,11 +30,19 @@ import ( ) // A test may enable non-standard options by containing (e.g.) "option:recursion". -func setOptions(src string) { - resolve.AllowGlobalReassign = option(src, "globalreassign") - resolve.LoadBindsGlobally = option(src, "loadbindsglobally") - resolve.AllowRecursion = option(src, "recursion") - resolve.AllowSet = option(src, "set") +func getOptions(src string) *syntax.FileOptions { + // TODO(adonovan): use new fine-grained names. + // And share with resolve_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 { @@ -113,7 +120,6 @@ func TestEvalExpr(t *testing.T) { } func TestExecFile(t *testing.T) { - defer setOptions("") testdata := starlarktest.DataFile("starlark", ".") thread := &starlark.Thread{Load: load} starlarktest.SetReporter(thread, t) @@ -148,9 +154,8 @@ func TestExecFile(t *testing.T) { "struct": starlark.NewBuiltin("struct", starlarkstruct.Make), } - setOptions(chunk.Source) - - _, err := starlark.ExecFile(thread, filename, chunk.Source, predeclared) + opts := getOptions(chunk.Source) + _, err := starlark.ExecFileOptions(opts, thread, filename, chunk.Source, predeclared) switch err := err.(type) { case *starlark.EvalError: found := false diff --git a/starlark/interp.go b/starlark/interp.go index b41905a0..d29e5253 100644 --- a/starlark/interp.go +++ b/starlark/interp.go @@ -10,7 +10,6 @@ import ( "go.starlark.net/internal/compile" "go.starlark.net/internal/spell" - "go.starlark.net/resolve" "go.starlark.net/syntax" ) @@ -24,19 +23,19 @@ func (fn *Function) CallInternal(thread *Thread, args Tuple, kwargs []Tuple) (Va // Postcondition: args is not mutated. This is stricter than required by Callable, // but allows CALL to avoid a copy. - if !resolve.AllowRecursion { + f := fn.funcode + if !f.Prog.Recursion { // detect recursion for _, fr := range thread.stack[:len(thread.stack)-1] { // We look for the same function code, // not function value, otherwise the user could // defeat the check by writing the Y combinator. - if frfn, ok := fr.Callable().(*Function); ok && frfn.funcode == fn.funcode { + if frfn, ok := fr.Callable().(*Function); ok && frfn.funcode == f { return nil, fmt.Errorf("function %s called recursively", fn.Name()) } } } - f := fn.funcode fr := thread.frameAt(0) // Allocate space for stack and locals. diff --git a/syntax/options.go b/syntax/options.go new file mode 100644 index 00000000..51b26382 --- /dev/null +++ b/syntax/options.go @@ -0,0 +1,63 @@ +// Copyright 2023 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 syntax + +import _ "unsafe" // for linkname + +// FileOptions specifies various per-file options that affect static +// aspects of an individual file such as parsing, name resolution, and +// code generation. (Options that affect global dynamics are typically +// controlled through [starlark.Thread].) +// +// The zero value of FileOptions is the default behavior. +// +// Many functions in this package come in two versions: the legacy +// standalone function (such as [Parse]) uses [LegacyFileOptions], +// whereas the more recent method (such as [Options.Parse]) honors the +// provided options. The second form is preferred. In other packages, +// the modern version is a standalone function with a leading +// FileOptions parameter and the name suffix "Options", such as +// [starlark.ExecFileOptions]. +type FileOptions struct { + // resolver + Set bool // allow references to the 'set' built-in function + While bool // allow 'while' statements + TopLevelControl bool // allow if/for/while statements at top-level + GlobalReassign bool // allow reassignment to top-level names + LoadBindsGlobally bool // load creates global not file-local bindings (deprecated) + + // compiler + Recursion bool // disable recursion check for functions in this file +} + +// TODO(adonovan): provide a canonical flag parser for FileOptions. +// (And use it in the testdata "options:" strings.) + +// LegacyFileOptions returns a new FileOptions containing the current +// values of the resolver package's legacy global variables such as +// [resolve.AllowRecursion], etc. +// These variables may be associated with command-line flags. +func LegacyFileOptions() *FileOptions { + return &FileOptions{ + Set: resolverAllowSet, + While: resolverAllowGlobalReassign, + TopLevelControl: resolverAllowGlobalReassign, + GlobalReassign: resolverAllowGlobalReassign, + Recursion: resolverAllowRecursion, + LoadBindsGlobally: resolverLoadBindsGlobally, + } +} + +// Access resolver (legacy) flags, if they are linked in; false otherwise. +var ( + //go:linkname resolverAllowSet go.starlark.net/resolve.AllowSet + resolverAllowSet bool + //go:linkname resolverAllowGlobalReassign go.starlark.net/resolve.AllowGlobalReassign + resolverAllowGlobalReassign bool + //go:linkname resolverAllowRecursion go.starlark.net/resolve.AllowRecursion + resolverAllowRecursion bool + //go:linkname resolverLoadBindsGlobally go.starlark.net/resolve.LoadBindsGlobally + resolverLoadBindsGlobally bool +) diff --git a/syntax/parse.go b/syntax/parse.go index f4c8fff4..260b91e3 100644 --- a/syntax/parse.go +++ b/syntax/parse.go @@ -23,19 +23,25 @@ const ( RetainComments Mode = 1 << iota // retain comments in AST; see Node.Comments ) +// Parse calls the Parse method of LegacyFileOptions(). +// Deprecated: relies on legacy global variables. +func Parse(filename string, src interface{}, mode Mode) (f *File, err error) { + return LegacyFileOptions().Parse(filename, src, mode) +} + // Parse parses the input data and returns the corresponding parse tree. // -// If src != nil, ParseFile parses the source from src and the filename +// If src != nil, Parse parses the source from src and the filename // is only used when recording position information. // The type of the argument for the src parameter must be string, // []byte, io.Reader, or FilePortion. -// If src == nil, ParseFile parses the file specified by filename. -func Parse(filename string, src interface{}, mode Mode) (f *File, err error) { +// If src == nil, Parse parses the file specified by filename. +func (opts *FileOptions) Parse(filename string, src interface{}, mode Mode) (f *File, err error) { in, err := newScanner(filename, src, mode&RetainComments != 0) if err != nil { return nil, err } - p := parser{in: in} + p := parser{options: opts, in: in} defer p.in.recover(&err) p.nextToken() // read first lookahead token @@ -47,6 +53,12 @@ func Parse(filename string, src interface{}, mode Mode) (f *File, err error) { return f, nil } +// ParseCompoundStmt calls the ParseCompoundStmt method of LegacyFileOptions(). +// Deprecated: relies on legacy global variables. +func ParseCompoundStmt(filename string, readline func() ([]byte, error)) (f *File, err error) { + return LegacyFileOptions().ParseCompoundStmt(filename, readline) +} + // ParseCompoundStmt parses a single compound statement: // a blank line, a def, for, while, or if statement, or a // semicolon-separated list of simple statements followed @@ -54,13 +66,13 @@ func Parse(filename string, src interface{}, mode Mode) (f *File, err error) { // ParseCompoundStmt does not consume any following input. // The parser calls the readline function each // time it needs a new line of input. -func ParseCompoundStmt(filename string, readline func() ([]byte, error)) (f *File, err error) { +func (opts *FileOptions) ParseCompoundStmt(filename string, readline func() ([]byte, error)) (f *File, err error) { in, err := newScanner(filename, readline, false) if err != nil { return nil, err } - p := parser{in: in} + p := parser{options: opts, in: in} defer p.in.recover(&err) p.nextToken() // read first lookahead token @@ -79,18 +91,24 @@ func ParseCompoundStmt(filename string, readline func() ([]byte, error)) (f *Fil } } - return &File{Path: filename, Stmts: stmts}, nil + return &File{Options: opts, Path: filename, Stmts: stmts}, nil +} + +// ParseExpr calls the ParseExpr method of LegacyFileOptions(). +// Deprecated: relies on legacy global variables. +func ParseExpr(filename string, src interface{}, mode Mode) (expr Expr, err error) { + return LegacyFileOptions().ParseExpr(filename, src, mode) } // ParseExpr parses a Starlark expression. // A comma-separated list of expressions is parsed as a tuple. // See Parse for explanation of parameters. -func ParseExpr(filename string, src interface{}, mode Mode) (expr Expr, err error) { +func (opts *FileOptions) ParseExpr(filename string, src interface{}, mode Mode) (expr Expr, err error) { in, err := newScanner(filename, src, mode&RetainComments != 0) if err != nil { return nil, err } - p := parser{in: in} + p := parser{options: opts, in: in} defer p.in.recover(&err) p.nextToken() // read first lookahead token @@ -112,9 +130,10 @@ func ParseExpr(filename string, src interface{}, mode Mode) (expr Expr, err erro } type parser struct { - in *scanner - tok Token - tokval tokenValue + options *FileOptions + in *scanner + tok Token + tokval tokenValue } // nextToken advances the scanner and returns the position of the @@ -139,7 +158,7 @@ func (p *parser) parseFile() *File { } stmts = p.parseStmt(stmts) } - return &File{Stmts: stmts} + return &File{Options: p.options, Stmts: stmts} } func (p *parser) parseStmt(stmts []Stmt) []Stmt { @@ -275,10 +294,11 @@ func (p *parser) parseSimpleStmt(stmts []Stmt, consumeNL bool) []Stmt { } // small_stmt = RETURN expr? -// | PASS | BREAK | CONTINUE -// | LOAD ... -// | expr ('=' | '+=' | '-=' | '*=' | '/=' | '%=' | '&=' | '|=' | '^=' | '<<=' | '>>=') expr // assign -// | expr +// +// | PASS | BREAK | CONTINUE +// | LOAD ... +// | expr ('=' | '+=' | '-=' | '*=' | '/=' | '%=' | '&=' | '|=' | '^=' | '<<=' | '>>=') expr // assign +// | expr func (p *parser) parseSmallStmt() Stmt { switch p.tok { case RETURN: @@ -415,21 +435,23 @@ func (p *parser) consume(t Token) Position { } // params = (param COMMA)* param COMMA? -// | +// +// | // // param = IDENT -// | IDENT EQ test -// | STAR -// | STAR IDENT -// | STARSTAR IDENT +// +// | IDENT EQ test +// | STAR +// | STAR IDENT +// | STARSTAR IDENT // // parseParams parses a parameter list. The resulting expressions are of the form: // -// *Ident x -// *Binary{Op: EQ, X: *Ident, Y: Expr} x=y -// *Unary{Op: STAR} * -// *Unary{Op: STAR, X: *Ident} *args -// *Unary{Op: STARSTAR, X: *Ident} **kwargs +// *Ident x +// *Binary{Op: EQ, X: *Ident, Y: Expr} x=y +// *Unary{Op: STAR} * +// *Unary{Op: STAR, X: *Ident} *args +// *Unary{Op: STARSTAR, X: *Ident} **kwargs func (p *parser) parseParams() []Expr { var params []Expr for p.tok != RPAREN && p.tok != COLON && p.tok != EOF { @@ -651,9 +673,10 @@ func init() { } // primary_with_suffix = primary -// | primary '.' IDENT -// | primary slice_suffix -// | primary call_suffix +// +// | primary '.' IDENT +// | primary slice_suffix +// | primary call_suffix func (p *parser) parsePrimaryWithSuffix() Expr { x := p.parsePrimary() for { @@ -770,12 +793,13 @@ func (p *parser) parseArgs() []Expr { return args } -// primary = IDENT -// | INT | FLOAT | STRING | BYTES -// | '[' ... // list literal or comprehension -// | '{' ... // dict literal or comprehension -// | '(' ... // tuple or parenthesized expression -// | ('-'|'+'|'~') primary_with_suffix +// primary = IDENT +// +// | INT | FLOAT | STRING | BYTES +// | '[' ... // list literal or comprehension +// | '{' ... // dict literal or comprehension +// | '(' ... // tuple or parenthesized expression +// | ('-'|'+'|'~') primary_with_suffix func (p *parser) parsePrimary() Expr { switch p.tok { case IDENT: @@ -836,9 +860,10 @@ func (p *parser) parsePrimary() Expr { } // list = '[' ']' -// | '[' expr ']' -// | '[' expr expr_list ']' -// | '[' expr (FOR loop_variables IN expr)+ ']' +// +// | '[' expr ']' +// | '[' expr expr_list ']' +// | '[' expr (FOR loop_variables IN expr)+ ']' func (p *parser) parseList() Expr { lbrack := p.nextToken() if p.tok == RBRACK { @@ -865,8 +890,9 @@ func (p *parser) parseList() Expr { } // dict = '{' '}' -// | '{' dict_entry_list '}' -// | '{' dict_entry FOR loop_variables IN expr '}' +// +// | '{' dict_entry_list '}' +// | '{' dict_entry FOR loop_variables IN expr '}' func (p *parser) parseDict() Expr { lbrace := p.nextToken() if p.tok == RBRACE { @@ -904,8 +930,9 @@ func (p *parser) parseDictEntry() *DictEntry { } // comp_suffix = FOR loopvars IN expr comp_suffix -// | IF expr comp_suffix -// | ']' or ')' (end) +// +// | IF expr comp_suffix +// | ']' or ')' (end) // // There can be multiple FOR/IF clauses; the first is always a FOR. func (p *parser) parseComprehensionSuffix(lbrace Position, body Expr, endBrace Token) Expr { diff --git a/syntax/syntax.go b/syntax/syntax.go index 37566375..0911f542 100644 --- a/syntax/syntax.go +++ b/syntax/syntax.go @@ -70,7 +70,8 @@ type File struct { Path string Stmts []Stmt - Module interface{} // a *resolve.Module, set by resolver + Module interface{} // a *resolve.Module, set by resolver + Options *FileOptions } func (x *File) Span() (start, end Position) { @@ -99,9 +100,10 @@ func (*LoadStmt) stmt() {} func (*ReturnStmt) stmt() {} // An AssignStmt represents an assignment: +// // x = 0 // x, y = y, x -// x += 1 +// x += 1 type AssignStmt struct { commentsRef OpPos Position