Awesome!
Having turned off a couple of noisy linters at work, I'm down to 300 warnings from over 1,000, and hoping to get them all knocked out by next week.
If I see any Expected: warnings pop up, true or false, I'll let you know! It did find one redundant str based on type flow analysis (I think I mentioned that?), and the inst-ms issue.
This is amazing work!
(those two noisy linters were responsible for about a third of the original volume, so I've already knocked out another 300 since then)
more work coming this afternoon, closing a lot of "type" gaps. the machinery was mostly there already but I just needed to put more time into it
what are the noisy linters?
Missing docstring from Kondo. Unused public var from LSP. We have a lot of the latter because we tended to build speculative APIs to subsystems and ended up using very little of the surface area in some cases. I'll tackle those unused vars once I've gotten all the regular Kondo warnings cleaned up.
I wonder if I should change clj-kondo to warn about this:
(defn foo* [{:keys [x]}] (inc x))
where x is optional whereas it's used unconditionally in a non-nillable call, so the :x should actually be in :keys!cc @alexmiller
:keys! can still be nil, right?
true. well maybe this isn't good idea, you'll still get a warning in clj-kondo about x being nil with (foo {:x nil}) now though
I'd say this is a good idea
it can still be nil, sure, but it definitely must be present if that function is to run without an exception
@ericdallo can you check the new clj-kondo PR in lsp: π§΅
FAIL in clojure-lsp.feature.diagnostics-test/find-diagnostics-test (diagnostics_test.clj:234)
wrong arity for threading macros
Expected:
["user/foo is called with 2 args but expects 1 or 3"
"user/bar is called with 1 arg but expects 0"
"user/bar is called with 1 arg but expects 0"
"user/bar is called with 3 args but expects 0"
"user/foo is called with 2 args but expects 1 or 3"
"user/bar is called with 1 arg but expects 0"]
Actual:
["user/foo is called with 2 args but expects 1 or 3"
"user/bar is called with 1 arg but expects 0"
+"Expected: function, received: positive integer."
"user/bar is called with 1 arg but expects 0"
"user/bar is called with 3 args but expects 0"
"user/foo is called with 2 args but expects 1 or 3"
"user/bar is called with 1 arg but expects 0"]
This is probably correctFAIL in clojure-lsp.feature.diagnostics-test/find-diagnostics-test (diagnostics_test.clj:254)
wrong arity with annotations
Expected:
["user/foo is called with 2 args but expects 1"]
Actual:
[+"Expected: number, received: function." "user/foo is called with 2 args but expects 1"]
too?not sure what this is:
ERROR in clojure-lsp.refactor.command-test/command-arguments (reader.cljc:26)
Uncaught exception, not in assertion.
Exception: clojure.lang.ExceptionInfo: Unmatched delimiter: ) [at line 1, column 21]the tests usually are not flaky, probably related with kondo changes
I will be busy today, maybe tomorrow or so I can check, will be a little bit off since it's my birthday
those first two are genuine clj-kondo improvements
the third one is a flake probably
happy birthday!
thanks!
I've some exciting (but also potentially dangerous...) changes in clj-kondo so it'd be good if people could test those
I rerun the tests to make sure
merged!
nice! I've another coming up later today - but enjoy your birthday. this is going to hugely improve type checking in clj-kondo
oh awesome! sounds exciting
nix-flake is a test that keeps failing on CI btw - is it called flake for a reason? ;)
haha no, it's a test that checks if the flakes.nix is passing, it fails when we changed deps and don't update the deps-edn-lock file
there is a automatic PR for that, but it didn't open for some reason
Took me a few seconds to figure out why this got Condition always true:
(when-let [[lat lon] (map #(Double/parseDouble %) (str/split lat-lon #","))]
(when (valid-lat-lon? lat lon)
[lat lon]))
Yup, Kondo is right, that when-let should just be a let...I guess unless you put (seq ..) around the map
Well, either the map throws (`lat-lon` is not a pair of numbers) or it produces a pair of Double values, so it's never an empty sequence. Nor nil.
Q about cond-> I sometimes have a series of conditional things with a single "always do this" embedded. I typically use :then or :always for that condition, which of course Kondo flags as Condition always true. Is there an idiom for this that Kondo accepts, or do I need to just suppress the warning for cases where I mean to do this?
(cond-> (base-auv-photos-query member-id photo-id)
(= :uploaded section)
(add-uploaded-auv-photos)
(not= :uploaded section)
(add-rejected-or-deleted-auv-photos section)
:then
(sql/format))yes, there is one... I thought :always was respected, but let me search
Ah, yes, it's happy with :always. Thanks.
Turns out I used a lot of variants for this, including :-> π
Perhaps there is a configuration for this... but perhaps you're also fine with it ;)
Redundant do: I have some code that is (do (let [..] ..) false) and that is flagged as a redundant do. Technically, it's not: there are two expressions thereβa side-effecting let and the false literal. But the false literal could be moved into the let and then do would truly be redundant. I guess my question is: why does it flag this case?
Actual code:
(do
(let [into-frame (mod unix-time unit-seconds)]
(logger/warn "over_limit"
{:endpoint api-name
:threshold threshold
:unit unit-seconds
:unitstoexceed into-frame
:userid user-id
:clientid client-id
:ip (str ip-as-long)}))
false)And, yes, the logger/warn call happens to return nil, if that matters?
this seems like a bug to me. report please
huh wait
is false the last value of do or let?
do. See my channel post
just this does not report a redundant do for me, it does if I wrap it in a defn
ah yes, in defn it's redundant
Oh, of course! It's redundant because it's in fn!
that's it
Maybe there could be two different messages: redundant do (body has only one expr) and redundant do (inside fn/let/when/etc)?
I think we can do that... maybe just mention the real context
I think @jonurnieta will have a PR ready for this in 5 minutes... if I don't stop him ;)
hehe j/k - I'll take a look at your open PRs soon - and one for this is also welcome
π I'd be happy to take a look if you're interested in integrating it.
sure!
thanks @seancorfield and @borkdude!
Some of this code I'm linting hasn't been touched for six years. Just humming along, doing its thing. I'm down to 229 warnings now!
Ooh, just found some code untouched since 2018 that had a lint warning to get rid of π This is fun!
Okay, all the remaining 199 warnings are in tests...
maybe you will find some tests that are not wrapped in is - those are always great to find
@seancorfield merged the map key inference to master now. you should be getting a lot more type warnings in case things are really wrong (passing the wrong types in the expected keys or missing keys when something will NPE etc) Hopefully no false positives, but would really appreciate it you could double check in your corporate application
this stuff will play really nice with the new :keys! support too hopefully
Cool. Once LSP builds its nightly from that new Kondo, I'll clear my caches and restart VS Code to pick it up and see what it finds.
I got down to ZERO Kondo warnings across both src and tests at work. Now I have a lot of Splint warnings to tackle at some point, but that's a separate process (CI-adjacent).
nice! if you have something in splint that would make a lot of sense for clj-kondo too, let me know
Once I start in on those, I'll let you know. I think Splint is focused more on "structural" linting, but I'll run some things past you.
all we need is a little hindley-milner and we'll be set for life π
OK, from the changelogs merged to master: The type checker now infers much more. It derives argument types from how a param is used in the body:
(defn f [s] (subs s 1))
(f 42)
^ Expected: string, received: positive integer.
It types the keys and values of destructured maps:
(defn f [{:keys [x]}] (inc x))
(f {:x "foo"})
^ Expected: number, received: string.
It flows map types from return values into destructured bindings:
(defn cfg [] {:port "8080"})
(let [{:keys [port]} (cfg)] (inc port))
^ Expected: number, received: string.
It flags keys that are provably nil:
(inc (:y {}))
^ Expected: number, received: nil.
And it narrows the type of a local after it flowed through a known predicate:
(defn f [x] (if (string? x) (inc x) x))
^ Expected: number, received: string.
To lint your current project with master:
clj -Sforce -Sdeps '{:deps {clj-kondo/clj-kondo {:mvn/version "RELEASE"}}}' -M -m clj-kondo.main --lint src:test
Please let me know of any false positives.Kondo just spotted that one of our tests is passing a dummy keyword into some code being tested and said it should either be nil or a string. And it was "right" even tho' the test passes: the functions being tested do expect a string (but the test with-redefs one in a way that accepts a keyword instead).
So I fixed the mock to use a string instead of the keyword π
nice :)
this is the coolest thing I've seen in a long while! have you thought about infering "return types", for example:
(defn f [x] (if (str? x) 1 :something))
(f "hello there") ;; 1
(f 123) ;; :something
@meris.bahti hey, welcome! Yeah, we could do that, but it would complicate things a bit and I don't really know if we gain much by it since in most Clojure programs, the return types are regular, unless you really think out of the box :)