Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix parsing of pcase-let* and seq-let for newer Emacs #43

Closed
okamsn opened this issue Mar 14, 2021 · 4 comments
Closed

Fix parsing of pcase-let* and seq-let for newer Emacs #43

okamsn opened this issue Mar 14, 2021 · 4 comments
Labels
bug Something isn't working

Comments

@okamsn
Copy link
Owner

okamsn commented Mar 14, 2021

The macro expansion has changed drastically.

This

(pcase-let* ((`(,a (,_ ,b)) '(1 (2 3))))
         `(,a (,_ ,b)))

becomes

(progn
  (ignore (consp '(1 (2 3))))
  (let* ((x326 (car-safe '(1 (2 3))))
         (x327 (cdr-safe '(1 (2 3)))))
    (progn (ignore (consp x327))
           (let* ((x328 (car-safe x327)))
             (progn
               (ignore (consp x328))
               (let* ((x330 (cdr-safe x328)))
                 (progn
                   (ignore (consp x330))
                   (let* ((x331 (car-safe x330))
                          (x332 (cdr-safe x330)))
                     (progn
                       (ignore (null x332))
                       (let* ((x333 (cdr-safe x327)))
                         (progn
                           (ignore (null x333))
                           (let ((a x326) (b x331))
                             nil nil
                             `(,a (,_ ,b))))))))))))))

instead of something like the below, as it did previously.

(let* ((x326 (car-safe '(1 (2 3))))
       (x327 (cdr-safe '(1 (2 3))))
       (x328 (car-safe x327))
       (x330 (cdr-safe x328))
       (x331 (car-safe x330))
       (x332 (cdr-safe x330))
       (x333 (cdr-safe x327)))
  (let ((a x326)
        (b x331))
    `(,a (,_ ,b))))
@okamsn
Copy link
Owner Author

okamsn commented Mar 18, 2021

The current approach currently assumes to much. Fixing this might involve
reverting the changes that made our approach more generic.

The flags might need to replace the declaration of with vars using let* with
ones for -let*, pcase-let*, etc.

Thread on the mailing list where I ask about accessing variable used by Pcase:

https://lists.gnu.org/archive/html/help-gnu-emacs/2021-03/msg00089.html

The conversation is still ongoing.

@okamsn
Copy link
Owner Author

okamsn commented Apr 19, 2021

UPDATE: This is mostly fixed in testing, but the situation is still developing.

It remains to be seen whether the new behavior reliably assigns to variables in
a certain order. If not, than implicit return values will have to be removed
from accumulations commands with named variables, as the possible behavior is
too varied.

One problem found is destructuring with more complex patterns. It seems that
the values of variables is assigned nil when it is unused, and we need to
distinguish between that case and when it is actually supposed to be nil.

For example,

(loopy (flag pcase)
       (list i '((1 2) [3 4 5] (6 7) [8 9 10]))
       (sum (or `(,a ,b) `[,b ,c ,a]) i))

currently expands to

(let ((a 0)
      (c 0)
      (b 0))
  (let* ((list-248 '((1 2) [3 4 5] (6 7) [8 9 10]))
         (i nil))
    (cl-block nil
      (while (consp list-248)
        (setq i (car list-248))
        (cond
         ((consp i)
          (let* ((x249 (car-safe i))
                 (x250 (cdr-safe i)))
            (if (consp x250)
                (let* ((x251 (car-safe x250))
                       (x252 (cdr-safe x250)))
                  (if (null x252)
                      (progn
                        (setq a (+ x249 a))
                        (setq c (+ nil c))
                        (setq b (+ x251 b))))))))
         ((vectorp i)
          (let* ((x253 (length i)))
            (if (eql x253 3)
                (let* ((x254 (pcase--flip aref 0 i))
                       (x255 (pcase--flip aref 1 i))
                       (x256 (pcase--flip aref 2 i)))
                  (progn
                    (setq a (+ x256 a))
                    (setq c (+ x255 c))
                    (setq b (+ x254 b))))))))
        (setq list-248 (cdr list-248)))
      (list b c a))))

@okamsn okamsn added the bug Something isn't working label May 8, 2021
@okamsn okamsn mentioned this issue Jun 1, 2021
11 tasks
@okamsn
Copy link
Owner Author

okamsn commented Aug 15, 2021

UPDATE:

  • with has supported destructuring for a few months now.

  • We cannot control the order of the assigned variables. pcase allows for custom destructuring patterns, and there's no way to account for how those might work.

    A few weeks ago, the package was updated so that accumulation commands with named variables don't generate implicit return values. This is really the only viable option for allowing destructuring systems that we don't control.

  • In Emacs 28, pcase is now guaranteed to set unmatched variables to nil. Since we cannot distinguish between whether a variable is unmatched or whether the element was actually nil, it is up to the user to recognize that difference, as they would need to if using pcase-let*. As the user opted for that behavior, they must handle it.

  • All that's left for this bug is to test the package against multiple Emacs versions. Github actions might be of use in that.

@okamsn
Copy link
Owner Author

okamsn commented Aug 29, 2021

Emacs 27 is the minimum required version. We're now using Github Actions to test the package on Emacs 27 automatically. I am manually checking against Emacs 28.

All pcase and seq tests are passing on both versions. This issue appears to be resolved.

@okamsn okamsn closed this as completed Aug 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant