ClojureScript

doseq bindings with :let not properly scoped

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

This seems to be a subtler version of CLJS-39:

(let [fs (atom [])]
  (doseq [x (range 4)
          :let [y (inc x)
                f (fn [] y)]]
    (swap! fs conj f))
  (map #(%) @fs))

;; => (4 4 4 4)
;; should be (1 2 3 4)

I think this example is minimal. The bug is not present if we use a proper let.

Activity

Hide
Michał Marczyk added a comment - - edited

It appears that this works correctly on current master. In fact, it worked correctly on r1576 – but not on r1552. Also, splitting the :let into two ("adjacent") :let's fixes this, whereas moving both the y and f bindings into a single regular let inside the doseq does not. That means it's probably an instance of CLJS-442 (fns defined in local init exprs not closing over earlier locals), fixed in https://github.com/clojure/clojurescript/commit/84e9488f49bcfea4b4037a562f8f797c7ddd79f0 (included in releases since r1576).

Show
Michał Marczyk added a comment - - edited It appears that this works correctly on current master. In fact, it worked correctly on r1576 – but not on r1552. Also, splitting the :let into two ("adjacent") :let's fixes this, whereas moving both the y and f bindings into a single regular let inside the doseq does not. That means it's probably an instance of CLJS-442 (fns defined in local init exprs not closing over earlier locals), fixed in https://github.com/clojure/clojurescript/commit/84e9488f49bcfea4b4037a562f8f797c7ddd79f0 (included in releases since r1576).
Hide
David Nolen added a comment -

So I'm assuming we can just close this one?

Show
David Nolen added a comment - So I'm assuming we can just close this one?
Hide
Michał Marczyk added a comment -

Yeah, seems so.

Show
Michał Marczyk added a comment - Yeah, seems so.
Hide
David Nolen added a comment -
Show
David Nolen added a comment - added test, this test passes on master, http://github.com/clojure/clojurescript/commit/a1b41bed8921dbd9ef9ee7d735b158ae71ed6132
Hide
Gary Fredericks added a comment -

Sounds good to me. Thanks!

Show
Gary Fredericks added a comment - Sounds good to me. Thanks!

People

Vote (1)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: