clj-kondo 2022-10-24 | Slack Archive

@borkdude looks like there may be a false positive for unused value when checking mutative transient calls?

src/cognician/base/coll.cljc:151:17: warning: Unused value
src/cognician/base/coll.cljc:162:16: warning: Unused value

(defn distinct-by
  "Keeps items which have distinct `(key-fn item)` results. Not lazy"
  ([key-fn]
   (fn [rf]
     (let [^clojure.lang.ATransientSet seen (transient #{})]
       (fn
         ([] (rf))
         ([result] (rf result))
         ([result input]
          (let [key (key-fn input)]
            (if (.contains seen key)
              result
              (do
                (conj! seen key) ;; <----- here
                (rf result input)))))))))
  ([key-fn coll]
   (let [^clojure.lang.ATransientSet seen (transient #{})]
     (persistent!
      (reduce
       (fn [res item]
         (let [key (key-fn item)]
           (if (.contains seen key)
             res
             (do
               (conj! seen key) ;; <----- here
               (conj! res item)))))
       (transient []) coll)))))

hiredman06:10:27

That is correctly flagged, it is an example of "bashing in place" which the transient docs say not to do

hiredman06:10:55

The transient mutating operations all sometimes return a new object and sometimes mutate the existing object, so you cannot ignore the return value

👍 3

robert-stuttaford07:10:42

fascinating, TIL, thank you!

Noah Bogart10:10:48

This is the whole reason for volatile! to exist, in fact

borkdude10:10:45

Not really, volatile is just a more performant atom without concurrency safety https://clojure.atlassian.net/browse/CLJ-1512

Noah Bogart12:10:46

Maybe I wasn’t clear enough. I don’t mean replacing stateful transients but acting as transducer state management (as demonstrated in your link). Robert’s transducer above is using transients to hold transducer internal state and that is the domain of volatile!.

borkdude12:10:50

In this case I would use loop instead of volatile!

Noah Bogart12:10:12

In a transducer?

borkdude12:10:43

oh right, yeah, not there

robert-stuttaford13:10:54

TIL

borkdude13:10:55

The problem seems to be manifesting with transient of map more than anything else though:

user=> (let [t (transient #{})] (doseq [i (range 10000)] (conj! t i)) (count (persistent! t)))
10000
user=> (let [t (transient {})] (doseq [i (range 10000)] (conj! t [i i])) (count (persistent! t)))
8

hiredman16:10:31

using transients not according to how all the docs say they can be used is less likely to lead to an observable bug with sets, but is still ignoring the docs

borkdude16:10:08

I do agree (and this is the main reason we now have :unused-value actually), but just to be clear, which docs are you referring to - the docstrings of transient and conj! or the Clojure website?

hiredman16:10:58

the clojure website

borkdude16:10:13

I think the exclamation marks send people in the wrong direction, thinking that it's bashing in place, so the return value doesn't matter that much

borkdude16:10:37

Perhaps the docstrings of transient related functions could emphasize this more

hiredman16:10:55

or a linter that marks using transients as an error until the user pinky swears that they have read the docs

borkdude16:10:40

or just :unused-value, and then they'll ask here and have "the conversation" on transients :)

robert-stuttaford19:10:34

"the conversation" 😂

dchelimsky17:10:51

Feature request (or "please show me where the existing feature that I can't find is" request): option to check that every file ends in a single newline. WDYT?

👍 1

borkdude17:10:00

@dchelimsky All available linters are here: https://github.com/clj-kondo/clj-kondo/blob/master/doc/linters.md All available configuration options are here: https://github.com/clj-kondo/clj-kondo/blob/master/doc/config.md Your request isn't in there yet, but is very similar to the :line-length linter, implementation-wise https://github.com/clj-kondo/clj-kondo/blob/master/doc/linters.md#line-length Feel free to post an issue and if you want I could guide you how to implement it as well

dchelimsky19:10:26

@borkdude it's been suggested that this should be a cljfmt thing. WDYT?

borkdude20:10:42

I'm fine with both

2022-10-24

Channels