[WIP] Add helpful messages for invalid expression diagnostics (#33)

* Add extended diagnostics for syntax errors (unclosed strings, brackets, comments)
* Simplify diagnostics to use parser directly instead of duplicating logic
* Fix tests to match actual parser error messages and handle generic errors

Author: Copilot

src/language-service/diagnostics.ts

@@ -1,6 +1,9 @@
 /**
  * Diagnostics module for the language service.
- * Provides function argument count validation.
+ * Provides function argument count validation and syntax error detection.
+ * 
+ * This module leverages the existing parser infrastructure for error detection,
+ * avoiding duplication of tokenization and parsing logic.
  */
 
 import {
@@ -16,6 +19,13 @@ import { DiagnosticSeverity } from 'vscode-languageserver-types';
 import type { TextDocument } from 'vscode-languageserver-textdocument';
 import type { GetDiagnosticsParams, ArityInfo } from './language-service.types';
 import { FunctionDetails } from './language-service.models';
+import { ParseError } from '../types/errors';
+
+/**
+ * Length of the error highlight range when position is known but token length is not.
+ * Used to visually indicate the location of an error in the source text.
+ */
+const ERROR_HIGHLIGHT_LENGTH = 10;
 
 /**
  * Represents a token with its position in the source text.
@@ -332,3 +342,62 @@ export function getDiagnosticsForDocument(
 
   return diagnostics;
 }
+
+/**
+ * Creates a diagnostic from a ParseError.
+ * This function converts errors thrown by the parser/tokenizer into diagnostics
+ * that can be displayed to the user.
+ */
+export function createDiagnosticFromParseError(
+  textDocument: TextDocument,
+  error: ParseError
+): Diagnostic {
+  const position = error.context.position;
+  let startOffset = 0;
+  let endOffset = textDocument.getText().length;
+
+  if (position) {
+    // Convert line/column to offset
+    startOffset = textDocument.offsetAt({
+      line: position.line - 1,  // ParseError uses 1-based line numbers
+      character: position.column - 1 // ParseError uses 1-based column numbers
+    });
+    // Highlight a fixed-length region from the error position
+    endOffset = Math.min(startOffset + ERROR_HIGHLIGHT_LENGTH, textDocument.getText().length);
+  }
+
+  const range: Range = {
+    start: textDocument.positionAt(startOffset),
+    end: textDocument.positionAt(endOffset)
+  };
+
+  return {
+    range,
+    severity: DiagnosticSeverity.Error,
+    message: error.message,
+    source: 'expr-eval'
+  };
+}
+
+/**
+ * Creates a diagnostic from a generic Error.
+ * This function handles errors thrown by the parser that are not ParseError instances.
+ * Since these errors don't have position information, the diagnostic highlights the whole text.
+ */
+export function createDiagnosticFromError(
+  textDocument: TextDocument,
+  error: Error
+): Diagnostic {
+  const text = textDocument.getText();
+  const range: Range = {
+    start: textDocument.positionAt(0),
+    end: textDocument.positionAt(text.length)
+  };
+
+  return {
+    range,
+    severity: DiagnosticSeverity.Error,
+    message: error.message,
+    source: 'expr-eval'
+  };
+}

src/language-service/language-service.ts

@@ -37,7 +37,13 @@ import {
   iterateTokens
 } from './ls-utils';
 import { pathVariableCompletions, tryVariableHoverUsingSpans } from './variable-utils';
-import { getDiagnosticsForDocument } from './diagnostics';
+import {
+  getDiagnosticsForDocument,
+  createDiagnosticFromParseError,
+  createDiagnosticFromError,
+  TokenSpan
+} from './diagnostics';
+import { ParseError } from '../types/errors';
 
 export function createLanguageService(options: LanguageServiceOptions | undefined = undefined): LanguageServiceApi {
   // Build a parser instance to access keywords/operators/functions/consts
@@ -299,22 +305,54 @@ export function createLanguageService(options: LanguageServiceOptions | undefine
 
   /**
    * Analyzes the document for function calls and checks if they have the correct number of arguments.
-   * Returns diagnostics for function calls with incorrect argument counts.
+   * Returns diagnostics for function calls with incorrect argument counts, as well as
+   * syntax errors detected by the parser (unclosed strings, brackets, unknown characters, etc.).
    */
   function getDiagnostics(params: GetDiagnosticsParams): Diagnostic[] {
     const { textDocument } = params;
     const text = textDocument.getText();
+    const diagnostics: Diagnostic[] = [];
+
+    // Try to parse the expression to catch syntax errors
+    // The parser will throw ParseError for issues like:
+    // - Unknown characters
+    // - Unclosed strings
+    // - Illegal escape sequences
+    // - Unexpected tokens
+    // - Missing expected tokens (like closing brackets)
+    try {
+      parser.parse(text);
+    } catch (error) {
+      if (error instanceof ParseError) {
+        diagnostics.push(createDiagnosticFromParseError(textDocument, error));
+      } else if (error instanceof Error) {
+        // Handle generic errors thrown by the parser (e.g., invalid object definition)
+        diagnostics.push(createDiagnosticFromError(textDocument, error));
+      }
+    }
 
-    const ts = makeTokenStream(parser, text);
-    const spans = iterateTokens(ts);
+    // Try to tokenize for function argument checking
+    let spans: TokenSpan[] = [];
+    try {
+      const ts = makeTokenStream(parser, text);
+      spans = iterateTokens(ts);
+    } catch (error) {
+      // If tokenization fails, we already have a parse error diagnostic
+      // Return early since we can't do function argument checking without tokens
+      return diagnostics;
+    }
 
     // Build a map from function name to FunctionDetails for quick lookup
     const funcDetailsMap = new Map<string, FunctionDetails>();
     for (const func of allFunctions()) {
       funcDetailsMap.set(func.name, func);
     }
 
-    return getDiagnosticsForDocument(params, spans, functionNamesSet(), funcDetailsMap);
+    // Get function argument count diagnostics
+    const functionDiagnostics = getDiagnosticsForDocument(params, spans, functionNamesSet(), funcDetailsMap);
+    diagnostics.push(...functionDiagnostics);
+
+    return diagnostics;
   }
 
   return {

test/language-service/language-service.ts

@@ -829,5 +829,164 @@ describe('Language Service', () => {
       const diagnostics = ls.getDiagnostics({ textDocument: doc });
       expect(diagnostics).toEqual([]);
     });
+
+    // Extended diagnostics tests for syntax errors
+    // These tests verify that parser errors are properly converted to diagnostics
+    describe('syntax error diagnostics', () => {
+      it('should detect unclosed string literal with double quotes', () => {
+        const text = '"hello world';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+
+        expect(diagnostics.length).toBeGreaterThanOrEqual(1);
+        // Parser reports this as 'Unknown character' since unclosed string is not tokenized
+        const stringDiag = diagnostics.find(d => d.message.includes('Unknown character'));
+        expect(stringDiag).toBeDefined();
+        expect(stringDiag?.severity).toBe(DiagnosticSeverity.Error);
+      });
+
+      it('should detect unclosed string literal with single quotes', () => {
+        const text = "'hello world";
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+
+        expect(diagnostics.length).toBeGreaterThanOrEqual(1);
+        // Parser reports this as 'Unknown character' since unclosed string is not tokenized
+        const stringDiag = diagnostics.find(d => d.message.includes('Unknown character'));
+        expect(stringDiag).toBeDefined();
+      });
+
+      it('should detect unclosed parenthesis', () => {
+        const text = '(1 + 2';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+
+        expect(diagnostics.length).toBeGreaterThanOrEqual(1);
+        // Parser expects closing parenthesis
+        const parenDiag = diagnostics.find(d => d.message.includes('Expected )'));
+        expect(parenDiag).toBeDefined();
+      });
+
+      it('should detect unclosed bracket', () => {
+        const text = '[1, 2, 3';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+
+        expect(diagnostics.length).toBeGreaterThanOrEqual(1);
+        // Parser reports unexpected end of input
+        const bracketDiag = diagnostics.find(d => d.message.includes('Unexpected token'));
+        expect(bracketDiag).toBeDefined();
+      });
+
+      it('should detect unclosed brace', () => {
+        const text = '{a: 1, b: 2';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+
+        expect(diagnostics.length).toBeGreaterThanOrEqual(1);
+        // Parser reports invalid object definition
+        const braceDiag = diagnostics.find(d => d.message.includes('invalid object definition'));
+        expect(braceDiag).toBeDefined();
+      });
+
+      it('should detect unexpected closing parenthesis', () => {
+        const text = '1 + 2)';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+
+        expect(diagnostics.length).toBeGreaterThanOrEqual(1);
+        // Parser expects EOF but found )
+        const parenDiag = diagnostics.find(d => d.message.includes('Expected EOF'));
+        expect(parenDiag).toBeDefined();
+      });
+
+      it('should detect unexpected closing bracket', () => {
+        const text = '1 + 2]';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+
+        expect(diagnostics.length).toBeGreaterThanOrEqual(1);
+        // Parser expects EOF but found ]
+        const bracketDiag = diagnostics.find(d => d.message.includes('Expected EOF'));
+        expect(bracketDiag).toBeDefined();
+      });
+
+      it('should detect unexpected closing brace', () => {
+        const text = '1 + 2}';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+
+        expect(diagnostics.length).toBeGreaterThanOrEqual(1);
+        // Parser expects EOF but found }
+        const braceDiag = diagnostics.find(d => d.message.includes('Expected EOF'));
+        expect(braceDiag).toBeDefined();
+      });
+
+      it('should detect unclosed comment', () => {
+        const text = '1 + /* this is a comment 2';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+
+        expect(diagnostics.length).toBeGreaterThanOrEqual(1);
+        // Parser reports unexpected end of input
+        const commentDiag = diagnostics.find(d => d.message.includes('Unexpected token'));
+        expect(commentDiag).toBeDefined();
+      });
+
+      it('should detect unknown character', () => {
+        const text = '1 @ 2';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+
+        expect(diagnostics.length).toBeGreaterThanOrEqual(1);
+        const unknownCharDiag = diagnostics.find(d => d.message.includes('Unknown character'));
+        expect(unknownCharDiag).toBeDefined();
+      });
+
+      it('should not report errors for valid closed strings', () => {
+        const text = '"hello" + "world"';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+        expect(diagnostics).toEqual([]);
+      });
+
+      it('should not report errors for valid closed brackets', () => {
+        const text = '(1 + 2) * [3, 4][0]';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+        expect(diagnostics).toEqual([]);
+      });
+
+      it('should not report errors for valid closed comments', () => {
+        const text = '1 + /* comment */ 2';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+        expect(diagnostics).toEqual([]);
+      });
+
+      it('should handle nested brackets correctly', () => {
+        const text = '((1 + 2) * (3 + 4))';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+        expect(diagnostics).toEqual([]);
+      });
+
+      it('should handle escaped quotes in strings', () => {
+        const text = '"hello \\"world\\""';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+        expect(diagnostics).toEqual([]);
+      });
+
+      it('should detect syntax errors in complex expressions', () => {
+        const text = '(1 + "unclosed';
+        const doc = TextDocument.create('file://test', 'plaintext', 1, text);
+        const diagnostics = ls.getDiagnostics({ textDocument: doc });
+
+        // Parser will report the first error it encounters (unknown character for unclosed string)
+        expect(diagnostics.length).toBeGreaterThanOrEqual(1);
+        expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error);
+      });
+    });
   });
 });