From da2d1faca8293490b30f66f89eee9f59cf2337b5 Mon Sep 17 00:00:00 2001
From: Aaron Zuspan <50475791+aazuspan@users.noreply.github.com>
Date: Tue, 20 Aug 2024 18:19:10 -0700
Subject: [PATCH] Inconsistent diagnostics (#32)

* WIP Fix inconsistent diagnostics (needs tests)

Diagnostic positions were reported inconsistently based on newlines
after the error line. This was caused by updating the parser's
previous character every time the parser moved to a new line, rather
than leaving the previous character on the last parsed token. Because
asfv1 doesn't raise errors until it hits a non-whitespace token,
this meant that the previous character would be moved from the token
that caused the error to the whitespace that followed it.

Now, the previous character is only updated once at the beginning of
__next__, meaning that it stays on the last successfully parsed token.

* Add diagnostic test for error w/ and w/o newline
---
 src/spinasm_lsp/parser.py              | 14 ++++++++++----
 tests/server_tests/test_diagnostics.py | 19 +++++++++++++++++--
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/src/spinasm_lsp/parser.py b/src/spinasm_lsp/parser.py
index 883b92d..2540b94 100644
--- a/src/spinasm_lsp/parser.py
+++ b/src/spinasm_lsp/parser.py
@@ -30,8 +30,9 @@ def sline(self, value):
         """Update the current line and reset the column."""
         self._sline = value
 
-        # Reset the column to 0 when we move to a new line
-        self._previous_character = self._current_character
+        # Reset the column to 0 when we move to a new line. Note that we do NOT update
+        # the previous character here, as that will be handled when the next token is
+        # parsed.
         self._current_character = 0
 
     @property
@@ -51,6 +52,9 @@ def parsed_symbol(self) -> ASFV1Token:
 
     def __next__(self) -> None:
         """Parse the next token and update the current character and line."""
+        # Store the current character before advancing to the next token.
+        self._previous_character = self._current_character
+
         super().__next__()
 
         # Don't advance position on EOF token, since we're done parsing
@@ -60,7 +64,6 @@ def __next__(self) -> None:
         current_line_txt = self._source[self._current_line]
         current_symbol = self.parsed_symbol.txt
 
-        self._previous_character = self._current_character
         # Start at the current column to skip previous duplicates of the symbol
         self._current_character = current_line_txt.index(
             current_symbol, self._current_character
@@ -99,11 +102,14 @@ def parseerror(self, msg: str, line: int | None = None):
         """Override to record parsing errors as LSP diagnostics."""
         if line is None:
             line = self.prevline
+            character = self._previous_character
+        else:
+            character = self._current_character
 
         # Offset the line from the parser's 1-indexed line to the 0-indexed line
         self._record_diagnostic(
             msg,
-            position=lsp.Position(line=line - 1, character=self._current_character),
+            position=lsp.Position(line=line - 1, character=character),
             severity=lsp.DiagnosticSeverity.Error,
         )
 
diff --git a/tests/server_tests/test_diagnostics.py b/tests/server_tests/test_diagnostics.py
index af8b3ff..0e03e9a 100644
--- a/tests/server_tests/test_diagnostics.py
+++ b/tests/server_tests/test_diagnostics.py
@@ -54,8 +54,23 @@ class DiagnosticTestCase(TestCase):
         expected=[
             lsp.Diagnostic(
                 range=lsp.Range(
-                    start=lsp.Position(line=0, character=0),
-                    end=lsp.Position(line=0, character=0),
+                    start=lsp.Position(line=0, character=5),
+                    end=lsp.Position(line=0, character=5),
+                ),
+                message="Register 0x64 out of range for MULX",
+                severity=lsp.DiagnosticSeverity.Error,
+                source="SPINAsm",
+            ),
+        ],
+    ),
+    DiagnosticTestCase(
+        name="out of range (without newline)",
+        source="""MULX 100""",
+        expected=[
+            lsp.Diagnostic(
+                range=lsp.Range(
+                    start=lsp.Position(line=0, character=5),
+                    end=lsp.Position(line=0, character=5),
                 ),
                 message="Register 0x64 out of range for MULX",
                 severity=lsp.DiagnosticSeverity.Error,