Clojurians Log v2

I think performance of comp, concat, every-pred, some-fn should still be verified since those are fixed + varargs

user=> (time (dotimes [i 100000000] (let [f (comp inc inc inc inc)] (f 1))))
"Elapsed time: 2560.654917 msecs"
nil
user=> (time (dotimes [i 100000000] (let [f (comp inc (comp inc (comp inc inc)))] (f 1))))
"Elapsed time: 998.339292 msecs"

user=> (let [l1 '(+ 1 2) l2 '(+ 1 2) l3 '(+ 1 2)] (time (dotimes [i 10000000] (doall (concat l1 l2 l3)))))
"Elapsed time: 3636.4735 msecs"
nil
user=> (let [l1 '(+ 1 2) l2 '(+ 1 2) l3 '(+ 1 2)] (time (dotimes [i 10000000] (doall (concat l1 (concat l2 l3))))))
"Elapsed time: 2059.557875 msecs"

I think similar results will happen for the others, so perhaps it's better to remove those

curious. I wonder if the core team think this is rarely enough a perf bottleneck that nesting is an acceptable solution? couldn't find anything on ask.clojure so maybe it hasn't bothered anyone enough?

I guess the answer will be: it's context dependent. But given that it's not always better to unnest and unnesting is mostly cosmetic I think clj-kondo should err on the cautious side

yup, completely agree

yeah, I guess it's best to just disable this new linter for now then

if you're calling merge with a one-sized map, why not just call assoc?

sorry this was just to produce a minimal example, not the real code

the real code looks more like

(-> ri
    ...
    (merge (select-keys enrichment-data [:product-code :software-a :instance]))
    (merge (select-keys (get arn->enrichment arn) [:applicable-to :size-flexible :normalization-factor]))
    ...)

You can disable this specific linter at the call site. (Or globally, if this is something you do often.)

IMO it's sensible to enable this by default though, as I imagine the counterexamples are much rarer than the real issues it finds.

fair enough, i was thinking that if a value is being threaded then what is being interpreted as a redundant nested call is not as its something the author had intentionally called separately. But if that's not how others see it then i will make a change to our config.

Feel free to leave some feedback here: https://github.com/clj-kondo/clj-kondo/issues/2431

I'll generalize the issue a bit

ok, i'm still ruminating on whether the linter is right here, as i think some readability will be lost following it but it does guide you to better performance 😄

Is something like this both clearer, more performant, and linter compliant?

(let [product-enrichment (select-keys enrichment-data [:product-code :software-a :instance])
      arn-enrichment (get arn->enrichment arn) [:applicable-to :size-flexible :normalization-factor])]
  (-> ri
      ...
      (merge product-enrichment arn-enrichment)
      ...))

I think thats missing the point I was trying to convey, so apologies for my bad example. I'll try again with an example that intentionally threads twice because those steps are logically different...

(-> person
    (merge address-details)
    (merge billing-details)
    (assoc :account :premium))

(-> person
    (merge address-details billing-details)
    (assoc :account :premium))

I think the latter example is perfectly clear if the terms of the merge are extracted and named like this

Yes it probably is but my point is that logically distinct steps are collapsed due to the coincidental common implementation. If I factored those steps out to functions then I assume the linter wouldn’t complain. So I think the linter is prioritising performance, which is fine I guess if that’s the idiomatic style.

Ya, some linters are annoying, that's just the reality. Linters always grow to annoy you. Probably just disable this linter. I mean, this linter as a whole is really just stylistic,.there's no errors made here, it's not a recognized best practice, etc.

yep. Initially I was more enthousiastic about it than I am now. I might just disable it in the next release

also considering the perf regression it can cause (see issue)

merge was the one function that, when I saw it flagged in the linting output for the test corpus, felt there might be some pushback. Probably should have raised that in the PR. Seeing as this linter is primarily stylistic, I wonder if adding logic to exclude redundant nested calls inside threading macros could be worthwhile? otherwise, agree disabling by default might be best.

FWIW, I did like the unnesting of and/or that I found here: https://github.com/babashka/sci/commit/fd0cb735c8b332ec4c6603e7da8b69c01be96fad

also str in the above

thread feels like something that could be relevant, as in, only unnest when it's positionally nested

like, if you wrote literally (merge a (merge b c)) probably nobody would object against the linter telling you about that right

while (-> (merge a) (merge b) (merge c)) is visually pretty apparent already, so yeah this makes sense to me

I added this feedback here: https://github.com/clj-kondo/clj-kondo/issues/2431#issue-2665689772

> also considering the perf regression it can cause (see issue) as mentioned in the issue, there shouldn’t be any perf regression. hopefully the issue will become just a tracker for the issue of nesting directly inside threading macros, which is still a fair point

The issue does mention a few valid perf regressions

Am I missing something? Once you forget about < > etc I can't see anything..?

(see my comment on the issue)

Aaah sorry, thanks for the clarification.

Maybe we should add a test that it doesn’t warn for those, to make it future proof

Good idea. Will take a look

on second thoughts, no one is going to nest those fns, certainly not without getting a :type-mismatch lint error. because < and friends produce booleans, but expect numbers. e.g.

(< 1 (< 2 3) 4)

(< 1 true 4)

(lint! "(< 1 (< 2 3) 4)" conf/default-config)
;; => ({:file "<stdin>", :row 1, :col 6, :level :error, :message "Expected: number, received: boolean."})

Good point :)