From b8834c2669b72be6d7d206235c5898f0788f2bb2 Mon Sep 17 00:00:00 2001 From: "Alon Zakai (kripken)" Date: Sat, 9 May 2020 07:16:19 -0700 Subject: [PATCH 1/3] Add useful errors on bad use of the Promise instead of the instance in new MODULARIZE --- src/parseTools.js | 20 ++++++++++++++++++++ src/shell.js | 3 +++ src/shell_minimal.js | 3 +++ tests/test_other.py | 23 +++++++++++++++++++++++ 4 files changed, 49 insertions(+) diff --git a/src/parseTools.js b/src/parseTools.js index b493280b2e5ef..584f31c8abd89 100644 --- a/src/parseTools.js +++ b/src/parseTools.js @@ -1708,3 +1708,23 @@ function sendI64Argument(low, high) { return low + ', ' + high; } } + +// Add assertions to catch common errors when using the Promise object we +// create on Module.ready() and return from MODULARIZE Module() invocations. +function addReadyPromiseAssertions(promise) { + // Warn on someone doing + // + // var instance = Module(); + // ... + // instance._main(); + var properties = keys(EXPORTED_FUNCTIONS); + // Also warn on onRuntimeInitialized which might be a common pattern with + // older MODULARIZE-using codebases. + properties.push('onRuntimeInitialized'); + return properties.map(function(property) { + return "if (!Object.getOwnPropertyDescriptor(" + promise + ", '" + property + "')) {" + + " Object.defineProperty(" + promise + ", '" + property + "', { configurable: true, get: function() { abort('You are getting " + property + " on the Promise object, instead of the instance. Use .then() to get called back with the instance, see the MODULARIZE docs in src/settings.js') } });" + + " Object.defineProperty(" + promise + ", '" + property + "', { configurable: true, set: function() { abort('You are setting " + property + " on the Promise object, instead of the instance. Use .then() to get called back with the instance, see the MODULARIZE docs in src/settings.js') } });" + + "}"; + }).join('\n'); +} diff --git a/src/shell.js b/src/shell.js index d820d97fe4d3a..8a3f1994d8979 100644 --- a/src/shell.js +++ b/src/shell.js @@ -45,6 +45,9 @@ Module['ready'] = new Promise(function(resolve, reject) { readyPromiseResolve = resolve; readyPromiseReject = reject; }); +#if ASSERTIONS +{{{ addReadyPromiseAssertions("Module['ready']") }}} +#endif #endif // --pre-jses are emitted after the Module integration code, so that they can diff --git a/src/shell_minimal.js b/src/shell_minimal.js index f872b1fe6ec07..02664f3b5fdef 100644 --- a/src/shell_minimal.js +++ b/src/shell_minimal.js @@ -22,6 +22,9 @@ Module['ready'] = new Promise(function(resolve, reject) { readyPromiseResolve = resolve; readyPromiseReject = reject; }); +#if ASSERTIONS +{{{ addReadyPromiseAssertions("Module['ready']") }}} +#endif #endif #if ENVIRONMENT_MAY_BE_NODE diff --git a/tests/test_other.py b/tests/test_other.py index f2e3caeae4191..7bc2f18c7171c 100644 --- a/tests/test_other.py +++ b/tests/test_other.py @@ -10355,6 +10355,29 @@ def test_assertions_on_outgoing_module_api_changes(self): Module.arguments has been replaced with plain arguments_ ''', run_js('a.out.js', assert_returncode=None, stderr=PIPE)) + def test_assertions_on_ready_promise(self): + # check that when assertions are on we give useful error messages for + # mistakenly thinking the Promise is an instance. I.e., once you could do + # Module()._main to get an instance and the main function, but after + # the breaking change in #10697 Module() now returns a promise, and to get + # the instance you must use .then() to get a callback with the instance. + create_test_file('test.js', r''' + try { + Module()._main; + } catch(e) { + console.log(e); + } + try { + Module().onRuntimeInitialized = 42; + } catch(e) { + console.log(e); + } + ''') + run_process([PYTHON, EMCC, path_from_root('tests', 'hello_world.c'), '-s', 'MODULARIZE', '-s', 'ASSERTIONS', '--extern-post-js', 'test.js']) + out = run_js('a.out.js') + self.assertContained('You are getting _main on the Promise object, instead of the instance. Use .then() to get called back with the instance, see the MODULARIZE docs in src/settings.js', out) + self.assertContained('You are setting onRuntimeInitialized on the Promise object, instead of the instance. Use .then() to get called back with the instance, see the MODULARIZE docs in src/settings.js', out) + def test_em_asm_duplicate_strings(self): # We had a regression where tow different EM_ASM strings from two diffferent # object files were de-duplicated in wasm-emscripten-finalize. This used to From 41218e84ab4ac98b2e430e5299bb7a4642d9e862 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 11 May 2020 09:11:11 -0700 Subject: [PATCH 2/3] modernize with feedback --- src/parseTools.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/parseTools.js b/src/parseTools.js index 584f31c8abd89..26455d426c56d 100644 --- a/src/parseTools.js +++ b/src/parseTools.js @@ -1722,9 +1722,12 @@ function addReadyPromiseAssertions(promise) { // older MODULARIZE-using codebases. properties.push('onRuntimeInitialized'); return properties.map(function(property) { - return "if (!Object.getOwnPropertyDescriptor(" + promise + ", '" + property + "')) {" + - " Object.defineProperty(" + promise + ", '" + property + "', { configurable: true, get: function() { abort('You are getting " + property + " on the Promise object, instead of the instance. Use .then() to get called back with the instance, see the MODULARIZE docs in src/settings.js') } });" + - " Object.defineProperty(" + promise + ", '" + property + "', { configurable: true, set: function() { abort('You are setting " + property + " on the Promise object, instead of the instance. Use .then() to get called back with the instance, see the MODULARIZE docs in src/settings.js') } });" + - "}"; + const warningEnding = `${property} on the Promise object, instead of the instance. Use .then() to get called back with the instance, see the MODULARIZE docs in src/settings.js`; + return ` + if (!Object.getOwnPropertyDescriptor(${promise}, '${property}')) { + Object.defineProperty(${promise}, '${property}', { configurable: true, get: function() { abort('You are getting ${warningEnding}') } }); + Object.defineProperty(${promise}, '${property}', { configurable: true, set: function() { abort('You are setting ${warningEnding}') } }); + } + `; }).join('\n'); } From 4dcc7147abad2f8feed2060ee2dfae26537d054a Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 12 May 2020 09:00:00 -0700 Subject: [PATCH 3/3] changelog --- ChangeLog.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ChangeLog.md b/ChangeLog.md index aedfee2d08372..806581db20faa 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -22,8 +22,10 @@ Current Trunk will need to wait on the returned Promise, using `await` or its `then` callback, to get the module instance (#10697). This fixes some long-standing bugs with that option which have been reported multiple times, but is a - breaking change - sorry about that. See detailed examples for the - current usage in `src/settings.js` on `MODULARIZE`. + breaking change - sorry about that. To reduce the risk of confusing breakage, + in a build with `ASSERTIONS` we will show a clear warning on common errors. + For more, see detailed examples for the current usage in `src/settings.js` on + `MODULARIZE`. - A new `PRINTF_LONG_DOUBLE` option allows printf to print long doubles at full float128 precision. (#11130) - `emscripten_async_queue_on_thread` has been renamed to