Clojurians Log v2

I think it's in line with the rest of clj-kondo's design, in that complex macros don't lint very well until you create a custom hook.

and it will take care of itself once clj-kondo can expand macros

I don't think thats related to how macros expand in clj-kondo. It's just related to users making silly errors while writing a macro. Even when clj-kondo would run the macro itself, it would expand in (clojure.core/unquote x) without warning

(clojure.core/unquote 1)

sure I'm thinking about the Sometimes this can be valid for weird macros step

assuming we don't care about the weird macros, then I completely agree that it makes sense.

but if there are weird macros, it can be handled by writing a hook or probably just expanding them, because if a macro has special handling for unquote it's probably not going to expand to code that uses unquote.

I think https://github.com/brandonbloom/backtick is probably a good lib to explore this latter point

an example of a weird macro that supports unquote in calls:

(defmacro $
  "Convenience macro around `process`. Takes command as varargs. Options can
  be passed via metadata on the form or as a first map arg. Supports
  interpolation via `~`"
  {:arglists '([opts? & args])}
  [& args]
  (let [opts (meta &form)
        farg (first args)
        args (if (and (= 1 (count args))
                      (string? farg)
                      (not (.exists (io/file farg))))
               (tokenize farg)
               args)
        farg (first args)
        marg? (map? farg)
        cmd (if marg?
              (vec (cons farg (mapv format-arg (rest args))))
              (mapv format-arg args))]
    `(let [cmd# ~cmd
           opts# ~opts
           fcmd# (first cmd#)
           [prev# cmd#]
           (if (:proc fcmd#)
             [fcmd# (rest cmd#)]
             [nil cmd#])
           fcmd# (first cmd#)
           [opts# cmd#]
           (if (map? fcmd#)
             [(merge opts# fcmd#) (rest cmd#)]
             [opts# cmd#])]
       (process* {:prev prev# :cmd cmd# :opts opts#}))))

but note that unquote itself doesn't even occurs literally in the macro

so no problem there

right, the problem is linting uses of this macro that contain unquotes

that's why I want to restrict the check to defmacro

ah missed that

it's in the original post :)

I boldened it now

an example of the above $ macro as a call:

($ {:out sw} ~(symbol bb) ~(symbol u/wd) :out ~config)

but maybe it's more of a rare exception that begs for a clj-kondo configuration to only turn the linter off inside calls to $

lol, it seems someone is going wild on this already. https://github.com/clj-kondo/clj-kondo/pull/2673

my revised opinion is that activating only in defmacro would prevent some false positives and seems like a good targeted step, but I'd be interested in enforcing elsewhere too.

Sounds like a configuration option

yeah good call. I'm seeing it as related to macros that introduce names/bindings, also highly configurable.

in terms of "advanced macrology that clj-kondo handles"

I think I've only ever encountered mistakes with unquote in defmacro so defaulting to this behavior seems good to me...

or just supporting that use case, without config options for v0

yeah, maybe in macro helper defn's as a runner-up. 👍

hmyeah

maybe if you are surrounded by a syntax quote, always activate?

(as an additional rule)

that's a good idea yeah

I found one case here which may actually be a bug: https://github.com/metabase/metabase/blob/28460b6d6c842dfef14c792e9d008d50cacb0bc1/src/metabase/premium_features/defenterprise.clj#L97 @camsaul @dpsutton

maybe this is also one, not sure: https://github.com/metabase/metabase/blob/aa0cdb546d7c9e4ef5c52ad23c656272b7599e23/src/metabase/util/log.clj#L192

my macro rules are a bit hazy. but can syntax quote contexts be nested?

@ambrosebs does thumbs up/down mean: yes, it's a bug, no it's not a bug?

yes

why is the second one not a bug?

the pprint is evaluated at runtime afaict, it prints the current expression

hello! I'm working in the issue. what about

(defmacro spyf
  "Evaluates an expression, and may write both the form and its formatted result to the log.
  Defaults to the `:debug` level."
  ([fmt expr]
   `(spyf :debug ~fmt ~expr))
  ([level fmt expr]
   (macros/case
     :cljs (glogi-spy (str *ns*) level expr #(format ~fmt %))
     :clj  `(spyf ~level ~fmt ~expr))))

(spy (+ 1 2)) => (pprint '(+ 1 2))

@ambrosebs but what does unquote mean here, without a syntax quote? it will expand into (cljs.core/unquote expr) , not to the quoted expression

unless glogy-something does something weird

exactly @jonurnieta

it does something weird

it looks broken to me, not compatible with AOT compilation. it splices a fn into the expansion.

I skipped the quoted ones like that in pprint, do you think this rule is correct?

@jonurnieta seems a bit arbitrary to me

IMO this could well be a warning by default even outside of macro contexts given that the false positive is exceedingly rare compared to the true positive.

Yes, it was to see if regression tests pass like that but I found more like the one in spyf macro

@borkdude yeah you're right, the final glogi-spy arg should be syntax quoted. it's a true positive.

I tried to replicate the bug here:

(defn- glogi-spy
  "Macro helper for [[spy]] and [[spyf]] in CLJS."
  [logger-name level expr formatter]
  `(let [a# ~expr
         s# (~formatter a#)]
     (prn s#)
     a#))

(defmacro spy
  "Evaluates an expression, and may write both the form and its result to the log.
  Returns the result of `expr`.
  Defaults to the `:debug` level."
  ([expr] `(spy :debug ~expr))
  ([level expr]
   (glogi-spy (str *ns*) level expr
              #(do (print '~expr)
                   (print "=> ")
                   (println %)))))

(spy 1)

(clojure.core/unquote expr)=> 1
nil
1

The broken bit is behind the :cljs case

same https://github.com/metabase/metabase/blob/aa0cdb546d7c9e4ef5c52ad23c656272b7599e23/src/metabase/util/log.clj#L205 and https://github.com/metabase/metabase/blob/aa0cdb546d7c9e4ef5c52ad23c656272b7599e23/src/metabase/util/log.clj#L243

I could try to start the metabase front-end myself but that's another rabbit hole possibly :)

in cljs it would splice a clojure.lang.Fn into javascript. last arg should be syntax quoted, you were right.

@jonurnieta you can update the expected findings with this:

CLJ_KONDO_REGRESSION_UPDATE=true bb test:regression

it would still be lovely if @dpsutton could confirm later if the spy macros are broken in CLJS

(or anyone else at metabase)

about to jump into a meeting. I can look in ~ an hour (quite interested just lack the immediate time)

sure, thanks!

thank you!

we might want a similar rule for ~@, this final line also seems to be missing a syntax quote https://github.com/metabase/metabase/blob/aa0cdb546d7c9e4ef5c52ad23c656272b7599e23/src/metabase/util/log.clj#L243

yes, linked that above. already detected with @jonurnieta’s PR

https://clojurians.slack.com/archives/CHY97NXE2/p1765901879781399?thread_ts=1765896926.873239&cid=CHY97NXE2

good. did the :oss (validate-oss-args '~ee-ns options)) one show up? that's a bit tricker since it's valid code, just semantically incorrect.

It did show up and you also have it a thumbs up above?

yes it's a bug in this specific case, I'm speculating whether it's a good idea to lint under a quote. I don't know the right answer. Probably it's rare to actually want to quote an unquote.

it's easier to decide for code that blows up at runtime with "unquote not bound", clearly a linter error.

which I think the other three cases fall under (EDIT: 2/3? the ~@body and (format ~) ones)

ah there's two or three different kinds of mistakes here. one needs more quoting, another less.

and there's a final kind of usage where someone actually wants to quote a form verbatim that happens to contain an unquote, that's not represented here.

that's probably the rarest of all. after thinking it through the proposed approach seems good.

@ambrosebs double checking: do you agree with @doppiaelle1999 that it should also lint outside of defmacro?

nuanced: I think it's a good idea to lint outside of defmacro for forms that throw exceptions at runtime (like your original example). I'm back on the fence for forms that don't throw (like '~foo). But could be quite useful for bug finding, as demonstrated above. I think my vote would be: • default: lint error for all (inside+outside defmacro) forms that look like they throw at runtime like (fn [] ~x) ◦ precisely, the "unquote is not bound" error • default: flag unquotes under quotes like '~a where a is in scope, like (fn [a] '~a) • opt-in: flag all unquotes under quotes (that aren't already syntax quoted)

can you name an example of 3 that isn't a case of 1 or 2?

Maybe something with multiple mistakes like:

(defn foo [x] '~y)

(defn foo [x] `~x)

but the reason I suggested opt-in is because it could be valid code under certain circumstances.

since unlike the other two it actually returns a value

tbh I could go either way on 3.

the scenario where I'd use this are pretty rare. maybe macro generating macros to avoid nested syntax quotes.

even that is a stretch. you can't generate a macro that has a syntax quote, so an unquote form isn't going to be useful.

I guess I've talked myself back to what everyone else agreed on 🙂 so yes, enable outside defmacro too

I just have trouble imagining an example that isn't a case of 1 or 2 which you proposed as default, not opt-in

I think I'm now leaning to: apply it globally with the option to disable it in specific calls (which is already possible now)

yeah, I specifically narrowed 2 down for the most likely almost-correct code involving quote and unquote. I didn't consider whether any remaining idioms might be left, I'm not sure there is.

So the preconditions of 2 might just be redundant. it might be just as effective to simply warn always. i.e., • always warn (error?) if unquote will throw • always warn if unquote is under a (not-syntax-quoted) quote

I went with :unquote-not-syntax-quoted and decided not to split the linter for now. Merged on master

@dpsutton did you manage to double-check metabase btw?

haven’t gotten to it unfortunately

I guess you will notice soon when upgrading clj-kondo 👻

the best way to fix it 🙂

A darn, lein defproject uses unquote to allow templating some values. Ah, I knew there would be a macro that did this... https://github.com/technomancy/leiningen/blob/40227328d4a9c8945362d6d626d19c2449175df6/leiningen-core/src/leiningen/core/project.clj#L203

https://clojurians.slack.com/archives/CHY97NXE2/p1766667111695549

What would be a good name for this linter? 1.

:unquote-not-in-syntax-quote

:unquote-outside-syntax-quote

I think I like 1 better since 2 suggests the presence of a syntax-quote, where the unquote it outside of, but 1 doesn't assume that?

I think of "syntax-quoted" and "not syntax-quoted" contexts. I'll offer :unquote-not-syntax-quoted as another option.

that name may be a little confusing in the sense that people might understand it as an unquote that is not directly syntax-quoted, like

`~x

I don't follow, how is that example "an unquote that is not directly syntax-quoted"?

that is in fact the example :)

That is a directly syntax-quoted example, but this one isn't:

`[~x]

ok, I don't understand the significance of directly/not directly, but my original (minor) concern with the proposed names are the words "in" and "outside", because you can still get this bug by being "in" a syntax quote like this:

`~~a

I wasn't sure if "in" disambiguated between "nested under" and "syntax-quoted context".

you're right. I think the conclusion is that no name is perfect and we just have to pick one...

now I thought more about it, yours is probably better

what about :unquote-without-matching-syntax-quote? Although it's quite long and doesn't indicate the way the unquote should be "matched" by the syntax-quote, it does convey the tendency of the two to come in pairs better.

match and pair doesn't say something about that they should be nested. I think :unquote-not-syntax-quoted captures that better perhaps

yeah indeed

Maybe I'm retreading the same conversation we had above, but now we're at the naming stage might we want to split this into two linters? 1. an error linter :unquote-not-syntax-quoted for forms that throw like (do ~x) and 2. a warning linter (say) :quoted-unquote for (do '~x) which don't throw but are likely a typo?

I might not be following the naming philosophy of the linters tho. one direction we could name the potential remedy (`:missing-syntax-quote`) and the other we could name the symptom (`:unquote-in-function-position`), we're landing somewhere in the middle.