Fork me on GitHub
#clj-kondo
<
2020-11-05
>
Jim Newton08:11:02

What is the correct way to handle this warning from cl-kondo?

(defmulti rte-match
  (fn [rte _items & {:keys [promise-disjoint
                            hot-spot]}]
    (dispatch rte 'rte-match)))

(defmethod rte-match :sequential
  [pattern items & {:keys [promise-disjoint
                           hot-spot]}]
  (rte-match (rte-compile pattern) items :promise-disjoint true :hot-spot hot-spot))
It warns that promise-disjoint and hot-spot are not used in the defmulti, and that promise-disjoint is not used in the first defmethod. However, these keys ARE used in other defmethods. I'm not sure whether this is a clojure question or a clj-kondo question actually

Jim Newton08:11:07

shouldn't the defmulti declare all the keys which methods are allowed to consume?

delaguardo08:11:33

no, they don’t have to

Jim Newton08:11:04

"have to"? OK I agree it's not strictly necessary. but it seems to me that the defmulti should serve as documentation to applications which want to add methods. right?

delaguardo08:11:25

you can write it like this

(defmulti rte-match
  (fn [rte _items & _]
    (dispatch rte 'rte-match)))

(defmethod rte-match :sequential
  [pattern items & {:keys [hot-spot]}]
  (rte-match (rte-compile pattern) items :promise-disjoint true :hot-spot hot-spot))

delaguardo08:11:13

defmulti is a strange beast (imho) in terms of documentation

delaguardo08:11:37

since dispatch function can be detached from declaration of defmulti

Jim Newton08:11:40

If I do that, then where is the documentation for the callers of the methods to use, and where is the documentation for implementors of methods to use?

delaguardo08:11:52

(defmulti foo some-dispatch-fn)

delaguardo08:11:19

in a docstring after the name of defmulti

delaguardo08:11:32

(defmulti foo "docstring" some-dispatch-fn)

Jim Newton08:11:04

on the other hand, clj-kondo seems to have the feature that normal parameters which are not used can be prefixed with _ to disable the warning.

(defn foo [a b _c d] ...)
clj-kondo will not warn that _c is unused. Is there something similar for key arguments?

delaguardo08:11:17

by convention arguments beginning with _ (including single character _) are “might not be used”

delaguardo08:11:57

but you still can use them in function’s body

(fn foo [_x _y] (+ _x _y))

Jim Newton08:11:05

I see that clj-kondo fails to warn if I use an _ variable. That's unfortunate.

Jim Newton08:11:25

in Common Lisp you can mark them as the three different cases.

Jim Newton08:11:50

I.e., in CL i can declare the variable as either unused or maybe-unused

delaguardo08:11:23

as I mention this is a convention not a rule in clojure

Jim Newton08:11:34

and thus if I mark something as unused, but still use it i get a welcomed warning.

delaguardo08:11:46

_ or _x are just normal symbols that can be used as a names of vars

delaguardo08:11:57

(defn foo [_] (inc _))

Jim Newton08:11:20

indeed, but kondo is applying some conventions to the names to aid the programmer in static analysis, right, clojure doesnt care.

delaguardo08:11:26

exactly, and you always can switch this warnings on and off

delaguardo08:11:18

but maybe I’m wrong, better to ask @borkdude if it is possible to turn on warning about “unused vars” beeing used

borkdude08:11:09

I’m afk today until the evening. Check config.md for documentation about config.

delaguardo08:11:45

I will open an issue because there is no option (as I can see in config.md) to turn on such warnings

borkdude09:11:47

This has already been suggested in one of the issues. Can’t really Google it on my phone :-)

delaguardo09:11:38

my google fu failed me) will check one more time

Jim Newton13:11:52

ah that was my issue from long ago. I forgot about that. but now it rings a bell.

Jim Newton15:11:56

I looked for a bug all day. The bug occurred when I moved a bunch of functions around, basically refactoring name spaces. The problem was that when I run my tests from the lein command line they fail. But when I run them interactively the succeeded. The problem finally turned out to be I was defining the method in two different files. The defmulti is in a file whose namespace is genus and is abbreviated gns every where which requires it. The intent is that other files can add methods to gns/-disjoint?. Unfortunately two different files added the method

(defmethod gns/-disjoint :rte ...)
with two different implementations. QUESTION: Shouldn't clj-kondo warn me about this?

Darin Douglass15:11:33

on the surface that feels like a nice linter

Jim Newton15:11:58

it is perfectly legal to redefine methods. otherwise debugging would be more difficult. But static analysis should warn.

Jim Newton15:11:06

I'll file an issue.

Jim Newton15:11:01

everyone should rush there and give it a thumbs up. 👍:skin-tone-2:

delaguardo15:11:07

I think it doesn’t really matter where defmethod is redefining same dispatch value right now clj-kondo will not complain even about this

(defmulti foo identity)

(defmethod foo :x [_] :x)

(defmethod foo :x [_] :y)
all in the same namespace

Darin Douglass15:11:50

should it though is the question really

delaguardo15:11:17

(defmulti foo identity)
(defmethod foo :x [_] :x)
(defmethod foo :x [_] :y)
this example right now looks ok for clj-kondo but might be considered as perfect place for warning about defining different implementations for the same dispatch value of multi method

borkdude15:11:28

ok, makes sense

Darin Douglass15:11:42

that code above feels smelly and is more likely than not the cause of a bad copy/paste

3
delaguardo15:11:22

don’t mind right hand side of my examples ) the important part is on the left and it is not necessary bad copy/paste but could be some leftovers after refactoring or intensive REPL usage

3
Darin Douglass15:11:46

yeah artifacts of refactoring/updates/etc

3
borkdude15:11:49

makes sense

Jim Newton16:11:27

Agree that it doesn't matter where the method is defined. My comment was simply that it might be defined as a naked name in one place (defmethod foo :x ...) and as a decorated name elsewhere (defmethod mynamespace/foo :x ...) and that still deserved a complaint from kondo. in my opinion.

benny17:11:46

@borkdude do you want me to create an additional circleci path to create the with-profiles standalone.jar?

borkdude17:11:02

@b Just commented. I think it's better if you removed the native-image profile for the uberjar creation, since that needs JDK11 (it pulls in JDK11-specific deps)

borkdude17:11:51

But an uberjar created with Java 8 should be compileable with Java 11, so I think then it should work

benny17:11:04

I must be blind, but there is no mention of java8 in any .circleci/config.yml

benny17:11:31

it downloads graalvm11 only

borkdude17:11:57

circleci/clojure:lein-2.8.1
has java 8 by default

benny17:11:00

that's quite unfortunate because I had to use both profiles for it to work with native-image for me

benny17:11:31

I used the clojure:lein-2.8.1 image on my own system to generate the standalone jar and then transfer it to my real host

benny17:11:18

weird that I didn't get that error, that's why I wrote about the caching of .m2 as a potential cause

benny17:11:34

but I also build everything after I already had prepended the graalvm directory to my PATH

borkdude17:11:34

@b It might be better if the native-image step on CircleCI just posted the uberjar as an artifact as well. This way you are guaranteed to use the same one

borkdude17:11:31

So instead of in the JVM step

benny17:11:31

so sidestepping the normal jobs and creating an additional single job that does everything needed to create the standalone jar?

borkdude17:11:12

could do, but could also do it in the linux job

borkdude17:11:31

And then I will post that in the github releases in the future

benny17:11:32

oh wow, I just tried to reproduce it and I found why I thought it worked in my setup: I tested the circleci steps in circleci/clojure:lein-2.9.1 instead of lein-2.8.1

borkdude17:11:51

and that one has java11 right

benny17:11:55

lein-2.9.1 comes with java11 yeah

benny18:11:02

I'm using up all your circleci credits with my fails 😮

borkdude18:11:13

@b

"$VERSION" -eq "linux" 
should probably be:
"$CLJ_KONDO_PLATFORM" -eq "linux" 
(not sure about -eq, I always use = for strings in bash)

borkdude18:11:35

environment:
      LEIN_ROOT: "true"
      GRAALVM_HOME: /home/circleci/graalvm-ce-java11-20.2.0
      CLJ_KONDO_PLATFORM: linux # used in release script
      CLJ_KONDO_TEST_ENV: native
 

borkdude18:11:49

so you were close

benny18:11:11

fair enough and you're probably also right about the bash thing, -eq is for checking return code results 😉

borkdude18:11:14

but I think it might not hurt to copy the uberjar if it's not on linux

Michael W18:11:15

Technically -eq is for checking integers, = is for strings. In bash.

benny18:11:30

do you manually download the artifacts then? I didn't want to overwrite anything in case you automatically merged all the artefacts

borkdude18:11:39

yeah, I do it manually

borkdude18:11:13

@b

cp: cannot stat 'target/clj-kondo--standalone.jar': No such file or directory
  adding: clj-kondo (deflated 71%)

borkdude18:11:29

We might want to add set -e to the script

borkdude18:11:50

set -eo pipefail

benny18:11:45

man I don't know what I'm doing right now, sorry for not paying attention.

benny18:11:15

laptop on couch is not great for concentration

borkdude18:11:02

as a last step, I think we can now remove the uberjar from the jvm tests

benny18:11:27

I'll test the resulting standalone jar (but it looks good due to the size) and then remove the jvm uberjar creation and rewrite the commit message

benny18:11:06

looks good, but while we're at it. should we save the earth and also zip the jar? a simple zip invocation reduces the file size by more than 50%

borkdude18:11:36

a jar is already a zip file

😉 3
benny18:11:50

yeah I know

benny18:11:10

but I don't know how to tweak the settings to have it user better compression

benny18:11:26

while zip just reduces the filesize to 45%

borkdude18:11:23

I would say, let's leave it at this for now

benny18:11:00

:thumbsup: