search

jonase / eastwood

Clojure lint tool
1.09k stars 66 forks source link

:warn-if-ret-val-unused should not be true for clojure.tools.reader.edn/read #451

Open borkdude opened 5 months ago

borkdude commented 5 months ago

I believe that :warn-if-ret-val-unused should not be true for clojure.tools.reader.edn/read:

https://github.com/jonase/eastwood/blob/34c729cd7a1e562eacfa6db497deffa08f7eeb47/resource/var-info.edn#L1238

as this function may be called just for side effects, like here:

  (require '[clojure.tools.reader.edn :as ctr.edn]) 
  (let [rdr (whatever)]
    (ctr.edn/read rdr) ;; omit first form
    (ctr.edn/read rdr) ;; return second form

One such case can be found here: https://github.com/clojure/tools.reader/blob/0d3a7cd946e0ef377b7fe2c85df6cf940ff39c81/src/main/clojure/clojure/tools/reader/edn.clj#L299-L302

clj-kondo uses the :warn-if-ret-val-unused field for detecting function calls that could be omitted and the above caused a false positive.

vemv commented 5 months ago

Thanks! Yes, fully makes sense.

I'll release this asap.

borkdude commented 5 months ago

@vemv I found the following vars for which :warn-if-ret-val-unused is true but also :side-effect is true:

I'm not sure if that is intentional or not. If it is, then perhaps the above is also intentional?

$  bb -e '(->> (slurp "https://raw.githubusercontent.com/jonase/eastwood/34c729cd7a1e562eacfa6db497deffa08f7eeb47/resource/var-info.edn") edn/read-string (keep (fn [[k v]] (when (and (:warn-if-ret-val-unused v) (:side-effect v)) k))))'
(clojure.data.codec.base64/encode clojure.math/random clojure.core/read+string clojure.core/random-uuid clojure.core/random-sample clojure.java.jdbc/get-connection clojure.data.csv/read-csv clojure.core/rand clojure.data.codec.base64/decode clojure.data.csv/read-csv-from clojure.core/rand-int clojure.core/rand-nth)
borkdude commented 5 months ago

Most of the above vars make sense to me when you look at them case by case, but perhaps read is a good one to change still.

vemv commented 5 months ago

Most of the above vars make sense to me when you look at them case by case

Indeed, I was going to say this. They're bit of a mixed bag.

It also doesn't help that side-effect isn't a very precise term.

but perhaps read is a good one to change still.

Yup

borkdude commented 5 months ago

In addition: clojure.tools.reader/read

borkdude commented 5 months ago

Btw, I was able to work around this manually, no need for a quick release