replete-repl / replete-ios

ClojureScript REPL iOS app
Eclipse Public License 1.0
395 stars 25 forks source link

divide fn warning #34

Closed mfikes closed 9 years ago

mfikes commented 9 years ago

(reduce / [1 2])

WARNING: Use of undeclared Var cljs.core//
0.5

Note: this works in ClojureScript.net

karlmikko commented 9 years ago

I have been playing around with this today, results of tests so far.

I have been looking at cljs-bootstrap also and can't really see any difference that causes this so far.

screen shot 2015-07-08 at 6 16 17 pm
mfikes commented 9 years ago

Here's a theory: When I was messing around with inlining the compiler analysis metadata (see this post), there was a problem with the symbol cljs.core// being parsed by the compiler. So perhaps it isn't making its way properly through edn (or Transit) and back in a way that works like all of the other symbols.

Also see that the compiler has special handling in places:

https://github.com/clojure/clojurescript/blob/903f39c99bcc44eaf6b5ef87a2e65f86300b4886/src/main/clojure/cljs/compiler.cljc#L93

From the big picture, in case that is unclear (it was to me a few months ago), the function exists, as can be seen; it is only the analysis metadata that causes the analyzer to complain. Also, FWIW ClojureScript—unlike Clojure—has both functions and macros. That's why (/ 2 3) works without complaining: It involves the macro.

Why this works in Joel Martin's stuff is a double mystery.

mfikes commented 9 years ago

Also, (reduce cljs.core// [1 2]) works without causing the analyzer to complain.

karlmikko commented 9 years ago

Thanks for the extra info!

karlmikko commented 9 years ago

I decided to not load the cache files. It seems that reduce us in cljs.user and / is in cljs.core. Don't know if this is anything or not.

screen shot 2015-07-10 at 5 17 56 pm
mfikes commented 9 years ago

@karlmikko By not loading the analysis cache files, then the REPL no longer knows about cljs.core/reduce, and when it attempts to resolve the symbol reduce it can't, and the error message reflects that the symbol would be expected to be defined in your current namespace. So, if you first (ns foo.bar), the error message would probably display an error regarding foo.bar/reduce.

karlmikko commented 9 years ago

The issue is in the edn traveling through transit.

The issue goes away when you load the edn directly.

(defn load-cache [cache where]
  (swap! cenv assoc-in [::ana/namespaces where] cache)
  nil)
screen shot 2015-07-10 at 11 20 08 pm

I can make a PR if you want - had to change the build script and the project

mfikes commented 9 years ago

@karlmikko Awesome discovery. What would you change?

karlmikko commented 9 years ago

I created a PR so you can see the changes.

mfikes commented 9 years ago

@karlmikko It looks like that PR would roughly revert https://github.com/mfikes/replete/commit/08ca5fb92e17af356ff23c3aa2f3a6b59ebcd795

The reason for using Transit is that it provides an order of magnitude faster startup. See http://blog.fikesfarm.com/posts/2015-07-02-mobile-transit-ftw.html and see #6

But... it is awesome you pinned this down to a Transit issue. Knowing that is half the battle. Perhaps we can sort out what is going on wrong with Transit (or even just patch up that one function if need be after the transit is read.) Dunno, just thinking out loud.

karlmikko commented 9 years ago

@mfikes very hard question. I think it may be worth reverting till we do know what is the issue with transit.

What I have done is similar to what clojurescript.net is doing. (They put the edn as a string in the js and call it prior to starting the repl loop https://github.com/kanaka/cljs-bootstrap/blob/master/script/gen_single.sh#L97)

Speed after this change on my iPhone 6+ is super fast (under 2 seconds), I can't tell the difference to how it was using transit.

karlmikko commented 9 years ago

Further to before: it would be hard to know exactly what other functions may be effected so just patching the known effected functions doesn't sound the best to me. There is potential that later builds may cause the issue to show up on different functions also.

karlmikko commented 9 years ago

@mfikes I have got the edn compiled to js and load it with (js/eval) also updated the PR.

Speed seems ok from my end - however it has always performed well for me.

mfikes commented 9 years ago

@karlmikko 's solution also works at same speed on iPad 2.