[analysis] Add defined-by on missing var definitions

[ericdallo]

Fixes #1216

This should add the :defined-by with the full-qualified-symbol for all var-definitions.

[ericdallo]

@borkdude I don't know if that is going to work with hook var-defintions like state-flow/defflow, could you point me to the function that should reg-var! that?

[borkdude]

@ericdallo So you wrote a hook for state-flow/defflow but after expansion it's linted as normal clojure.core/defn right? Is that the problem? Btw, some tests are failing.

[ericdallo]

Fixed tests, sorry for that!

@borkdude yes, and I just realized that the hook is using a def node, so I imagine that its defined-by will be def, right?

how can we make that work? is there anything we can add to the hook to tell clj-kondo to use a different defined-by?

[ericdallo]

BTW, I think this PR should solve clojure-lsp/clojure-lsp#340 as well :)

[borkdude]

@ericdallo We could return an extra value in the hook response, {:node ... :defined-by ...} but a more general name is maybe better...

[ericdallo]

@borkdude it makes sense, any tips how to implement that?
I checked that here we could get the defined-by informed by user via the transformed local and I suppose we should pass to analyze-expression** and pass to some reg-var somewhere?

[borkdude]

@ericdallo We can maybe put it on the ctx argument here:

clj-kondo/src/clj_kondo/impl/analyzer.clj

Line 1420 in 3f63419

(analyze-expression** ctx node)))

[ericdallo]

That analyze-expression** is used by a lot of places and the places that reg-var!, call it before calling the analyze-expression** function, so I think maybe we should reg-var! before calling the analyze-expression** here?
I'm kind of lost on that code :p

[borkdude]

@ericdallo My suggestion was to pass the :defined-by ... value as part of the ctx, so (assoc ctx :defined-by defined-by) and then pick up on it again in analyze-call if it's available. Not sure if that is the right approach, but worth a try. Since defs are usually not nested, this may work.

[ericdallo]

@borkdude just tested your suggestion with the hook and it worked like a charm :)
I changed the hook to:

...
{:node (with-meta new-node (meta node))
 :defined-by 'state-flow.cljtest/defflow}

And clojure-lsp got it correctly!

[borkdude]

@ericdallo Cool! Maybe add a test for it and then we can merge?

[ericdallo]

Sure, should I create a test for the stateflow macro?

[borkdude]

Just capturing the behavior that hooks can return that :defined-by key.

[ericdallo]

Done @borkdude, but the tests are passing local but not on the CI, it seems related with a wrong config-dir here, but I'm passing it just like other tests

[ericdallo]

Finally fixed @borkdude! IMO we are good to go