BetterThanTomorrow / calva

Clojure & ClojureScript Interactive Programming for VS Code
https://marketplace.visualstudio.com/items?itemName=betterthantomorrow.calva
Other
1.67k stars 217 forks source link

Used/unused symbol check is not run when the actual usages are changed #1261

Open p-himik opened 3 years ago

p-himik commented 3 years ago

It's only run when the file containing the symbol definition changes. To reproduce, create two files:

;; src/proj/a.clj
(ns proj.a)

(defn f [])

;; src/proj/b.clj
(ns proj.b
  (:require [proj.a]))

(defn f []
  (proj.a/f))

At this point, proj.a/f will be marked as unused, despite it being used in proj.b. Changing proj.b in any way won't re-run the check. Changing proj.a in any way will run it and proj.a/f will stop being highlighted as unused. You can do the reverse as well - after doing the above, remove (proj.a/f) from proj.b and notice how proj.a/f is still not highlighted. Change proj.a in any way, and the function will become highlighted as unused.

bpringe commented 3 years ago

That unused public symbol feature comes from clojure-lsp. Let's see what @ericdallo has to say about this.

At this point, proj.a/f will be marked as unused, despite it being used in proj.b

It seems like at this point the references haven't been updated (at least on the client, from the server). Maybe something needs to happen on file change that isn't currently to update those references.

p-himik commented 3 years ago

Maybe something needs to happen on file change that isn't currently to update those references.

It seems to be the case, yes. When I change b.clj, only its metadata is requested from clojure-lsp, at least as far as I can tell from the logs. So no rechecking of a.clj is done.

ericdallo commented 3 years ago

This is a known issue on clojure-lsp indeed as ATM we only update the diagnostics for the file when we run clj-kondo on that file, which is done for all project files when server starts and when user change that file. There is a flag to fix that issue :notify-references-on-change that is disabled by default for performance issues, it updates the references of the changed function/var. I want to make that as the default behavior but we will need to think how to improve that

ericdallo commented 3 years ago

I made some huge improvements on the :notify-references-on-file-change feature, it's still disabled by default but probably soon it'll be the default if no performance issue is found. if you want to try with calva, you just need to have something like this in your .lsp/config.edn:

{:notify-references-on-file-change true}
bpringe commented 3 years ago

@ericdallo Did you mean .lsp/config.edn?

ericdallo commented 3 years ago

Yes, thanks haha