shadow-grove icon indicating copy to clipboard operation
shadow-grove copied to clipboard

feat: clj-kondo defc linter

Open rschmukler opened this issue 2 years ago • 14 comments

Introduce a linter for the defc macro. It has the following features:

  • Ensure that a defc has a render definition
  • Ensure that only valid hooks can be defined inside defc (ie. hook, render, bind, event)
  • Ensure that events are defined with valid arity
  • Provide information to clj-kondo so that symbols from bind can be linted (eg. unused vars)

Ultimately this could be extended with other things (eg. ensuring that definitions are provided for events specified in render functions, linting hiccup, ensuring render uses <<, etc.) but this felt like a good-enough start.

linter_example

rschmukler avatar Jul 13 '23 16:07 rschmukler

I have never used clj-kondo, so I have no idea what this is or how it works.

It'll likely take some time before I get a chance to figure this out. I like the idea though.

thheller avatar Jul 13 '23 17:07 thheller

Sure! I've done a fair bit of work w/ clj-kondo hooks so feel free to ask (here or Clojurians slack) if you have any questions or want any specific functionality.

rschmukler avatar Jul 13 '23 22:07 rschmukler

This is great and something I've been putting off doing for a long time :) Thank you!

Some observations:

  • << can be used in place of render, so that should be supported.
  • Technically, event can be defined with arity 1-3. I use this often, to avoid e.g. (event :e [env _ _]). (It seems it will compile even with (event :e [] ...), though I then get an error related to get_hook_value when I trigger the event.)
  • Probably not important, but there is an expected ordering to hooks, iirc. Something like binds need to be placed before render/<<, event hooks can go both before and after.

After making minor modifications for the first two points above, my project is linted correctly. Thanks again!

zeitstein avatar Jul 14 '23 05:07 zeitstein

@zeitstein Thanks for the feedback! I have updated the PR to handle << and allow for arity 1-3 on event. I also added the order detection on ensuring that there are no binds after render.

rschmukler avatar Jul 14 '23 15:07 rschmukler

Hey @thheller I wonder if you might reconsider merging this for those of us that do use clj-kondo - I just rebased it in case you're interested.

rschmukler avatar Dec 15 '24 16:12 rschmukler

I ended up updating this to also support effect hooks with the following:

  1. Checks that effect's "when" is either: :mount, :render, :auto or a vector of dependencies.
  2. Checks that the second argument to effect is an rarity 1 binding vector

rschmukler avatar Dec 15 '24 23:12 rschmukler

Also @thheller if you still aren't familiar with clj-kondo but want to see what this does... In a project with theller/shadow-grove pointed at my branch, you can run:

clj-kondo --lint $(clojure -Spath) --dependencies --skip-lint --copy-configs

Followed by

clj-kondo --lint src/

And you will get error messages for invalid usage of shadow-grove

rschmukler avatar Dec 16 '24 00:12 rschmukler

I still have not looked into how clj-kondo works, or rather how it discovers these files. Do they actually need to be in the library .jar itself or just on the classpath?

I don't mind merging this, but I'd rather know what I'd get myself into beforehand. Also if anything skip the resources directory and move it into src/main instead.

thheller avatar Dec 16 '24 08:12 thheller

@thheller clj-kondo works by looking for clj-kondo.exports/<org>/<project> on the class path of the library. Here's the section in the docs if you want to read more. Otherwise, I've moved it into main as requested.

rschmukler avatar Dec 16 '24 16:12 rschmukler

@rschmukler This is useful, but perhaps put this into it's own repository so it can be used only by those using clj-kondo.

There is no need for this to be in the shadow-grove repo right?

stuartrexking avatar Jun 29 '25 01:06 stuartrexking

The advantage of merging it into the repo is that it can be used by clj-kondo automatically just by virtue of having the dependency of shadow-grove in your project.

This is somewhat common practice, eg. manifold exports clj-kondo

rschmukler avatar Jun 29 '25 02:06 rschmukler

FWIW the problem isn't merging this. The problem is maintaining this over time. I have no interest in doing that, since I'm unlikely to ever use this myself. Also most of the things this checks, the compiler already checks and will warn/error out itself, so I'd just get the same infos during compilation wihout an extra tool running. If the compiler errors are worse then what clj-kondo gives then I'd rather make those errors better.

thheller avatar Jun 29 '25 05:06 thheller

Do as you will @thheller - I'm happy maintaining my fork with it if you don't want to merge it. For me, I appreciate the feedback as I type, which is what is offered with clj-kondo - but I realize not everyone has the same tool chain. I wrote it for myself but figured I would share since clj-kondo is pretty popular in the ecosystem.

If the API does diverge, and nobody (eg. me) is willing to update it, you can just delete the hooks and be in no worse state than where you are right now.I personally think you need to obligate yourself to maintaining it to the same level of functionality as the library itself.

That being said, I'm not currently using shadow-grove in anything so I'm not sure if the hooks are even up to date with whatever the latest API is, and since it hasn't been merged, I haven't been following very closely myself.

rschmukler avatar Jun 30 '25 00:06 rschmukler

I'm using this everyday and it is still up to date. The important thing is that it exists as an option. We should point it out to new users, wherever we point to. If @thheller decides to include it in the repo, I can also help maintain it.

@thheller One thing I noticed compared with the linter is the compiler doesn't warn on unused bindings in defc?

zeitstein avatar Jul 09 '25 08:07 zeitstein