Fork me on GitHub
#clj-kondo
<
2021-10-18
>
lread13:10:15

Before I raise an issue, thought I’d check here to see if this is now expected behaviour. I was reviewing clj-kondo dev docs and it has this example:

$ clojure -M:clj-kondo/dev --lint - <<< "(defn foo [x] (if-let [x 1] x x x))"
<stdin>:1:15: error: if-let body requires one or two forms
linting took 73ms, errors: 1, warnings: 0
But if I run this today, I get no warnings:
❯ clojure -M:clj-kondo/dev --lint - <<< "(defn foo [x] (if-let [x 1] x x x))"
linting took 115ms, errors: 0, warnings: 0
Retry with current version of clj-kondo:
❯ clj-kondo --lint - <<< "(defn foo [x] (if-let [x 1] x x x))"
linting took 50ms, errors: 0, warnings: 0

borkdude13:10:05

Seems to be a regression

borkdude13:10:28

Issue welcome. Could be nice to bisect to which clj-kondo version this changed and to see what caused it

👍 1
borkdude13:10:37

@UE21H2HHD

[email protected] /tmp $ clojure -Sdeps '{:deps {clj-kondo/clj-kondo {:mvn/version "2020.03.20"}}}' -M -m clj-kondo.main --lint - <<< "(defn foo [x] (if-let [x 1] x x x))"
<stdin>:1:15: error: if-let body requires one or two forms
linting took 171ms, errors: 1, warnings: 0
[email protected] /tmp $ clojure -Sdeps '{:deps {clj-kondo/clj-kondo {:mvn/version "2020.04.05"}}}' -M -m clj-kondo.main --lint - <<< "(defn foo [x] (if-let [x 1] x x x))"
linting took 71ms, errors: 0, warnings: 0

lread14:10:26

My pleasure

borkdude14:10:47

but now spec does check for it

borkdude14:10:51

so perhaps core could also change it

borkdude14:10:08

but I don't think they won't ;)

lread14:10:04

Huh. Ok interesting.

lread14:10:06

So it looks like they allowed for more args to support better assert errors? Or maybe some backward compat since it is called oldform?

borkdude14:10:39

I think so, but the clojure.spec doen't allow it

borkdude14:10:47

I posted a message about it in #clojure-dev

👍 1
borkdude14:10:27

if-some has the same problem

borkdude14:10:50

I think we must just must provide some overrides for this

borkdude14:10:53

let me try to do this

👍 1
borkdude14:10:02

yep, special casing works

borkdude14:10:36

There is some low hanging fruit to move the override thing to a different place which prevents us from doing this at runtime each time

borkdude14:10:07

but not high priority, might be fun

borkdude14:10:35

The :doc issue is still coming right

borkdude14:10:55

oh lol, you just posted it

borkdude14:10:12

@UE21H2HHD did you know you can write jet --query ':foo :bar :baz' ? :)

lread15:10:16

Oh you just optimized my jet workflow! jet

lread15:10:05

I shall also raise one more issue. Not sure if you’ll want to it fixed. For, what seem to me obscure, signatures of defn and defmacro, there can be an optional 2nd attr-map, which clj-kondo currently ignores.

(defn name doc-string? attr-map? ([params*] prepost-map? body) + attr-map?)
(defmacro name doc-string? attr-map? ([params*] body) + attr-map?)

borkdude15:10:28

you mean in the tail end right?

borkdude15:10:39

I don't think anyone uses that

borkdude15:10:51

can support later

borkdude15:10:58

not high priority now

lread15:10:04

I think I won’t bother with an issue then.

lread15:10:34

I just learned it existed yesterday!

borkdude15:10:44

actually SCI does support it, let me look up the issue of why I supported it, it was because of some lib

lread15:10:46

Could grasp find usages in the wild?

lread15:10:11

I would guess it would be very rare, but really don’t know.

borkdude15:10:36

yes, grasp would find this

borkdude15:10:54

we can do a grasp

borkdude15:10:04

actually we can use the clojure core spec for defn here in grasp I think

lread15:10:44

Ok, maybe I’ll raise the issue, and your grasp results can be a good enough reason to close it. Documentation of why we don’t do it, in case it crops up again. Or grasp might tell us we should do it.

borkdude16:10:58

yeah good idea

borkdude16:10:23

I'm cooking up a grasp

borkdude16:10:47

I'm grasping like this:

(ns trailing-meta)

(require
 '[clojure.core.specs.alpha :as specs]
 '[clojure.spec.alpha :as s]
 '[grasp.api :as g])

(s/def ::defn (s/cat :defn #{'defn} :args ::specs/defn-args))
(s/def ::defn-trailing-attr-map (s/and ::defn (fn [conformed]
                                                (->> conformed
                                                     :args :fn-tail second :attr-map))))

(def matches (g/grasp "/Users/borkdude/.m2" ::defn-trailing-attr-map))

;; (prn (s/conform ::defn-trailing-attr-map (first matches)))
(run! (fn [match]
        (print (meta meta) " " match)
        (println)) matches)

borkdude16:10:53

but so far no match

borkdude16:10:15

ah I found something

borkdude16:10:47

but I made a typo in my program so I didn't see where that was ;)

borkdude16:10:51

running again

lread16:10:04

Oh. You might curse me. Because it might kind of work already.

clojure -M:clj-kondo/dev --config '{:config-paths ^:replace [] :output {:format :edn :analysis {:var-definitions {:meta true}}}}' \
  --lint - <<< '(defn x ([]) {:doc "attr-map2"})' \
 | jet --query ':analysis :var-definitions first' --pretty
{:fixed-arities #{0},
 :end-row 1,
 :meta {:doc "attr-map2"},
 :name-end-col 8,
 :name-end-row 1,
 :name-row 1,
 :ns user,
 :name x,
 :defined-by clojure.core/defn,
 :filename "<stdin>",
 :col 1,
 :name-col 7,
 :end-col 33,
 :doc "attr-map2",
 :row 1}
Might be just the merge order that is broken when multiple specified? I’ll do more tests.

borkdude16:10:58

ah, it's omniconf

borkdude16:10:10

I think that was also the reason I supported it in SCI

lread16:10:25

Ok there is something amiss certainly with :doc derivation from this source but we already have an issue for that. And I think the new :meta analysis might not merge the 2nd attr-map properly if at all. Soo… I do have a small bit of work here.

borkdude16:10:02

> {:line 272, :column 1, :url "jar:file:/Users/borkdude/.m2/repository/com/grammarly/omniconf/0.4.3/omniconf-0.4.3.jar!/omniconf/core.clj"}

lread16:10:26

That grasp tool is very cool!

borkdude16:10:50

{:line 272, :column 1, :url "jar:file:/Users/borkdude/.m2/repository/com/grammarly/omniconf/0.4.3/omniconf-0.4.3.jar!/omniconf/core.clj"}
{:line 312, :column 1, :url "jar:file:/Users/borkdude/.m2/repository/com/grammarly/omniconf/0.4.3/omniconf-0.4.3.jar!/omniconf/core.clj"}
{:line 372, :column 1, :url "jar:file:/Users/borkdude/.m2/repository/com/grammarly/omniconf/0.4.3/omniconf-0.4.3.jar!/omniconf/core.clj"}
{:line 390, :column 1, :url "jar:file:/Users/borkdude/.m2/repository/com/grammarly/omniconf/0.4.3/omniconf-0.4.3.jar!/omniconf/core.clj"}

borkdude16:10:56

That's all I found in my .m2

borkdude16:10:17

I'll post this info in the SCI issue

borkdude16:10:29

and in the clj-kondo issue if you make one

lread16:10:33

Yeah, I think I have to do a bit more digging to see what is not working!

lread16:10:20

Yeah… :meta analysis is missing the 2nd attr-map. I can raise for that.

lread16:10:02

And at same time fix meta merge order (which might be fixed by :doc derivation fix).

lread20:10:40

Oh, I guess we’ll have to address this one too:

❯ clj-kondo --config '{:config-paths ^:replace []}' --lint - <<< '(defn x ([]) ([x] x) {:added "attr-map2"})'
<stdin>:1:22: error: Function arguments should be wrapped in vector.
linting took 8ms, errors: 1, warnings: 0