ClojureScript

`with-meta` does not work on function objects

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Environment:
    CLJS Rhino-REPL and during Compilation
  • Patch:
    Code and Test

Description

`with-meta` does not working on function objects in CLJS. Compilation fails with the following error:

Error: No protocol method IWithMeta.-with-meta defined for type function: function { return x; }

I tried it out on the REPL and found the following:

---------- BEGIN: repl-rhino ----------
ClojureScript:cljs.user> (with-meta #(do :foo) {:foo :bar})
"Error evaluating:" (with-meta (fn* [] (do :foo)) {:foo :bar}) :as "cljs.core.with_meta.call(null,(function (){\nreturn \"\\uFDD0'foo\";\n}),cljs.core.ObjMap.fromObject([\"\\uFDD0'foo\"],{\"\\uFDD0'foo\":\"\\uFDD0'bar\"}));\n"
org.mozilla.javascript.JavaScriptException: Error: No protocol method IWithMeta.-with-meta defined for type function:
function () { return "\ufdd0'foo"; }
(cljs/core.cljs#222)
at cljs/core.cljs:222 (anonymous)
at cljs/core.cljs:214 (_with_meta)
at cljs/core.cljs:806 (with_meta)
at <cljs repl>:2 (anonymous)
at <cljs repl>:2

nil
---------- END: repl-rhino ----------

Reference: https://groups.google.com/forum/?fromgroups=#!topic/clojure/pRO-5IlilNM

  1. CLJS-359-v1.patch
    21/Nov/12 3:29 PM
    4 kB
    Brandon Bloom
  2. CLJS-359-v2.patch
    21/Nov/12 4:16 PM
    4 kB
    Brandon Bloom

Activity

David Nolen made changes -
Field Original Value New Value
Assignee David Nolen [ dnolen ]
David Nolen made changes -
Status Open [ 1 ] In Progress [ 3 ]
Hide
David Nolen added a comment -

We could create a fn wrapper type of some kind that implements all the supported arities of IFn (this means the compiler needs to guarantee that args past 20 are collected into an array-seq) that include the extra meta fields. Seems doable.

Show
David Nolen added a comment - We could create a fn wrapper type of some kind that implements all the supported arities of IFn (this means the compiler needs to guarantee that args past 20 are collected into an array-seq) that include the extra meta fields. Seems doable.
Hide
Paul deGrandis added a comment -

I have created that exact function wrapper - it's in Shoreleave. We can move it into CLJS proper if you like.

https://github.com/shoreleave/shoreleave-core/blob/master/src/shoreleave/efunction.cljs

Show
Paul deGrandis added a comment - I have created that exact function wrapper - it's in Shoreleave. We can move it into CLJS proper if you like. https://github.com/shoreleave/shoreleave-core/blob/master/src/shoreleave/efunction.cljs
Hide
David Nolen added a comment -

That's nice but would prefer to have as little overhead as possible.

Show
David Nolen added a comment - That's nice but would prefer to have as little overhead as possible.
Hide
Paul deGrandis added a comment -

ala unrolling the invoke call?

More than happy to start pulling together a patch.

Show
Paul deGrandis added a comment - ala unrolling the invoke call? More than happy to start pulling together a patch.
Hide
David Nolen added a comment -

Yes there's actually an existing ticket for that. Thanks. We also need compiler support for function invocations when there's more then 20 arguments - though that's a different existing ticket.

Show
David Nolen added a comment - Yes there's actually an existing ticket for that. Thanks. We also need compiler support for function invocations when there's more then 20 arguments - though that's a different existing ticket.
David Nolen made changes -
Status In Progress [ 3 ] Open [ 1 ]
Hide
Paul deGrandis added a comment - - edited

I was just waiting on CLJS-365 to be completed. Did you think of a better approach than the deftype-wrapper? More than happy to push on this.

Show
Paul deGrandis added a comment - - edited I was just waiting on CLJS-365 to be completed. Did you think of a better approach than the deftype-wrapper? More than happy to push on this.
Hide
David Nolen added a comment -

I thought about this some more - this patch is really about adding metadata to functions at runtime - static metadata on defs are stored in cljs.analyzer/namespaces. Given that it's runtime I think your simple patch is actually OK, we can't generally optimize functions like that anyway since we won't have that kind of information available.

Show
David Nolen added a comment - I thought about this some more - this patch is really about adding metadata to functions at runtime - static metadata on defs are stored in cljs.analyzer/namespaces. Given that it's runtime I think your simple patch is actually OK, we can't generally optimize functions like that anyway since we won't have that kind of information available.
Hide
Brandon Bloom added a comment -

Patch adds a Function type and extends js/function to support metadata. There is now an Fn marker protocol.

Patch also includes an additional commit to fix the behavior of the this variable for IFn definitions.

Not tested on JavaScriptCore.

Show
Brandon Bloom added a comment - Patch adds a Function type and extends js/function to support metadata. There is now an Fn marker protocol. Patch also includes an additional commit to fix the behavior of the this variable for IFn definitions. Not tested on JavaScriptCore.
Brandon Bloom made changes -
Patch Code and Test [ 10002 ]
Attachment CLJS-359-v1.patch [ 11695 ]
Hide
Brandon Bloom added a comment -

v2 of patch improves speed of fn? and utilizes reify to avoid creating an explicit Function type

Show
Brandon Bloom added a comment - v2 of patch improves speed of fn? and utilizes reify to avoid creating an explicit Function type
Brandon Bloom made changes -
Attachment CLJS-359-v2.patch [ 11696 ]
David Nolen made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Resolved [ 5 ]
David Nolen made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

Vote (1)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: