Fork me on GitHub
#clj-kondo
<
2021-09-02
>
Joshua Suskalo17:09:54

Continuing from #tools-deps, where should I look to see about integrating with clj-kondo's analysis to get information out of vars?

borkdude17:09:49

@suskeyhose Let me get back to you on that in 15 minutes or so

Joshua Suskalo17:09:48

So I'm taking a peek at the analysis folder and it looks like only specific metadata is exported, and for my usecase I'd need the :style/indent metadata to be exported as well. Possibly a very small change. In any case I need to keep reading.

๐Ÿงก 2
borkdude18:09:47

@suskeyhose This is correct. But! There is an issue to collect arbitrary metadata and the fix for this is pretty easy, so perhaps you could provide some input and we can finally implement this. https://github.com/clj-kondo/clj-kondo/issues/1280

borkdude18:09:34

I would be ok with just adding all metadata on a :meta key or so, when providing an option. The only "danger" is that metadata contains stuff that isn't readable as EDN or so.

Joshua Suskalo18:09:05

Ah, I see, since the data is passed over stdout it could cause issues.

borkdude18:09:18

well, clj-kondo is also available as a JVM library

Joshua Suskalo18:09:27

Right, that's what I'd want to use it as.

borkdude18:09:45

but it'd be nice if we supported both use cases at once: the EDN output should preferably be valid.

borkdude18:09:04

And the most secure way to do that is to just provide values as strings perhaps

borkdude18:09:15

using pr-str

borkdude18:09:01

OR we could make the selection of metadata optional (like in the issue) and then any EDN errors are the problem of the caller.

borkdude18:09:58

or we could choose to only export metadata with keywords as keys and constants as values

Joshua Suskalo18:09:01

I think having the :var-meta key like in the config is valid, and make it so just having true for it will return everything.

borkdude18:09:56

Note that this is valid: ^{(+ 1 2 3) (+ 4 5 6)}

borkdude18:09:04

that's what I'm thinking of

borkdude18:09:11

those cases are rare, but they do exist

borkdude18:09:53

so if the metadata has ^{:foo "bar"} then we'd export :foo "bar" but when it's :foo (+ 1 2 3) we don't, for example

Joshua Suskalo18:09:24

I think a valid constraint to put on it for now is to only grab the constant keys and values, and provide nothing for these dynamic ones, with the potential in the future that these may be added.

Joshua Suskalo18:09:21

Just entirely excluding these dynamic ones would help prevent breaking changes by leaving the possibility they're added later, without giving in the meantime a quoted version of it for people to depend on.

borkdude18:09:23

I think attrs would need an extra :meta key on which we do some processing before we add it to the analysis, if the option {:var-definitions {:metadata true}} is given, I think

Joshua Suskalo18:09:14

I think that would solve all the needs that I have, being able to start with the -T classpath with just . and my own dependencies on the classpath, and then using kondo to get all the metadata to generate the config file. Are there any other hangups I gotta be aware of before I start planning out my tool properly? Like having to figure out how to procure deps or something for kondo to lint them? Or will I be good just using tools.build's facilities for starting a clojure subproces to get the classpath with a set of aliases?

borkdude18:09:50

you can just pass the classpath into :lint ["the-classpath"]

borkdude18:09:14

@suskeyhose Would you like to have a stab at this?

borkdude18:09:44

It would also be interesting to see how many times the metadata is dynamic

Joshua Suskalo18:09:23

Yeah, I'll take a look at it tonight to see what I can get going.

Joshua Suskalo16:09:32

Okay, so I'm looking at this today @borkdude, and it looks like the line you pointed me to will allow a meta key to come from whatever is passed to reg-var!, so does that mean the rest of this implementation is to just go through all the usages of reg-var! and make them include metadata? It seems like that's the obvious direction to go.

borkdude16:09:55

yeah I think so

Joshua Suskalo16:09:49

Yeah, so there's the namespace/reg-var!, and there's analysis/reg-var!, and it looks like the namespace one is the only one used, and it calls to the analysis one, so I just gotta update the namespace one I think.

Joshua Suskalo16:09:56

'cause that already gets the metadata

Joshua Suskalo16:09:41

Hmm. There's a lot of metadata in there that's coming from clj-kondo and not from the original var. Is there a way to select just the stuff coming from the source?

borkdude16:09:44

hmm, like what for example?

borkdude16:09:25

I'll be back after dinner

Joshua Suskalo16:09:00

Actually I think I figured it out

Joshua Suskalo17:09:38

This is still a "draft" PR, but I'd love to know if this is the right direction, or if I need to be doing something drastically different. I'm thinking then I can have the analysis/reg-var! do the actual filtering of which meta values are allowed, so that no code changes are needed elsewhere if we expand which kinds of meta are allowed.

borkdude17:09:21

Yep, that's going into the right direction. Tests are the next step I think

ericdallo18:09:07

Would be possible to clj-kondo correctly understand a var imported from potemkin and avoid unresolved-var lint?

borkdude18:09:45

clj-kondo already has built-in support for potemkin

ericdallo18:09:23

Hum, we have this lib at nubank that uses potenkim a lot, and when using the vars from its api, I'm getting unresolved-var :thinking_face:

borkdude18:09:45

Repro welcome

ericdallo18:09:10

I'll try to build a repro!

ericdallo18:09:42

I found the time to repro the issue in a separated repo/branch :) More details here: https://github.com/clj-kondo/clj-kondo/issues/1370

borkdude18:09:04

oh nice, I didn't know midje had clj-kondo exports

ericdallo18:09:54

oh yeah, I merged that last month ๐Ÿ˜…

borkdude18:09:20

thanks for the repro. I think it should work but I don't know why it doesn't

borkdude18:09:29

hopefully I'll find some time to find out next week or so

borkdude18:09:38

or if you beat me to it...

borkdude18:09:23

The relevant code is in clj-kondo.impl.analyzer.potemkin

ericdallo18:09:14

cool! I can try to debug tomorrow :)

borkdude18:09:45

ah, it seems the potemkin code isn't hit at all somehow

๐Ÿ‘€ 2
borkdude18:09:10

oh I see, I think it currently only supports potemkin syntax with some vector or so

borkdude18:09:16

and not a single symbol

borkdude18:09:59

So this works:

(ns clojure-sample.api
  (:require
   [clojure-sample.impl.foo]
   [potemkin :refer [import-vars]]))

(import-vars
 [clojure-sample.impl.foo some-func])

borkdude18:09:18

so I've done the debugging, I'm sure the fix is pretty easy now

borkdude18:09:31

I'll past this info in the issue

ericdallo18:09:22

oh, really good catch!

ericdallo18:09:29

I tried and it works indeed :)

ericdallo18:09:02

there is this lib at nubank that uses a lot of import-vars with full qualified symbols

borkdude18:09:33

I think it will be a small change

๐Ÿ™ 2
borkdude18:09:41

for clj-kondo

ericdallo18:09:09

I imagine that is just make clj-kondo lint this other way of import-vars I think right

borkdude18:09:45

yes, check if the thing is a symbol and act accordingly

borkdude18:09:00

So this is the code:

(defn analyze-import-vars [ctx expr]
  (let [ns-name (-> ctx :ns :name)
        import-groups (next (:children expr))]
    [{:type :import-vars
      :used-namespaces
      (for [g import-groups
            :let [[imported-ns & imported-vars] (:children g)
                  imported-ns-sym (:value imported-ns)]]
        (do (doseq [iv imported-vars]
              (let [iv-sym (:value iv)]
                (prn :reg-var ns-name iv-sym expr {:imported-ns imported-ns-sym
                                                   :imported-var iv-sym})
                (namespace/reg-var! ctx ns-name iv-sym expr {:imported-ns imported-ns-sym
                                                             :imported-var iv-sym})))
            imported-ns-sym))}]))

borkdude18:09:17

instead of destructuring in the let, first check if it's a fully qualified symbol token

borkdude18:09:33

and then set imported-ns and imported-vars to that

borkdude18:09:36

else leave it like it is

borkdude18:09:20

so g can be either a vector or fully qualified symbol

ericdallo18:09:04

Makes sense ๐Ÿ‘ I'll try to fix that and let you know, thanks for the explanation

borkdude20:09:52

Merged. I will make a small tweak, but nothing major.

borkdude20:09:13

One thing is, I would use qualified-symbol? instead of qualified-ident. Is there a reason you chose the latter?

ericdallo20:09:32

yes, I didn't know about the former ๐Ÿ˜‚

ericdallo20:09:13

I see, the ident checks for keyword as well while the other checks if it's a symbol only

borkdude20:09:40

teamwork ๐Ÿค

๐Ÿš€ 2
ericdallo20:09:34

oh that makes sense indeed, good catch

Joshua Suskalo19:09:07

With regards to the dynamic metadata stuff, is there anything that'd get in the way of checking if all the vars used in the definition are from clojure.core and then evaluating it with sci? I figure an eventual goal for this kinda thing would be to get something that's basically like constexpr evaluation that would just recursively check vars for sci compatibility and then eval them. Although for clarity, I'm not looking to implement this at all for the part I'm looking at tonight. I still want to stick to filtering metadata to constants.

borkdude19:09:20

> is there anything that'd get in the way of checking if all the vars used in the definition are from clojure.core and then evaluating it with sci? yeah, I think you can do that, unrelated to the metadata feature I think

borkdude19:09:20

also some vars like clojure.string, etc are in sci

borkdude19:09:35

so as long as the program is pure Clojure, it should mostly work

Joshua Suskalo19:09:10

Fair enough. That'll be something that I might think about in the future once the basics of general metadata are into analysis

Joshua Suskalo19:09:42

Although at that point it might be worth just putting it into a tracking issue to see if there's interest in it before the work gets put in.

borkdude19:09:44

why would you eval the code, what's the goal with that?

Joshua Suskalo19:09:20

to get ^{(+ 2 2) (+ 4 4)} metadata working

Joshua Suskalo19:09:23

Like you mentioned before

borkdude19:09:51

oh right :)

borkdude19:09:23

I really do think these are rare cases though, so perhaps some exploration would be good to see often this occurs

Joshua Suskalo20:09:01

Yeah, that's why I was thinking about a tracking issue to see for demand

borkdude20:09:48

just evaluating random expressions isn't safe to do even with SCI, e.g. you can have (defn ^{:foo (range)} dude []) so I wouldn't recommend it

borkdude20:09:33

in the case the metadata isn't a constant, we could also just stringify it

borkdude20:09:15

but leaving it out for now seems also fine

Joshua Suskalo20:09:47

I think leaving it out for now is the best choice because it leaves us many options for what to do with it in the future.

Joshua Suskalo20:09:40

I'd rather provide something for 80% of cases, and for the 20% provide nothing, than provide something for 100% of cases with a potential for breaking changes to 20% of it.

Joshua Suskalo20:09:00

(although obviously the actual percentage of this kind of thing being uses is lower than that)