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 is not font-locking required vars #1985

Open
dpsutton opened this Issue May 7, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@dpsutton
Contributor

dpsutton commented May 7, 2017

Expected behavior

CIDER font locks vars and functions from the current namespace and clojure.core functions and vars. It should also font-lock imported vars from your own namespace. But it does not.

Actual behavior

It fails to font-lock functions required from other namespaces.
font-lock-bug

Note that (i/bar x) is not font-locked.

This behavior is controlled by cider--dynamic-font-lock-keywords

;; describe-var:
;; cider--dynamic-font-lock-keywords is a variable defined in ‘cider-mode.el’.
;; Its value is
;; (("\\_<\\(foo\\|i/bar\\|uses-import\\)\\_>" 0
;;   (cider--unless-local-match font-lock-function-name-face)))
;; Local in buffer font_lock_bug/core.clj; global value is nil

Steps to reproduce the problem

Minimum example is found at https://github.com/dpsutton/font-lock-bug

But make your namespace as small as possible and import a namespace with only a single function in it. I also changed this so it wasn't adding clojure.core, just for making the regex easier to watch. This function is in cider-refresh-dynamic-font-lock in cider-mode.el. and just pass in '() instead of the core namespace.

Environment & Version information

CIDER version information

latest snapshot

Lein/Boot version

a good lein

Emacs version

tip of master

Operating system

fedora 25

@dpsutton

This comment has been minimized.

Contributor

dpsutton commented May 8, 2017

Interestingly, if you change the :require imports :as i to :require imports :refer bar then bar is correct highlighted. I'm wondering if this is some kind of escaping issue with the / in the regex.

Otherwise, as far as I can tell, there are two things that control font-locking: this regex and if something is marked to not be font-locked with some substring properties, either ns forms or local vars. Everything seems like it should work so it must be just the slash escaping.

And then when i change the separator from "/" to "-" in cider-resolve-ns-symbols, I can get it to font lock i-bar.
font-lock-with-dash

@bbatsov

This comment has been minimized.

Member

bbatsov commented May 8, 2017

That's odd. I don't recall any changes to this code after @Malabarba wrote it originally. But your assumption sounds correctly - likely a regexp issue.

@xiongtx

This comment has been minimized.

Member

xiongtx commented Jul 22, 2017

@dpsutton Any update here?

@dpsutton

This comment has been minimized.

Contributor

dpsutton commented Jul 23, 2017

The best that I can find here is that this is a bug in emacs font locking with the / character. I can raise it on the emacs mailing list to proceed.

@oskarkv

This comment has been minimized.

Contributor

oskarkv commented Sep 17, 2017

I played around with this today, because I really want it to work. It seems to be caused by clojure-mode, which font locks the namespace part. https://github.com/clojure-emacs/clojure-mode/blob/master/clojure-mode.el#L840

;; foo/ Foo/ @Foo/ /FooBar
(,(concat "\\(?:\\<:?\\|\\.\\)@?\\(" clojure--sym-regexp "\\)\\(/\\)")
 (1 font-lock-type-face) (2 'default))

If I comment that out, it starts working. But I don't know where to go from here to produce a good solution. I hardly know anything about font locking.

By the way, things defined with (def thing a-function) gets the wrong color (like a non-function var, instead of a function). Is there an issue for that already? Couldn't find one.

@dpsutton

This comment has been minimized.

Contributor

dpsutton commented Sep 17, 2017

@oskarkv I looked into this bug a few months ago. I don't know too much about font-locking either but I wrote down some of my findings. I think you've made some real insight there and I hope this can help you progress.

@oskarkv

This comment has been minimized.

Contributor

oskarkv commented Sep 19, 2017

Fixing this is kind of annoying, I think. The problem stems from the fact that the font locking is essentially complex. Clojure-mode has a long list of things that it checks for, and the order matters. We want to essentially remove one (or perhaps even two, because there is a rule that says anything starting with 'def', 'with' or 'let' should be colored as macros) element from clojure-mode's clojure-font-lock-keywords, and add extra rules to the list, complecting cider and clojure-mode.

As far as I can see, there are a couple of solutions, none of which is great.

  1. Remove 1 or 2 elements from clojure-font-lock-keywords in clojure-mode. The rules are not super important, but it nevertheless means losing features in clojure-mode. And then not add them again later.
  2. Writing some code in clojure-mode to detect CIDER's dynamic font locking and modifying the font-lock-keywords dynamically. This is not great either because it would still be hacky and complected. There would have to be some way to detect CIDER's font locking, a function to modify the font lock keywords, refontifying the buffer (which I didn't find a way to do besides turning clojure-mode on and off). (And, by the way, I think doing anything in CIDER to solve the problem is even worse.)

If I had to decide I would chose 1.

But maybe someone has a better idea?

@oskarkv

This comment has been minimized.

Contributor

oskarkv commented Sep 20, 2017

One pretty good idea would perhaps be to add an option in clojure-mode to disable those rules that interact badly with CIDER's dynamic font locking. What do you guys think?

@bbatsov

This comment has been minimized.

Member

bbatsov commented Sep 25, 2017

I think it might be easiest to just modify the clojure-mode rules when cider-mode gets enabled and dynamic font-locking is enabled.

There might be some better solution as well, but I don't have time to look into this myself. I wrote this ns font-locking code way before CIDER had this feature and because many people use Clojure mode without CIDER I don't think it's a good idea to just remove it.

@bbatsov bbatsov added the bug label Jan 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment