Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix nrepl evaluating multi forms #604

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

curist
Copy link

@curist curist commented Jan 22, 2025

Please answer the following questions and leave the below in as part of your PR.

#603

  • This PR contains a test to prevent against future regressions

  • I have updated the CHANGELOG.md file with a description of the addressed issue.

@curist
Copy link
Author

curist commented Jan 22, 2025

I cheated and use claude to create a working patch. Here are what it has to say

The root cause of the bug was that the original implementation wasn't properly handling multiple forms in a single code string. Here's a breakdown:

Original problematic code:

(defn compile [the-val]
  (let [{js-str :javascript
         cljs-ns :ns
         :as new-state} (compiler/compile-string* the-val {...})]
    ;; Only handles compiling as one big string
    js-str))

(defn do-handle-eval [{:keys [ns code file] :as request} send-fn]
  (->
   (js/Promise.resolve code)
   (.then compile)  ;; <-- Problem: Tries to compile entire string at once
   (.then (fn [v]
            (js/eval v)))
   ...))

The issues were:

  1. No Form Parsing: The original code treated the entire input as one unit instead of parsing it into individual forms. When you send multiple forms like:

    (def a 1)
    (def b 2)
    (+ a b)

    It tried to compile and evaluate them as one chunk rather than separate expressions.

  2. Single Evaluation: The code only performed one evaluation and sent one result, even when there were multiple forms.

  3. Missing Sequential Processing: There was no mechanism to ensure forms were evaluated in order and that state changes from earlier forms (like defining variables) were available to later forms.

The fix addresses these by:

  1. Form Parsing: Added read-forms to properly parse individual forms:
(defn read-forms [code]
  (let [rdr (e/reader code)]
    (loop [forms []]
      (let [form (try 
                   (e/parse-next rdr compiler/squint-parse-opts)
                   ...)]
        ...))))
  1. Sequential Processing: Using reduce with Promises to ensure sequential evaluation:
(reduce (fn [promise form]
          (.then promise #(eval-form form send-fn request)))
        (js/Promise.resolve nil)
        forms)
  1. Individual Results: Sending results for each evaluation:
(defn eval-form [form send-fn request]
  (-> (js/Promise.resolve form)
      (.then compile-form)
      (.then (fn [val]
               (send-fn request 
                       {"ns" (str @last-ns)
                        "value" (format-value ...)})))
      ...))

@curist curist force-pushed the fix-nrepl-multi-form-eval branch from 819d59e to 829a42c Compare January 22, 2025 05:54
@curist
Copy link
Author

curist commented Jan 22, 2025

It doesn't seem to contain existing code to test nrepl implementation; I tried to add some provided by LLM, but it's not clear to me what's the expected/correct response from a nrepl server.

For example, below is a failing test case

request {:code "(def x 1)\n(def y 2)\n(+ x y)", :id "1", :op "eval", :session "8bd3f28e-5f01-43d4-a1d2-6a601f21a8ca"}

... redacted ...

expected: (= ["#'user/x" "#'user/y" "3"] (->> responses (filter :value) (mapv :value)))
  actual: (not (= ["#'user/x" "#'user/y" "3"] ["nil" "nil" "3"]))

I could still push the tests if that could provide some help.

@curist
Copy link
Author

curist commented Jan 22, 2025

On a second thought, I just made the tests to match current nrepl responses.

Some caveats:

  1. the test read .nrepl-port, so it could conflicts with existing nrepl connection.
  2. when done testing bb test:node, it currently hangs up there for no good reason :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant