clj-commons / manifold

A compatibility layer for event-driven abstractions
1.02k stars 106 forks source link

let-flow can't be used inside record methods #171

Closed nhurden closed 3 years ago

nhurden commented 5 years ago

Using let-flow inside defrecord causes a java.lang.UnsupportedOperationException: Can't type hint a primitive local (See also https://github.com/clojure/clojure/blob/b19b781b1f0f3f46aee5e951f415e0456a39cbcb/src/jvm/clojure/lang/Compiler.java#L6015)

e.g.

(defprotocol TestProtocol
  (f [this x]))

(defrecord TestRecord []
  TestProtocol
  (f [this _]
    (d/let-flow [x {}] 0)))

It appears that this has only been an issue since the introduction of __hash and __hasheq to records in Clojure 1.9, since these have a type hint of ^int: https://github.com/clojure/clojure/commit/a1c3dafec01ab02fb10d91f98b9ffd3241e860c0#diff-03234b041c0917ec98f2ad9477a0a014

It looks like this happens because expand-let-flow's call to (riddley.compiler/locals) returns the reserved symbols defined by defrecord: __hash __meta __hasheq __extmap.

A possible fix might be just to filter out those reserved symbols:

(defn- remove-reserved-locals [locals]
  (remove #{'__meta '__extmap '__hash '__hasheq} locals))

(defn- expand-let-flow [chain-fn zip-fn bindings body]
  (let [[_ bindings & body] (walk/macroexpand-all `(let ~bindings ~@body))
        locals              (remove-reserved-locals (keys (compiler/locals)))
…
KingMob commented 3 years ago

@nhurden Can you verify this is still an issue? I believe some later changes that Zach made to let-flow cleared up this problem. (Maybe https://github.com/clj-commons/manifold/commit/dae833 or https://github.com/clj-commons/manifold/commit/651594)

I tried with both Clojure 1.9.0 and 1.10.3, and I can't recreate the exception unless I specifically refer to a reserved field, which I imagine is extremely rare in real code.

nhurden commented 3 years ago

Looks like it works now 👍 , though I needed to add an explicit manifold dependency since the latest version of aleph still has 0.1.9-alpha3 and this issue is fixed in 0.1.9-alpha4.

KingMob commented 3 years ago

@nhurden Yes, we've fixed a few bugs, but haven't quite cut a new release yet.