diff --git a/0.21.0-release-notes.md b/0.21.0-release-notes.md index 938b6a0ec..b204a2605 100644 --- a/0.21.0-release-notes.md +++ b/0.21.0-release-notes.md @@ -23,6 +23,12 @@ - Temporary assignments now work correctly on map and list elements ([#1515](https://b.elv.sh/1515)). +- The terminal line editor is now more aggressive in suppressing compilation + errors caused by the code not being complete. + + For example, during the process of typing out `echo $pid`, the editor no + longer complains that `$p` is undefined when the user has typed `echo $p`. + # Deprecations - The implicit cd feature is now deprecated. Use `cd` or location mode diff --git a/pkg/edit/highlight/highlighter_test.go b/pkg/edit/highlight/highlighter_test.go index fb28032e5..fd86b574a 100644 --- a/pkg/edit/highlight/highlighter_test.go +++ b/pkg/edit/highlight/highlighter_test.go @@ -92,16 +92,16 @@ func TestHighlighter_AutofixesAndCheckErrors(t *testing.T) { tt.Test(t, tt.Fn(hl.Get).Named("hl.Get"), // Check error is highlighted and returned - Args("ls $a").Rets( + Args("ls $a ").Rets( ui.MarkLines( - "ls $a", styles, - "vv ??"), + "ls $a ", styles, + "vv ?? "), matchTexts("1:4")), // Multiple check errors - Args("ls $a $b").Rets( + Args("ls $a $b ").Rets( ui.MarkLines( - "ls $a $b", styles, - "vv ?? ??"), + "ls $a $b ", styles, + "vv ?? ?? "), matchTexts("1:4", "1:7")), // Check errors at the end are ignored Args("set _").Rets(any, noTips), @@ -110,9 +110,8 @@ func TestHighlighter_AutofixesAndCheckErrors(t *testing.T) { Args("nop $mod1:").Rets( ui.MarkLines( "nop $mod1:", styles, - "vvv ??????"), + "vvv $$$$$$"), matchTexts( - "1:5", // error "autofix: use mod1", // autofix )), ) diff --git a/pkg/edit/highlight_test.go b/pkg/edit/highlight_test.go index 83fe8a36d..5a65e4156 100644 --- a/pkg/edit/highlight_test.go +++ b/pkg/edit/highlight_test.go @@ -27,10 +27,10 @@ func TestHighlighter(t *testing.T) { ) // Check errors - feedInput(f.TTYCtrl, "x") + feedInput(f.TTYCtrl, "x ") f.TestTTY(t, - "~> put $truex", Styles, - " vvv ??????", term.DotHere, "\n", + "~> put $truex ", Styles, + " vvv ?????? ", term.DotHere, "\n", "compilation error: [interactive]:1:5-10: variable $truex not found", ) } @@ -42,8 +42,7 @@ func TestHighlighter_Autofix(t *testing.T) { feedInput(f.TTYCtrl, "put $mod1:") f.TestTTY(t, "~> put $mod1:", Styles, - " vvv ??????", term.DotHere, "\n", - "compilation error: [interactive]:1:5-10: variable $mod1: not found\n", + " vvv $$$$$$", term.DotHere, "\n", "Ctrl-A autofix: use mod1 Tab Enter autofix first", Styles, "++++++ +++ +++++", ) diff --git a/pkg/eval/builtin_special.go b/pkg/eval/builtin_special.go index cd448194c..a231e0314 100644 --- a/pkg/eval/builtin_special.go +++ b/pkg/eval/builtin_special.go @@ -114,13 +114,14 @@ func compileTmp(cp *compiler, fn *parse.Form) effectOp { func compileWith(cp *compiler, fn *parse.Form) effectOp { if len(fn.Args) < 2 { - cp.errorpf(fn, "with requires at least two arguments") + cp.errorpfPartial(fn, "with requires at least two arguments") return nopOp{} } lastArg := fn.Args[len(fn.Args)-1] bodyNode, ok := cmpd.Lambda(lastArg) if !ok { - cp.errorpf(lastArg, "last argument must be a lambda") + // It's possible that we just haven't seen the last argument yet. + cp.errorpfPartial(diag.PointRanging(fn.To), "last argument must be a lambda") return nopOp{} } @@ -194,7 +195,7 @@ func (op *withOp) exec(fm *Frame) (opExc Exception) { func compileLHSRHS(cp *compiler, args []*parse.Compound, end int, lf lvalueFlag) (lvaluesGroup, valuesOp) { lhs, rhs := compileLHSOptionalRHS(cp, args, end, lf) if rhs == nil { - cp.errorpf(diag.PointRanging(end), "need = and right-hand-side") + cp.errorpfPartial(diag.PointRanging(end), "need = and right-hand-side") } return lhs, rhs } @@ -240,7 +241,7 @@ func compileDel(cp *compiler, fn *parse.Form) effectOp { var f effectOp ref := resolveVarRef(cp, qname, nil) if ref == nil { - cp.errorpf(cn, "no variable $%s", head.Value) + cp.errorpfPartial(cn, "no variable $%s", head.Value) continue } if len(indices) == 0 { @@ -810,7 +811,7 @@ func compileTry(cp *compiler, fn *parse.Form) effectOp { if elseNode != nil && catchNode == nil { cp.errorpf(fn, "try with an else block requires a catch block") } else if catchNode == nil && finallyNode == nil { - cp.errorpf(fn, "try must be followed by a catch block or a finally block") + cp.errorpfPartial(fn, "try must be followed by a catch block or a finally block") } var catchVar lvalue @@ -904,11 +905,11 @@ func compilePragma(cp *compiler, fn *parse.Form) effectOp { case "external": cp.currentPragma().unknownCommandIsExternal = true default: - cp.errorpf(valueNode, + cp.errorpfPartial(valueNode, "invalid value for unknown-command: %s", parse.Quote(value)) } default: - cp.errorpf(fn.Args[0], "unknown pragma %s", parse.Quote(name)) + cp.errorpfPartial(fn.Args[0], "unknown pragma %s", parse.Quote(name)) } return nopOp{} } diff --git a/pkg/eval/builtin_special_test.elvts b/pkg/eval/builtin_special_test.elvts index 8622cf9a1..36ffc8efb 100644 --- a/pkg/eval/builtin_special_test.elvts +++ b/pkg/eval/builtin_special_test.elvts @@ -376,7 +376,7 @@ Compilation error: with requires at least two arguments ~> var x with x = val foobar Compilation error: last argument must be a lambda - [tty]:2:14-19: with x = val foobar + [tty]:2:20: with x = val foobar ## compound expressions ## ~> with a'x' = foo { } diff --git a/pkg/eval/compile_effect.go b/pkg/eval/compile_effect.go index 1353198ac..a4cd47d78 100644 --- a/pkg/eval/compile_effect.go +++ b/pkg/eval/compile_effect.go @@ -197,7 +197,7 @@ func (cp *compiler) formBody(n *parse.Form) formBody { if cp.currentPragma().unknownCommandIsExternal || fsutil.DontSearch(head) { headOp = literalValues(n.Head, NewExternalCmd(head)) } else { - cp.errorpf(n.Head, "unknown command disallowed by current pragma") + cp.errorpfPartial(n.Head, "unknown command disallowed by current pragma") } } } else { diff --git a/pkg/eval/compile_lvalue.go b/pkg/eval/compile_lvalue.go index 37bc2d14a..8c72cc6c8 100644 --- a/pkg/eval/compile_lvalue.go +++ b/pkg/eval/compile_lvalue.go @@ -63,7 +63,7 @@ func (cp *compiler) compileIndexingLValue(n *parse.Indexing, f lvalueFlag) lvalu varUse := n.Head.Value sigil, qname := SplitSigil(varUse) if qname == "" { - cp.errorpf(n, "variable name must not be empty") + cp.errorpfPartial(n, "variable name must not be empty") return dummyLValuesGroup } @@ -78,7 +78,7 @@ func (cp *compiler) compileIndexingLValue(n *parse.Indexing, f lvalueFlag) lvalu if ref == nil { if f&newLValue == 0 { cp.autofixUnresolvedVar(qname) - cp.errorpf(n, "cannot find variable $%s", parse.Quote(qname)) + cp.errorpfPartial(n, "cannot find variable $%s", parse.Quote(qname)) return dummyLValuesGroup } if len(n.Indices) > 0 { diff --git a/pkg/eval/compile_value.go b/pkg/eval/compile_value.go index 532bef82d..e7ba96c97 100644 --- a/pkg/eval/compile_value.go +++ b/pkg/eval/compile_value.go @@ -263,7 +263,11 @@ func (cp *compiler) primaryOp(n *parse.Primary) valuesOp { ref := resolveVarRef(cp, qname, n) if ref == nil { cp.autofixUnresolvedVar(qname) - cp.errorpf(n, "variable $%s not found", parse.Quote(qname)) + // The variable name might be a prefix of a valid variable name. + // Ideally, we'd want to match the variable name to all possible + // names to check if that's actually the case, but it's a bit + // expensive and let's call this good enough for now. + cp.errorpfPartial(n, "variable $%s not found", parse.Quote(qname)) } return &variableOp{n.Range(), sigil != "", qname, ref} case parse.Wildcard: @@ -390,7 +394,7 @@ func (cp *compiler) lambda(n *parse.Primary) valuesOp { cp.errorpf(arg, "argument name must be unqualified") } if name == "" { - cp.errorpf(arg, "argument name must not be empty") + cp.errorpfPartial(arg, "argument name must not be empty") } if sigil == "@" { if restArg != -1 { @@ -400,7 +404,7 @@ func (cp *compiler) lambda(n *parse.Primary) valuesOp { } if name != "_" { if seenName[name] { - cp.errorpf(arg, "duplicate argument name '%s'", name) + cp.errorpfPartial(arg, "duplicate argument name '%s'", name) } else { seenName[name] = true } @@ -418,11 +422,11 @@ func (cp *compiler) lambda(n *parse.Primary) valuesOp { cp.errorpf(opt.Key, "option name must be unqualified") } if name == "" { - cp.errorpf(opt.Key, "option name must not be empty") + cp.errorpfPartial(opt.Key, "option name must not be empty") } optNames[i] = name if opt.Value == nil { - cp.errorpf(opt.Key, "option must have default value") + cp.errorpfPartial(opt.Key, "option must have default value") } else { optDefaultOps[i] = cp.compoundOp(opt.Value) } diff --git a/pkg/eval/compiler.go b/pkg/eval/compiler.go index eb218093b..06f17b6ff 100644 --- a/pkg/eval/compiler.go +++ b/pkg/eval/compiler.go @@ -84,13 +84,24 @@ type CompilationErrorTag struct{} func (CompilationErrorTag) ErrorTag() string { return "compilation error" } +// Reports a compilation error. func (cp *compiler) errorpf(r diag.Ranger, format string, args ...any) { + cp.errorpfInner(r, fmt.Sprintf(format, args...), false) +} + +// Reports a compilation error, and mark it as partial iff the end of r happens +// to coincide with the end of the source code. +func (cp *compiler) errorpfPartial(r diag.Ranger, format string, args ...any) { + cp.errorpfInner(r, fmt.Sprintf(format, args...), r.Range().To == len(cp.src.Code)) +} + +func (cp *compiler) errorpfInner(r diag.Ranger, msg string, partial bool) { cp.errors = append(cp.errors, &CompilationError{ - Message: fmt.Sprintf(format, args...), + Message: msg, Context: *diag.NewContext(cp.src.Name, cp.src.Code, r), // TODO: This criteria is too strict and only captures a small subset of // partial compilation errors. - Partial: r.Range().From == len(cp.src.Code), + Partial: partial, }) } diff --git a/pkg/eval/compiler_test.go b/pkg/eval/compiler_test.go index 8ad3c050a..e95726fd3 100644 --- a/pkg/eval/compiler_test.go +++ b/pkg/eval/compiler_test.go @@ -2,8 +2,10 @@ package eval_test import ( "testing" + "unicode/utf8" "github.com/google/go-cmp/cmp" + "src.elv.sh/pkg/diag" . "src.elv.sh/pkg/eval" "src.elv.sh/pkg/parse" ) @@ -64,3 +66,38 @@ func TestAutofix(t *testing.T) { }) } } + +// TODO: Turn this into a fuzz test. +func TestPartialCompilationError(t *testing.T) { + for _, code := range transcriptCodes { + testPartialError(t, code, func(src parse.Source) []*CompilationError { + _, _, err := NewEvaler().Check(src, nil) + return UnpackCompilationErrors(err) + }) + } +} + +// TODO: Deduplicate this against a similar helper in pkg/parse/fuzz_test.go +func testPartialError[T diag.ErrorTag](t *testing.T, full string, fn func(src parse.Source) []*diag.Error[T]) { + if !utf8.ValidString(full) { + t.Skip("not valid UTF-8") + } + errs := fn(parse.Source{Name: "fuzz.elv", Code: full}) + if len(errs) > 0 { + t.Skip("code itself has error") + } + // If code has no error, then every prefix of it (as long as it's valid + // UTF-8) should have either no errors or only partial errors. + for i := range full { + if i == 0 { + continue + } + prefix := full[:i] + errs := fn(parse.Source{Name: "fuzz.elv", Code: prefix}) + for _, err := range errs { + if !err.Partial { + t.Errorf("\n%s\n===========\nnon-partial error: %v\nfull code:\n===========\n%s\n", prefix, err, full) + } + } + } +} diff --git a/pkg/eval/evaltest/test_transcript.go b/pkg/eval/evaltest/test_transcript.go index cf795d140..982657394 100644 --- a/pkg/eval/evaltest/test_transcript.go +++ b/pkg/eval/evaltest/test_transcript.go @@ -148,6 +148,11 @@ func TestTranscriptsInFS(t *testing.T, fsys fs.FS, setupPairs ...any) { if err != nil { t.Fatalf("parse transcript sessions: %v", err) } + TestTranscriptNodes(t, nodes, setupPairs...) +} + +// TestTranscriptsInFS runs parsed Elvish transcript nodes as tests. +func TestTranscriptNodes(t *testing.T, nodes []*transcript.Node, setupPairs ...any) { var run *runCfg if runEnv := os.Getenv("ELVISH_TRANSCRIPT_RUN"); runEnv != "" { filename, lineNo, ok := parseFileNameAndLineNo(runEnv) diff --git a/pkg/eval/node_utils.go b/pkg/eval/node_utils.go index df01dc91d..383549e3f 100644 --- a/pkg/eval/node_utils.go +++ b/pkg/eval/node_utils.go @@ -11,7 +11,11 @@ import ( func stringLiteralOrError(cp *compiler, n *parse.Compound, what string) string { s, err := cmpd.StringLiteralOrError(n, what) if err != nil { - cp.errorpf(n, "%v", err) + if len(n.Indexings) == 0 { + cp.errorpfPartial(n, "%v", err) + } else { + cp.errorpf(n, "%v", err) + } } return s } @@ -34,12 +38,19 @@ func (ag *argsGetter) errorpf(r diag.Ranger, format string, args ...any) { } } +func (ag *argsGetter) errorpfPartial(r diag.Ranger, format string, args ...any) { + if ag.ok { + ag.cp.errorpfPartial(r, format, args...) + ag.ok = false + } +} + func (ag *argsGetter) get(i int, what string) *argAsserter { if ag.n < i+1 { ag.n = i + 1 } if i >= len(ag.fn.Args) { - ag.errorpf(diag.PointRanging(ag.fn.To), "need %s", what) + ag.errorpfPartial(diag.PointRanging(ag.fn.To), "need %s", what) return &argAsserter{ag, what, nil} } return &argAsserter{ag, what, ag.fn.Args[i]} @@ -64,7 +75,15 @@ func (ag *argsGetter) optionalKeywordBody(i int, kw string) *parse.Primary { func (ag *argsGetter) finish() bool { if ag.n < len(ag.fn.Args) { - ag.errorpf( + // In general, the "superfluous" argument may actually be incomplete + // optional keywords (like "el" in an "if" form), hence this calls + // errorpfPartial instead of errorpf. + // + // TODO: Make this more accurate. We should have all the information we + // need to say that whether this is partial or not, but in order to + // retain that information we'll probably need a more declarative API + // for parsing special forms. + ag.errorpfPartial( diag.Ranging{From: ag.fn.Args[ag.n].Range().From, To: ag.fn.To}, "superfluous arguments") } @@ -99,8 +118,15 @@ func (aa *argAsserter) lambda() *parse.Primary { } lambda, ok := cmpd.Lambda(aa.node) if !ok { - aa.ag.errorpf(aa.node, - "%s must be lambda, found %s", aa.what, cmpd.Shape(aa.node)) + if p, ok := cmpd.Primary(aa.node); ok && p.Type == parse.Braced { + // If we have seen just a "{", the parser will parse a braced + // expression, but this could as well be the start of a lambda. + aa.ag.errorpfPartial(aa.node, + "%s must be lambda, found %s", aa.what, cmpd.Shape(aa.node)) + } else { + aa.ag.errorpf(aa.node, + "%s must be lambda, found %s", aa.what, cmpd.Shape(aa.node)) + } return nil } return lambda diff --git a/pkg/eval/transcripts_test.go b/pkg/eval/transcripts_test.go index 5065950c9..f60b2280d 100644 --- a/pkg/eval/transcripts_test.go +++ b/pkg/eval/transcripts_test.go @@ -18,13 +18,18 @@ import ( "src.elv.sh/pkg/must" "src.elv.sh/pkg/prog" "src.elv.sh/pkg/testutil" + "src.elv.sh/pkg/transcript" ) -//go:embed *.elvts *.elv -var transcripts embed.FS +var ( + //go:embed *.elvts *.elv + transcripts embed.FS + transcriptNodes = must.OK1(transcript.ParseFromFS(transcripts)) + transcriptCodes = extractAllCodes(transcriptNodes) +) func TestTranscripts(t *testing.T) { - evaltest.TestTranscriptsInFS(t, transcripts, + evaltest.TestTranscriptNodes(t, transcriptNodes, "args", func(ev *eval.Evaler, arg string) { ev.Args = vals.MakeListSlice(strings.Fields(arg)) }, @@ -190,3 +195,19 @@ func (v *badVar) Set(any) error { v.allowedSets-- return nil } + +func extractAllCodes(nodes []*transcript.Node) []string { + var codes []string + for _, node := range nodes { + var codeBuf strings.Builder + for i, interaction := range node.Interactions { + if i > 0 { + codeBuf.WriteByte('\n') + } + codeBuf.WriteString(interaction.Code) + } + codes = append(codes, codeBuf.String()) + codes = append(codes, extractAllCodes(node.Children)...) + } + return codes +} diff --git a/pkg/parse/fuzz_test.go b/pkg/parse/fuzz_test.go index 1b2e05d7f..e25610836 100644 --- a/pkg/parse/fuzz_test.go +++ b/pkg/parse/fuzz_test.go @@ -3,6 +3,8 @@ package parse import ( "testing" "unicode/utf8" + + "src.elv.sh/pkg/diag" ) func FuzzParse_CrashOrVerySlow(f *testing.F) { @@ -14,31 +16,34 @@ func FuzzParse_CrashOrVerySlow(f *testing.F) { }) } -func FuzzError_Partial(f *testing.F) { +func FuzzPartialError(f *testing.F) { for _, test := range testCases { f.Add(test.code) } + fuzzPartialError(f, func(src Source) []*Error { + _, err := Parse(src, Config{}) + return UnpackErrors(err) + }) +} + +func fuzzPartialError[T diag.ErrorTag](f *testing.F, fn func(src Source) []*diag.Error[T]) { f.Fuzz(func(t *testing.T, code string) { if !utf8.ValidString(code) { t.SkipNow() } - _, err := Parse(Source{Name: "fuzz", Code: code}, Config{}) - if err != nil { + errs := fn(Source{Name: "fuzz.elv", Code: code}) + if len(errs) > 0 { t.SkipNow() } - // If code has no parse error, then every prefix of it (as long as it's - // valid UTF-8) should have either no parse errors or only partial parse - // errors. + // If code has no error, then every prefix of it (as long as it's valid + // UTF-8) should have either no errors or only partial errors. for i := range code { if i == 0 { continue } prefix := code[:i] - _, err := Parse(Source{Name: "fuzz", Code: prefix}, Config{}) - if err == nil { - continue - } - for _, err := range UnpackErrors(err) { + errs := fn(Source{Name: "fuzz.elv", Code: prefix}) + for _, err := range errs { if !err.Partial { t.Errorf("prefix %q of valid %q has non-partial error: %v", prefix, code, err) }