Fork me on GitHub
#clj-kondo
<
2021-01-23
>
snoe19:01:50

@borkdude can we chat about https://github.com/clj-kondo/clj-kondo/issues/1129 ? I think my view comes down to (s/def ::a) and (def a) are very similar, and in my projects, I would treat both a and ::a as definitions even though clojure or spec, under the hood, is just storing some state for the var and the keyword. In lsp. I appreciate that kondo analysis differentiates between var-usages and var-definitions, and, I think, there's an argument to be made for keywords behaving similarly.

borkdude19:01:07

This is very library specific and not a core clojure construct

borkdude19:01:25

So if you would do something like this, you would have to include all kinds of library specific code

borkdude20:01:38

e.g. def is a special form in clojure, but any macro can make up any kind of def thing using strings, symbols, keywords, etc. This is user-land behavior

borkdude20:01:49

What I think would be generally possible is a list of all keywords, with filename, location, source form (what we call :str so far) and maybe, for some libs we could add a key that this is the defining location or something

borkdude20:01:53

But this will be very custom

borkdude20:01:13

for clojure.spec it would be a no-brainer

borkdude20:01:29

but what other libs are using this construct? we can't possibly support all of clojars?

snoe20:01:40

yeah, I agree, it's user-space. But clojure.spec is pretty close to core. And the concept of using a keyword to mean something is generally applicable. One of the things that I ask myself about these analysis prs, is how would one implement linting from this feature?

borkdude20:01:28

When you look at the namespace graph, one could maybe derive the first mention of the keyword

borkdude20:01:57

but since keywords (e.g. defmethods) are usually used to decouple things, this isn't reliable

snoe20:01:03

writing a sci macro for other libraries that maybe does`(s/def ~name)` would be enough to support re-frame/reg stuff

borkdude20:01:48

wouldn't it be sufficient to get all mentions of a keyword (regardless of alias)?

snoe20:01:39

for lsp it would be sufficient for rename, but there'a goto def/find references (that generally doesn't include the def) that the distinction would be useful for. for linting, I imagine the distinction would allow you to say a spec is unused.

borkdude20:01:32

If I would have this use case of unused-ness of a spec, I think I would personally just go through the list of all occurrences of this keyword. Same for re-frame events. Keywords that only occur once are suspicious (for specs, but in general, these are usually typos).

borkdude20:01:56

(ns foo)
(s/def ::bar int?)
:foo/bar
I think it's a bit weird to say ":foo/bar" is defined somewhere. I need to think about this a bit more.

snoe20:01:49

yeah, I agree it's definitely a different way to think about it. When I added that to the lsp parser though, it made a big difference in usability.

borkdude20:01:09

was this a feature that was supported in LSP before?

snoe20:01:42

yup. my macro-defs were able to treat a call to a symbol like {clojure.spec.alpha/def [:declaration :element]} and I would tag the first arg (the keyword) as a :declaration . Kondo obviously has a different model.

borkdude20:01:15

So it would be useful to have this in hooks then I guess.

(reg-keyword-def ....)
or something

borkdude20:01:42

and have this built-in for spec

snoe20:01:57

in the only one usage lint model, I think you'd have troubles if someone simply forgets to s/def so if the user just (->> x (s/validate ::a) (s/conform ::a)) it's moving to a very heuristic approach.

borkdude20:01:03

so maybe a list of all keywords and some have :def true ?

snoe20:01:06

I haven't looked at hooks much, but yeah, that might do it. It could add an extra flag to :keywords or we could do a similar split of usages/definitions that vars use

snoe20:01:14

aye, that works for me

borkdude20:01:31

re-frame calls this :reg but the intent might be clear

snoe20:01:00

spec might call it a registry as well.

borkdude20:01:11

that's true

borkdude20:01:25

ok, we can think more about the name, but I think we have an idea what should be done now

snoe20:01:43

yup, thanks a bunch!

borkdude20:01:49

I'll ask some other people too what they think of this

borkdude20:01:15

What if both re-frame and spec use the same keyword to register something?

borkdude20:01:23

Then you would have two :def true

snoe20:01:19

could be reg-by set, in the code I've run into it hasn't been a concern but it's a good question.

borkdude20:01:16

:def clojure.spec.alpha

borkdude20:01:22

:def re-frame.core

borkdude20:01:36

:registered-by ...

snoe20:01:45

could be, not sure how hard that would be

borkdude20:01:12

Maybe :registered-by clojure.spec.alpha works. :registered-by clojure.core/add-watch

borkdude20:01:42

well in hook code this is easy, since you know what macro you are dealing with

snoe20:01:29

ok I can start moving the pr towards this.

borkdude20:01:08

yeah, we can do :def <truthy> where truthy is either a boolean or the macro / fn that registered it

borkdude21:01:30

@U0BUV7XSA Since this analysis isn't fully formed in our heads yet, I think the best way forward is to add it as discussed and also write some docs, but comment out the docs (using HTML comments) so it's only private to both clj-kondo and clojure-lsp for now, so we have freedom to make changes until we are satisfied.

👍 3
didibus06:01:45

Let me think through the use cases here... sorry if I'm repeating what you both discussed already. It seems the benefit here is to be able to do go to definition. There is also find reference, but I think that's covered just by having analysis return all keywords and where they are found correct? For go-to-definition we need to know which of the many places the keyword is used in its canonical definition. Figuring out what is canonical is the hard part. Ya, I can imagine if you can teach clj-kondo what functions or macro forms where a keyword appear is the "canonical definition" for it. That would be neat actually. Like @borkdude said, its possible more than one thing define things with the same keyword. So I'm guessing it needs to be each keyword can have 0 or more definitions. And the editor needs to support listing multiple choices when you do go to definition. I can do that in Anakondo easily, not sure if LSP supports this? Now there's the issue of when things are defined dynamically, so now where do you go? Can clj-kondo be smart enough to learn about those as well, and give us the place where the keyword will get dynamically defined? That's what you meant by supporting hooks for it @borkdude? And a nice linter for this would be that say if you are inside of a spec function, and you use a keyword that is not defined anywhere it warns.

didibus07:01:27

Something else I'm thinking, on a similar vein, it be nice if when auto-completing keywords, it could show only keywords that are relevant. So if you s/def some keyword, and then you s/validate like only keywords that have been s/def should show up in the auto-completion. Just thinking how to do this...

didibus07:01:25

Also, why not make it a separate entry like var-definitions? Maybe I understood wrong, but it sounded here you wanted on keyword-usages to add a :def key?

snoe05:01:08

I think the reasoning for keeping it all in a single entry is because the interpretation of a keyword as a def is up to userspace.

snoe05:01:37

> And the editor needs to support listing multiple choices when you do `go to definition`. I can do that in Anakondo easily, not sure if LSP supports this? The protocol seems to allow for multiple definition results it will depend on the clients, as always, to support it.

didibus22:01:20

> I think the reasoning for keeping it all in a single entry is because the interpretation of a keyword as a def is up to userspace. Ok, but I mean we're talking in either case, userspace needs to configure clj-kondo for it no? But, ya ok, I see the point, like it might look weird for the map to be empty most of the time, unless userspace configured some keyword defs

didibus22:01:24

I guess from a consumer perspective, it seems like you'd have a different way to parse symbols from keywords for "go to def" and "find references". Which is why I was wondering like if that made sense.

borkdude22:01:10

@U0K064KQV the idea is that a keyword in and of itself doesn't mean anything. e.g.:

(defn foo [x])
(foo :bar)

;; vs

(defn reg-something [k])
(reg-something :bar)
are analogous, whereas
(def x 1)
has definition meaning regardless of what semantics users give to their functions

borkdude22:01:41

This is something that isn't fully clear to me yet, that's why I propose keeping this analysis output private until it's been used by LSP for a while

borkdude22:01:14

we could make them separate, but having a list of all occurring keywords seems useful to have anyway for renaming purposes

didibus22:01:34

I feel like either or, we're still adding the concept of keywords can be considered as having a definition no? Even though by default they don't in Clojure, but then per-library or user usage they might. I feel for me its more what's easiest to parse in the analysis structure for the common use. I guess its a small detail, either or you can parse out the information. But say someone runs: "go to definition" on a keyword, now you'd filter through the keyword usage for that keyword for the one with "def = true".

didibus22:01:28

Where as if it was a separate map, you'd look it up in keyword-usage map

didibus22:01:29

Actually, sorry, maybe that's more a general critique. I remember now. When I implemented anakondo, I think I wished that things were pivoted, instead of vector of maps, it be maps of vectors of maps

didibus22:01:06

Where the map is keyed on the name

didibus22:01:47

I found needed to actually parse everything back into maps indexed on names for the different constructs.

didibus22:01:38

Anyways, might have missed the boat on that now.

didibus22:01:11

I'll be happy either way, if :def is on the keyword map or in a separate one.

borkdude22:01:25

> I feel for me its more what's easiest to parse in the analysis structure for the common use. That's a good point

borkdude22:01:15

@U0K064KQV the reason you get everything as a list is that it's not always clear what the index should be. either clj-kondo does this for you (and has to guess what is the right format), or you do it yourself. if clj-kondo would do this for you and you wanted a different index, then it would be hairy to go from one index to another. the work has to happen somewhere and it's most flexible if that somewhere is the end user

borkdude22:01:40

people might also want to index on filename, for example, or namespace and then name, etc

didibus22:01:21

Ya, that's true. Actually I can't remember fully, but I probably did end up indexing in a slightly more custom to my needs way.

didibus22:01:59

I think had it been in Clojure, I wouldn't have feel the pain 😛, but transforming datastructures in Emacs Lisp is not as simple haha

borkdude22:01:17

write a graalvm binary ;)

borkdude22:01:00

I think now you could do this part using babashka

borkdude22:01:07

using clj-kondo as a pod

didibus22:01:40

Ya, I've been considering it. The thing is, with any separate process option, now I need to figure out a protocol to communicate between the editor and that, and the editor needs to manage a long running process, maybe multiple if you have multiple project, etc. And I think there's a lot of brittleness there, and also I'm just re-inventing LSP at this point.

borkdude22:01:57

we could even let clj-kondo take some processing function that is executed using sci, since it already has sci

didibus22:01:35

Hum... like pass it a post-processor on the analysis map, that could be nice.

didibus22:01:16

So you could use Clojure to re-structure it however is best for your use case, and then your editor gets a JSON in that new structure.

borkdude22:01:03

I think the function could be applied to the :findings as well, just one function that gets the end result before it's written to EDN or JSON

didibus22:01:50

Ok, well, it would be cool. I don't know if I'd say its at the top of the Wishlist though 😛 Even though I'm here complaining about how painful it is to do in Emacs Lisp, that's pain I chose for myself haha

borkdude22:01:48

I think we have to rewrite emacs to clojure one day ;P

3
didibus22:01:36

The only thing I can give props to Emacs Lisp, its use of memory is quite impressive, like its very memory efficient

snoe01:01:46

in clojure-lsp, we probably have 4 or 5 things to index on, so I just flatten everything into a singular list and search. if perf becomes. a problem we can reify the indexes we need.

didibus02:01:12

Even for auto-complete?