clj-kondo

https://github.com/clj-kondo/clj-kondo
Yehonathan Sharvit 2021-04-29T14:42:14.372900Z

A linter idea: When a defmulti is defined as a keyword, warns in case the arity of one of the methods is greater than 1. Otherwise keyword-as-a-function treats the second argument as a default value. This mistake in my code caused an out of memory error due to the caching mechasim of multi-methods. https://ask.clojure.org/index.php/10532/memory-leak-using-the-default-method-of-a-multimethod

borkdude 2021-04-29T14:48:38.373800Z

Thanks. It's a pretty niche problem I think, but I'll keep it in mind

Darin Douglass 2021-04-29T14:58:08.375300Z

speaking of: thoughts on having a linter that checks that a defmulti dispatch fn is a var? if you provide a fn, trying to later change that fn in a repl is tricky

borkdude 2021-04-29T15:02:39.375500Z

also sounds pretty niche :)

borkdude 2021-04-29T15:03:00.375800Z

do you mean: give a warning so you can remove it before going into production?

Darin Douglass 2021-04-29T15:25:36.377600Z

i guess you could consider it niche, though i suspect it's good practice everywhere defmultis are used

Darin Douglass 2021-04-29T15:26:07.378300Z

it'd definitely be a warning, though in production theoretically fn dispatch values are ok since you shouldn't be mucking around in a repl in production 😛

borkdude 2021-04-29T15:27:16.378600Z

I don't understand which of the two you find good practice

Darin Douglass 2021-04-29T15:29:36.380200Z

;; this is better than ...
(defn dispatch
  [args]
  <some logic>)

(defmulti my-multi #'dispatch)

;; ... this
(defmulti my-multi (fn [args] <some logic>))

borkdude 2021-04-29T15:30:35.380600Z

and you want clj-kondo to warn on the latter? I don't think I agree with this, but maybe I just haven't run into trouble enough with re-defining multi-methods. In my experience this is pretty rare.

Darin Douglass 2021-04-29T15:33:13.382200Z

i think it would be useful, we've been hit a couple of times by it. the solution is to just reload your repl, but still it's annoying

2021-05-01T10:30:11.387100Z

@didibus The reason you don’t want to redefine multimethods is because this would remove all extensions in other namespaces potentially. For this same reason (def my-multi nil) is not (always) a good idea. I’m using the trick of @ddouglass as well. I don’t use defmulti in performance heavy areas normally so (defmulti my-multi #'dispatch) is a better approach for my workflow

2021-05-01T10:39:52.387600Z

I saw that, normally actually I have it in a comment block above and I just know to evaluate it in my REPL after I changed my dispatch FN

2021-05-01T10:40:33.387800Z

Most of the time, when I change my dispatch-fn, it's because it didn't work and I don't mind re-evaluating the defmethods afterwards

2021-05-01T10:41:12.388Z

But I see the advantage of Var indirection over it, appart for performance, seems you get all the benefits and none of the negatives

âž• 1
2021-05-01T10:42:31.388300Z

yeah for me it was the best approach. I have a project where I have 5 or more namespaces extending one multimethod. So this means i have to reload those 5 namespaces. I have had long debugging sessions because of this. So I decided to always go for the var approach

2021-05-01T10:45:30.388800Z

Maybe the idea of having a macro that would remove the indirection in production could be useful here. To remove the performance argument

borkdude 2021-04-29T15:34:03.383100Z

Originally I thought you were arguing the opposite: don't use a var because this is worse for performance :)

borkdude 2021-04-29T15:34:30.383500Z

I guess you could even write (when dev? #'foo foo).

Darin Douglass 2021-04-29T15:35:54.384500Z

Ah yah, rereading what I typed I can see the confusion, sorry bout that :P

2021-04-29T15:41:16.384600Z

You don't need to reload the REPL

2021-04-29T15:42:08.384900Z

Just do: (def my-multi nil) and then re-evaluate the defmulti and it's going to work

Darin Douglass 2021-04-29T15:42:29.385100Z

oh interesting, i hadn't thought of that

Darin Douglass 2021-04-29T15:43:12.385300Z

i'll have to rethink my stance then (though i suspect i'll still lean towards the "use a var" side)

2021-04-29T15:44:06.385500Z

To be honest, I don't know why defmulti behaves as it does, but for some reason they went above and beyond to prevent it from being redefined and I wonder why, like is there a good reason

2021-04-29T15:45:56.385700Z

What I tend to do is this:

(def my-multi nil)
(defmulti my-multi (fn [...] ...))
That way when I reload the namespace in the REPL it reloads the defmulti as well in case I changed the dispatch-fn. But in prod I'm not messing with the standard defmulti in case there's a reason it is how it is.

Yehonathan Sharvit 2021-04-29T16:20:43.386400Z

very niche but when it happened it cost me 5 days of painful of investigation with all kind of JVM profiling tools