<< Back to previous view

[CLJS-1191] Update clojure.walk to the current version on clojure Created: 06/Apr/15  Updated: 03/Jul/15  Resolved: 03/Jul/15

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 0.0-3308
Fix Version/s: Next

Type: Defect Priority: Major
Reporter: Stuart Mitchell Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: enhancement, performance

Attachments: Text File 50.patch     Text File CLJS-1191.patch     Text File CLJS-1191-rebase.patch     Text File CLJS-1191-tests.patch    
Patch: Code

 Description   

Currently clojure.walk can not handle records

It is using an old version of clojure.walk and clojure has improved the implementation because of

http://dev.clojure.org/jira/browse/CLJ-1105


src/cljs/clojure/walk.cljs | 4 ++++
1 file changed, 4 insertions

diff --git a/src/cljs/clojure/walk.cljs b/src/cljs/clojure/walk.cljs
index f2ebd8d..541ecea 100644
— a/src/cljs/clojure/walk.cljs
+++ b/src/cljs/clojure/walk.cljs
@@ -43,7 +43,11 @@ the sorting function."}
{:added "1.1"}
[inner outer form]
(cond
+ (list? form) (outer (apply list (map inner form)))
+ (satisfies? IMapEntry form) (outer (vec (map inner form)))
(seq? form) (outer (doall (map inner form)))
+ (satisfies? IRecord form)
+ (outer (reduce (fn [r x] (conj r (inner x))) form form))
(coll? form) (outer (into (empty form) (map inner form)))
:else (outer form)))



 Comments   
Comment by David Nolen [ 07/Apr/15 5:56 AM ]

Please attach the patch as a file. Thanks!

Comment by Stuart Mitchell [ 07/Apr/15 7:18 PM ]

I think this one works

it is a mail formatted patch

Comment by David Nolen [ 08/Apr/15 6:07 AM ]

Please follow the patch conventions described here https://github.com/clojure/clojurescript/wiki/Patches. Thank you.

Comment by David Nolen [ 12/Apr/15 4:53 PM ]

Stuart, I don't see you on the list of contributors. Please submit a CA so I can apply the patch. Thanks!

Comment by Stuart Mitchell [ 13/Apr/15 7:49 PM ]

Hopefully this is the right format

Comment by Stuart Mitchell [ 13/Apr/15 7:50 PM ]

Contributor agreement signed as well

Comment by Sebastian Bensusan [ 01/May/15 1:24 PM ]

Tested the patch on the REPL, works as expected except for walking fns that modify the keys:

> (defrecord Foo [name])
cljs.user/Foo
> (w/prewalk #(if (keyword? %) (str %) %) (Foo. "foo"))
#cljs.user.Foo{:name "foo", ":name" "foo"}

It is not consistent with walking the same fn over a map:

> (w/prewalk #(if (keyword? %) (str %) %) {:name "foo"})

{":name" "foo"}

This behavior was noted in the original Clojure patch as well: http://dev.clojure.org/jira/browse/CLJ-1239 and might be undesirable since it surprises the user.

Comment by Daniel Skarda [ 10/Jun/15 9:59 AM ]

Our project shares many files between server and client (Clojure and ClojureScript). I noticed the same bug today, because tests in CLJ went fine, while same tests in CLJS failed (then I prepared a patch just to find this ticket...)

From my perspective, I would prefer that CLJ and CLJS have the same implementation of clojure.walk.

For example - you implement some clever scheme to avoid kind of surprise you described. If it is not implemented in Clojure in the same way, it will be surprise for people writing code portable between CLJ and CLJS.

So my proposal is to take current Clojure implementation as a master and wait, how clojure.walk implementation will be improved in future releases of Clojure. Then we can port it to ClojureScript.

Comment by Daniel Skarda [ 10/Jun/15 10:10 AM ]

Tests for patch by Stuart

Comment by David Nolen [ 01/Jul/15 1:38 PM ]

Happy to apply these but they need to be rebased to master. Thanks.

Comment by Stuart Mitchell [ 02/Jul/15 9:21 PM ]

Hi this patch does not include the tests as that patch still applies cleanly.

Comment by David Nolen [ 03/Jul/15 2:58 PM ]

fixed
https://github.com/clojure/clojurescript/commit/f706fabfd5f952c4dfb4dc2caeea92f9e00d8287
https://github.com/clojure/clojurescript/commit/f1ff78d882bbaaa5135f205dfa19b01895462a3b





[CLJS-1326] In bootstrapped cljs, read-number of "0" gives "invalid number format" Created: 03/Jul/15  Updated: 03/Jul/15  Resolved: 03/Jul/15

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: Next
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Joel Martin Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bootstrap
Environment:

cljs-bootstrap REPL (https://github.com/kanaka/cljs-bootstrap)



 Description   

tools.reader/read-number (https://github.com/swannodette/tools.reader) of "0" throws an error. The same for "1" works fine.

cljs-bootstrap.repl> 0
Error: Invalid number format [0]
    at new cljs$core$ExceptionInfo (/home/joelm/scratch/cljs-bootstrap/.cljs_bootstrap/cljs/core.js:33157:10)
    at Function.cljs.core.ex_info.cljs$core$IFn$_invoke$arity$3 (/home/joelm/scratch/cljs-bootstrap/.cljs_bootstrap/cljs/core.js:33219:9)
    at Function.cljs.core.ex_info.cljs$core$IFn$_invoke$arity$2 (/home/joelm/scratch/cljs-bootstrap/.cljs_bootstrap/cljs/core.js:33215:26)
    at cljs$core$ex_info (/home/joelm/scratch/cljs-bootstrap/.cljs_bootstrap/cljs/core.js:33201:26)
    at Function.cljs.tools.reader.reader_types.reader_error.cljs$core$IFn$_invoke$arity$variadic (/home/joelm/scratch/cljs-bootstrap/.cljs_bootstrap/cljs/tools/reader/reader_types.js:802:25)
    at cljs$tools$reader$reader_types$reader_error (/home/joelm/scratch/cljs-bootstrap/.cljs_bootstrap/cljs/tools/reader/reader_types.js:798:52)
    at cljs$tools$reader$read_number (/home/joelm/scratch/cljs-bootstrap/.cljs_bootstrap/cljs/tools/reader.js:446:52)
    at /home/joelm/scratch/cljs-bootstrap/.cljs_bootstrap/cljs/tools/reader.js:1550:38
    at cljs$tools$reader$reader_types$log_source (/home/joelm/scratch/cljs-bootstrap/.cljs_bootstrap/cljs/tools/reader/reader_types.js:873:16)
    at cljs$tools$reader$target (/home/joelm/scratch/cljs-bootstrap/.cljs_bootstrap/cljs/tools/reader.js:1528:50)

This is because reader/read-number has:

(or (match-number s) (reader-error rdr "Invalid number format [" s "]")))

which compiled to this JS:

var or__4073__auto__ = cljs.tools.reader.impl.commons.match_number.call(null,s);
if(or__4073__auto__){
return or__4073__auto__;
} else {
return cljs.tools.reader.reader_types.reader_error.call(null,rdr,"Invalid number format [",s,"]");
}

Since match_number returns a JS number, or_4073auto_ is 0, therefore falsey.



 Comments   
Comment by Joel Martin [ 03/Jul/15 9:31 AM ]

FYI, to reproduce, run this in https://github.com/kanaka/cljs-bootstrap

lein run -m clojure.main script/build.clj
node repl.js
cljs-bootstrap.repl> 0
Error: Invalid number format [0]
...
Comment by David Nolen [ 03/Jul/15 2:55 PM ]

This ticket doesn't have nearly enough information. I checked the output of tools.reader and I don't see this generated code at all. The test is wrapped in the required call to truth_.





[CLJS-1321] defrecord broken in bootstrapped cljs Created: 01/Jul/15  Updated: 02/Jul/15  Resolved: 02/Jul/15

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: Next
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Joel Martin Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap
Environment:

cljs-bootstrap REPL (https://github.com/swannodette/cljs-bootstrap)



 Description   

First problem that I run into is that emit-defrecord makes use of .getNamespace and .getName on rname. That should probably be (namespace rname) and (name rname). After that change is made the next error is "Can't set! local var or non-mutable field" somewhere in defrecord. Not sure what the cause of that one is.



 Comments   
Comment by David Nolen [ 02/Jul/15 6:03 PM ]

fixed https://github.com/clojure/clojurescript/commit/8a5023b849cfc509931b3ff509a9f7ee48dd03ec

Comment by David Nolen [ 02/Jul/15 6:04 PM ]

I did not look into the set! issue. Separate ticket should be opened for that if it persists.

Comment by Mike Fikes [ 02/Jul/15 6:46 PM ]

Confirmed fixed downstream (https://github.com/mfikes/replete/issues/25), apart from other Can't set! local var or non-mutable field error, which needs a separate ticket.





[CLJS-1323] possible to reload cljs.core in browser REPL Created: 01/Jul/15  Updated: 02/Jul/15  Resolved: 02/Jul/15

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 0.0-3308
Fix Version/s: Next

Type: Defect Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Not Reproducible Votes: 0
Labels: None


 Description   

Accidentally doing so will wreck havoc on the browser REPL as all the core types will get redefined. For some reason cljs.core is the only namespace that is not on the goog.dependencies_.written list, once as the relative path and once again as the absolute path. Uncovered while attempting to test the bootstrap support in browser environments.



 Comments   
Comment by David Nolen [ 02/Jul/15 6:02 PM ]

Misinterpreted the issue. Was loading the macros ns which shares the same name. Needed to rewrite in to a macro ns instead.





Generated at Sat Jul 04 05:27:16 CDT 2015 using JIRA 4.4#649-r158309.