clojure-emacs / cider

The Clojure Interactive Development Environment that Rocks for Emacs
https://cider.mx
GNU General Public License v3.0
3.54k stars 645 forks source link

Dynamic font-locking is broken #1889

Closed bbatsov closed 7 years ago

bbatsov commented 7 years ago

I've recently noticed this regression while I was doing a CIDER demo at a conference.

Expected behavior

Identifiers are font-locked dynamically when some bit of code is evaluated.

Actual behavior

Nothing really changes.

Steps to reproduce the problem

Just evaluate any Clojure namespace.

Environment & Version information

CIDER version information

;; CIDER 0.15.0snapshot (package: 20161127.714), nREPL 0.2.12
;; Clojure 1.8.0, Java 1.8.0_31

Emacs version

25.1

bbatsov commented 7 years ago

@Malabarba any idea what might be causing this? I don't think we've done any changes to the related logic in quite a while.

dpsutton commented 7 years ago

I'm reading into this issue and learning about font-locking in emacs. Can you post a screenshot of your example? Everything looks pretty good to me, but I think i'm seeing mainly static font locking mostly instead of the dynamic stuff.

bbatsov commented 7 years ago

@dpsutton See my slide deck here - https://speakerdeck.com/bbatsov/cider-inside-the-brewery (The section about 0.10 features).

dpsutton commented 7 years ago

So this is the function that does most of the work in cider-mode.el:

    (defun cider-refresh-dynamic-font-lock (&optional ns)
      "Ensure that the current buffer has up-to-date font-lock rules.
    NS defaults to `cider-current-ns', and it can also be a dict describing the
    namespace itself."
      (interactive)
      (when (and cider-font-lock-dynamically
                 font-lock-mode)
        (font-lock-remove-keywords nil cider--dynamic-font-lock-keywords)
        (when-let ((ns (or ns (cider-current-ns)))
                   (symbols (cider-resolve-ns-symbols ns)))
          (setq-local cider--dynamic-font-lock-keywords
                      (cider--compile-font-lock-keywords
                       symbols (cider-resolve-ns-symbols (cider-resolve-core-ns))))
          (font-lock-add-keywords nil cider--dynamic-font-lock-keywords 'end))
        (cider--font-lock-flush)))

And as far as I can tell, the cider-resolve-core-ns comes back with "clojure.core" but the cider-resolve-ns-symbols returns nil and then everything "nils" out from there. And the setq local binds nil so nothing happens from there. But it seems that cider--compile-font-lock-keywords can handle nil in this position for the core list, so i'm not sure why the chain breaks in this manner.

dpsutton commented 7 years ago

And this is certainly bad:

(let ((symbols (cider-resolve-ns-symbols "clojure.core.match")))
  (cider--compile-font-lock-keywords symbols nil))
nil
bbatsov commented 7 years ago

I'm not really familiar with this code, as @Malabarba wrote it and was maintaining it. I've been super busy lately, just noticed the functionality stopped working, which means we broke it by accident recently and opened the ticket hoping someone would have time investigate.

But yeah, this certainly looks bad...

dpsutton commented 7 years ago

Ok, so I'm starting to think that this is actually working. You can see that it is so here: Before jacking in: before_jack_in

and after jacking in: after_jack_in

The function fizzbuzz is dynamically font-locked. The reason it doesn't seem so is that the core functions are not (ie, mod in this case). And this is because they are missing from the cider-repl-ns-cache.

So if anyone is working on this, following along, making sure that "clojure.core" is populated in the ns-cache should make this work again.

bbatsov commented 7 years ago

The function fizzbuzz is dynamically font-locked. The reason it doesn't seem so is that the core functions are not (ie, mod in this case). And this is because they are missing from the cider-repl-ns-cache.

So, it's not really working. At least not completely. :-)

dpsutton commented 7 years ago

haha yeah. I just meant to point out that the mechanism is fine, we are just missing information. this info gets added in cider-nrepl so i'm looking through clojure code to find out why that's missing

bbatsov commented 7 years ago

Great! Hopefully you'll manage to sort this out.

dpsutton commented 7 years ago

So in cider-nrepl, we have the following line that should add clojure.core to the ns-cache:

(calculate-changed-ns-map (or old-data {'clojure.core clojure-core-map}))

While its tough to debug middleware, just adding println's, I'm seeing that old-data is never empty now, and begins life as

old data:
{user {:aliases {}, :interns {}}}

I'm wondering where this assumption changed? Has the loading of user happened earlier in the cycle than it used to? I don't even know where to start looking for that. But I think an easy fix would be to make sure that clojure is injected either way, but I'd like to know why this changed as well.

dpsutton commented 7 years ago

before_jack_in

after_jack_in

bbatsov commented 7 years ago

Great! 👍

Malabarba commented 7 years ago

Sorry I couldn't help here. I've been gradually getting more responsibilities at work, which has somewhat extinguished my time for OSS. 😢

Anyway, thanks for taking the time to debug and fix it @dpsutton.

Malabarba commented 7 years ago

On a lighter note, @dpsutton what's your color theme? I've been wanting to change mine for a while now.

dpsutton commented 7 years ago

@Malabarba this is 'brin from https://github.com/owainlewis/emacs-color-themes/blob/master/resources/previews/brin.png except that I've modified it a bit.

I spent some time on the highlight bar as well to make comments readable underneath it. In particular, this line (highlight ((t (:background "SteelBlue4")))) took quite a bit of tweaking :). I feel like the emacs community should celebrate Steel Blue 4 more.