`:defined-by` in analysis output uses `:lint-as` rather than symbol itself

[SevereOverfl0w]

[ Leave this text in if you want to promote it. ]

To upvote this issue, give it a thumbs up. See this list for the most upvoted issues.

[ To keep development of this project going, consider sponsoring. If you are already a sponsor, thank you! You can remove this message. ]

Is your feature request related to a problem? Please describe.

I'm trying to find uses of a particular var, when defined inside another custom type of var. In this case, a is within defspec (which isn't valid, and I want to find out how rampant this problem is).

Describe the solution you'd like

:defined-by shouldn't use nil for lint.as/def-catch-all, nor should it copy the value from :lint-as to :defined-by.

That is, for:

{:lint-as {ns/a clj-kondo.lint-as/def-catch-all ns/b clojure.core/def}}

:defined-by should be ns/a and ns/b respectively for any uses. Rather than nil and clojure.core/def as it is now.

Describe alternatives you've considered

I'll probably take another stab at solving my problem with grasp and postwalk.

Additional context

https://clojurians.slack.com/archives/CHY97NXE2/p1683106125566899

[borkdude]

I agree it's not optimal to use the lint-as value (rather than key) for defined-by.

In some tools like https://github.com/borkdude/carve and probably also clojure-lsp, :defined-by is used to make decisions. E.g if you use {:lint-as {my/defprotocol clojure.core/defprotocol}} then vars created using that won't be stripped by carve:

https://github.com/borkdude/carve/blob/14a3a5db4b372e33d20763a8d1344bddae72c735/src/carve/impl.clj#L199-L204

Also defined-by may be used in clojure-lsp in similar ways, @ericdallo?

So if we would change :defined-by to the lint-as key instead of val, I think there should be another key:

:defined-by-as

that we could use instead? Perhaps there are other solutions possible as well.

[ericdallo]

In clojure-lsp, defined-by is mostly used for excluding a var when defined by via the seting :linters :clojure-lsp/unused-public-var :exclude-when-defined-by #{...} and to know when a var is a interface or protocol or defmulti which is pretty important to handle those vars differently

[borkdude]

I guess we could change :defined-by to the original lint-as key and additionally you could compute the :lint-as val via the clj-kondo config yourself. This would require a change in carve and lsp, but I think it would be more correct.

[ericdallo]

I think it's not that easy, there are defined-bys defined in hooks, like state-flow here, with that, we can have the :exclude-when-defined-by config for those hooks

[borkdude]

@ericdallo I'm not following why it would be problematic here?

[ericdallo]

I'm not sure I understand what will be the change on clj-kondo, so not entirely sure how it will affect lsp, could you elaborate with an example?

[borkdude]

@ericdallo The change would be that if a user provides a configuration:

:lint-as {my.ns/def clojure.core/def}

and you write:

(my.ns/def x)

:defined-by for var x would no longer be clojure.core/def but my.ns/def (so people can see what macro or function defined that var).

[borkdude]

We could include in :var-definitions for my.ns/def that it is :linted-as clojure.core/def so you could look up that is (at least syntactically) behaves as clojure.def so we could still have that info.

[ericdallo]

I see, probably some breaking changes would happen on places that expect to defined-by be always the macro and not the key of the lint-as, but makes sense, maybe we should have a :behave-as clojure.core/def for those cases which would just be the rename of current defined-by to behave-as

[ericdallo]

My biggest concern is: there are configs (used in Nubank for example) that have :exclude-when-defined-by #{state-flow/defflow} that work for lots of vars that are linted as :hooks {:analyze-call {my-ns/defflow nubank/state-flow}}, if we have this breaking change, that exclude-when-defined-by config would stop working and we would need to declare each one of those new defined-by values

[borkdude]

@ericdallo What if we add extra information on the :var-definition so you can pick up the :lint-as value in there?

[ericdallo]

That would work, still a breaking change right?

[borkdude]

Yeah, the breaking change would not be so great. I guess we could just add another :defined-by* key like I proposed in the original comment.

[ericdallo]

I think it's ok, we can live with that if we have the old value as another key name somewhere

[borkdude]

I think it's ok

What exactly do you think is ok?

[ericdallo]

I was ok with the breaking change as the current value of :defined-by looks not ideal indeed, use the :lint-as key looks better, but I just realized we have the option to do the breaking change in clj-kondo but on clojure-lsp change to use the new field and things would keep working, althought would be a little bit confusing to have :exclude-when-defined-by not really using the :defined-by but the new :defined-by* 😂

[borkdude]

:defined-by-lint-as ...

I guess you could take both values into account for excluding things.

[borkdude]

I'll also have to update carve but I think this is for the better.

[ericdallo]

yes, looks for the better indeed

:defined-by-lint-as ...

maybe only :lint-as?

[borkdude]

@ericdallo Yes, we could add :lint-as but if you want to exclude some var that was :defined-by my.ns/def and my.ns/def was linted as clojure.core/def you would need to look up the :lint-as key in another var. Is this not a problem for you?

[borkdude]

@ericdallo An example by adding :lint-as to show what I mean:

{:var-definitions 
  [{:name my-def :ns my.ns :lint-as clojure.test/deftest}
   {:name x :ns my.ns :defined-by my.ns/my-def}]}

In order to know that you would have to "exclude" the var x from unused public vars, because it was linted as clojure.test/deftest you have to make two hops: look at the :defined-by key for the x var, which is my-def and the look up the :lint-as key for my-def which is clojure.test/deftest.

[ericdallo]

I see, any reason why not include the :lint-as to the same var-def? this way we know that var-def is defined as my.ns/def and linted as clojure.core/def

[borkdude]

@ericdallo I'll talk to you on Slack since I feel we're talking past each other.

[borkdude]

Digging into decision matrix:

https://docs.google.com/spreadsheets/d/14bDj85LJwUrImLbt87oz9-_OMfdorU16sT4FqFdBL1s/edit?usp=sharing

[borkdude]

@SevereOverfl0w There is now a new key :defined-by* which contains the :lint-as key rather than the value. Could you test if this works for your purposes before I merge?

@ericdallo Could you look over this PR before I merge?

[ericdallo]

@borkdude looks good to me, still think the name is not ideal, I'd suggest :parent-lint-as

[borkdude]

@ericdallo This isn't what it is: it is NOT the linted-as name, it is the original name

[borkdude]

Other names welcome but this is what :defined-by should have been, but we can't change this due to breaking existing tooling.

[ericdallo]

Ah true, we didn't change the old value (that would be the lint-as indeed) I just thought that maybe another option to the DM is to rename current one to parent-lint-as and the new one leave defined-by, not sure it's too late now

[borkdude]

@ericdallo

https://docs.google.com/spreadsheets/d/14bDj85LJwUrImLbt87oz9-_OMfdorU16sT4FqFdBL1s/edit?usp=sharing

What you're suggesting is basically option 3 with a different/better name.

[ericdallo]

@borkdude not sure, I'm a little bit confused if current defined-by uses lint-as key or val, I thought used lint-as val already and OP wants lint-as key.

If so, then I see option 3 different and should not be breaking change. Sorry for the confusion, I'm really trying to understand that 😂

[borkdude]

@ericdallo You're right, I swapped the word key and val in [3]. I fixed that now in the excel sheet, sorry about that.

To re-iterate:

The problem is that the current :defined-by key uses the :lint-as val (so let's say clojure.core/def instead of my.ns/def.

Solution [1] was implemented in this PR by adding a new key :defined-by* which points to my.ns/def instead of clojure.core/def. I personally prefer [1] over [2] and [3] since existing tooling would not break (carve, lsp, ...). A better name for :defined-by* is still on the table. Or if you have really strong arguments againt [1], we could re-consider [2] or [3].

[ericdallo]

Gotcha, now with that fixed I can say [1] works. However, I think [3] would be ideal even with the breaking change, for LSP I could change every place to use the new parent-lint-as and have [2] settings :exclude-when-defined-by and :exclude-when-parent-lint-as and would look correct in the end and way more clear than [1] which introduces a new field with a not good name and the old field is not really is correct.

I can't think of a better name for defined-by* for [1] since it has a "wrong" semantic

[borkdude]

I guess I missed a 4th solution:

A configuration option to let :defined-by preserve the function and not point to the :lint-as val.

{:analysis {:var-definitions {:defined-by :lint-as-val}}} ;; the current state

and

{:analysis {:var-definitions {:defined-by :lint-as-key}}} 

I'll add it to the matrix as [5]. But since there are use cases to have both keys, this isn't a complete solution.

Agreed that option 3 is more attractive for the long term, so I guess we should wonder: what is the impact of the breaking change beyond clojure-lsp and carve. Maybe not that many, so I guess we can manage. I'm not to keen on the "parent" in the name since parent can mean all kinds of things whereas :defined-by still captures the meaning of "a var came into existence by the call to a macro". Perhaps :defined-by->lint-as or :defined-by+lint-as, meaning: the defined var was linted as.

[borkdude]

Other users of the current :defined-by key:

• https://github.com/borkdude/carve
• https://github.com/borkdude/api-diff (not used that much I think)
• https://github.com/borkdude/quickdoc
• https://github.com/phronmophobic/dewey

[borkdude]

If we are re-considering option [3], the only downsize is "breaking change" but in how far is this breaking change severe? I guess if we just upgrade the above tools to work correctly with the newest clj-kondo key as follows:

(or (:defined-by->lint-as var) (:defined-by var))

instead of

(:defined-by var)

we're back in business, so perhaps we should just bite the bullet and go for [3] then. If I haven't received better suggestions than :defined-by->lint-as I'll go with that one.

[ericdallo]

I like option 4, doesn't solve everything, but solves OP's issue.

I'm ok with :defined-by->lint-a 👍🏻

Regarding the breaking change, I just mentioned it may be worth it because I know is not a feature used a lot by LSP users, I'd guess Nubank is one of the few that use that and I can quickly fix Nubank's codestyle lib to fix the breaking change. So from LSP the affected breaking change would be the :exclude-when-defined-by setting which is what is exposed to users, but I could add a :exclude-when-defined-by->lint-as or just do a (or ...) considering both fields in the same setting (would that avoid the breaking change? 🤔 )

[borkdude]

One problem with option [4] is that clj-kondo internally also uses the :defined-by key and if the config sets this to a different value, clj-kondo itself will also work differently.

I think we'll go for [3] then. We can already start preparing external tools before the next release with:

(or (:defined-by->lint-as var) (:defined-by var))

as this expression should work before and after the change.

[ericdallo]

Sounds good!

[SevereOverfl0w]

All sounds good to me, too!

[borkdude]

@SevereOverfl0w I assume you can use clj-kondo as a git dep now to try it out. Let me know if this works for you.