`cider-locals` - false positives

[vemv]

For the following input:

(def foo-routes
  [["/foos"
    {:get {:handler (fn [{{{:keys [archived]} :query} :parameters
                          :keys [sub]
                          :as req}]
                      {:pre [sub]}
                      (let [foo (reduce + (range 4))]
                        {:status 200}))}}]])

...if I hover over range and M-x describe-char, I'll get:

There are text properties here:
  cider-locals         ("200" "4" "range" "+" "reduce" "foos" "let" "sub" "req" "sub" "archived" "fn" "/foos" "req" "sub" "archived" "collections")
  fontified            t

A lot of those aren't locals (e.g. number/string literals, function names).

This happens because of the def x [[ format, which the cider-locals code isn't prepared for.

As a consequence, there can be subtle bugs with font locking.

[vemv]

@bbatsov: wondering if this cider-locals code (which presumably will never be perfect - hard problem to tackle with Elisp) could be replaced with a simpler approach, namely regarding any symbol that doesn't belong to the namespace interns (defs, refers, imports) as a local.

(this can be queried instantly as there's a per-ns cache with that data)

There's the case of var shadowing, but these days local shadowing is less common and there are kondo/etc linters that guard against it.

The worst case is that we sometimes font-lock slightly badly for code that shadows vars, but that bug can also be considered a feature (as it nudges people to do the right thing if they want pretty highlighting).

As a bonus, by not running cider-locals code constantly as the user types stuff, there should be a slight performance/reliability improvement.

[vemv]

Hammocking it a bit, a very easy change for now would be to introduce a defcustom controlling

cider/cider-mode.el

Lines 777 to 782 in 176a8e7

	(defun cider--unless-local-match (value)
	"Return VALUE, unless `match-string' is a local var."
	(unless (or (get-text-property (point) 'cider-block-dynamic-font-lock)
	(member (match-string 0)
	(get-text-property (point) 'cider-locals)))
	value))

i.e. optionally make it a no-op.

I wouldn't change the default behavior and simply document in the manual when/how to change this.

[bbatsov]

@bbatsov: wondering if this cider-locals code (which presumably will never be perfect - hard problem to tackle with Elisp) could be replaced with a simpler approach, namely regarding any symbol that doesn't belong to the namespace interns (defs, refers, imports) as a local.

Yeah, I guess that's not a bad idea, although it might highlight as locals misspelled names. Not a big deal in the end, though.

The idea with the defcustom is a good one IMO, as it will also make it possible to disable this completely if someone runs into problems.

[vemv]

Yeah, I guess that's not a bad idea, although it might highlight as locals misspelled names. Not a big deal in the end, though.

Yes, it might also be the case that clj-kondo indicates a non-resolved var as a "squiggly", anyway.

The idea with the defcustom is a good one IMO, as it will also make it possible to disable this completely if someone runs into problems.

Nice, I'll bundle this as part of #3646 .

(when time allows - still running tight in terms of availability)

[github-actions]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed soon if no further activity occurs. Thank you for your contribution and understanding!

[bbatsov]

Quick note for whoever picks this up: the easy part is a defcustom to switch off dynamic locals parsing, but a real fix needs per-def-form scoping. Right now the parser's regexp lumps def and defn together (both come through as def), so it can't tell a value form from an arglist and reads the def's value as if it were args. Leaving open for that.