Fork me on GitHub
#lsp
<
2021-03-20
>
anonimitoraf12:03:27

Weird, I always get this:

LSP :: keys.cljs not in project or it is blacklisted.
when I load a (doom) emacs session that my clojure project is in. Possible race condition between (doom) emacs session loading and LSP initialization? When I open the project directly (via projectile), it's fine

anonimitoraf12:03:57

I can make a repro project tomorrow, if necessary. So essentially all I do is: • Open a project, add it to LSP known projects • Save session as "blah" • Close emacs • Load session "blah" • I get asked this:

ericdallo12:03:36

I use doom-emacs but never saw this, but yeah, it could be some race condition, I'd suggest asking for help on lsp-mode or doom discord as @U5B6G208G is more familiar with that than me

👍 1
anonimitoraf13:03:14

Thanks, I'll try those

anonimitoraf13:03:42

Oh, just FYI, this only started happening recently

anonimitoraf13:03:55

So maybe I'll look at recent emacs lsp commits too

ericdallo15:03:19

Hum, yeah, good start

anonimitoraf07:03:05

Update on this: I found the cause: • I had treemacs sync mode on • In treemacs I had an inner folder registered as a project. Treemacs doesn't allow projects that overlap. • Removed it, now works fine

ericdallo17:03:10

So, I'd like to give a try later on implementing https://cursive-ide.com/userguide/macros.html#customising-symbol-resolution on clojure-lsp 🧵

ericdallo17:03:55

This is the only one big feature I see it's missing from non-cursive IDEs, and if we could add that, it'd improve a lot the UX. it could be really nice for other editors as well like Calva (c/c @U9A1RLFNV) I was discussing with @U5B6G208G (lsp-mode maintainer) and we could do that via code actions, like: 1. clojure-lsp detect a unresolved macro and return a code action "Resolve macro as..." 2. LS client (for now, lsp-mode), wrap that code action asking to user how it should resolve, for now we can show only the common clojure ones: def , defn , let -> etc 3. clojure-lsp receive that and somehow update user/project clj-kondo config lint-as . @U04V15CAJ I'd like you opinion/suggestion on how we can achieve the 3 step. In a ideal world, clojure-lsp could call a clj-kondo function to update the user config and clojure-lsp trigger a re-lint of that file. Or clojure-lsp could ask to clj-kondo the clj-kondo config-path and change that itself manually, WDYT?

👍 1
borkdude18:03:42

@UKFSJSM38 That seems good to me. You can use https://github.com/borkdude/rewrite-edn or just rewrite-clj to update the clj-kondo config.

ericdallo18:03:12

Nice, any suggestion if this logic should be on clj-kondo side or clojure-lsp? I think that on clj-kondo we could use existing functions to know what is the user clj-kondo config

borkdude18:03:52

clj-kondo has only a forked version of rewrite-clj currently, so it isn't able to do any code editing

borkdude18:03:20

is there something in the response of clj-kondo/run! that points at the config dir?

ericdallo18:03:43

Not that I recall

borkdude18:03:51

I'll take a look

borkdude18:03:58

we can add a core function for this

borkdude18:03:06

in clj-kondo.core

ericdallo18:03:55

Yeah, that would help :)

borkdude18:03:50

but it also depends on the :config-dir argument to clj-kondo/run! so it's call-dependent

borkdude18:03:08

can users configure this in lsp?

borkdude18:03:29

I think 99% of the time the config is in .clj-kondo/config.edn but maybe not in monorepo projects

ericdallo18:03:34

No, only if it's available via the .clj-kondo/config.edn file

ericdallo18:03:00

Yeah, I thought we could use the same logic clj-kondo use to find the config

ericdallo18:03:19

The core function would just call that logic I imagine

borkdude18:03:21

I think what you should do is look up from the current file towards the first .clj-kondo dir and pick the config.edn from there

ericdallo18:03:58

So, On clojure-lsp side?

borkdude18:03:08

I have this function in clj-kondo.impl.core:

(defn config-dir
  ([] (config-dir
       (io/file
        (System/getProperty "user.dir"))))
  ([start]
   (let [start (io/file start)
         ;; NOTE: .getParentFile doesn't work on relative files
         start (.getAbsoluteFile start)
         start (if (.isFile start)
                 (.getParentFile start)
                 start)]
     (when start
       (loop [dir start]
         (let [cfg-dir (io/file dir ".clj-kondo")]
           (if (.exists cfg-dir)
             (if (.isDirectory cfg-dir)
               cfg-dir
               (throw (Exception. (str cfg-dir " must be a directory"))))
             (when-let [parent (.getParentFile dir)]
               (recur parent)))))))))

borkdude18:03:30

so I think you can call that one, but we should maybe make it public

ericdallo18:03:42

Yes, that makes sense

borkdude18:03:01

feel free to make a PR to make a public wrapper around it

borkdude18:03:25

but first you can call it from impl.core

borkdude18:03:29

just for testing

ericdallo18:03:19

Sure! Thanks for the guide, I'll make that later today

ericdallo20:03:29

@U04V15CAJ it seems that function almost always will return the project .clj-kondo since clj-kondo creates it for the .clj-kondo/.cache

ericdallo20:03:52

maybe clojure-lsp should check if there is a config.edn file from the return of that function?

ericdallo20:03:27

still it'd be logic to check the home dir on clojure-lsp side, it'd be better if clj-kondo return the project .clj-kondo only if there is a config.edn file there

ericdallo20:03:34

but user may have no config.edn on ~/.clj-kondo as well :thinking_face:

borkdude21:03:23

@UKFSJSM38 clj-kondo never creates a .clj-kondo directory

borkdude21:03:58

I think it may be better to show the user a list of possible candidates

ericdallo21:03:42

I see, presenting from the current folder until home I suppose

borkdude21:03:22

candidate config.edn files

ericdallo21:03:23

like:

- /home/user/mono-repo/project-a/.clj-kondo/config.edn
- /home/user/mono-repo/.clj-kondo/config.edn
- /home/user/.clj-kondo/config.edn

borkdude21:03:46

well, in that example on the first and last are relevant

ericdallo21:03:52

it seems better indeed, so we don't need to use the clj-kondo config-dir, right?

borkdude21:03:04

since clj-kondo doesn't reach the middle one when it encounters the top one

borkdude21:03:28

I think in most cases people want to use it to the project config (the top one)

borkdude21:03:38

since it fixes it for their teammates as well

ericdallo21:03:47

I see, so I think we can present either the home one or the project, right?

ericdallo21:03:30

ok, so we don't need to touch clj-kondo code I think

ericdallo21:03:48

I'll check if we are missing anything, but looks good to me

ericdallo22:03:03

@U04V15CAJ I realized that when we have a unknown macro, we don't necessarily have a lint/diagnostics on the macro, but on other places inside the macro usage that may be because of a unknown macro

ericdallo22:03:31

The only way I see that working is clj-kondo return in the analysis if that macro is known or not

ericdallo22:03:05

otherwise I can't see clojure-lsp suggesting correctly a Resolve macro as code action

borkdude22:03:39

clj-kondo does report whether a var is a macro

ericdallo22:03:19

yes, but it reports if that macro is known? like is defined via lint-as or some common macros that are coded on clj-kondo?

ericdallo22:03:04

for example, it should not return a unknown macro for defflow if I have it correctly configured with hooks etc

ericdallo22:03:26

only specifying as :macro true is not enough

borkdude22:03:49

"known macro" is not a thing since not all macros use syntax that gives problems

ericdallo22:03:09

yeah, I wonder how cursive handles that

borkdude22:03:11

where does cursive give this hint?

ericdallo22:03:06

good question

ericdallo22:03:51

but when you resolve it, Cursive save that config to not ask later again, so the way we handle that should consider that as well

borkdude22:03:02

I think cursive just shows this when you are on the macro call name: https://cursive-ide.com/userguide/macros.html

borkdude22:03:16

and from the analysis you can infer you are calling a macro, so maybe you could do the same

borkdude22:03:54

I'm not sure if cursive always suggests this or only in the presence of warnings, but this you can also check, since you know the start and end location

ericdallo22:03:28

ok, we can suggest it if it's a macro, but how we'd know if it was already resolved as before?

borkdude22:03:04

by looking at the config in :lint-as maybe?

borkdude22:03:40

I guess that part doesn't have to be that intelligent

ericdallo22:03:40

yeah, that would not be totally reliable as I imagine there is some common macros clj-kondo support by default?

borkdude22:03:49

the user can think for himself when changing this

ericdallo22:03:08

Hum, yeah, it could be a start

borkdude22:03:18

yeah, but for supported macros the user would probably not reach for this

ericdallo22:03:35

So I can always return the code action if cursor is inside a macro function

borkdude22:03:56

on the macro name I would say?

borkdude22:03:17

this is how Cursive seems to do it from the screenshot in the docs

borkdude22:03:29

have you got a running version of Cursive?

ericdallo22:03:37

it'd be now warnings/lints on the macro definition, it may not be clear to user that there is a action only when hovering the macro name I think

ericdallo22:03:04

> have you got a running version of Cursive? Nope, only saw teammates using

borkdude22:03:31

I do have an intellij license, for free, open source

borkdude22:03:46

you can request it from jetbrains, but you can even run cursive with the CE free edition

borkdude22:03:00

and cursive also has open source licenses

ericdallo22:03:20

good to know, I'll setup it later so

borkdude22:03:38

maybe also ask some feedback from other people in the channel

borkdude22:03:55

I don't have strong feelings about this, as I'm likely to edit the config manually

👍 1
borkdude22:03:19

but maybe people don't know this option exists, so it would be good to have it

☝️ 1
ericdallo22:03:58

I know it's something commonly used by cursive users

ericdallo23:03:38

I got to install and repro:

ericdallo23:03:34

so, cursive always show as a code action only when on macro name indeed

ericdallo23:03:49

and always show the code action, even if you already chose one before, exactly as you said @U04V15CAJ 🙂

ericdallo23:03:55

So I think we can do the same

ericdallo17:03:28

Still need some corner cases fixes though, but it's working nice 🙂

borkdude17:03:18

you might also want to add clj-kondo.lint-as/def-catch-all to this list

borkdude17:03:26

which works for weirder def-like macros

borkdude17:03:55

and I would probably remove -> since I have almost never encountered this in lint-as

borkdude17:03:08

oh you have, well, then leave it :)

ericdallo17:03:22

Cursive thas that, I don't see any harm keep it, right?

borkdude17:03:33

no harm. good job!

ericdallo17:03:44

Thank you for the help! I'm glad this worked 🙂

borkdude17:03:25

LSP is quickly becoming the most complete IDE after Cursive

borkdude17:03:45

I hope you get funding by CT

☝️ 1
ericdallo17:03:22

Yeah 😄 That'd be really good 🙂

borkdude17:03:29

Was there a survey announcement recently? Haven't seen one

ericdallo17:03:51

No, I think they will start the voting on april

ericdallo17:03:25

they were closing the members register this month

borkdude17:03:35

So how did it end with the incremental diffing? reverted it?

borkdude17:03:51

yeah, but usually they send out a survey to the sponsors before this

borkdude17:03:19

to see which projects should get funded by the opinion of the sponsors

borkdude17:03:31

I will ask Daniel

ericdallo17:03:31

> So how did it end with the incremental diffing? reverted it? I still need to find the out-of-sync corner case issue, it's behind a feature-flag

borkdude17:03:22

I was thinking, maybe generative testing could be used to find this corner case

ericdallo17:03:34

yes, it could help indeed, I think we could improve the integration tests to be able to do that

ericdallo17:03:46

the clojure-lsp integration tests base is working good

borkdude17:03:13

is it a problem with the async stuff, or with the diff function?

ericdallo17:03:42

Probably the async stuff

ericdallo17:03:54

the diff function seems to work good

ericdallo17:03:15

also it seems to happen with medium/big buffers

borkdude17:03:48

Maybe you can test it by putting some random Thread/sleeps into the async handlers

ericdallo17:03:00

hum, good idea indeed

borkdude17:03:12

Btw, I will probably make a new clj-kondo release tonight with a bunch of bug fixes in it

ericdallo17:03:51

good to know! I'll include in this feature release if don't find any issues with that bump 🤞

ericdallo17:03:08

thanks for telling me

borkdude17:03:20

I will push one more fix to master within the next two hours

👍 1
borkdude18:03:19

Pushing 2021.03.21, will be on clojars in a few minutes hopefully

ericdallo18:03:51

Nice, I'm finishing the resolve macro as, then I'll bump clj-kondo, if everything goes fine, I'll release it later tonight

borkdude19:03:28

Hmm, I found one more edge. I will do another release tonight

ericdallo19:03:53

👍 , I'll wait for clojure-lsp release so