Recent pcase changes

Recent pcase changes

[Manuel Uberti]

Hello,

yesterday I reported a bug on the clojure-mode bug tracker[1] because with a
recent Emacs built from master[2] marking the whole buffer and calling
`indent-region' results in an error.

By looking at clojure-mode code I see it sets `lisp-indent-function' to
`clojure-indent-function', which makes use of `pcase'. More specifically, this
is what it does at line 1497 of clojure-mode.el:

(pcase method
  ((or (pred integerp) `(,method))
   (let ((pos -1))
     (condition-case nil
         (while (and (<= (point) indent-point)
                     (not (eobp)))
           (clojure-forward-logical-sexp 1)
           (cl-incf pos))
       ;; If indent-point is _after_ the last sexp in the
       ;; current sexp, we detect that by catching the
       ;; `scan-error'. In that case, we should return the
       ;; indentation as if there were an extra sexp at point.
       (scan-error (cl-incf pos)))
     (cond
      ;; The first non-special arg. Rigidly reduce indentation.
      ((= pos (1+ method))
       (+ lisp-body-indent containing-form-column))
      ;; Further non-special args, align with the arg above.
      ((> pos (1+ method))
       (clojure--normal-indent last-sexp 'always-align))
      ;; Special arg. Rigidly indent with a large indentation.
      (t
       (+ (* clojure-special-arg-indent-factor lisp-body-indent)
          containing-form-column)))))
  (`:defn
   (+ lisp-body-indent containing-form-column))
  ((pred functionp)
   (funcall method indent-point state))
  ;; No indent spec, do the default.
  (`nil
   (let ((function (thing-at-point 'symbol)))
     (cond
      ;; Preserve useful alignment of :require (and friends) in `ns' forms.
      ((and function (string-match "^:" function))
       (clojure--normal-indent last-sexp 'always-align))
      ;; This should be identical to the :defn above.
      ((and function
            (string-match "\\`\\(?:\\S +/\\)?\\(def[a-z]*\\|with-\\)"
                          function)
            (not (string-match "\\`default" (match-string 1 function))))
       (+ lisp-body-indent containing-form-column))
      ;; Finally, nothing special here, just respect the user's
      ;; preference.
      (t (clojure--normal-indent last-sexp clojure-indent-style))))))

Is there something wrong with this usage of `pcase'? If so, how can it be
adapted to the latest changes on master to work again?

Note that to have clojure-mode working again for now I have reset my Emacs build
to a previous commit[3].

Note also that this is me wild guessing that the problem is related to the
recent `pcase' changes. It could very well be that the problem is elsewhere, and
if so, I'll try debugging more.


All the best


[1] https://github.com/clojure-emacs/clojure-mode/issues/584
[2] Commit: ba33089d50a4f74b88cd7fc9d862d183d096c27d Fix a doc/misc clean rule
[3] Commit: 6bf56a3614ccd23a31e34ae997b2a6bb0d158489 Fix documentation of a
recent change

-- 
Manuel Uberti
www.manueluberti.eu

Re: Recent pcase changes

[Mattias Engdegård]

4 mars 2021 kl. 11.06 skrev Manuel Uberti <manuel.uberti@inventati.org>:

> By looking at clojure-mode code I see it sets `lisp-indent-function' to
> `clojure-indent-function', which makes use of `pcase'. More specifically, this
> is what it does at line 1497 of clojure-mode.el:
> 
> (pcase method
>  ((or (pred integerp) `(,method))

Now all variables in an `or`-pattern are always bound, to nil if nothing else. Try:

   ((or (pred integerp) `(m))
    (let ((method (if (integerp method) method m)))
     ...

Re: Recent pcase changes

[Omar Polo]

Mattias Engdegård <mattiase@acm.org> writes:

> 4 mars 2021 kl. 11.06 skrev Manuel Uberti <manuel.uberti@inventati.org>:
>
>> By looking at clojure-mode code I see it sets `lisp-indent-function' to
>> `clojure-indent-function', which makes use of `pcase'. More specifically, this
>> is what it does at line 1497 of clojure-mode.el:
>> 
>> (pcase method
>>  ((or (pred integerp) `(,method))
>
> Now all variables in an `or`-pattern are always bound, to nil if nothing else. Try:
>
>    ((or (pred integerp) `(m))
>     (let ((method (if (integerp method) method m)))
>      ...

Yes, that fixes the issue for me, thanks!

Re: Recent pcase changes

[Manuel Uberti]

On 04/03/21 12:30, Mattias Engdegård wrote:
> Now all variables in an `or`-pattern are always bound, to nil if nothing else. Try:
> 
>    ((or (pred integerp) `(m))
>     (let ((method (if (integerp method) method m)))
>      ...

Thank you, this fixes it for me. I'll suggest it on clojure-mode.

-- 
Manuel Uberti
www.manueluberti.eu

Re: Recent pcase changes

[Stefan Monnier]

>> By looking at clojure-mode code I see it sets `lisp-indent-function' to
>> `clojure-indent-function', which makes use of `pcase'. More specifically, this
>> is what it does at line 1497 of clojure-mode.el:
>> 
>> (pcase method
>>  ((or (pred integerp) `(,method))

No good deed goes unpunished, eh?

> Now all variables in an `or`-pattern are always bound, to nil if nothing else. Try:
>
>    ((or (pred integerp) `(m))
                          ^^^^^
                          `(,m)

>     (let ((method (if (integerp method) method m)))

With the new semantics you could also use

    (pcase method
      ((or (pred integerp) `(,m))
       (let ((method (or m method)))
       ...

And to get something to work reliably with both the old and the new
semantics, you can go with:

    (pcase method
      ((or (and (pred integerp) method) `(,method))
       ...


-- Stefan

Re: Recent pcase changes

[Manuel Uberti]

On 04/03/21 15:39, Stefan Monnier wrote:
> With the new semantics you could also use
> 
>     (pcase method
>       ((or (pred integerp) `(,m))
>        (let ((method (or m method)))
>        ...
> 
> And to get something to work reliably with both the old and the new
> semantics, you can go with:
> 
>     (pcase method
>       ((or (and (pred integerp) method) `(,method))
>        ...

Thank you, I'll see if I can propose a PR with this last suggestion.

-- 
Manuel Uberti
www.manueluberti.eu