Skip to content

cider-locals - false positives #3657

Open
@vemv

Description

@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.

Activity

vemv

vemv commented on May 11, 2024

@vemv
MemberAuthor

@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

vemv commented on May 11, 2024

@vemv
MemberAuthor

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 commented on May 12, 2024

@bbatsov
Member

@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

vemv commented on May 12, 2024

@vemv
MemberAuthor

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

github-actions commented on Jan 20, 2025

@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!

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @bbatsov@vemv

        Issue actions

          `cider-locals` - false positives · Issue #3657 · clojure-emacs/cider