Clojurians Log v2

One "false positive" may be this one here: https://github.com/babashka/sci/blob/47e48bbc74facf0c649b4245c616eb69fbf7f57e/src/sci/impl/opts.cljc#L212 The "calculation" is just a map lookup which is fairly cheap

cc @jonurnieta

> people may expect that :or defaults are never executed, like in (or 1 (expensive-expression)) but this isn't true. They are always executed oh wow- I'm people, TIL about that...

another one here: https://github.com/metabase/metabase/blob/6f00f47c479b8d6f019d0536fb26cf1662c8cc04/src/metabase/channel/render/preview.clj#L103

https://github.com/metabase/metabase/blob/6f00f47c479b8d6f019d0536fb26cf1662c8cc04/src/metabase/embedding_rest/api/common.clj#L406

etc.

Re the original question I would say both - we know it's eager and perf impact is not relevant here

wow i had no idea, that's really surprising to me

I think that linter would be very useful and I wouldn't be surprised if the authors of many of the expressions it would flag just don't realize that is always evaluated. For folks who know and are okay with such evaluations, they can add an "ignore" form. I'd argue most of those uses you flagged should be refactored so the default value is bound conditionally and explicitly. It's kind of bad enough that :or doesn't bind the key inside the :as portion which catches a lot of people out, so I think anything that makes people think a bit harder about :or is a good thing 🙂

is it intentional? is this something worthy of an Ask to potentially change the behavior?

It is intentional and we’re not going to change that

interesting

what's the reasoning?

The primary use case here is providing an alternate value

You should think of it like the not-found value provided with get which is, hopefully obviously, evaluated

oh wait, is it implemented as a default in a get call? in that case, makes sense and i would expect it to not change lol

@nbtheduke when would you expect it to change?

implementation to change, i mean

@borkdude Does the linter flag things like this? i.e., is an anonymous fn considered a "calculation" here:

(let [{:keys [db-only form-only form->db
                insert->key key-fn key-gen primary-key
                schema-spec
                transformer validator]
          :or {insert->key   (fn [result _cols] (:GENERATED_KEY result))
               primary-key   :id}}

Are you talking about the code I linked to or about :or implementation changing? I certainly am not advocating for that as it would break lots of code!

lol sorry, ignore me

the linter is a good idea

@seancorfield up for debate... I also found one such case:

{:or {a (if something #(foo) #(bar))}}

also:

{:or {a (:foo b)}}

As long as b isn't another binding in the destructuring, right? Since you cannot guarantee order of bindings here.

yeah, we already have a linter for that

it sounds like this may be useful but setting it to off may be good (certainly initially)

I think (fn [..] ..) or #(..) are okay but (if something #(foo) #(bar)) probably warrants refactoring IMO. (:foo b) def. gives me pause for thought tho'... That said, the above example I posted is the only instance in our work codebase that has a non-simple value.

i did a bunch of micro benchmarking for various gets with default for splint, and i found (:a foo (:b foo)) to be faster than basically everything else

i think expressions in general are cause for concern but keyword invocations are very very fast

I would like to have this linter available -- and I would want to turn it on globally -- but I may not be a typical user in your demographic 🙂

ok, I replied to the PR here: https://github.com/clj-kondo/clj-kondo/pull/2702#issuecomment-3700339176 I think we also need to have a better name. Suggestions welcome

'or-expression' sounds like we're talking about (or ...) - would 'or-binding' be more precise? I'd go for something like 'evaluated-or-binding' unless the plan is to have whitelists for eg keyword calls

Sean proposed a name in the Github PR which I think is fine

hmm the distinction is probably something closer to 'self-evaluating' in other lisps, not sure if Clojure has such a notion

'eager evaluation' might suggest that something like {:or (map ...)} would pass the linter

good point

consideration posted in github PR

https://clojure.org/reference/evaluation > Any object other than those discussed above will evaluate to itself.

I don't think it's about self-evaluation since locals don't evaluate to themselves but are fine in :or defaults

oh that's true - don't know if there's a technical name for it then, in my macro utils lib I've just been calling it 'simple'

(defn constant?
  "Returns true if the form always evaluates to the same value."
  [form]
  (or (nil? form)
    (boolean? form)
    (keyword? form)
    (number? form)))

(defn simple?
  "Returns true if the form can be inlined instead of let-bound."
  [form]
  (or (constant? form)
    (symbol? form)))

I think the name ":destructured-or-default-always-evaluates" is fine and some cases aren't warned about, namely local or var references

I replaced eager with always in this name because of your point about laziness

perhaps :destructured-or-unconditional-call to be more aligned with the analyze-call impl? but I think it sounds alright

the 'default' seems redundant since it wasn't in the other linter name

true

:destructured-or-always-evaluated-call ?

Personally I like :destructed-or-always-evaluates and as the message Default :or value is always evaluated

sorry, it should be :destructured-or-always-evaluates (not destructed) @jonurnieta Perhaps you can change this in the PR. Also disable the linter by default

👌🏿 nice!

@borkdude done!

Merged!

thank you! 🎉

thank you too!

Thanks from me, too! 🙂