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

-if-let* should bind all vars in ELSE clause #273

Open
alphapapa opened this issue Aug 22, 2018 · 2 comments
Open

-if-let* should bind all vars in ELSE clause #273

alphapapa opened this issue Aug 22, 2018 · 2 comments
Labels
bug Existing behavior is incorrect

Comments

@alphapapa
Copy link

I just noticed a fairly significant difference between how if-let* and -if-let* handle binding in the ELSE clause:

(if-let* ((a 'a)
          (b nil)
          (c 'c)
          (d 'd))
    t
  (list :a a :b b :c c :d d))  ;; => (:a a :b nil :c nil :d nil)

(-if-let* ((a 'a)
           (b nil)
           (c 'c)
           (d 'd))
    t
  (list :a a :b b :c c :d d))  ;; => Debugger entered--Lisp error: (void-variable c)

You might think that this isn't important, but if the ELSE clause contains (if (and b d) ...), and b was nil, b will be defined, but d will be undefined, and it will raise an error.

I think this should probably be fixed to always bind all variables in the ELSE clause.

@Fuco1
Copy link
Collaborator

Fuco1 commented Aug 22, 2018

if-let has a pretty clever way to do this... it expands to

(let*
    ((a
      (and t 'a))
     (b
      (and a nil))
     (c
      (and b 'c))
     (d
      (and c 'd)))
  (if d t
    (list :a a :b b :c c :d d)))

whereas -if-let expands to

(let
    ((--dash-source-289--
      (b nil)))
  (if --dash-source-289--
      (let
          ((a
            (pop --dash-source-289--)))
        (if a
            (let
                ((--dash-source-290--
                  (car --dash-source-289--)))
              (if --dash-source-290--
                  (let
                      ('(pop --dash-source-290--))
                    (if quote
                        (let
                            ((a
                              (car --dash-source-290--)))
                          (if a t
                            (list :a a :b b :c c :d d)))
                      (list :a a :b b :c c :d d)))
                (list :a a :b b :c c :d d)))
          (list :a a :b b :c c :d d)))
    (list :a a :b b :c c :d d))) 

ouch...

I think we can do better indeed :D

@basil-conto basil-conto added the bug Existing behavior is incorrect label Jan 6, 2021
@yuhan0
Copy link

yuhan0 commented Jun 18, 2023

The macroexpansion should be comparing -if-let* instead of -if-let - the latter only supports a single (VAR VAL) binding.
So it's actually treating (a (quote a)) as the match form, (b nil) as the value, and discarding the rest of the list (note that c and d don't show up in the macroexpansion.

Not that the actual expansion is much better:

(macroexpand
 '(-if-let* ((a 'a)
            (b nil)
            (c 'c)
            (d 'd))
      t
    (list :a a :b b :c c :d d)))
;; =>
(let ((a 'a))
  (if a
      (let ((b nil))
        (if b
            (let ((c 'c))
              (if c
                  (let ((d 'd))
                    (if d t
                      (list :a a :b b :c c :d d)))
                (list :a a :b b :c c :d d)))
          (list :a a :b b :c c :d d)))
    (list :a a :b b :c c :d d)))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Existing behavior is incorrect
Projects
None yet
Development

No branches or pull requests

4 participants