Google Closure Compiler Java update #5720

nazar-pc · 2017-10-27T08:15:29Z

As agreed in #5464 this is a part of #5464 without immediate transition to JavaScript version of Closure Compiler.
This should be a smaller diff to review.

Essentially this PR updates Google Closure Compiler to latest version and introduces necessary tweaks both to externs and to source code so that Closure Compiler doesn't complain too much in advanced mode.

After merging this PR #5464 will be rebased and reduced to just Java -> JavaScript version transition.

Probably obsoletes #5109
Enables #5185 (ECMASCRIPT5 needs to be changed to ECMASCRIPT6 for this, which is trivial)

UPD: Use this link to review changes ignoring whitespaces: https://github.com/kripken/emscripten/pull/5720/files?w=1

…solve some issues with newer version

…e JavaScript code inside

Fixes to externs (added File API externs).

… but don't cause everything to fail)

…ulted in corrupted JS build). More externs fixes.

Fix for `EmterpreterAsync` usage appearing in builds without `EmterpreterAsync` definition. Suppress `FUNCTION_TABLE` undefined variable. Fix for `getterReturnType` and `setterArgumentType` used while undefined (probably copy-paste typo, replaced with `rawFieldType`).

nazar-pc · 2017-10-27T14:45:25Z

Some changes are fixes for multiple actual issues reported by Closure Compiler that are difficult to discover otherwise, like copy-paste typo here: 9b32d3e#diff-8f35bc569987bab9a3691d986d90e75fR223

kripken

Overall this looks good, thanks! Mostly minor stuff and questions below.

kripken · 2017-10-27T17:14:23Z

emscripten.py

@@ -1495,7 +1495,7 @@ def create_asm_start_pre(asm_setup, the_global, sending, metadata, settings):
  module_library = module_get.format(access=access_quote('asmLibraryArg'), val=sending)

  asm_function_top = ('// EMSCRIPTEN_START_ASM\n'
-                      'var asm = (function(global, env, buffer) {')
+                      'var asm = (/** @suppress {uselessCode} */function(global, env, buffer) {')


please add a space after the */ (so it's not right on top of the function keyword)

kripken · 2017-10-27T17:15:50Z

src/arrayUtils.js

@@ -10,7 +10,6 @@ function intArrayFromString(stringy, dontAddNull, length) {
 }

 // Temporarily duplicating function pending Python preprocessor support
-var ASSERTIONS;


Is this because we start to unconditionally print var ASSERTIONS later down in this PR?

Yes. Under some circumstances ASSERTIONS was defined twice and Closure Compiler complained. So I've made sure it is always defined exactly once. Closure Compiler can remove it from source code later if not used at all.

kripken · 2017-10-27T17:18:43Z

src/embind/embind.js

@@ -2186,14 +2187,14 @@ var LibraryEmbind = {
        var humanName = classType.name + '.' + fieldName;
        var desc = {
            get: function() {
-                throwUnboundTypeError('Cannot access ' + humanName + ' due to unbound types', [getterReturnType, setterArgumentType]);
+                throwUnboundTypeError('Cannot access ' + humanName + ' due to unbound types', [rawFieldType]);


this looks like an actual change, not just fixing a closure warning? i'm not familiar enough with the embind code to know if it's valid or not, but was this intentional?

Closure Compiler complained about undefined variables and I've made a change that seems appropriate (I'm not familiar with this part of the code base either). Looks like a copy-paste from few pages of code above before this place.
The lines were introduced in 1f6b09d by @chadaustin, could you confirm this is a correct fix? Anyway, this is not a main code path, so doesn't seem that critical.

also cc @AlexPerrot

It might make sense to split this off into another embind-focused PR (but let's see what they say first, maybe not)

This change looks fine to me. I'm a little bit surprised that this made it in there in the first place (since we had linters at the time) but who knows. :)

kripken · 2017-10-27T17:22:30Z

src/shell.js

@@ -220,7 +221,7 @@ else if (ENVIRONMENT_IS_WEB || ENVIRONMENT_IS_WORKER) {
        return new Uint8Array(xhr.response);
 #if SUPPORT_BASE64_EMBEDDING
      } catch (err) {
-        var data = tryParseAsDataURI(f);
+        var data = tryParseAsDataURI(url);


this looks correct, but it suggests we don't have a test for this code path. @buu700, can you please look into that?

Oops, sorry about the typo there. As far as the code path, it looks like to test this we would need to be combining SINGLE_FILE and/or mem init mode 0 with a Worker, and the only SINGLE_FILE-related browser test we have now is to make sure it still works without a native atob implementation.

Ahh, I just spent way too long trying to figure out what code path was hitting this. It looks like there isn't actually a code path that hits this line; the test @nazar-pc fixed must've been from Closure Compiler doing some static analysis on the output.

If that's the case, the test that failed for him is probably already the best test we're gonna get without refactoring and writing a bunch of unit tests (and/or incrementally adding more static analysis from Closure/TypeScript/Flow).

Yes, Closure Compiler was complaining about the variable being undefined

Got it, thanks.

kripken · 2017-10-27T17:24:12Z

tools/js-optimizer.js

@@ -5730,12 +5730,13 @@ function safeHeap(ast) {
  traverseGeneratedFunctions(ast, function(func) {
    if (func[1] in SAFE_HEAP_FUNCS) return null;
    traverseGenerated(func, function(node, type) {
+      var heap, prt;


prt => ptr

does closure even run on this file? that would surprise me but maybe I forgot something.

Sorry, fixed.
I don't think closure runs here, but variables were defined twice in the same scope, so I though it is an appropriate change as I was fixing similar things in other places.

kripken · 2017-10-30T18:33:29Z

Thanks, looks good aside from the embind stuff that we are waiting to hear about.

kripken · 2017-11-07T19:25:52Z

Great, thanks everyone.

dcodeIO · 2017-11-08T13:49:22Z

FYI: Seems this breaks compilation of binaryen.js: https://travis-ci.org/AssemblyScript/binaryen.js/builds/299063033#L4549

/tmp/tmpj1P7Io.cl.js:7504: ERROR - variable define is undeclared
  if (typeof define === "function" && define["amd"])
             ^^^^^^
1 error(s)

nazar-pc · 2017-11-08T14:33:06Z

@dcodeIO, well, this will require a bit more externs, I'll debug it today and provide PR either for Emscripten or Binaryen. Modern Closure Compiler is much stricter to such kinds of stuff.

juj · 2017-11-10T17:20:10Z

This broke Closure compiler from running in Emsdk on Windows, with error

  File "C:\code\emsdk\emscripten\incoming\emcc", line 13, in <module>
    emcc.run()
  File "C:\code\emsdk\emscripten\incoming\emcc.py", line 1780, in run
    optimizer.flush()
  File "C:\code\emsdk\emscripten\incoming\emcc.py", line 220, in flush
    self.run_passes(chunks[0], title, just_split=False, just_concat=False)
  File "C:\code\emsdk\emscripten\incoming\emcc.py", line 253, in run_passes
    just_concat=just_concat)
  File "C:\code\emsdk\emscripten\incoming\tools\shared.py", line 2074, in js_optimizer
    ret = js_optimizer.run(filename, passes, NODE_JS, debug, extra_info, just_split, just_concat)
  File "C:\code\emsdk\emscripten\incoming\tools\js_optimizer.py", line 570, in run
    return temp_files.run_and_clean(lambda: run_on_js(filename, passes, js_engine, source_map, extra_info, just_split, just_concat))
  File "C:\code\emsdk\emscripten\incoming\tools\tempfiles.py", line 79, in run_and_clean
    return func()
  File "C:\code\emsdk\emscripten\incoming\tools\js_optimizer.py", line 570, in <lambda>
    return temp_files.run_and_clean(lambda: run_on_js(filename, passes, js_engine, source_map, extra_info, just_split, just_concat))
  File "C:\code\emsdk\emscripten\incoming\tools\js_optimizer.py", line 505, in run_on_js
    cld = shared.Building.closure_compiler(cld, pretty='minifyWhitespace' not in passes)
  File "C:\code\emsdk\emscripten\incoming\tools\shared.py", line 2194, in closure_compiler
    raise Exception('closure compiler error: ' + cc_output + ' (rc: %d)' % process.returncode)
Exception: closure compiler error: java.lang.UnsupportedClassVersionError: com/google/javascript/jscomp/CommandLineRunner : Unsupported major.minor version 52.0
        at java.lang.ClassLoader.defineClass1(Native Method)
        at java.lang.ClassLoader.defineClass(Unknown Source)
        at java.security.SecureClassLoader.defineClass(Unknown Source)
        at java.net.URLClassLoader.defineClass(Unknown Source)
        at java.net.URLClassLoader.access$100(Unknown Source)
        at java.net.URLClassLoader$1.run(Unknown Source)
        at java.net.URLClassLoader$1.run(Unknown Source)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.net.URLClassLoader.findClass(Unknown Source)
        at java.lang.ClassLoader.loadClass(Unknown Source)
        at sun.misc.Launcher$AppClassLoader.loadClass(Unknown Source)
        at java.lang.ClassLoader.loadClass(Unknown Source)
        at sun.launcher.LauncherHelper.checkAndLoadMain(Unknown Source)
Exception in thread "main"  (rc: 1)

nazar-pc added 14 commits October 27, 2017 11:02

Google Closure Compiler updated to latest v20171023.0.0

4e1aead

Some new externs added to please Closure Compiler, small tweaks to re…

e8adafb

…solve some issues with newer version

Rewrite multiple f.write() calls into single call with more readabl…

247a17c

…e JavaScript code inside

Fix for multiple duplicated var ptr declarations.

5b13367

Fixes to externs (added File API externs).

More externs tweaks

8b40672

Suppress a lot of warnings that naturally occur in asm.js code

6e0cb98

Closure Compiler should run without errors now (warnings are present,…

d6fe775

… but don't cause everything to fail)

Fix JS optimizer to properly work with added suppression comment (res…

595dd54

…ulted in corrupted JS build). More externs fixes.

WebIDL fixes for modern Closure Compiler

3572ac8

Fixing last failing asm2 tests

bb8907a

Removed externs already included in latest Closure Compiler

b2e9fb8

Fixed typo that caused test to fail under Closure Compiler

c27814b

Suppress errors about undefined variables

a5ef87f

kripken reviewed Oct 27, 2017

View reviewed changes

Small tweaks as suggested in review

0a1cb86

buu700 mentioned this pull request Nov 2, 2017

SINGLE_FILE html and worker fixes + tests #5736

Merged

kripken merged commit 93479ec into emscripten-core:incoming Nov 7, 2017

nazar-pc deleted the google-closure-compiler-java-update branch November 7, 2017 19:28

nazar-pc mentioned this pull request Nov 8, 2017

Added AMD externs so that Binaryen builds fine #5748

Merged

nazar-pc mentioned this pull request Nov 10, 2017

Update to Java 8 emscripten-core/emsdk#112

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Google Closure Compiler Java update #5720

Google Closure Compiler Java update #5720

nazar-pc commented Oct 27, 2017 •

edited

Loading

nazar-pc commented Oct 27, 2017

kripken left a comment

kripken Oct 27, 2017

nazar-pc Oct 27, 2017

kripken Oct 27, 2017

nazar-pc Oct 27, 2017

kripken Oct 27, 2017

nazar-pc Oct 27, 2017

kripken Oct 30, 2017

chadaustin Nov 7, 2017

kripken Oct 27, 2017

buu700 Oct 27, 2017

buu700 Nov 5, 2017

nazar-pc Nov 5, 2017

buu700 Nov 5, 2017

kripken Oct 27, 2017

nazar-pc Oct 27, 2017

kripken commented Oct 30, 2017

kripken commented Nov 7, 2017

dcodeIO commented Nov 8, 2017

nazar-pc commented Nov 8, 2017

juj commented Nov 10, 2017

Google Closure Compiler Java update #5720

Google Closure Compiler Java update #5720

Conversation

nazar-pc commented Oct 27, 2017 • edited Loading

nazar-pc commented Oct 27, 2017

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Oct 30, 2017

kripken commented Nov 7, 2017

dcodeIO commented Nov 8, 2017

nazar-pc commented Nov 8, 2017

juj commented Nov 10, 2017

nazar-pc commented Oct 27, 2017 •

edited

Loading