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

cider-repl-set-ns has become artificial #909

Closed
johnbendi opened this issue Dec 4, 2014 · 16 comments
Closed

cider-repl-set-ns has become artificial #909

johnbendi opened this issue Dec 4, 2014 · 16 comments

Comments

@johnbendi
Copy link

C-c M-n temporarily sets ns which gets reset to cljs.user after evaling an expression.

foo.bar.ns > (+ 1)
1
cljs.user >

On closer look. My suspicion seems confirmed:

(defun cider-repl-set-ns (ns)
  "Switch the namespace of the REPL buffer to NS.

If invoked in a REPL buffer the command will prompt you for the name of the
namespace to switch to."
  (interactive (list (if (or (derived-mode-p 'cider-repl-mode)
                             (null (cider-ns-form)))
                         (completing-read "Switch to namespace: "
                                          (cider-sync-request:ns-list))
                       (cider-current-ns))))
  (if ns
      (with-current-buffer (cider-current-repl-buffer)
        (setq nrepl-buffer-ns ns)
        (cider-repl-emit-prompt (current-buffer)))
    (error "Cannot determine the current namespace")))

Also calling in-ns from the repl gets the job done. So somewhere along the line something like in-ns doesn't get called.

ielm => (cider-version)
"CIDER 0.8.2snapshot (package: 20141130.803)"

Regards.

@johnbendi
Copy link
Author

I just noticed this is happening only at the cljs repl.

@danskarda
Copy link
Contributor

I guess it is related to #830 - more general problems with CLJS evaluation. CIDER is making assumptions about properties of eval and load-file operations, which are true for Clojure but not true for Piggieback.

/cc @bbatsov @cemerick

@bbatsov
Copy link
Member

bbatsov commented Dec 5, 2014

@danskarda This command doesn't use eval, so I don't see the connection to #830.

@mitchelkuijpers
Copy link

I have the same bug with CIDER 0.8.2

@bbatsov
Copy link
Member

bbatsov commented Dec 8, 2014

Someone suggested that aeb125e might have caused this, but the REPL ns is associated with REPL buffer and doing in-ns should not actually be necessary. Someone should send a bit of nREPL messages log related to this problem.

@johnbendi
Copy link
Author

In a cljs situation it may come to pass that the server needs to send the client unsolicited messages that the developer has instructed it to do. What does the client do then when the message looks like this:

(<-
  id  "21"
  ns  "cljs.user"
  session  "db49c245-4481-460f-b80d-9f11c5e7eda1"
  value  "red alert!"
)
(<-
  id  "21"
  session  "db49c245-4481-460f-b80d-9f11c5e7eda1"
  status  ("done")
)

when the ns was previously at foo.bar.ns?

This makes me think that we can't help but resort to calling the real in-ns as against the artificial nrepl-set-ns mantra.

@johnbendi
Copy link
Author

To be clear, these messages originate within the same session as this transcript shows:

(--->
  ns  "foo.crm.start"
  op  "eval"
  session  "db49c245-4481-460f-b80d-9f11c5e7eda1"
  code  "(+ 1)\n"
  id  "22"
)
(<-
  id  "14"
  out  "Handling result...\n"
  session  "db49c245-4481-460f-b80d-9f11c5e7eda1"
)
(<-
  id  "22"
  ns  "cljs.user"
  session  "db49c245-4481-460f-b80d-9f11c5e7eda1"
  value  "1"
)
(<-
  id  "22"
  session  "db49c245-4481-460f-b80d-9f11c5e7eda1"
  status  ("done")
)

Actually I don't even think the unsolicited messages matter. The returned result simply insists on using the ns it knows about.

@bbatsov
Copy link
Member

bbatsov commented Jan 10, 2015

While I can add a workaround for this I strongly feel this is a piggieback bug. It seems pretty strange that the request and the response have different ns values (which is not consistent with the Clojure behaviour). Guess @cemerick should join this discussion.

@cemerick
Copy link
Contributor

If (setq nrepl-buffer-ns ns) doesn't issue in-ns (I presume it doesn't, unless there's some kind of hooking mechanism in elisp that would trigger sending that message), then I'm not sure how it could work. The only thing that really matters is *ns* in the remote nREPL session.

I think the thought behind aeb125e was that all REPL evaluations can be sent with an ns argument, so setting the remote *ns* doesn't matter. This is not the case, as nREPL messages are evaluated asynchronously; if you ever have results streaming back over time, those will likely carry ns values different than the ns you most recently evaluated with, which most clients will use to change the prompt, etc. So again, tl;dr: the only thing that really matters is *ns* in the remote nREPL session.

This and nrepl/weasel#26 popped up in my notifications at the same time. FWIW, I'm using piggieback 0.1.5-SNAPSHOT with cider 0.6.0alpha and CLJS 2665 and cider-repl-set-ns works as expected.

@bbatsov
Copy link
Member

bbatsov commented Jan 10, 2015

@cemerick, thanks for the quick response.

I think the thought behind aeb125e was that all REPL evaluations can be sent with an ns argument, so setting the remote ns doesn't matter.

Indeed, it was.

This is not the case, as nREPL messages are evaluated asynchronously; if you ever have results streaming back over time, those will likely carry ns values different than the ns you most recently evaluated with, which most clients will use to change the prompt, etc.

I understand what you're saying, although in CIDER this is unlikely to ever the a problem. This change was done half an year ago and nobody has complained about problems caused by it. For me the actual problem is that piggieback's behaviour differs from the standard nREPL behaviour - piggieback returns *ns* in eval's responses, as the standard eval returns the ns that was submitted in the request.

@cemerick
Copy link
Contributor

I'm mobile a.t.m., so I've bit looked at the code, but I'm not sure that nrepl isn't in the wrong here, if it's really returning the ns argument instead of *ns*. At the very least, what should happen is not well defined, even in my head. IIRC, the argument was introduced originally to enable easy evaluate of e.g. top levels from namespaces other than *ns*. If the user explicitly indicates a *ns* change, then that should definitely be effected in their session.

@bbatsov
Copy link
Member

bbatsov commented Jan 10, 2015

If the user explicitly indicates a ns change, then that should definitely be effected in their session.

Forms like in-ns do result in a different ns in the eval response:

(--->
  ns  "user"
  op  "eval"
  session  "11d51e64-112a-42b0-b9f3-3ed13f7e47bb"
  code  "(in-ns 'test)\n"
  id  "16"
)
(<-
  id  "16"
  ns  "test"
  session  "11d51e64-112a-42b0-b9f3-3ed13f7e47bb"
  value  "#<Namespace test>"
)
(<-
  id  "16"
  session  "11d51e64-112a-42b0-b9f3-3ed13f7e47bb"
  status  ("done")
)

Other forms, however, return the ns of the request:

(--->
  ns  "cider.nrepl"
  op  "eval"
  session  "11d51e64-112a-42b0-b9f3-3ed13f7e47bb"
  code  "(+ 1 2)\n"
  id  "19"
)
(<-
  id  "19"
  ns  "cider.nrepl"
  session  "11d51e64-112a-42b0-b9f3-3ed13f7e47bb"
  value  "3"
)
(<-
  id  "19"
  session  "11d51e64-112a-42b0-b9f3-3ed13f7e47bb"
  status  ("done")
)

In the previous example no ns/in-ns was executed. The ns was set internally for CIDER's REPL buffer only, but still it got returned in the response.

Perhaps the eval requests are altering the *ns*? My REPL tests seem to indicate so.

@cemerick
Copy link
Contributor

The nREPL test related to the argument doesn't check what *ns* such an evaluation; meanwhile, piggieback does include an explicit test, which ensures that the ns argument doesn't affect *ns*.

Combined with the fact that IMO, the ns argument loses its utility given the Clojure side's current behaviour (it's really just functioning as a convenience to avoid (in-ns), and doesn't allow for what I remember to be the original use case [evaluating forms from arbitrary namespaces without affecting *ns*), I think this is an nREPL bug, not a piggieback bug. (Or, at the very least, some undefined but undesirable behaviour on the part of nREPL, Clojure-side.)

@bbatsov
Copy link
Member

bbatsov commented Jan 10, 2015

I'm fine with filing this as an nREPL bug and making it to behave like piggieback.

@bbatsov
Copy link
Member

bbatsov commented Jan 11, 2015

@cemerick Filed a nREPL ticket a reverted to the old behaviour in CIDER. All related tickets can now be closed.

@cemerick
Copy link
Contributor

Yup, thanks @bbatsov. People can follow http://dev.clojure.org/jira/browse/NREPL-72.

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

No branches or pull requests

5 participants