GNU bug report logs - #51982
Erroneous handling of local variables in byte-compiled nested lambdas

Previous Next

Full log

View this message in rfc822 format

From: Mattias Engdegård <mattiase <at> acm.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, Paul Pogonyshev <pogonyshev <at> gmail.com>, 51982 <at> debbugs.gnu.org
Subject: bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas
Date: Tue, 30 Nov 2021 18:01:59 +0100

[Message part 1 (text/plain, inline)]

30 nov. 2021 kl. 15.12 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:

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

I extracted a big part of the code into a common function but left the free variable access and mutation outside. (Really want to get rid of `let*`!)

> These two tests are identical aren't they?

No, they exercise different code paths (let and let*).

>  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?

Yes, thank you, it was an editing mistake. Fixed.

> Looks good (better than patch A).

And here I was prepared to apply patch A since it's slightly more conservative and it seems to be a rare problem anyway.
I've now split the patches in a more sensible (and easily reviewed) way: the first corresponds to patch A, and the second is the diff to B. Take a second look before making up your mind.

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

There are comments and doc strings such as

  EXTEND is a list of variables which might need to be accessed even from places
  where they are shadowed, because some part of ENV causes them to be used at
  places where they originally did not directly appear.

but with the B patch we put things into `extend` that are not strictly variables but (international-get-closed-var N).
Similarly, `env` has entries like (VAR . (apply-partially F ARG1 ARG2 ..)) where the ARGi are always treated as variables but now they can be access forms as well.

I suppose it doesn't matter much. There is an assertion at the very top of `cconv-convert` which compares the elements by `eq` but it seems to work all right...

Thanks for the review – new patches attached.

[0001-Fix-closure-conversion-of-shadowed-captured-lambda-l.patch (application/octet-stream, attachment)]

[0002-Improved-closure-conversion-of-shadowed-captured-lam.patch (application/octet-stream, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.