clojure--valid-put-clojure-indent-call-p signals error with the indentation spec used for letfn

[camsaul]

(clojure--valid-put-clojure-indent-call-p '(put-clojure-indent 'letfn '(1 ((:defn)) nil)))
;; => *** Eval error ***  Unrecognized put-clojure-indent call: (put-clojure-indent 'letfn '(1 ((:defn)) nil))

I was trying to use the same indentation spec for clojure.tools.macro/macrolet. In my .dir-locals.el I have

((clojure-mode . ((eval . (put-clojure-indent 'tools.macro/macrolet '(1 ((:defn)) nil))))))

put-clojure-indent has the safe-local-eval-function property clojure--valid-put-clojure-indent-call-p, i.e. that form in my .dir-locals.el would be considered safe to eval without prompting so long as clojure--valid-put-clojure-indent-call-p returned t when passed that form. It doesn't, however -- it signals an error.

The form seems to be correct as far as I can tell -- If I manually eval (put-clojure-indent 'tools.macro/macrolet '(1 ((:defn)) nil)) then it works as expected. So I'm assuming there's a bug in clojure--valid-put-clojure-indent-call-p

[camsaul]

Actually I guess this is a problem with clojure--valid-indent-spec-p, which it uses under the hood:

(clojure--valid-indent-spec-p '(1 ((:defn)) nil))
;; -> nil

There seem to be a lot of things it doesn't like:

ELISP> (clojure--valid-indent-spec-p '(1 ((:defn)) nil))
nil
ELISP> (clojure--valid-indent-spec-p '(1))
nil
ELISP> (clojure--valid-indent-spec-p '1)
t
ELISP> (clojure--valid-indent-spec-p '(1))
nil
ELISP> (clojure--valid-indent-spec-p ':defn)
(:defn)
ELISP> (clojure--valid-indent-spec-p '(:defn))
nil

[bbatsov]

I've moved the ticket to clojure-mode where it belongs. @rlbdv would you mind taking a look at this? I guess the valid spec check needs to be updated to account for the problematic cases.

[rlbdv]

I think I was working from the docs, but maybe I just missed something, or maybe the docs are incomplete. I'll take a look.

[rlbdv]

These docs iirc: https://docs.cider.mx/cider/indent_spec.html

[rlbdv]

Looks like I might not get to this today, but it's now on the short list, fwiw.

[rlbdv]

I think I have a fix (overlooked a bit of the spec), and should be able to raise a pr tomorrow, but wondered about this example from above: '(1 ((:defn)) nil). I didn't see where the spec allowed nil there unless that's what it means by "if it’s not a form the spec is irrelevant", i.e. perhaps we must allow anything, and ignore anything that's not listp?

[rlbdv]

@bbatsov @camsaul Wondered how either of you read that spec (or maybe you already know the expectations), i.e. I need to decide what, if anything we should (or even can) be checking there.

[bbatsov]

I’ll follow up on Monday. Totally forgot I had to respond here last week.

…

On Sat, Dec 4, 2021, at 7:12 PM, Rob Browning wrote: @bbatsov <https://github.com/bbatsov> @camsaul <https://github.com/camsaul> Wondered how either of you read that spec (or maybe you already know the expectations), i.e. I need to decide what, if anything we should (or even can) be checking there. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#600 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZLSV6MV7PTRO6SMR6MKDUPJDY5ANCNFSM5C7CNR7Q>.

[rlbdv]

No worries.

[bbatsov]

The real problem with the spec is that we put it quickly together at first and we never spent much time polishing it. I think we should probably spent a bit of time on the spec first and take it from there.

I think I have a fix (overlooked a bit of the spec), and should be able to raise a pr tomorrow, but wondered about this example from above: '(1 ((:defn)) nil). I didn't see where the spec allowed nil there unless that's what it means by "if it’s not a form the spec is irrelevant", i.e. perhaps we must allow anything, and ignore anything that's not listp?

nil is the same as an empty list in Emacs Lisp. :-)

[rlbdv]

Ahh, right -- just meant anything that's not a list with items. I suppose it's also true that if we were "certain" that the function itself is "safe", i.e. always handles the form safely itself, then we might not need a separate validation function/step.

[rlbdv]

@bbatsov so how should we proceed? Assuming we still want form validation, should I adjust the predicate to handle more of the cases @camsaul listed above? For example, should (quote 1) be a valid form? The current code only accepts quoted lists, not quoted self-evaluating forms. As demonstrated, it also doesn't accept additional nesting like ((:defn)).