Lexbind fix #18

Merged
merged 5 commits into from Oct 21, 2013

Projects

None yet

3 participants

@DarwinAwardWinner
Contributor

This pull request adds a test for and fixes #17.

@thierryvolpiatto
Collaborator

What about using apply-partially instead of eval?

That appears to work too.

Actually, it seems to fail some tests when I use apply-partially.

Collaborator

Ok, I got apply-partially to work correctly. However, I don't really see the advantage over eval.

@thierryvolpiatto
Collaborator

These modifications (using whether apply-partially or eval) are fixing this particular bug,i.e

(mapcar #'async-get
        (cl-loop repeat 10 collect
                 (async-start (lambda () t))))

but are breaking other code like this in a lexical environment:

(mapcar #'async-get
        (cl-loop repeat 10 collect
                 (async-start `(lambda () ,(* 150 2)))))

So I don't think it is a good idea to introduce such code in async.el,
although to make working the above test that initiate this bug, only quoting the lambda make it working:

(mapcar #'async-get
        (cl-loop repeat 10 collect
                 (async-start '(lambda () t))))

@jwiegley WDYT

@DarwinAwardWinner DarwinAwardWinner Have test file add its own directory to load path
This makes it so that one can do "emacs -batch -l async-test.el ..."
without having to also add "-l async.el -l async-file.el" at the
command line.
5ef546a
@DarwinAwardWinner DarwinAwardWinner Add more comprehensive testing for anti-closure feature
Based on suggestions in
jwiegley#18 (comment)
9b5bb5c
@DarwinAwardWinner DarwinAwardWinner More robust evaluation and closure-protection of start-func 93b05a9
@DarwinAwardWinner
Contributor

I've modified my pull request to pass all the tests you give above.

@jwiegley jwiegley merged commit f6d7a74 into jwiegley:master Oct 21, 2013
@DarwinAwardWinner
Contributor

Keep in mind that this patch changes the behavior of async.el. You might want to document the change, since my patch doesn't document anything.

@jwiegley
Owner

Can you describe the exact nature of this change and whom it may effect, so that I can add that to the docs? Thanks!

@DarwinAwardWinner
Contributor

Here's a case that is broken by the changes:

(eval
 '(let ((x 1)
        (y 2)
        (z 3))
    (async-sandbox (lambda () (+ x y z))))
 t)

The lambda being passed to async should actually evaluate a lexical closure that encapsulates the values of x, y, and z. Obviously it will produce a void-variable error if those variables are not available. In this case it is trivial to pre-substitute the variables using e.g. the backquote macro, but this may not be generally true. The point is that this change disallows conversion of lambdas into closures in a call to async-start.

I'm writing up some tests and working on a potentially better fix that will allow the above code to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment