Deprecate or change default setting for `cider-auto-mode`

yuhan0 commented 1 year ago

Context from #3343:

About cider-enable-on-all-existing-clojure-buffers, I'm unconvinced why this function exists at all? Cider users should already have it added to their clojure-mode-hook, otherwise it seems intrusive to automatically add it and force Cider on all other buffers, even ignoring the performance issues. I also found in my local copy of Cider that I had commented out cider-possibly-disable-on-existing-clojure-buffers a few years ago for some reason, I think due to unexpected behaviour on repl disconnect.

That's mostly for the benefit of people who are using both CIDER and inf-clojure, as them enabling them the minor modes in clojure-mode-hook would create some issues with conflicting functionality. So, it was basically some shortcut to prevent them from doing a bit of manual work. Not sure how many people rely on this - at any rate I'm not attached to this functionality at all and I'm open to rethinking/removing it.

The default t setting for cider-auto-mode causes performance and other issues detailed in the above issue, and only benefits a (presumably small) subset of Cider users with workflows involving inf-clojure and not adding cider-mode to their major mode hooks before until the first Cider-initiated connection.

I suggest the default config should at least be changed to nil, solving the downsides for most users. Users of inf-clojure can re-enable it in their own configs if needed. If the feature is deprecated or removed entirely, it is also relatively simple to implement in one's own config, adding custom functions to cider-connected-hook and cider-disconnected-hook.

Possible issues:

New and inexperienced users of Emacs/Cider might unknowingly depend on this behaviour, manually calling M-x cider-mode in a clojure buffer the first time they start Emacs, jacking in and then magically not having to do it in subsequent buffers. (Perhaps treating it like some sort of global minor mode)

I think the docs should encourage the usual Emacs convention of adding minor modes to a major mode hook, instead of supporting incorrect mindsets or habits which may cause future confusion for users.

https://docs.cider.mx/cider/config/basic_config.html#disable-automatic-cider-mode-in-clojure-mode-buffers

Environment & Version information

CIDER version information

;; CIDER 1.7.0 (Côte d'Azur), nREPL 1.0.0
;; Clojure 1.11.1, Java 20

Lein / Clojure CLI version

Clojure CLI version 1.11.1.1273

Emacs version

GNU Emacs 29.0.90

Operating system

macOS 12.5.1

JDK distribution

Temurin jdk-20.0.1+9

vemv commented 1 year ago

This sounds good to me. Anyway, could you please exhaustively list the problems caused by this var?

It's useful to have that info at hand.

bbatsov commented 1 year ago

cider-enable-on-existing-clojure-buffers after an initial connection, which reinitializes (cider-mode +1) on every Clojure-mode buffer in (buffer-list), including edn files, background diff-syntax buffers used to fontify diffs, and various other temp buffers. Among other things this forces recompilation of the font-lock-keywords in that buffer.

Btw, it might be a good idea to make this a bit smarter, even if it's no longer the default - e.g. to check if cider-mode is enabled, before trying to enable it. I'm guessing this will solve much of the described problem.

yuhan0 commented 1 year ago

could you please exhaustively list the problems caused by this var?

cider-enable-on-existing-clojure-buffers is run every time a REPL is connected.
- This re-initializes (cider-mode +1) on every clojure-mode buffer in the background, including edn files, background *diff-syntax* buffers used to fontify diffs, and various other temp buffers.
- Calling cider-mode involves recompiling the font-lock keywords, which when multiplied over all buffers causes a significant lag.
- Should otherwise be idempotent if Cider is already enabled, but other user-added functions in cider-mode-hook would also be triggered, maybe causing unintended effects, who knows
cider-possibly-disable-on-existing-clojure-buffers called when a REPL is disconnected
- If there are no remaining live connections it disables cider-mode on all clojure buffers (!)
- This definitely shouldn't be the correct behaviour - it means eg. that you lose all bindings from cider-mode-map in the source buffer, and can't do a C-c M-j to restart the connection that was just lost.
- Despite the naming this behaviour isn't actually gated by cider-auto-mode! Even if the above is intended it should at least be wrapped in a (when cider-auto-mode) block.
- (How has this never been reported before? I imagine that having to manually start cider-mode after a REPL disconnect is quite a noticeable bug, I seem to have commented out those lines years ago as a quick fix and then forgotten about it.)

bbatsov commented 1 year ago

(How has this never been reported before? I imagine that having to manually start cider-mode after a REPL disconnect is quite a noticeable bug, I seem to have commented out those lines years ago as a quick fix and then forgotten about it.)

That's a fair question. 😆 Oh, well... At the very least this should be guarded by cider-auto-mode as well - obviously it was meant to work with it and make sure that cider-mode won't stay around without active connections, so you could easily stop CIDER and switch to using inf-clojure or something else.

vemv commented 1 year ago

cider-possibly-disable-on-existing-clojure-buffers called when a REPL is disconnected

That function is called unconditionally, regardless of cider-auto-mode. Right?

cider-enable-on-existing-clojure-buffers is run every time a REPL is connected.

WDTY of https://github.com/clojure-emacs/cider/issues/3346#issuecomment-1555154970 ? I also feel that going that route (making things smarter here and there) would also be a sufficient, non-breaking change.

yuhan0 commented 1 year ago

Yeah, making the -enable hook smarter and gating the -disable hook behind cider-auto-mode would be the minimal non-breaking change.

I still think the default setting should still be nil for most users, to follow the principle of "least surprise"... the current behaviour is pretty unconventional in larger context of the Emacs ecosystem, even if the issues it causes are subtle / infrequent enough to be ignored.

Of course that's just my opinion and it's up to the maintainers to weigh the benefits of a breaking change ⚖️ Not too attached to this issue aside from reporting it, FWIW I'll be setting it to nil in my config and I'm sure the target users who regularly switch between Cider / inf-clojure can re-enable it in theirs if the defaults are ever changed.

vemv commented 1 year ago

In general we try to avoid breaking changes. We had https://github.com/clojure-emacs/cider/discussions/2960

From what I've seen we're not dogmatic about it, but the less they happen, the more we will reinforce good habits.

yuhan0 commented 1 year ago

For reference: my proposed change to the user manual in #3355, after changing the default value to nil:

-== Disable Automatic cider-mode in clojure-mode Buffers
+== Enable CIDER in clojure-mode buffers

-By default, CIDER enables `cider-mode` in all `clojure-mode` buffers
-after it establishes the first CIDER connection. It will also add a
-`clojure-mode` hook to enable `cider-mode` on newly-created `clojure-mode`
-buffers. You can override this behavior, however:
+The main entry point for CIDER is `cider-mode`, an Emacs https://www.gnu.org/software/emacs/manual/html_node/emacs/Minor-Modes.html[minor mode] 
+which enables its key bindings, dynamic syntax highlighting, and code navigation tools, among many other features.
+
+You'll most likely want to use it together with the major mode https://github.com/clojure-emacs/clojure-mode/[clojure-mode], by adding the following to your Emacs config:

 [source,lisp]
 ----
-(setq cider-auto-mode nil)
+(add-hook 'clojure-mode-hook #'cider-mode)
 ----

+This causes `cider-mode` to be automatically enabled on all `clojure-mode` buffers,
+as well as its derived modes like `clojurescript-mode`.
+
+TIP: Many Emacs starter kits like Spacemacs and Prelude come with this already pre-configured.
+You can also toggle CIDER in any buffer with the command kbd:[M-x cider-mode].
+
+NOTE: Previous versions of CIDER enabled the option `cider-auto-mode` by default,
+which automatically enabled `cider-mode` in all `clojure-mode` buffers after
+establishing the first CIDER connection, added itself to `clojure-mode-hook`,
+and disabled `cider-mode` on all buffers after the last connection was closed.
+
+The default setting for this option was changed in Cider 1.8, and we recommend
+that most users explicitly configure mode hooks per the above snippet.

https://docs.cider.mx/cider/config/basic_config.html#disable-automatic-cider-mode-in-clojure-mode-buffers

clojure-emacs / cider