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

Occasionally, the wrong REPL type is selected. #151

Open
austinhaas opened this issue Jul 9, 2018 · 9 comments
Open

Occasionally, the wrong REPL type is selected. #151

austinhaas opened this issue Jul 9, 2018 · 9 comments

Comments

@austinhaas
Copy link
Contributor

Occasionally, inf-clojure will select the wrong REPL type. The problem appears to start here at inf-clojure--detect-repl-type: https://github.com/clojure-emacs/inf-clojure/blob/master/inf-clojure.el#L297

I added debug messages around inf-clojure--process-reponse-match-p ( https://github.com/clojure-emacs/inf-clojure/blob/master/inf-clojure.el#L1277) and it looks like what is happening is that one of the calls to inf-clojure--process-response, to check if a certain namespace, like planck.repl, is available, will return a blank string, and then the corresponding type will be selected because it isn't nil.

The most obvious symptom of this issue is that the eldoc output will either be absent or show part of an error message. I added a message to print out the type as soon as it was selected, but you can also see it calling the wrong functions if you follow the instructions for logging process activity here: https://github.com/clojure-emacs/inf-clojure#log-process-activity.

To test this, I had to M-: (setq-local inf-clojure-repl-type nil) RET in the buffer that I was testing in order for inf-clojure to reattempt to determine the REPL type.

Initially, I thought I could reproduce the problem by evaluating the buffer after the REPL starts, but before triggering eldoc (by moving the cursor inside an expression), but I haven't been able to trigger the issue consistently (only about 3 out of 25 attempts).

@arichiardi
Copy link
Contributor

The repl detection part is not tested and I am not surprised this happens. Maybe at some point I had empty string detection but I removed it for ...reasons...

Character based interaction is not good and imho should be at some point deprecated in inf-clojure - only socket REPL and in the future pREPL are reliable and linear enough for us to spend time on it.

@arichiardi
Copy link
Contributor

Having said that, this is a bug so you caught it. Nice!

@bbatsov
Copy link
Member

bbatsov commented Jul 9, 2018

Character based interaction is not good and imho should be at some point deprecated in inf-clojure - only socket REPL and in the future pREPL are reliable and linear enough for us to spend time on it.

It's at the heart of comint, so I doubt we can ever deprecate it, but we should certainly promote the usage of the socket REPL (or pREPL).

@saikyun
Copy link

saikyun commented Sep 21, 2018

I've had some issues with various functions using inf-clojure with Arcadia, and one of them is inf-clojure--detect-repl-type, but what's worse is inf-clojure--process-response randomly returns an empty string instead of the result of the evaluation! When this occurs the result is instead printed in the repl. It makes it very hard to use inf-clojure--process-response meaningfully.

@sogaiu
Copy link

sogaiu commented Jan 14, 2019

Regarding inf-clojure-detect-repl-type, I found the following in inf-clojure.el:

(defvar-local inf-clojure-repl-type nil
  "Symbol to define your REPL type.
Its root binding is nil and it can be further customized using
either `setq-local` or an entry in `.dir-locals.el`." )

I wanted to try the setq-local approach and am currently trying the following in an initialization file:

(defun arcadia-set-repl-type ()
  (setq-local inf-clojure-repl-type 'clojure))

(add-hook 'inf-clojure-mode-hook #'arcadia-set-repl-type)

So far it seems to be working -- i.e. I haven't yet observed any attempts at detection via the sending of forms.

Does that seem appropriate?

@arichiardi
Copy link
Contributor

Yes that var will take precedence over the detection machinery so it is probably good to set it in case things go wrong.

@sogaiu
Copy link

sogaiu commented Jan 14, 2019

Thanks for the explanation :)

@charignon
Copy link
Contributor

Here is how to repro the issue:

lein new app testapp
# open testapp/src/core.clj with emacs
# start inf-clojure (which starts lein repl)
# start inf-clojure-minor-mode
# kill the *inf-clojure* buffer 
# disable inf-clojure-minor-mode
cd testapp && lumo -n 5555
# connect to the lumo repl with inf-clojure-connect 
# start inf-clojure-minor-mode

At this point the repl type is set to clojure instead of lumo and things break unless you forcibly force recomputation of the type (e.g. by evaluating (setq inf-clojure-repl-type nil)).

I think either of the following two approaches would solve the repro case I showed above:
1/ force recomputation of the repl type whenever we connect to a repl or start a new repl
2/ set inf-clojure-repl-type to nil when someone leaves the minor mode

What do you think @arichiardi ?

@arichiardi
Copy link
Contributor

Thanks for the repro. That var needs to be reset to nil when the "session" finishes. I would go for option number two in case the mode is disabled manually.

However, we might need option number one in any case if we want to manage "reconnecting".

It sounds like a different issue though. Not the original one in this open issue.

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

6 participants