Fork me on GitHub
#clj-kondo
<
2019-12-09
>
viesti09:12:08

hohum, thinking out loud, would it be possible to have a linter that warns about unregistered multimethod implementations..

viesti10:12:42

Since now, there might be a case where a required namespace is marked as unused, but is required because a multimethod is registered

borkdude10:12:01

@viesti In that case you should just not use an alias for the namespace and then clj-kondo will ignore it

qythium10:12:42

I've been playing around a bit with clj-kondo's type checker and found a few 'bugs' I wasn't sure were intentional

borkdude11:12:33

feel free to post them

qythium11:12:55

peek and pop are specced as taking vectors only whereas they actually take lists just fine

borkdude11:12:44

ok, that can be fixed

qythium11:12:48

Does the type system support union types?

borkdude11:12:48

feel free to post an issue

borkdude11:12:03

yeah, you can do something like #{:list :vector}

qythium11:12:29

nice, I'll do a PR soon

qythium11:12:41

there are also lots of predicate any->boolean core functions that aren't specced yet, is it just incomplete or are there some performance costs to having a larger database?

borkdude11:12:55

just incomplete

qythium11:12:53

Is it right to say that nil is always Associative? I can't think of a core function on maps that doesn't accept nils.

borkdude11:12:10

what's the background of that question?

qythium11:12:41

right now there seem to be a mix of :nilable/associative and :associative in the database

borkdude11:12:45

(merge nil {})
(assoc nil 1 2)
are both accepted by clj-kondo

qythium11:12:58

(dissoc nil 1) isn't

qythium11:12:35

or (select-keys nil [])

qythium11:12:03

and I was wondering if they could be simplified to a single type

borkdude11:12:09

I'm not sure what would be the consequence of spec'ing nil as associative

borkdude11:12:43

user=> (associative? nil)
false

borkdude11:12:07

so I guess all of those have to be spec'ed as nilable/associative

qythium11:12:58

the nilable return types don't seem to be currently propagated anyway

qythium11:12:36

(dissoc (merge) 1) doesn't show an error although merge returns nilable

borkdude11:12:07

this is because merge could also have returned a map.

borkdude11:12:47

relying on empty colls or nils in clojure is often an implementation detail

qythium11:12:22

yeah, but it's broader than the (currently incorrect) input spec of dissoc

borkdude11:12:36

(clojure.set/union)
#{}

qythium11:12:55

eg. I haven't tried it out yet, but given a function f that was specced as {:ret nilable/number}

qythium11:12:20

(inc (f)) should be a type error

borkdude11:12:41

that's not true, since f could have returned a number, this code could be correct.

borkdude11:12:13

often this relies on the input values of f

borkdude11:12:29

and the knowledge of the programmer who uses f in such a way that this is correct

borkdude11:12:39

clojure isn't statically typed

qythium11:12:56

hmm, alright - I guess this makes sense from the perspective of a dynamic language

borkdude11:12:17

clj-kondo only gives type errors when it knows for sure there will be an error

qythium11:12:29

I was thinking it would be a great way of spotting potential places where null pointer exceptions could occur

borkdude11:12:37

e.g. (inc "foo") will never be correct, since a string can never be a number

borkdude11:12:20

maybe that's more a job for core.typed

borkdude11:12:20

if you want to have such checks, you will have to convince the type checker that (f) will return a number, but often these checks rely on (if (number? (f)) (inc (f)) in dynamic code

borkdude11:12:54

it could be that CLJS already has this kind of type inference, but the type system in clj-kondo is pretty basic and does not attempt to resolve using occurrence typing

borkdude11:12:23

it might have one day, the type checker is pretty new

borkdude11:12:55

but for now it's better to err of preventing false positives

qythium11:12:28

Got it, thanks for the explanations! Type theory and tooling is such a huge space to cover

borkdude11:12:27

the implementation is pretty straightforward: https://github.com/borkdude/clj-kondo/blob/master/src/clj_kondo/impl/types.clj#L42 any x that could be an y, will be considered as an y to satisfy the spec

qythium11:12:27

last thing for now - what's the rationale behind the :seqable-out type?

qythium11:12:11

the few functions that are specced with return types of :seqable-out like map keep always return lazy seqs, unless I'm mistaken

borkdude11:12:19

@qythium seqable-out doesn't include nil, whereas seqable does. so you should not rely on seqable-out being a nil, that's the point of it

borkdude11:12:37

if you include nil in seqable-out, then a lot of type warnings won't work anymore

borkdude11:12:05

I have some notes on it here:

** why seqable-out?
If we would make seqable functions to return a seqable, that would include
string and would turn this warning off:

$ clj -A:clj-kondo --lint - <<< '(defn foo [^String _x]) (foo (next [1 2 3]))'
<stdin>:1:30: warning: Expected: string or nil, received: seqable collection.

borkdude11:12:22

oh so it was more about the string than nil I guess

borkdude11:12:48

seqable? is a bit of an odd predicate, it includes nil and strings, that's why I have a seperate one

borkdude11:12:07

it's also about nil, because (next ..) could return nil (since nil is seqable) and then nilable X are satisfied as well, regardless of what X is

qythium11:12:42

but functions like map and filter will never return any seqable other than a seq

qythium11:12:22

so eg. (peek (map ...)) should be a type error, whereas it currently thinks map could return a vector

borkdude11:12:33

yeah you should not rely on any of the seq functions to return a vector, so this is something we need to look at

borkdude12:12:10

feel free to post that in an issue with the other findings you have

qythium12:12:15

ah I was planning to submit a PR directly, I hope that's okay

borkdude12:12:48

for the nilable/associative stuff that's fine.

borkdude12:12:38

please compare the output of .circleci/script/performance in your PR branch to the one on master for any regressions

borkdude12:12:56

automating that somehow would be nice, so a diff of that script output would show up in the PR, but that's another issue 😉

borkdude12:12:10

I check the output of that script for any differences in linting output

borkdude12:12:26

It lints a bunch of libraries (clojure, clojurescript, cheshire, etc)

qythium13:12:21

Is that supposed to be run locally? I tried executing .circleci/script/performance and it resulted in find: ./clj-kondo: No such file or directory errors

borkdude13:12:58

it runs the locally compiled binary

borkdude13:12:06

in the current directory

borkdude13:12:40

but you can also just download the result from circleci once you made a branch / PR

borkdude13:12:59

I usually do the latter

qythium13:12:24

I've never worked with circleci before, so I have to actually create the PR first?

borkdude13:12:14

I don't think so actually

borkdude13:12:49

if you push your branch to github and then go to http://circleci.com/gh/<your-user-name>/clj-kondo I think you should see something

borkdude13:12:06

maybe you have to click on it once to "follow" the project

marc-omorain16:12:28

❤️ @borkdude you just saved me hours of debugging

{:keys [state pc input] :as state}
        ~~~~~ unused binding state

marc-omorain16:12:05

I had one binding shadow another

borkdude22:12:11

@qythium I think I would prefer multiple smaller PRs instead of one big PR for the type annotations

borkdude22:12:36

Since we already discussed the nilable/associative thing, that would be a trivial change

borkdude22:12:48

Other changes might be less trivial