clojure-emacs/clojure-mode

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

camsaul opened this issue · 12 comments

camsaul commented 
(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 commented

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 commented

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.

  • 👍1
rlbdv commented

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 commented

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

  • 👍1
rlbdv commented

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 commented

@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 commented
rlbdv commented

No worries.

bbatsov commented

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. :-)

  • 👍1
rlbdv commented

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 commented

@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)).