From f4dab1e3a3d16260336d91b340ddafcb0a24ef90 Mon Sep 17 00:00:00 2001 From: Tim Voronov Date: Wed, 6 Aug 2025 16:30:51 -0400 Subject: [PATCH] Refactor error handling for FOR and COLLECT statements: enhance error messages for missing variables, improve error span tracking, and streamline error analysis for better clarity and maintainability. --- .../internal/diagnostics/error_listener.go | 23 ++++++++++++---- pkg/compiler/internal/diagnostics/handler.go | 25 +++++++++++++----- pkg/compiler/internal/diagnostics/helpers.go | 4 +++ .../diagnostics/match_for_loop_errors.go | 18 +++++++++++++ pkg/file/snippet.go | 11 ++++++-- .../compiler/compiler_errors_syntax_test.go | 26 +++++++++++++++++-- 6 files changed, 92 insertions(+), 15 deletions(-) diff --git a/pkg/compiler/internal/diagnostics/error_listener.go b/pkg/compiler/internal/diagnostics/error_listener.go index 68bc41e6..8024220f 100644 --- a/pkg/compiler/internal/diagnostics/error_listener.go +++ b/pkg/compiler/internal/diagnostics/error_listener.go @@ -38,7 +38,11 @@ func (d *ErrorListener) SyntaxError(_ antlr.Recognizer, offendingSymbol interfac offending = tok } - d.handler.Add(d.parseError(msg, offending)) + if err := d.parseError(msg, offending); err != nil { + if !d.handler.HasErrorOnLine(line) { + d.handler.Add(err) + } + } } func (d *ErrorListener) parseError(msg string, offending antlr.Token) *CompilationError { @@ -55,8 +59,9 @@ func (d *ErrorListener) parseError(msg string, offending antlr.Token) *Compilati } for _, handler := range []func(*CompilationError) bool{ - d.extraneousError, - d.noViableAltError, + d.extraneousInput, + d.noViableAlternative, + d.mismatchedInput, } { if handler(err) { break @@ -66,7 +71,7 @@ func (d *ErrorListener) parseError(msg string, offending antlr.Token) *Compilati return err } -func (d *ErrorListener) extraneousError(err *CompilationError) (matched bool) { +func (d *ErrorListener) extraneousInput(err *CompilationError) (matched bool) { if !strings.Contains(err.Message, "extraneous input") { return false } @@ -88,10 +93,18 @@ func (d *ErrorListener) extraneousError(err *CompilationError) (matched bool) { return true } -func (d *ErrorListener) noViableAltError(err *CompilationError) bool { +func (d *ErrorListener) noViableAlternative(err *CompilationError) bool { if !strings.Contains(err.Message, "viable alternative at input") { return false } return AnalyzeSyntaxError(d.src, err, d.history.Last()) } + +func (d *ErrorListener) mismatchedInput(err *CompilationError) bool { + if !strings.Contains(err.Message, "mismatched input") { + return false + } + + return AnalyzeSyntaxError(d.src, err, d.history.Last()) +} diff --git a/pkg/compiler/internal/diagnostics/handler.go b/pkg/compiler/internal/diagnostics/handler.go index 746d3fbf..a4d0baff 100644 --- a/pkg/compiler/internal/diagnostics/handler.go +++ b/pkg/compiler/internal/diagnostics/handler.go @@ -9,9 +9,10 @@ import ( ) type ErrorHandler struct { - src *file.Source - errors []*CompilationError - threshold int + src *file.Source + errors []*CompilationError + linesWithErrors map[int]bool + threshold int } func NewErrorHandler(src *file.Source, threshold int) *ErrorHandler { @@ -20,9 +21,10 @@ func NewErrorHandler(src *file.Source, threshold int) *ErrorHandler { } return &ErrorHandler{ - src: src, - errors: make([]*CompilationError, 0), - threshold: threshold, + src: src, + errors: make([]*CompilationError, 0), + linesWithErrors: make(map[int]bool), + threshold: threshold, } } @@ -60,6 +62,13 @@ func (h *ErrorHandler) Add(err *CompilationError) { err.Source = h.src } + for _, span := range err.Spans { + if err.Source != nil { + line, _ := err.Source.LocationAt(span.Span) + h.linesWithErrors[line] = true + } + } + h.errors = append(h.errors, err) if len(h.errors) == h.threshold { @@ -71,6 +80,10 @@ func (h *ErrorHandler) Add(err *CompilationError) { } } +func (h *ErrorHandler) HasErrorOnLine(line int) bool { + return h.linesWithErrors[line] +} + func (h *ErrorHandler) VariableNotUnique(ctx antlr.ParserRuleContext, name string) { // TODO: Add information where the variable was defined h.Add(&CompilationError{ diff --git a/pkg/compiler/internal/diagnostics/helpers.go b/pkg/compiler/internal/diagnostics/helpers.go index a44b2f4a..289f8976 100644 --- a/pkg/compiler/internal/diagnostics/helpers.go +++ b/pkg/compiler/internal/diagnostics/helpers.go @@ -107,3 +107,7 @@ func is(node *TokenNode, expected string) bool { return strings.ToUpper(node.GetText()) == expected } + +func has(msg string, substr string) bool { + return strings.Contains(strings.ToLower(msg), strings.ToLower(substr)) +} diff --git a/pkg/compiler/internal/diagnostics/match_for_loop_errors.go b/pkg/compiler/internal/diagnostics/match_for_loop_errors.go index f5ce9463..75f7ebd3 100644 --- a/pkg/compiler/internal/diagnostics/match_for_loop_errors.go +++ b/pkg/compiler/internal/diagnostics/match_for_loop_errors.go @@ -46,5 +46,23 @@ func matchForLoopErrors(src *file.Source, err *CompilationError, offending *Toke return true } + if is(offending, "COLLECT") { + msg := err.Message + + if has(msg, "COLLECT =") { + span := spanFromTokenSafe(offending.Token(), src) + span.Start = span.End + span.End = span.Start + 1 + + err.Message = "Expected variable before '=' in COLLECT" + err.Hint = "COLLECT must group by a variable." + err.Spans = []ErrorSpan{ + NewMainErrorSpan(span, "missing variable"), + } + + return true + } + } + return false } diff --git a/pkg/file/snippet.go b/pkg/file/snippet.go index 36e09f86..4c534dea 100644 --- a/pkg/file/snippet.go +++ b/pkg/file/snippet.go @@ -23,8 +23,15 @@ func NewSnippetWithCaret(lines []string, span Span, line int) Snippet { } srcLine := lines[line-1] - startCol := computeVisualOffset(srcLine, span.Start) - endCol := computeVisualOffset(srcLine, span.End) + lineStartOffset := 0 + + // Compute actual start-of-line offset + for i := 0; i < line-1; i++ { + lineStartOffset += len(lines[i]) + 1 // +1 for \n + } + + startCol := computeVisualOffset(srcLine, span.Start-lineStartOffset+1) + endCol := computeVisualOffset(srcLine, span.End-lineStartOffset+1) caret := "" diff --git a/test/integration/compiler/compiler_errors_syntax_test.go b/test/integration/compiler/compiler_errors_syntax_test.go index 7fe64e2c..06adcbc3 100644 --- a/test/integration/compiler/compiler_errors_syntax_test.go +++ b/test/integration/compiler/compiler_errors_syntax_test.go @@ -67,8 +67,30 @@ func TestSyntaxErrors(t *testing.T) { RETURN i `, E{ Kind: compiler.SyntaxError, - Message: "--", - Hint: "Use 'FOR x IN [iterable]' syntax.", + Message: "Expected loop variable before 'IN'", + Hint: "FOR must declare a variable.", }, "FOR without variable"), + ErrorCase( + ` + LET users = [] + FOR x IN users + COLLECT = + RETURN x + `, E{ + Kind: compiler.SyntaxError, + Message: "Expected variable before '=' in COLLECT", + Hint: "COLLECT must group by a variable.", + }, "COLLECT with no variable"), + ErrorCase( + ` + LET users = [] + FOR x IN users + COLLECT i = + RETURN x + `, E{ + Kind: compiler.SyntaxError, + Message: "Expected expression after '=' for variable 'i'", + Hint: "Did you forget to provide a value?", + }, "COLLECT with no variable assignment"), }) }