Update embind to work with the Closure Compiler

[rharkeadsk]

We discovered that certain parts of embind do not work when the code is run through the Closure Compiler (--closure 1). Several names within the embind JavaScript itself need to be guarded from name
mangling in the Closure Compiler.

The existing tests would be sufficient to detect this case, except that they were never being run with the Closure Compiler enabled. This pull request also updates other.test_embind to run a pass with the Closure Compiler enabled.

Because the tests were never designed to themselves be Closure'd, I updated the test to manually append the tests and the test adapter to the output JavaScript, instead of using --post-js. We considered updating the tests themselves to be Closure'able, but that turned into a minor nightmare- I think this way is better, but if you feel otherwise please let me know. We would be happy to finish that work if it is the preferred road forward.

Small updates were also needed to the test adapter to make it work- it was relying on the assert function from Emscripten, and now needs to create its own object rather than appending to it.

[kripken]

cc @chadaustin This is surprising, since imvu uses closure and embind?

Perhaps not by appending code, though. The normal usage in emscripten is generally to use --post-js so that all the code can be optimized together, for this exact reason. Is there a reason for you to prefer to append?

[rharkeadsk]

I don't necessarily prefer appending versus --post-js - in practice, we use a combination of the two on our projects.

In this particular case though, it is using --post-js to optimize all of the code together that was breaking it. When you expose classes/functions/etc. with embind from C++, the names are all expressed as string literals, so Closure will never touch them. But if you reference them with the Module.emboundSymbolName syntax, then Closure will mangle that form, resulting in a mismatch.

The solutions I know of are to either use Module["emboundSymbolName"], or to altogether prevent the referencing code from being touched by Closure (by avoiding --post-js). In most of the internal stuff I work on, I use the former solution, but in this case I went with the latter solution because there would be literally thousands of changes needed to embind.test.js to stringify the names.

As to why @chadaustin hasn't run into problems if imvu does indeed use Closure, my wild guess would be that it could be because they weren't exercising the part that was broken, which specifically is the code for extending a C++ class from JavaScript.

[kripken]

Ok, if the problem happens with --post-js as well, then I would prefer to keep the test using that approach, as it's what we normally recommend, or to test both ways. Also I'm curious as to how @chadaustin 's team use this.

Looking at the changes, it makes sense that extending a C++ class from JavaScript would need extra closure annotations. Changes look good to me (except for the testing issue), but leaving to @chadaustin .

[rharkeadsk]

OK, if --post-js is preferred, then I can update the PR to use that method instead. I will need to finish updating the tests in embind.test.js to stringify all of the embound symbols, so it'll take a little bit.

[rharkeadsk]

I've updated the pull request so that everything is going through --post-js again. You'll note that there is now a sea of changes in embind.test.js, and I also needed to add an "externs" file for underscore.js (which the embind test adapter uses) because it also exposes some symbols via string literal.

Let me know what you think.

[rharkeadsk]

By the way, I've left the original PR branch at https://github.com/rharkeadsk/emscripten/tree/dev/embind-closure-old for reference.

[kripken]

why was this change needed?

[chadaustin]

Heh, I had the same question. :)

[kripken]

Looks good, one question above (and we need @chadaustin 's feedback here).

[rharkeadsk]

$$ is an internal property used by embind, and is referred to throughout embind.js as $$ without quoting. It is safe for Closure to name-mangle it, because it's not meant to be exposed to anyone. However, because it was initially defined with Object.defineProperty, and defineProperty takes the property name as a string, it was not getting name-mangled along with the rest of the references. By switching to Object.defineProperties, I can specify the property name as an object key instead, and it will get consistent treatment from Closure.

[kripken]

Got it, thanks.

[chadaustin]

I am also surprised we haven't run into this before! We've used Closure Compiler since day one. cc @waywardmonkeys Any ideas?

That said, the changes look fine to me. It would be great if @waywardmonkeys could give them a once over too.

[waywardmonkeys]

That is a bit of a mystery. I will look into that some.

[rharkeadsk]

Any update on this?

[chadaustin]

Sorry for the delay. I think I meant to merge this but didn't. Thanks again!

[kripken]

Wait, are we decided on all the issues here? In particular, look at the size of the diff on embind.test.js - is the new version there (with more quotes) how you expect embind to be used?

[chadaustin]

Good point. I didn't see the embind.test.js changes because the github PR diff UI hid them for some reason. Yes, those are not necessary - you don't need to run closure on your tests. I can revert that single file.

[chadaustin]

I reverted the changes to embind.test.js in 36cd7a4

[kripken]

LLVM 3.5 has been merged to incoming. This has a known issue with embind which might mask breakage caused by that revert (see emscripten-core/emscripten-fastcomp#51 ). To test the revert, can use master branches + the revert commit.

[kripken]

Fix for that known issue has been pushed. The breakage from that revert is unmasked, i.e., can just run tests/runner.py other.test_embind to see it, on incoming.

[chadaustin]

My changes in 36cd7a4 broke tests, so I had to rework them in 36cd7a4. Now the tests are appended after compilation so that closure compiler does not run on them.