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

Def highlighting is out of control #377

Closed
expez opened this issue Apr 22, 2016 · 15 comments · Fixed by #630
Closed

Def highlighting is out of control #377

expez opened this issue Apr 22, 2016 · 15 comments · Fixed by #630

Comments

@expez
Copy link
Member

expez commented Apr 22, 2016

Expected behavior

Proper syntax highlighting

Actual behavior

image

Steps to reproduce the problem

Write a form containing a word starting with def anywhere, e.g. (defface 'angry-face)

I get that the intent was to highlight defresource etc the same way as defn but this really isn't working as intended. All calls to functions like (default-user-settings) are highlighted like a def now, no matter where in the source they appear.

@Malabarba
Copy link
Member

I was pretty sure we had fixed this, at least for the word default. ...

@expez
Copy link
Member Author

expez commented Apr 22, 2016

image

@bbatsov bbatsov added the bug label Apr 30, 2016
@vspinu
Copy link
Contributor

vspinu commented Jun 19, 2017

It's not possible to fix this without enumerating proper defforms or requiring defforms authors to tag their macros with some kind of metadata like ^{:defform true}.

Proper defs can occur inside let forms, so highlighting top forms only is not a real fix.

One way to go about it is to highlight only defxyz if it doesn't contain - so that things like default-breakage wont break.

@expez
Copy link
Member Author

expez commented Jun 19, 2017

Maybe we should remove this from clojure-mode and add the smarter behavior to CIDER, where we actually have the ability to understand the code?

@Malabarba
Copy link
Member

I could swear we had fixed this ages ago, something related to not highlighting the symbol default. I even remember some brief discussion of English words that start with "def".

@sooheon
Copy link

sooheon commented Jul 9, 2017

I'm coming across this requiring a lib as [manifold.deferred :as deferred]. default is also still highlighted as a defform. If whitelisting correct defforms is a PITA, can we have some way to blacklist symbols?

@j-cr
Copy link
Contributor

j-cr commented Jul 31, 2018

I've been bitten by this too. The problem is the following line inside (defconst clojure-font-lock-keywords ...):

"\\(def[^ \r\n\t]*\\)"

See https://github.com/clojure-emacs/clojure-mode/blob/master/clojure-mode.el#L777 for context.

I've changed that to:

"\\("
(regexp-opt '("defn" "defmacro" "defmulti" "defmethod"))
"[^ \r\n\t]*\\)"

-- so only the calls we know for sure about are highlighted. I think it's much better to under-highlight than to over-highlight; the editor being "too smart" is simply annoying.

We should also probably have the stuff as we have for indentation, i.e.

  1. define-clojure-font-lock, analogous to define-clojure-indent
  2. support for font-lock metadata in cider

While that's not done, I propose to go for the fixed set of highlighted def*s. Let me know if you want a pull request for that.

Also, we can add a defcustom for the list of symbols to highlight, so it'll actually be trivial to add other symbols to the list.

@bbatsov
Copy link
Member

bbatsov commented Jul 31, 2018

Sounds like a reasonable approach. I like it. It's clear that we can't have a one-regexp fits all solution here.

support for font-lock metadata in cider

Actually CIDER will highlight things differently even now based on their type - macros are font-locked as keywords, built-in as built-ins, function names as functions and so on.

@j-cr
Copy link
Contributor

j-cr commented Jul 31, 2018

Sounds like a reasonable approach.

Good! So do you want a pull request (defcustom list of symbols + the change of regex) or would you rather hack on it yourself?

Actually CIDER will highlight things differently even now based on their type

Right - but what I had in mind is some kind of :style/font-lock metadata, so a library author could specify exactly how he wants his macro\fn to be highlighted.

@bbatsov
Copy link
Member

bbatsov commented Jul 31, 2018

Good! So do you want a pull request (defcustom list of symbols + the change of regex) or would you rather hack on it yourself?

PR would be great. The work on nREPL is eating all my spare time these days.

Right - but what I had in mind is some kind of :style/font-lock metadata, so a library author could specify exactly how he wants his macro\fn to be highlighted.

Yeah, I guess that makes sense if you want to override the defaults. It should also be relatively simple to do.

@vspinu
Copy link
Contributor

vspinu commented Aug 1, 2018

Before changing this at clojure-mode side, could we have :style/defform metadata implemented first? Before breaking long term behavior we should have a workaround. I often use defxyz macros and occasional false positives don't really bother me that much.

Some suggestion from above are milder solutions than an explicit list of keywords:

  • one can blacklist a list of common English words
  • limit defxyz to a single word symbols (no - allowed)
  • check if defxyz is a macro

Let's also consider the exclusion metadata :style/not-a-defform. We could keep the current regex but maintain a list of known not-a-defform from popular libraries. I am pretty sure the number of false-positives right now are way fewer than the potential false-negatives if we restrict the regexp to a fixed list of basic keywords.

@expez
Copy link
Member Author

expez commented Aug 1, 2018

I love the :style/defform suggestion!

I think we should be conservative because you can always opt-in your own code (though not library code) but you can't opt-out with a :style/not-a-defform on library code that gives a false positive.

@vspinu
Copy link
Contributor

vspinu commented Aug 1, 2018

I am not sure I understand. You can modify other library var's metadata from your code. Or you could PR to that library and popularize :style/not-a-defform, or you could fix it on emacs side by adding keywords to the exclusion list. There are some possibilities.

In any case, for the completeness sake both :style/defform and :style/not-a-defform could be supported.

@quanticle
Copy link

I think I'm running into a related issue. My problem is that if I have a def form at the top level in my Clojure file, I lose fontification for the next few forms. I've attached a screenshot of the issue. The test file that reproduces the issue can be found here.

Removing the def at the top of that file causes fontification to work correctly.

broken_def_highlighting

OknoLombarda pushed a commit to OknoLombarda/clojure-mode that referenced this issue Aug 29, 2022
OknoLombarda pushed a commit to OknoLombarda/clojure-mode that referenced this issue Aug 30, 2022
@arichiardi
Copy link
Contributor

Good job! this was a long standing bug, one that you have to forcefully tell your self is not a big deal (but it is 😄 ).
Thank you thank you thank you!

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

Successfully merging a pull request may close this issue.

8 participants