Fork me on GitHub
#cljs-dev
<
2016-05-26
>
dnolen00:05:36

@rohit looks interesting but it’s not in any shape that I would look at it

dnolen00:05:00

no variables preferred

dnolen00:05:25

No Node.js, no tooling or projects - just a file I can compile is all

dnolen00:05:13

file an issue in JIRA with the minimal ClojureScript source needed to reproduce the issue

rohit06:05:12

@dnolen: I can create an example with just a cljs file and no other dependencies. But the variables will be there as its that particular alignment of things which is triggering the issue. Thank you!

rohit08:05:53

If the bug report isn’t clear enough, please do let me know.

rohit08:05:47

bummer. there is a typo in the issue summary. I can’t seem to edit it.

rohit08:05:06

@dnolen: the real doozy (atleast for me) is when you get the expected behaviour by doing any of the things listed in How to get the expected behaviour

rohit08:05:48

they seem to be completely orthogonal to the problem in my eyes. but they address the issue.

thheller10:05:02

@rohit I seem to remember a similar issue where embedding large strings in code caused problems

thheller10:05:15

can't exactly remember what it was though

thheller10:05:45

have you tried loading the edn string via some other mechanism? instead of embedding in code?

rohit10:05:12

@thheller: i’ve tried loading file from the filesystem using node.js and i get the same behaviour.

thheller10:05:55

ah ok, only saw the JIRA issue

rohit10:05:33

i’ve tried similar code in clojure and the behaviour is as expected.

rohit10:05:43

so its something in cljs

thheller10:05:17

yeah weird thing is that it completely breaks the runtime

thheller10:05:32

I dumped your code into my project which otherwise works perfectly

thheller10:05:39

when it also executes your code

thheller10:05:46

barely anything works at all

thheller10:05:04

well anything related to reader doesn't work anymore

thheller10:05:11

so somehow it breaks the reader

rohit10:05:20

thats what my colleague also discovered. (see jira comments)

rohit10:05:50

if you just remove the pprint, things work

rohit10:05:08

now that is something which makes no sense to me

thheller10:05:50

yeah it is very weird indeed

rohit10:05:34

even weirder: if your remove lines 3166-3181 in a custom cljs build, things work again. https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/pprint.cljs#L3166-L3181

rohit10:05:04

so its something about global maps

mfikes14:05:24

@dnolen: In the unlikely event that it matters for ClojureScript spec, just a heads up that there is a patch pending that would extend test.check to self-hosted ClojureScript: http://dev.clojure.org/jira/browse/TCHECK-105

thheller16:05:54

@rohit just verified that the bug you are seeing is related to hashing. we have seen things like that before, added comment in JIRA

rohit16:05:15

this one seems to be atleast independent of ios/safari.

thheller16:05:13

have you tried firefox or safari?

thheller16:05:21

maybe it is limited to v8

rohit16:05:48

@thheller: i’ve tried firefox and safari.

rohit16:05:03

i’ve tried on OS X - safari/firefox/chrome/nodejs

rohit16:05:10

on linux - firefox/chrome/nodejs

rohit16:05:33

so indeed the issue seems to be the hashing

rohit16:05:18

in buggy version hash of :version is 430259761 and the correct one is 425292698

rohit17:05:04

maybe the issue is in cljs.reader/read-keyword

darwin17:05:35

hi, I have a headaches reading a serialized clojuer map from clojurescript, the problem is a map key-value :pprint-fn #function[cider.nrepl.middleware.pprint/wrap-pprint-fn/fn--24400/fn—24402], it gets produced by https://github.com/clojure-emacs/cider-nrepl/blob/master/src/cider/nrepl/middleware/pprint.clj#L50

thheller17:05:27

@rohit nah it is just the caching. the compiler precomputes the hash (in clojure) when compiling

darwin17:05:28

I tried to use cljs.reader/read, cljs.tools.reader/read and cljs.tools.reader.edn/read

thheller17:05:40

so the hash clj generates is different from cljs in some cases

darwin17:05:58

all of them are failing to read that “symbol" cider.nrepl.middleware.pprint/wrap-pprint-fn/fn--24400/fn—24402

darwin17:05:14

it has “too many” slashes I guess 🙂

thheller17:05:21

@darwin yeah that doesnt look like a valid symbol

thheller17:05:01

probably better to fix the writer side 😛

darwin17:05:10

I don’t see any other way around that, because I’m not in control of the writer side

darwin17:05:14

people who wrote cider middleware didn’t expect those messages to be parsed by anything outside clojure (their own nrepl process) https://github.com/clojure-emacs/cider-nrepl/blob/master/src/cider/nrepl/middleware/pprint.clj#L50

darwin17:05:57

oh, wait, I will just fix it with a dirty regexp, replacing those extra slashes

rohit17:05:05

@thheller: for hashing a keyword, it seems that the following are needed: m3-hash-unencoded-chars of the name of the keyword and hash-string of its ns.

rohit17:05:43

i’ve found that m3-hash-unencoded-chars is the same for the reader and normal :version but the hash-string is different for both

rohit17:05:21

and hence the different hashes

rohit17:05:50

looking at the code of hash-string, there seems to be some sort of global cache: https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/core.cljs#L849

rohit17:05:26

If i replace that code with not using the cache, it all works!

dnolen19:05:54

@rohit this just sounds like a simple hashing bug - we want the actual fix - not using the cache is not the answer

rohit20:05:21

@dnolen: i’ve proposed a fix which addresses this issue in the jira issue page: http://dev.clojure.org/jira/browse/CLJS-1649

rohit20:05:43

it fixes the the hash-string function

rohit20:05:15

@dnolen: essentially we shouldn’t be putting nil as a key in the cache as it gets converted to ”null”

dnolen20:05:16

@rohit yeah I saw that, sorry had only read the backlog here

rohit20:05:21

this channel has ben really useful in figuring out the issue!

dnolen20:05:06

@rohit can we please add a test to the patch? thanks

dnolen20:05:28

@rohit I’m assuming you’ve submitted your CA etc.

rohit20:05:23

@dnolen: sure. i’ll do that. i’ve submitted my CA a while back.

rohit20:05:14

"Rohit Aggarwal"

dnolen20:05:57

@rohit: excellent, thanks again

dnolen20:05:25

@bhauman: left a comment on your patch, minor change, let’s not flip the arg order

rohit20:05:57

@dnolen: just uploaded a patch with a test.

dnolen20:05:25

@rohit: it appears maybe you didn’t run the test 🙂

dnolen20:05:26

FAIL in (test-nil-hashing-cljs-1649) (J_@builds/out-adv/core-advanced-test.js:1021:83) expected: (pos? (hash-string nil)) actual: (not (pos? 0))

rohit20:05:40

i did on jsc engine

dnolen20:05:51

it fails on SpiderMonkey and JSC

rohit20:05:58

ah…i submitted the wrong file

rohit20:05:25

pos? should have been zero?

dnolen20:05:34

no worries

rohit20:05:37

really sorry

rohit20:05:14

@dnolen: i uploaded the patch again. really sorry about this.

rohit20:05:19

@dnolen: thanks a lot!

rohit20:05:44

@thheller really led me in the right direction

dnolen20:05:45

applied to master

rohit20:05:58

awesome! 😄