jwiegley / emacs-async

Simple library for asynchronous processing in Emacs
GNU General Public License v3.0
838 stars 68 forks source link

Lexbind fix #18

Closed DarwinAwardWinner closed 11 years ago

DarwinAwardWinner commented 11 years ago

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

thierryvolpiatto commented 11 years ago

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 commented 11 years ago

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

DarwinAwardWinner commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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.