Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
borkdude opened this issue May 28, 2024 · 6 comments

Comments

@borkdude
Copy link
Contributor

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

clojure.tools.reader/read {:var-kind nil, :macro nil, :io-fn true, :evals-exprs true, :lazy false, :warn-if-ret-val-unused true},

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
Copy link
Collaborator

vemv commented May 28, 2024

Thanks! Yes, fully makes sense.

I'll release this asap.

@borkdude
Copy link
Contributor Author

borkdude commented May 28, 2024

@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
Copy link
Contributor Author

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
Copy link
Collaborator

vemv commented May 28, 2024

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
Copy link
Contributor Author

In addition: clojure.tools.reader/read

@borkdude
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants