From ffa9727f14a2b11264f4a9580903682dcec40ca6 Mon Sep 17 00:00:00 2001 From: Qi Xiao Date: Wed, 8 Apr 2020 23:33:22 +0100 Subject: [PATCH] pkg/eval: Remove Frame.srcRange. Maintaining a state of current range is error-prone. Instead, always keep track of range information explicitly. --- pkg/eval/builtin_special.go | 34 +++++++++++++------------- pkg/eval/closure.go | 2 -- pkg/eval/compile_effect.go | 13 +++++----- pkg/eval/compile_lvalue.go | 10 ++++---- pkg/eval/compile_value.go | 2 +- pkg/eval/frame.go | 48 +++++++++++++++++-------------------- pkg/eval/op.go | 11 ++++----- pkg/eval/unwrap.go | 14 +++++------ 8 files changed, 64 insertions(+), 70 deletions(-) diff --git a/pkg/eval/builtin_special.go b/pkg/eval/builtin_special.go index 02d595d7..12810a57 100644 --- a/pkg/eval/builtin_special.go +++ b/pkg/eval/builtin_special.go @@ -148,14 +148,14 @@ func (op *delElemOp) invoke(fm *Frame) error { return err } if len(indexValues) != 1 { - return fm.errorpf(indexOp.From, indexOp.To, "index must evaluate to a single value in argument to del") + return fm.errorpf(indexOp, "index must evaluate to a single value in argument to del") } indicies = append(indicies, indexValues[0]) } err := vars.DelElement(fm.ResolveVar(op.qname), indicies) if err != nil { if level := vars.ElementErrorLevel(err); level >= 0 { - return fm.errorpf(op.begin, op.ends[level], "%s", err.Error()) + return fm.errorp(diag.Ranging{From: op.begin, To: op.ends[level]}, err) } return err } @@ -239,13 +239,16 @@ func compileUse(cp *compiler, fn *parse.Form) effectOpBody { cp.thisScope().set(name + NsSuffix) - return useOp{name, spec} + return useOp{fn.Range(), name, spec} } -type useOp struct{ name, spec string } +type useOp struct { + diag.Ranging + name, spec string +} func (op useOp) invoke(fm *Frame) error { - ns, err := loadModule(fm, op.spec) + ns, err := loadModule(fm, op, op.spec) if err != nil { return err } @@ -253,7 +256,7 @@ func (op useOp) invoke(fm *Frame) error { return nil } -func loadModule(fm *Frame, spec string) (Ns, error) { +func loadModule(fm *Frame, r diag.Ranger, spec string) (Ns, error) { if strings.HasPrefix(spec, "./") || strings.HasPrefix(spec, "../") { var dir string if fm.srcMeta.Type == FileSource { @@ -266,21 +269,21 @@ func loadModule(fm *Frame, spec string) (Ns, error) { } } path := filepath.Clean(dir + "/" + spec + ".elv") - return loadModuleFile(fm, spec, path) + return loadModuleFile(fm, r, spec, path) } if ns, ok := fm.Evaler.modules[spec]; ok { return ns, nil } if code, ok := fm.bundled[spec]; ok { - return evalModule(fm, spec, NewInternalElvishSource(false, spec, code)) + return evalModule(fm, r, spec, NewInternalElvishSource(false, spec, code)) } if fm.libDir == "" { return nil, noSuchModule{spec} } - return loadModuleFile(fm, spec, fm.libDir+"/"+spec+".elv") + return loadModuleFile(fm, r, spec, fm.libDir+"/"+spec+".elv") } -func loadModuleFile(fm *Frame, spec, path string) (Ns, error) { +func loadModuleFile(fm *Frame, r diag.Ranger, spec, path string) (Ns, error) { if ns, ok := fm.modules[path]; ok { return ns, nil } @@ -291,10 +294,10 @@ func loadModuleFile(fm *Frame, spec, path string) (Ns, error) { } return nil, err } - return evalModule(fm, path, NewModuleSource(path, code)) + return evalModule(fm, r, path, NewModuleSource(path, code)) } -func evalModule(fm *Frame, key string, src *Source) (Ns, error) { +func evalModule(fm *Frame, r diag.Ranger, key string, src *Source) (Ns, error) { n, err := parse.AsChunk(src.Name, src.Code) if err != nil { return nil, err @@ -305,10 +308,9 @@ func evalModule(fm *Frame, key string, src *Source) (Ns, error) { newFm := &Frame{ fm.Evaler, src, - diag.Ranging{From: 0, To: len(src.Code)}, modGlobal, make(Ns), fm.intCh, fm.ports, - fm.addTraceback(), false, + fm.addTraceback(r), false, } op, err := compile(newFm.Builtin.static(), modGlobal.static(), n, src) @@ -514,7 +516,7 @@ func (op *forOp) invoke(fm *Frame) error { return err } if len(variables) != 1 { - return fm.errorpf(op.varOp.From, op.varOp.To, "only one variable allowed") + return fm.errorpf(op.varOp, "only one variable allowed") } variable := variables[0] iterable, err := fm.ExecAndUnwrap("value being iterated", op.iterOp).One().Any() @@ -676,7 +678,7 @@ func (op lvaluesOp) execMustOne(fm *Frame) (vars.Var, error) { return nil, err } if len(variables) != 1 { - return nil, fm.errorpf(op.From, op.To, "should be one variable") + return nil, fm.errorpf(op, "should be one variable") } return variables[0], nil } diff --git a/pkg/eval/closure.go b/pkg/eval/closure.go index c1c9cce0..dc5d460b 100644 --- a/pkg/eval/closure.go +++ b/pkg/eval/closure.go @@ -139,8 +139,6 @@ func (c *Closure) Call(fm *Frame, args []interface{}, opts map[string]interface{ } } - fm.traceback = fm.addTraceback() - fm.srcMeta = c.SrcMeta return c.Op.exec(fm) } diff --git a/pkg/eval/compile_effect.go b/pkg/eval/compile_effect.go index d442a57a..6524838a 100644 --- a/pkg/eval/compile_effect.go +++ b/pkg/eval/compile_effect.go @@ -7,6 +7,7 @@ import ( "strings" "sync" + "github.com/elves/elvish/pkg/diag" "github.com/elves/elvish/pkg/eval/vals" "github.com/elves/elvish/pkg/eval/vars" "github.com/elves/elvish/pkg/parse" @@ -245,7 +246,7 @@ func (cp *compiler) formOp(n *parse.Form) effectOp { redirOps := cp.redirOps(n.Redirs) // TODO: n.ErrorRedir - return makeEffectOp(n, &formOp{saveVarsOps, assignmentOps, redirOps, specialOpFunc, headOp, argOps, optsOp, spaceyAssignOp}) + return makeEffectOp(n, &formOp{n.Range(), saveVarsOps, assignmentOps, redirOps, specialOpFunc, headOp, argOps, optsOp, spaceyAssignOp}) } func (cp *compiler) formOps(ns []*parse.Form) []effectOp { @@ -257,6 +258,7 @@ func (cp *compiler) formOps(ns []*parse.Form) []effectOp { } type formOp struct { + diag.Ranging saveVarsOps []lvaluesOp assignmentOps []effectOp redirOps []effectOp @@ -268,10 +270,7 @@ type formOp struct { } func (op *formOp) invoke(fm *Frame) (errRet error) { - // Save the range for the entire form. - formRange := fm.srcRange - - // ec here is always a subevaler created in compiler.pipeline, so it can + // fm here is always a sub-frame created in compiler.pipeline, so it can // be safely modified. // Temporary assignment. @@ -372,8 +371,10 @@ func (op *formOp) invoke(fm *Frame) (errRet error) { } } - fm.srcRange = formRange if headFn != nil { + if _, isClosure := headFn.(*Closure); isClosure { + fm.traceback = fm.addTraceback(op) + } return headFn.Call(fm, args, convertedOpts) } return op.spaceyAssignOp.exec(fm) diff --git a/pkg/eval/compile_lvalue.go b/pkg/eval/compile_lvalue.go index eaa4dca5..c09b1c70 100644 --- a/pkg/eval/compile_lvalue.go +++ b/pkg/eval/compile_lvalue.go @@ -91,7 +91,6 @@ func (cp *compiler) lvalueElement(qname string, n *parse.Indexing) lvaluesOpBody cp.errorpf(n, "variable $%s not found", qname) } - begin, end := n.Range().From, n.Range().To ends := make([]int, len(n.Indicies)+1) ends[0] = n.Head.Range().To for i, idx := range n.Indicies { @@ -100,7 +99,7 @@ func (cp *compiler) lvalueElement(qname string, n *parse.Indexing) lvaluesOpBody indexOps := cp.arrayOps(n.Indicies) - return &elemOp{qname, indexOps, begin, end, ends} + return &elemOp{n.Range(), qname, indexOps, ends} } type seqLValuesOpBody struct { @@ -138,10 +137,9 @@ func (op varOp) invoke(fm *Frame) ([]vars.Var, error) { } type elemOp struct { + diag.Ranging qname string indexOps []valuesOp - begin int - end int ends []int } @@ -167,9 +165,9 @@ func (op *elemOp) invoke(fm *Frame) ([]vars.Var, error) { if err != nil { level := vars.ElementErrorLevel(err) if level < 0 { - return nil, fm.errorpf(op.begin, op.end, "%s", err) + return nil, fm.errorp(op, err) } - return nil, fm.errorpf(op.begin, op.ends[level], "%s", err) + return nil, fm.errorp(diag.Ranging{From: op.From, To: op.ends[level]}, err) } return []vars.Var{elemVar}, nil } diff --git a/pkg/eval/compile_value.go b/pkg/eval/compile_value.go index 1a80b675..cbfa8fe0 100644 --- a/pkg/eval/compile_value.go +++ b/pkg/eval/compile_value.go @@ -562,7 +562,7 @@ func (op *mapPairsOp) invoke(fm *Frame) ([]interface{}, error) { return nil, err } if len(keys) != len(values) { - return nil, fm.errorpf(op.begins[i], op.ends[i], + return nil, fm.errorpf(diag.Ranging{From: op.begins[i], To: op.ends[i]}, "%d keys but %d values", len(keys), len(values)) } for j, key := range keys { diff --git a/pkg/eval/frame.go b/pkg/eval/frame.go index 9ac8f8c9..64836225 100644 --- a/pkg/eval/frame.go +++ b/pkg/eval/frame.go @@ -17,8 +17,7 @@ import ( type Frame struct { *Evaler - srcMeta *Source - srcRange diag.Ranging + srcMeta *Source local, up Ns @@ -36,7 +35,6 @@ type Frame struct { func NewTopFrame(ev *Evaler, src *Source, ports []*Port) *Frame { return &Frame{ ev, src, - diag.Ranging{From: 0, To: len(src.Code)}, ev.Global, make(Ns), nil, ports, nil, false, @@ -129,7 +127,7 @@ func (fm *Frame) fork(name string) *Frame { } } return &Frame{ - fm.Evaler, fm.srcMeta, fm.srcRange, + fm.Evaler, fm.srcMeta, fm.local, fm.up, fm.intCh, newPorts, fm.traceback, fm.background, @@ -149,16 +147,14 @@ func (fm *Frame) Eval(op Op) error { // eval evaluates an effectOp. It does so in a protected environment so that // exceptions thrown are wrapped in an Error. -func (fm *Frame) eval(op effectOp) (err error) { - e := op.exec(fm) - return fm.makeException(e) +func (fm *Frame) eval(op effectOp) error { + return op.exec(fm) } // Call calls a function with the given arguments and options. It does so in a // protected environment so that exceptions thrown are wrapped in an Error. -func (fm *Frame) Call(f Callable, args []interface{}, opts map[string]interface{}) (err error) { - e := f.Call(fm, args, opts) - return fm.makeException(e) +func (fm *Frame) Call(f Callable, args []interface{}, opts map[string]interface{}) error { + return f.Call(fm, args, opts) } // CaptureOutput calls a function with the given arguments and options, @@ -189,29 +185,29 @@ func (fm *Frame) ExecWithOutputCallback(op Op, valuesCb func(<-chan interface{}) return pcaptureOutputInner(fm, op.Inner, valuesCb, bytesCb) } -// makeException turns an error into an Exception by adding traceback. If e is -// nil or already an *Exception, it is returned as is. -func (fm *Frame) makeException(e error) error { +func (fm *Frame) addTraceback(r diag.Ranger) *stackTrace { + return &stackTrace{ + head: diag.NewContext(fm.srcMeta.Name, fm.srcMeta.Code, r.Range()), + next: fm.traceback, + } +} + +// Returns an Exception with specified range and cause. +func (fm *Frame) errorp(r diag.Ranger, e error) error { switch e := e.(type) { case nil: return nil case *Exception: return e default: - return &Exception{e, fm.addTraceback()} + return &Exception{e, &stackTrace{ + head: diag.NewContext(fm.srcMeta.Name, fm.srcMeta.Code, r.Range()), + next: fm.traceback, + }} } } -func (fm *Frame) addTraceback() *stackTrace { - return &stackTrace{ - head: diag.NewContext(fm.srcMeta.Name, fm.srcMeta.Code, fm.srcRange), - next: fm.traceback, - } -} - -// Amends the being and end of the current frame and returns the result of -// fmt.Errorf. -func (fm *Frame) errorpf(begin, end int, format string, args ...interface{}) error { - fm.srcRange = diag.Ranging{From: begin, To: end} - return fmt.Errorf(format, args...) +// Returns an Exception with specified range and error text. +func (fm *Frame) errorpf(r diag.Ranger, format string, args ...interface{}) error { + return fm.errorp(r, fmt.Errorf(format, args...)) } diff --git a/pkg/eval/op.go b/pkg/eval/op.go index b98ad9de..fb2b0273 100644 --- a/pkg/eval/op.go +++ b/pkg/eval/op.go @@ -29,8 +29,7 @@ type effectOpBody interface { // Executes an effectOp for side effects. func (op effectOp) exec(fm *Frame) error { - fm.srcRange = op.Ranging - return op.body.invoke(fm) + return fm.errorp(op, op.body.invoke(fm)) } // An operation on an Frame that produce Value's. @@ -50,8 +49,8 @@ type valuesOpBody interface { // Executes a ValuesOp and produces values. func (op valuesOp) exec(fm *Frame) ([]interface{}, error) { - fm.srcRange = op.Ranging - return op.body.invoke(fm) + values, err := op.body.invoke(fm) + return values, fm.errorp(op, err) } // An operation on a Frame that produce Variable's. @@ -71,6 +70,6 @@ func (op lvaluesOp) exec(fm *Frame) ([]vars.Var, error) { if op.body == nil { return []vars.Var{}, nil } - fm.srcRange = op.Ranging - return op.body.invoke(fm) + lvalues, err := op.body.invoke(fm) + return lvalues, fm.errorp(op, err) } diff --git a/pkg/eval/unwrap.go b/pkg/eval/unwrap.go index 48b038bb..a86e3355 100644 --- a/pkg/eval/unwrap.go +++ b/pkg/eval/unwrap.go @@ -4,6 +4,7 @@ import ( "fmt" "strconv" + "github.com/elves/elvish/pkg/diag" "github.com/elves/elvish/pkg/eval/vals" "github.com/elves/elvish/pkg/util" ) @@ -13,14 +14,13 @@ import ( // properties are not satisfied. type unwrapper struct { + // Range in the source code to point to when error occurs. + diag.Ranging // ctx is the evaluation context. ctx *Frame // description describes what is being unwrapped. It is used in error // messages. description string - // begin and end contains positions in the source code to point to when - // error occurs. - begin, end int // values contain the Value's to unwrap. values []interface{} // Any errors during the unwrapping. @@ -33,7 +33,7 @@ func (u *unwrapper) error(want, gotfmt string, gotargs ...interface{}) { } got := fmt.Sprintf(gotfmt, gotargs...) u.err = u.ctx.errorpf( - u.begin, u.end, "%s must be %s; got %s", u.description, want, got) + u, "%s must be %s; got %s", u.description, want, got) } // ValuesUnwrapper unwraps []Value. @@ -43,7 +43,7 @@ type ValuesUnwrapper struct{ *unwrapper } // values. func (ctx *Frame) ExecAndUnwrap(desc string, op valuesOp) ValuesUnwrapper { values, err := op.exec(ctx) - return ValuesUnwrapper{&unwrapper{ctx, desc, op.From, op.To, values, err}} + return ValuesUnwrapper{&unwrapper{op.Range(), ctx, desc, values, err}} } // One unwraps the value to be exactly one value. @@ -113,7 +113,7 @@ func (u ValueUnwrapper) Fd() (int, error) { default: i, err := u.NonNegativeInt() if err != nil { - return 0, u.ctx.errorpf(u.begin, u.end, "fd must be standard stream name or integer; got %s", s) + return 0, u.ctx.errorpf(u, "fd must be standard stream name or integer; got %s", s) } return i, nil } @@ -126,7 +126,7 @@ func (u ValueUnwrapper) FdOrClose() (int, error) { } fd, err := u.Fd() if err != nil { - return 0, u.ctx.errorpf(u.begin, u.end, "redirection source must be standard stream name or integer; got %s", s) + return 0, u.ctx.errorpf(u, "redirection source must be standard stream name or integer; got %s", s) } return fd, nil }