clojure-emacs / inf-clojure

Basic interaction with a Clojure subprocess
250 stars 45 forks source link

Occasionally, the wrong REPL type is selected. #151

Open austinhaas opened 6 years ago

austinhaas commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

bbatsov commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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

sogaiu commented 5 years ago

Thanks for the explanation :)

charignon commented 5 years ago

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 commented 5 years ago

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.