Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Namespace a.core$macros causes planck to break #543

Closed
shawwn opened this issue Nov 6, 2017 · 12 comments
Closed

Namespace a.core$macros causes planck to break #543

shawwn opened this issue Nov 6, 2017 · 12 comments

Comments

@shawwn
Copy link

shawwn commented Nov 6, 2017

$ planck 
Planck 2.8.1
ClojureScript 1.9.946
    Docs: (doc function-name-here)
          (find-doc "part-of-name-here")
  Source: (source function-name-here)
    Exit: Control+D or :cljs/quit or exit or quit
 Results: Stored in vars *1, *2, *3, an exception in *e

cljs.user=> (ns a.core$macros)
nil
a.core$macros=> (defmacro hell [x] `(+ x 2))
undefined is not an object (evaluating 'a.core$macros')
$ planck --version
2.8.1

Credit to @jiyinyiyong for discovering this. anmonteiro/lumo#301 (comment)

@shawwn shawwn changed the title Namespace a.b$macros causes planck to break Namespace a.core$macros causes planck to break Nov 6, 2017
@mfikes
Copy link
Member

mfikes commented Nov 6, 2017

It is actually a more generic issue:

cljs.user=> (ns a.core)
nil
a.core=> (def a 3)
undefined is not an object (evaluating 'a.core.a = (3)')

@mfikes
Copy link
Member

mfikes commented Nov 6, 2017

As far as I can tell, this appears to be rooted in a Closure compiler error related to the application of simple optimizations to goog.exportPath_. In particular, if you try (goog.exportPath_ "a.core") and then evaluate a.core you will see that the namespace object is nil. This does not occur if you build Planck without these optimizations.

It is not yet clear to me how the optimized code fails in the case of "a.core" while not for "aa.core".

Here is the optimized version of goog.exportPath_:

#object[Function "function (a,b,c){a=a.split(".");c=c||goog.global;a[0]in c||!c.execScript||c.execScript("var "+a[0]);for(var d;a.length&&(d=a.shift());)!a.length&&goog.isDef(b)?c[d]=b:c=c[d]&&c[d]!==Object.prototype[d]?c[d]:c[d]={}}"]

And here is the unoptimized version you get if you build Planck without these optimizations:

#object[Function "function (name, opt_object, opt_objectToExportTo) {
  var parts = name.split('.');
  var cur = opt_objectToExportTo || goog.global;

  // Internet Explorer exhibits strange behavior when throwing errors from
  // methods externed in this manner.  See the testExportSymbolExceptions in
  // base_test.html for an example.
  if (!(parts[0] in cur) && cur.execScript) {
    cur.execScript('var ' + parts[0]);
  }

  for (var part; parts.length && (part = parts.shift());) {
    if (!parts.length && goog.isDef(opt_object)) {
      // last part and we have an object; use it
      cur[part] = opt_object;
    } else if (cur[part] && cur[part] !== Object.prototype[part]) {
      cur = cur[part];
    } else {
      cur = cur[part] = {};
    }
  }
}"]

@mfikes
Copy link
Member

mfikes commented Nov 6, 2017

@anmonteiro Is onto an idea. This only fails for (goog.exportPath_ "a.core") through (goog.exportPath_ "c.core") but starts working at (goog.exportPath_ "d.core"). Perhaps some odd shadowing is going on.

@mfikes
Copy link
Member

mfikes commented Nov 6, 2017

Yeah, when you start up Planck, you will see some global variables have already been defined:

cljs.user=> js/a
"a"
cljs.user=> js/b
nil
cljs.user=> js/c
nil
cljs.user=> js/d
Can't find variable: d

@mfikes
Copy link
Member

mfikes commented Nov 7, 2017

The root cause is that the locals here (along with the let local)
https://github.com/mfikes/planck/blob/3076055c5a71da5fa0161285a7c1584a3e659a1c/planck-cljs/src/planck/repl.cljs#L735
shadow global JavaScript variables when eval is called here
https://github.com/mfikes/planck/blob/3076055c5a71da5fa0161285a7c1584a3e659a1c/planck-cljs/src/planck/repl.cljs#L742

Without optimization, you can see that js/source is then "source", and you can't, for example do (ns source.core), as it will fail in the same way.

We end up having a problem with a, b, and c instead owing to simple optimizations.

@shawwn
Copy link
Author

shawwn commented Nov 7, 2017

Just wanted to chime in that it's very interesting seeing you debug and explain the problem. I learned a lot about planck from this.

@mfikes mfikes closed this as completed in bcfad56 Nov 7, 2017
@tiye
Copy link

tiye commented Nov 7, 2017

Why is it fixed by a binding?

(defn- non-shadowing-js-eval
  []
  (js/eval *source*))

(binding [*source* source]
  (non-shadowing-js-eval)

@mfikes
Copy link
Member

mfikes commented Nov 7, 2017

@jiyinyiyong It is not the use of binding, but the eliminating of locals (function arguments and lets) in the function calling js/eval.

The root problem is that the locals shadow any JavaScript symbols passed to js/eval. Here is a concrete illustrative example (in Lumo, just for variety, to prove it doesn't require Planck and its JavaScript engine):

$ lumo -q
cljs.user=> (defn f [x]
       #_=>   (js/eval x))
#'cljs.user/f
cljs.user=> (f "1+1")
2
cljs.user=> (f "x")
"x"
cljs.user=> (f "y")
y is not defined
	 (eval)
	 cljs$user$f (evalmachine.<anonymous>:3:8)
	 (evalmachine.<anonymous>:1:13)
	 ContextifyScript.Script.runInThisContext (vm.cljs:44:33)
	 Object.runInThisContext (vm.cljs:116:38)
	 (Object.lt)
	 (Object.lumo.repl.caching_node_eval)
	 (NO_SOURCE_FILE <embedded>:5720:273)
	 z (NO_SOURCE_FILE <embedded>:5721:263)
	 Object.cljs.js.eval_str_STAR_ (NO_SOURCE_FILE <embedded>:5722:328)

This is a little tricky to understand, but when you make the call (f "x"), this can be thought of as being replaced with (js/eval "x"), but when the JavaScript engine evaluates the symbol x, the local x is in scope, and has the value "x". Instead we desire the behavior seen when evaluating (f "y").

So the solution I went with is the kludge of making the call to js/eval inside a function that has no locals whatsoever, and to pass the string to be evaluated via binding.

@tiye
Copy link

tiye commented Nov 7, 2017

Which means in the previous version:

(defn- js-eval
  [source source-url]
  #_(when (:verbose @app-env)
    (println-verbose (str "Evaluating JavaScript:\n" source)))
  (if source-url
    (let [exception (js/PLANCK_EVAL source source-url)]
      (when exception
        (throw exception)))
    (js/eval source)))

there are variables a b c after optimizing. While in:

(defn- non-shadowing-js-eval
  []
  (js/eval *source*))

no local variables so no a b c anymore. So tricky... I thought we have require('vm') if that's in Node.

@mfikes
Copy link
Member

mfikes commented Nov 8, 2017

@jiyinyiyong The execution environment for Planck is JavaScriptCore.

@mfikes
Copy link
Member

mfikes commented Nov 27, 2017

Note that with #554, this issue is being solved in a simpler way, relying on an ECMAScript 5 feature, calling eval indirectly to force things to be in global scope.

In terms of ClojureScript code compare:

cljs.user=> (let [geval js/eval] (js/eval "geval"))
#object[eval]

to

cljs.user=> (let [geval js/eval] (geval "geval"))
Can't find variable: geval

(The 2nd indirect variant doesn't see the geval local.)

@arichiardi
Copy link

So is lumo affected as well? I am always scanning Planck for possible issues 😄

I wonder if we should label each others issues as affects-lumo and affects-planck when we modify our respective repl.cljs 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants