Pass nREPL client evaled code's line and col nums to the reader (#1038)

Hi,

could you please consider patch to pass the nREPL client's eval file
line and column numbers down to the reader for more accurate compiler
exception reporting. It fixes #1037.

I have attempted a focused approach to propagate the line and column
parameters to the error reporting mechanism. If this approach is not
optimal, I am open to reorganizing the code based on better suggestions.

I've included tests down the affected path to cover these changes.

Thanks

---------

Co-authored-by: ikappaki <[email protected]>

Author: ikappaki

CHANGELOG.md

@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
  * Fix a bug where `defrecord`/`deftype` constructors could not be used in the type's methods. (#1025)
  * Fix a bug where `keys` and `vals` would fail for records (#1030)
  * Fix a bug where operations on records created by `defrecord` failed for fields whose Python-safe names were mangled by the Python compiler (#1029)
+ * Fix incorrect line numbers for compiler exceptions in nREPL when evaluating forms in loaded files (#1037)
 
 ## [v0.2.1]
 ### Changed

src/basilisp/contrib/nrepl_server.lpy

@@ -106,8 +106,11 @@
                       "ns"     ns})))
 
 (defn- do-handle-eval
-  "Evaluate the ``request`` ``code`` of ``file`` in the ``ns`` namespace according
-  to the current state of the ``client*`` and sends its result with ``send-fn``.
+  "Evaluate the ``request`` ``code`` of ``file`` in the ``ns`` namespace
+  according to the current state of the ``client*`` and sends its
+  result with ``send-fn``. If ``line`` and/or ``column`` are provided,
+  they indicate the line and column numbers withing the ``file`` where
+  ``code`` is located.
 
   The result sent is either the last evaluated value or exception, followed by
   the \"done\" status.
@@ -119,7 +122,7 @@
   It binds the ``*1``, ``*2``, ``*3`` and ``*e`` variables for evaluation from
   the corresponding ones found in ``client*``, and updates the latter according
   to the result."
-  [{:keys [client* code ns file _column _line] :as request} send-fn]
+  [{:keys [client* code ns file column line] :as request} send-fn]
   (let [{:keys [*1 *2 *3 *e eval-ns]} @client*
         out-stream                    (StreamOutFn #(send-fn request {"out" %}))
         ctx                           (basilisp.lang.compiler.CompilerContext. (or file "<nREPL Input>"))
@@ -134,7 +137,10 @@
               *e    *e]
       (try
         (let [result (last
-                      (for [form (read-seq (io/StringIO code))]
+                      (for [form (read-seq (cond-> {}
+                                             line   (assoc :init-line   line)
+                                             column (assoc :init-column column))
+                                           (io/StringIO code))]
                         (basilisp.lang.compiler/compile-and-exec-form form
                                                                       ctx
                                                                       *ns*)))]

src/basilisp/core.lpy

@@ -4474,7 +4474,16 @@
   The stream must satisfy the interface of :external:py:class:`io.TextIOBase`\\, but
   does not require any pushback capabilities. The default
   ``basilisp.lang.reader.StreamReader`` can wrap any object implementing ``TextIOBase``
-  and provide pushback capabilities."
+  and provide pushback capabilities.
+
+  ``opts`` may include, among other things, the following keys:
+
+  :keyword ``:init-line``:  optional initial line number for reader metadata;
+      helpful for contextualizing errors for chunks of code read from a
+      larger source document.
+  :keyword ``:init-column``: optional initial column number for reader metadata;
+      helpful for contextualizing errors for chunks of code read from a
+      larger source document."
   ([stream]
    (read-seq {} stream))
   ([opts stream]
@@ -4486,7 +4495,10 @@
                  (= (:eof opts) :eofthrow)
                  (:features opts)
                  (not= :preserve (:read-cond opts))
-                 *default-data-reader-fn*)))))
+                 *default-data-reader-fn*
+                 **
+                 :init-line (:init-line opts)
+                 :init-column (:init-column opts))))))
 
 (defn read-string
   "Read a string of Basilisp code.

src/basilisp/lang/reader.py

@@ -223,12 +223,23 @@ class StreamReader:
 
     __slots__ = ("_stream", "_pushback_depth", "_idx", "_buffer", "_line", "_col")
 
-    def __init__(self, stream: io.TextIOBase, pushback_depth: int = 5) -> None:
+    def __init__(
+        self,
+        stream: io.TextIOBase,
+        pushback_depth: int = 5,
+        init_line: Optional[int] = None,
+        init_column: Optional[int] = None,
+    ) -> None:
+        """`init_line` and `init_column` refer to where the `stream`
+        starts in the broader context, defaulting to 1 and 0
+        respectively if not provided."""
+        init_line = init_line if init_line is not None else 1
+        init_column = init_column if init_column is not None else 0
         self._stream = stream
         self._pushback_depth = pushback_depth
         self._idx = -2
-        self._line = collections.deque([1], pushback_depth)
-        self._col = collections.deque([0], pushback_depth)
+        self._line = collections.deque([init_line], pushback_depth)
+        self._col = collections.deque([init_column], pushback_depth)
         self._buffer = collections.deque([self._stream.read(1)], pushback_depth)
 
         # Load up an extra character
@@ -1696,9 +1707,15 @@ def read(  # pylint: disable=too-many-arguments
     features: Optional[IPersistentSet[kw.Keyword]] = None,
     process_reader_cond: bool = True,
     default_data_reader_fn: Optional[DefaultDataReaderFn] = None,
+    init_line: Optional[int] = None,
+    init_column: Optional[int] = None,
 ) -> Iterable[RawReaderForm]:
     """Read the contents of a stream as a Lisp expression.
 
+    The optional `init_line` and `init_column` specify where the
+    `stream` location metadata starts in the broader context, if not
+    from the start.
+
     Callers may optionally specify a namespace resolver, which will be used
     to adjudicate the fully-qualified name of symbols appearing inside of
     a syntax quote.
@@ -1717,7 +1734,7 @@ def read(  # pylint: disable=too-many-arguments
     are provided.
 
     The caller is responsible for closing the input stream."""
-    reader = StreamReader(stream)
+    reader = StreamReader(stream, init_line=init_line, init_column=init_column)
     ctx = ReaderContext(
         reader,
         resolver=resolver,
@@ -1752,9 +1769,15 @@ def read_str(  # pylint: disable=too-many-arguments
     features: Optional[IPersistentSet[kw.Keyword]] = None,
     process_reader_cond: bool = True,
     default_data_reader_fn: Optional[DefaultDataReaderFn] = None,
+    init_line: Optional[int] = None,
+    init_column: Optional[int] = None,
 ) -> Iterable[RawReaderForm]:
     """Read the contents of a string as a Lisp expression.
 
+    The optional `init_line` and `init_column` specify where the
+    `stream` location metadata starts in the broader context, if not
+    from the beginning.
+
     Keyword arguments to this function have the same meanings as those of
     basilisp.lang.reader.read."""
     with io.StringIO(s) as buf:
@@ -1767,6 +1790,8 @@ def read_str(  # pylint: disable=too-many-arguments
             features=features,
             process_reader_cond=process_reader_cond,
             default_data_reader_fn=default_data_reader_fn,
+            init_line=init_line,
+            init_column=init_column,
         )
 
 

tests/basilisp/contrib/nrepl_server_test.lpy

@@ -5,14 +5,17 @@
    [basilisp.contrib.nrepl-server :as nr]
    [basilisp.set :as set]
    [basilisp.string :as str :refer [starts-with?]]
-   [basilisp.test :refer [deftest are is testing]])
+   [basilisp.test :refer [deftest are is testing use-fixtures]]
+   [basilisp.test.fixtures :as fixtures :refer [*tempdir*]])
   (:import
    os
    socket
    tempfile
    threading
    time))
 
+(use-fixtures :each fixtures/tempdir)
+
 (def ^:dynamic *nrepl-port*
   "The port the :lpy:py:`with-server` is bound to."
   nil)
@@ -518,16 +521,29 @@
 
 (deftest nrepl-server-load-file
   (testing "basic"
+
     (with-server {}
       (with-connect client
         (let [id* (atom 0)
-              id-inc! #(swap! id* inc)]
+              id-inc! #(swap! id* inc)
+              filename (str (bio/path *tempdir* "load-file-test.lpy"))]
           (client-send! client {:id (id-inc!) :op "clone"})
           (let [{:keys [status]} (client-recv! client)]
             (is (= ["done"] status)))
+
+          (spit filename "(ns abc.xyz (:require [clojure.string :as str])
+(:import [sys :as s]))
+(defn afn []
+  (str/lower-case \"ABC\"))
+(comment
+  (abc)
+  (xyz))
+
+(afn)")
+
           (client-send! client {:id (id-inc!) :op "load-file"
-                                :ns "user" :file "(ns abc.xyz (:require [clojure.string :as str]) (:import [sys :as s])) (defn afn []  (str/lower-case \"ABC\")) (afn)"
-                                :file-name "xyz.lpy" :file-path "/abc/xyz.lpy"})
+                                :ns "user" :file (slurp filename)
+                                :file-name "xyz.lpy" :file-path filename})
           (are [response] (= response (client-recv! client))
             {:id @id* :ns "abc.xyz" :value "\"abc\""}
             {:id @id* :ns "abc.xyz" :status ["done"]})
@@ -559,6 +575,40 @@
             {:id @id* :ns "abc.other" :value "\"abc\""}
             {:id @id* :ns "abc.other" :status ["done"]})
 
+          (client-send! client {:id (id-inc!) :ns "abc.xyz" :op "eval" :code "(abc)"
+                                :file filename
+                                :line 6 :column 2})
+          (let [{:keys [id err]} (client-recv! client)]
+            (is (= @id* id))
+            (is (= [""
+                    "  exception: <class 'basilisp.lang.compiler.exception.CompilerException'>"
+                    "      phase: :analyzing"
+                    "    message: unable to resolve symbol 'abc' in this context"
+                    "       form: abc"
+                    (str "   location: " filename ":6")
+                    "    context:"
+                    ""
+                    " 2   | (:import [sys :as s]))"
+                    " 3   | (defn afn []"
+                    " 4   |   (str/lower-case \"ABC\"))"
+                    " 5   | (comment"
+                    " 6 > |   (abc)"
+                    " 7   |   (xyz))"
+                    " 8   | "
+                    " 9   | (afn)"]
+                   (-> err
+                       ;; remove ansi control chars
+                       (str/replace #"\\x1B[@-_][0-?]*[ -/]*[@-~]" "")
+                       (str/split-lines)))))
+          (let [{:keys [id ex status ns]} (client-recv! client)]
+            (is (= @id* id))
+            (is (= "abc.xyz" ns))
+            (is (= ["eval-error"] status))
+            (is (str/starts-with? ex "Traceback (most recent call last):")))
+          (are [response] (= response (client-recv! client))
+            {:id @id* :ns "abc.xyz" :status ["done"]})
+
+
           (client-send! client {:id (id-inc!) :op "load-file" :ns "user" :file "(ns abc.third)\n\n(/ 3 0)"
                                 :file-name "third.lpy" :file-path "/abc/third.lpy"})
           (let [{:keys [id err]} (client-recv! client)]
@@ -571,18 +621,18 @@
             (is (not= -1 (.find ex "File \"/abc/third.lpy\", line 3")) ex)
             (is (str/starts-with? ex "Traceback (most recent call last):")))
           (are [response] (= response (client-recv! client))
-            {:id @id* :ns "abc.third" :status ["done"]}))))
+            {:id @id* :ns "abc.third" :status ["done"]})))))
 
-    (testing "no file"
-      (with-server {}
-        (with-connect client
-          (client-send! client {:id 1 :op "clone"})
-          (let [{:keys [status]} (client-recv! client)]
-            (is (= ["done"] status)))
-          (client-send! client {:id 2 :op "load-file" :ns "user"})
-          (are [response] (= response (client-recv! client))
-            {:id 2 :ns "user" :value "nil"}
-            {:id 2 :ns "user" :status ["done"]}))))))
+  (testing "no file"
+    (with-server {}
+      (with-connect client
+        (client-send! client {:id 1 :op "clone"})
+        (let [{:keys [status]} (client-recv! client)]
+          (is (= ["done"] status)))
+        (client-send! client {:id 2 :op "load-file" :ns "user"})
+        (are [response] (= response (client-recv! client))
+          {:id 2 :ns "user" :value "nil"}
+          {:id 2 :ns "user" :status ["done"]})))))
 
 (deftest nrepl-server-params
   (testing "buffer size"

tests/basilisp/reader_test.py

@@ -134,6 +134,40 @@ def test_stream_reader_loc():
     assert (3, 1) == sreader.loc
 
 
+def test_stream_reader_loc_other():
+    s = str("i=1\n" "b=2\n" "i")
+    sreader = reader.StreamReader(io.StringIO(s), init_line=5, init_column=3)
+    assert (5, 3) == sreader.loc
+
+    assert "i" == sreader.peek()
+    assert (5, 3) == sreader.loc
+
+    assert "=" == sreader.next_char()
+    assert (5, 4) == sreader.loc
+
+    assert "=" == sreader.peek()
+    assert (5, 4) == sreader.loc
+
+    sreader.pushback()
+    assert "i" == sreader.peek()
+    assert (5, 3) == sreader.loc
+
+    assert "=" == sreader.next_char()
+    assert (5, 4) == sreader.loc
+
+    assert "1" == sreader.next_char()
+    assert (5, 5) == sreader.loc
+
+    assert "\n" == sreader.next_char()
+    assert (5, 6) == sreader.loc
+
+    assert "b" == sreader.next_char()
+    assert (6, 0) == sreader.loc
+
+    assert "=" == sreader.next_char()
+    assert (6, 1) == sreader.loc
+
+
 class TestReaderLines:
     def test_reader_lines_from_str(self, tmp_path):
         _, _, l = list(reader.read_str("1\n2\n(/ 5 0)"))
@@ -145,6 +179,25 @@ def test_reader_lines_from_str(self, tmp_path):
             l.meta.get(reader.READER_END_COL_KW),
         )
 
+    def test_reader_lines_from_str_other_loc(self, tmp_path):
+        l1, _, l3 = list(
+            reader.read_str("(+ 1 2)\n2\n(/ 5 0)", init_line=6, init_column=7)
+        )
+
+        assert (6, 6, 7, 14) == (
+            l1.meta.get(reader.READER_LINE_KW),
+            l1.meta.get(reader.READER_END_LINE_KW),
+            l1.meta.get(reader.READER_COL_KW),
+            l1.meta.get(reader.READER_END_COL_KW),
+        )
+
+        assert (8, 8, 0, 7) == (
+            l3.meta.get(reader.READER_LINE_KW),
+            l3.meta.get(reader.READER_END_LINE_KW),
+            l3.meta.get(reader.READER_COL_KW),
+            l3.meta.get(reader.READER_END_COL_KW),
+        )
+
     def test_reader_lines_from_file(self, tmp_path):
         filename = tmp_path / "test.lpy"
 

tests/basilisp/test_core_fns.lpy

@@ -2438,6 +2438,45 @@
       (is (= "Thu Jan  1 00:00:01 1970" (eval '(time-alias/asctime (time-alias/gmtime 1))))))))
 
 
+;;;;;;;;;;
+;; Read ;;
+;;;;;;;;;;
+
+(deftest read-seq-test
+
+  (testing "read-seq plain"
+    (with [stream (io/StringIO "(+ 1 2)\n(+ 4 \n5)")]
+          (let [[l1 l2] (read-seq stream)
+                {l1-l  :basilisp.lang.reader/line
+                 l1-el :basilisp.lang.reader/end-line
+                 l1-c  :basilisp.lang.reader/col
+                 l1-ec :basilisp.lang.reader/end-col} (meta l1)
+                {l2-l  :basilisp.lang.reader/line
+                 l2-el :basilisp.lang.reader/end-line
+                 l2-c  :basilisp.lang.reader/col
+                 l2-ec :basilisp.lang.reader/end-col} (meta l2)]
+            (is (= '(+ 1 2) l1))
+            (is (= '(+ 4 5) l2))
+            (is (= [1 1 0 7] [l1-l l1-el l1-c l1-ec]))
+            (is (= [2 3 0 2] [l2-l l2-el l2-c l2-ec])))))
+
+  (testing "read-seq with specified line and col numbers"
+      (with [stream (io/StringIO "(+ 1 2)\n(+ 4 \n5)")]
+            (let [[l1 l2] (read-seq {:init-line 10 :init-column 15} stream)
+                  {l1-l  :basilisp.lang.reader/line
+                   l1-el :basilisp.lang.reader/end-line
+                   l1-c  :basilisp.lang.reader/col
+                   l1-ec :basilisp.lang.reader/end-col} (meta l1)
+                  {l2-l  :basilisp.lang.reader/line
+                   l2-el :basilisp.lang.reader/end-line
+                   l2-c  :basilisp.lang.reader/col
+                   l2-ec :basilisp.lang.reader/end-col} (meta l2)]
+              (is (= '(+ 1 2) l1))
+              (is (= '(+ 4 5) l2))
+              (is (= [10 10 15 22] [l1-l l1-el l1-c l1-ec]))
+              (is (= [11 12 0 2] [l2-l l2-el l2-c l2-ec]))))))
+
+
 ;;;;;;;;;;
 ;; Load ;;
 ;;;;;;;;;;