Fork me on GitHub
#clj-kondo
<
2024-08-02
>
borkdude17:08:37

Working on this issue now: https://github.com/clj-kondo/clj-kondo/issues/916 tl;dr: you should not refer to a destructured value in the same map, inside of an :or because the order of a map is undefined example:

{:keys [a b] :or {a b}}
usually this code works, but it's by accident. if we swap a and b in :keys it already falls apart:
user=> (let [[{:keys [b a] :or {b (inc a)}}] {}])
Syntax error compiling at (REPL:1:28).
Unable to resolve symbol: a in this context
While implementing the linter I found this case: https://github.com/clojure/clojurescript/blob/47a7bfdefd156027b7b98cb483ddd93add71fbb2/src/main/clojure/cljs/repl.cljc#L1051 Not only does this example use a binding in :or from the same destructuring map, the value is even more undeterministic since it depends on a default in the same :or map

😅 1
🆒 1
clj-kondo 1
ambrosebs19:08:42

same issue with {:keys [flush] :or {flush flush}} right?

ambrosebs20:08:13

the similarities being that in one case it resolves to a var, the other it resolves to a local depending on the order of your map.

borkdude20:08:09

your example is supposed to work I think, due to how destructuring expands, flush is only ever defined as a local once and there's a get expression that gets the :or fallback

ambrosebs20:08:47

on first glance Alex's comment is self-contradictory. I'll look into it.

ambrosebs21:08:25

Yeah I see now how it can never be resolved to a local with the current impl. FWIW I think with the license Alex gives in the first sentence this is a valid implementation of destructuring (which the default resolves to a local).

(let [{:keys [a] :or {a a}} {}] a)
=>
(let [_m {}
      _not-found (Object.)
      a (clojure.core/get _m :a _not-found)
      _default a
      a (if (identical? a _not-found)
          _default
          a)]
  a)

borkdude21:08:35

you mean, it works only with the current impl? yeah, fair enough

ambrosebs21:08:33

yeah pretty much. you're probably making the right call though.

ambrosebs21:08:46

I don't have to like it of course 😉

borkdude21:08:40

what would be the thing you would like, btw?

borkdude21:08:50

I'm not 100% sure if I've understood that ;)

ambrosebs21:08:24

a huge document describing what is undefined behavior in a "clojure" 😉

1
ambrosebs21:08:43

personally I would disallow this in Typed Clojure, but clj-kondo is a different beast and I think you're the best person to decide.

borkdude21:08:05

I think the above thing is just one special case of "a clojure map is unordered" and that's why certain things can go wrong in map destructuring combinations, that's what the check is about. So it very much checks against the current implementation and what can go wrong with that, not against a standard (because there isn't one). Your alternative impl of destructuring would come with yet other problems, clj-kondo can be adapted if clojure might change to a different implementation

ambrosebs21:08:44

I'm with you, I think it's a judgement call. For example, you decided in the OP you need to disallow the first form because swapping the vector would change the semantics. But given that small maps are ordered, in some ways it's deterministic AFAICT. But since it's error-prone, I think we both agree that it should be picked up.

borkdude21:08:48

This one is also pretty interesting:

((fn [{:keys [a b c] :foo/keys [d e f] :or {b (inc f)}}] [a b c d e f]) {:a 42 :foo/f 13})

borkdude21:08:01

posted by Sean in that issue

ambrosebs21:08:24

blows up for me

ambrosebs21:08:34

does it sometimes work?

borkdude21:08:21

it does when you change :or to :or {d (inc a)}

borkdude21:08:36

it again has to do with the ordering of the locals, I guess

ambrosebs21:08:20

here's one that will break your brain I found at work today. is this wrong? (plumatic schema)

(s/defn a [{:keys [a :- s/Num]}] ...)

ambrosebs21:08:09

clj-kondo said it was fine. hard to disagree

ambrosebs21:08:07

the correct syntax is:

(s/defn a [{:keys [a]} :- {:a s/Num}] ...)

borkdude21:08:03

perhaps it's more correct to say that clj-kondo didn't say it was wrong ;)

ambrosebs21:08:02

haha! completely deterministic, obviously take a {:a 1, :- 2, :s/Num 3}

😆 1
borkdude21:08:15

is this about plumatic or is this also about (defn a [a] a)?

ambrosebs21:08:21

oh, no sorry that's a subject change.

ambrosebs21:08:50

or, elaborating my thoughts on error-prone vs deterministic.

borkdude21:08:10

hmm:

user=> (defn foo [{:keys [s/Num]}] Num)
#'user/foo
user=> (foo {:s/Num 1})
1

borkdude21:08:24

yeah I guess this is valid ;)

💯 1
borkdude21:08:38

I'm not sure if plumatic itself allows this?

ambrosebs21:08:52

as you said, it's valid xD

borkdude21:08:34

now let's wait for someone to abuse this to make a kinda typed clojure with existing syntax

borkdude21:08:57

which is valid normal clojure

ambrosebs21:08:06

what have I done

borkdude21:08:26

don't tell #CLDK6MFMK

ambrosebs21:08:59

there's a wild experimental defn macro in malli that probably already does this.

borkdude08:08:14

I'm looking for a better name (if possible) for:

:destructured-or-refers-bindings-of-same-map

teodorlu09:08:15

Perhaps :ambiguous-keys-destructuring or :nondeterministic-keys-destructuring ? Those are less specific, for better or worse. Another alternative: :nondeterministic-keys-or-destructuring

borkdude10:08:19

this linter is specifically about the :or clause

lread11:08:08

How about :or-clause-key-bindings-in-destructure-ambiguous

lread11:08:54

The nice thing about starting with :or is that it matches what it is talking about and won't be misread as a boolean,

lread11:08:24

:or-destructure-ambiguous might be too vague?

lread11:08:32

:or-on-destructured-map-ambiguous?

lread12:08:08

:or-on-keys-in-destructured-map-ambiguous?

borkdude08:08:34

I think I'll land on :destructured-or-binding-of-same-map. Thanks for the input!

👍 2