bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas

[Stefan Monnier]

Sorry 'bout the delay, and thanks Mattias for finding the bug and the fix.

> @@ -428,10 +428,26 @@ cconv-convert
>                   ;; One of the lambda-lifted vars is shadowed, so add
>                   ;; a reference to the outside binding and arrange to use
>                   ;; that reference.
> -                 (let ((closedsym (make-symbol (format "closed-%s" var))))
> -                   (setq new-env (cconv--remap-llv new-env var closedsym))
> -                   (setq new-extend (cons closedsym (remq var new-extend)))
> -                   (push `(,closedsym ,var) binders-new)))
> +                 (let* ((mapping (cdr (assq var env)))
> +                        (var-def
> +                         (pcase-exhaustive mapping
> +                           (`(internal-get-closed-var . ,_)
> +                            ;; The variable is captured.
> +                            mapping)
> +                           (`(car-safe (internal-get-closed-var . ,_))
> +                            ;; The variable is mutably captured; skip
> +                            ;; the indirection step because the variable is
> +                            ;; passed "by rerefence" to the λ-lifted 
> function.
> +                            (cadr mapping))
> +                           ((or '() `(car-safe ,(pred symbolp)))
> +                            ;; The variable is not captured.  Add a
> +                            ;; reference to the outside binding and arrange
> +                            ;; to use that reference.
> +                            var))))
> +                   (let ((closedsym (make-symbol (format "closed-%s" var))))
> +                     (setq new-env (cconv--remap-llv new-env var closedsym))
> +                     (setq new-extend (cons closedsym (remq var new-extend)))
> +                     (push `(,closedsym ,var-def) binders-new))))
>  
>                 ;; We push the element after redefined free variables are
>                 ;; processed.  This is important to avoid the bug when free
> @@ -449,14 +465,28 @@ cconv-convert
>           ;; before we know that the var will be in `new-extend' (bug#24171).
>           (dolist (binder binders-new)
>             (when (memq (car-safe binder) new-extend)
> -             ;; One of the lambda-lifted vars is shadowed, so add
> -             ;; a reference to the outside binding and arrange to use
> -             ;; that reference.
> +             ;; One of the lambda-lifted vars is shadowed.
>               (let* ((var (car-safe binder))
> -                    (closedsym (make-symbol (format "closed-%s" var))))
> -               (setq new-env (cconv--remap-llv new-env var closedsym))
> -               (setq new-extend (cons closedsym (remq var new-extend)))
> -               (push `(,closedsym ,var) binders-new)))))
> +                    (mapping (cdr (assq var env)))
> +                    (var-def
> +                     (pcase-exhaustive mapping
> +                       (`(internal-get-closed-var . ,_)
> +                        ;; The variable is captured.
> +                        mapping)
> +                       (`(car-safe (internal-get-closed-var . ,_))
> +                        ;; The variable is mutably captured; skip
> +                        ;; the indirection step because the variable is
> +                        ;; passed "by rerefence" to the λ-lifted function.
> +                        (cadr mapping))
> +                       ((or '() `(car-safe ,(pred symbolp)))
> +                        ;; The variable is not captured.  Add a
> +                        ;; reference to the outside binding and
> +                        ;; arrange to use that reference.
> +                        var))))
> +               (let ((closedsym (make-symbol (format "closed-%s" var))))
> +                 (setq new-env (cconv--remap-llv new-env var closedsym))
> +                 (setq new-extend (cons closedsym (remq var new-extend)))
> +                 (push `(,closedsym ,var-def) binders-new))))))

Can we avoid this duplication by moving that code to a separate function?

> +    (let ((f (lambda (x)
> +               (let ((g (lambda () x))
> +                     (h (lambda () (setq x x))))
> +                 (let ((x 'b))
> +                   (list x (funcall g) (funcall h)))))))
> +      (funcall (funcall f 'b)))
> +    (let ((f (lambda (x)
> +               (let ((g (lambda () x))
> +                     (h (lambda () (setq x x))))
> +                 (let* ((x 'b))
> +                   (list x (funcall g) (funcall h)))))))
> +      (funcall (funcall f 'b)))

These two tests are identical aren't they?  Also, can we change the
(setq x x) into something like (setq x (list x x)) and avoid using the
same `b` value for both `x` vars, so as to catch more potential errors?

> @@ -428,10 +428,27 @@ cconv-convert
>                   ;; One of the lambda-lifted vars is shadowed, so add
>                   ;; a reference to the outside binding and arrange to use
>                   ;; that reference.
> -                 (let ((closedsym (make-symbol (format "closed-%s" var))))
> -                   (setq new-env (cconv--remap-llv new-env var closedsym))
> -                   (setq new-extend (cons closedsym (remq var new-extend)))
> -                   (push `(,closedsym ,var) binders-new)))
> +                 (let* ((mapping (cdr (assq var env)))
> +                        (remap-to
> +                         (pcase-exhaustive mapping
> +                           (`(internal-get-closed-var . ,_)
> +                            ;; The variable is captured; remap.
> +                            mapping)
> +                           (`(car-safe (internal-get-closed-var . ,_))
> +                            ;; The variable is mutably captured; remap, but 
> skip
> +                            ;; the indirection step because the variable is
> +                            ;; passed "by rerefence" to the λ-lifted 
> function.
> +                            (cadr mapping))
> +                           ((or '() `(car-safe ,(pred symbolp)))
> +                            ;; The variable is not captured.  Add a
> +                            ;; reference to the outside binding and arrange
> +                            ;; to use that reference.
> +                            (let ((closedsym
> +                                   (make-symbol (format "closed-%s" var))))
> +                              (push `(,closedsym ,var) binders-new)
> +                              closedsym)))))
> +                   (setq new-env (cconv--remap-llv new-env var remap-to))
> +                   (setq new-extend (cons remap-to (remq var new-extend)))))
>  
>                 ;; We push the element after redefined free variables are
>                 ;; processed.  This is important to avoid the bug when free

Looks good (better than patch A).

You say "On the other hand, patch B does abuse the cconv data structures
a little (but it works!)" so the code should say something about
this abuse.  A least I failed to see where the abuse lies.

> @@ -449,14 +466,29 @@ cconv-convert
>           ;; before we know that the var will be in `new-extend' (bug#24171).
>           (dolist (binder binders-new)
>             (when (memq (car-safe binder) new-extend)
> -             ;; One of the lambda-lifted vars is shadowed, so add
> -             ;; a reference to the outside binding and arrange to use
> -             ;; that reference.
> +             ;; One of the lambda-lifted vars is shadowed.
>               (let* ((var (car-safe binder))
> -                    (closedsym (make-symbol (format "closed-%s" var))))
> -               (setq new-env (cconv--remap-llv new-env var closedsym))
> -               (setq new-extend (cons closedsym (remq var new-extend)))
> -               (push `(,closedsym ,var) binders-new)))))
> +                    (mapping (cdr (assq var env)))
> +                    (remap-to
> +                     (pcase-exhaustive mapping
> +                       (`(internal-get-closed-var . ,_)
> +                        ;; The variable is captured; remap.
> +                        mapping)
> +                       (`(car-safe (internal-get-closed-var . ,_))
> +                        ;; The variable is mutably captured; remap, but skip
> +                        ;; the indirection step because the variable is
> +                        ;; passed "by rerefence" to the λ-lifted function.
> +                        (cadr mapping))
> +                       ((or '() `(car-safe ,(pred symbolp)))
> +                        ;; The variable is not captured.  Add a
> +                        ;; reference to the outside binding and arrange
> +                        ;; to use that reference.
> +                        (let ((closedsym
> +                               (make-symbol (format "closed-%s" var))))
> +                          (push `(,closedsym ,var) binders-new)
> +                          closedsym)))))
> +               (setq new-env (cconv--remap-llv new-env var remap-to))
> +               (setq new-extend (cons remap-to (remq var new-extend)))))))

Same comment as before about the code duplication.


        Stefan