Assert if MODULARIZE factory function is used with new.

Once we start using `async` for for this function then `new` stops
working: emscripten-core#23157.

Using `new` in this was was always an antipattern but there was nothing
stopping users from doing this.

Author: sbc100

ChangeLog.md

@@ -20,6 +20,11 @@ See docs/process.md for more on how version tagging works.
 
 3.1.75 (in development)
 -----------------------
+- When using `-sMODULARIZE` we now assert if the factory function is called with
+  the JS `new` keyword.  e.g. `var a = new Module()` rather than `var b =
+  Module()`.  This paves the way for marking the function as `async` which does
+  not allow `new` to be used.  This usage of `new` here was never documented and
+  is considered and antipattern. (#)
 - `PATH.basename()` no longer calls `PATH.normalize()`, so that
   `PATH.basename("a/.")` returns `"."` instead of `"a"` and
   `PATH.basename("a/b/..")` returns `".."` instead of `"a"`. This is in line with

test/code_size/hello_webgl2_wasm.json

@@ -1,10 +1,10 @@
 {
   "a.html": 454,
   "a.html.gz": 328,
-  "a.js": 4532,
-  "a.js.gz": 2315,
+  "a.js": 4527,
+  "a.js.gz": 2311,
   "a.wasm": 10402,
   "a.wasm.gz": 6703,
-  "total": 15388,
-  "total_gz": 9346
+  "total": 15383,
+  "total_gz": 9342
 }

test/code_size/hello_webgl2_wasm2js.json

@@ -1,8 +1,8 @@
 {
   "a.html": 346,
   "a.html.gz": 262,
-  "a.js": 22200,
-  "a.js.gz": 11583,
-  "total": 22546,
-  "total_gz": 11845
+  "a.js": 22195,
+  "a.js.gz": 11581,
+  "total": 22541,
+  "total_gz": 11843
 }

test/code_size/hello_webgl_wasm.json

@@ -1,10 +1,10 @@
 {
   "a.html": 454,
   "a.html.gz": 328,
-  "a.js": 4070,
-  "a.js.gz": 2158,
+  "a.js": 4065,
+  "a.js.gz": 2155,
   "a.wasm": 10402,
   "a.wasm.gz": 6703,
-  "total": 14926,
-  "total_gz": 9189
+  "total": 14921,
+  "total_gz": 9186
 }

test/code_size/hello_webgl_wasm2js.json

@@ -1,8 +1,8 @@
 {
   "a.html": 346,
   "a.html.gz": 262,
-  "a.js": 21726,
-  "a.js.gz": 11413,
-  "total": 22072,
-  "total_gz": 11675
+  "a.js": 21721,
+  "a.js.gz": 11409,
+  "total": 22067,
+  "total_gz": 11671
 }

test/other/codesize/test_codesize_minimal_esm.gzsize

@@ -1 +1 @@
-1487
+1533

test/other/codesize/test_codesize_minimal_esm.jssize

@@ -1 +1 @@
-3095
+3239

test/test_browser.py

@@ -4795,28 +4795,19 @@ def test_browser_run_from_different_directory(self):
 
   # Similar to `test_browser_run_from_different_directory`, but asynchronous because of `-sMODULARIZE`
   def test_browser_run_from_different_directory_async(self):
-    for args, creations in [
-      (['-sMODULARIZE'], [
-        'Module();',    # documented way for using modularize
-        'new Module();' # not documented as working, but we support it
-       ]),
-    ]:
-      print(args)
-      # compile the code with the modularize feature and the preload-file option enabled
-      self.compile_btest('browser_test_hello_world.c', ['-o', 'test.js', '-O3'] + args)
-      ensure_dir('subdir')
-      shutil.move('test.js', Path('subdir/test.js'))
-      shutil.move('test.wasm', Path('subdir/test.wasm'))
-      for creation in creations:
-        print(creation)
-        # Make sure JS is loaded from subdirectory
-        create_file('test-subdir.html', '''
-          <script src="subdir/test.js"></script>
-          <script>
-            %s
-          </script>
-        ''' % creation)
-        self.run_browser('test-subdir.html', '/report_result?0')
+    # compile the code with the modularize feature and the preload-file option enabled
+    self.compile_btest('browser_test_hello_world.c', ['-o', 'test.js', '-O3', '-sMODULARIZE'])
+    ensure_dir('subdir')
+    shutil.move('test.js', Path('subdir/test.js'))
+    shutil.move('test.wasm', Path('subdir/test.wasm'))
+    # Make sure JS is loaded from subdirectory
+    create_file('test-subdir.html', '''
+      <script src="subdir/test.js"></script>
+      <script>
+        Module();
+      </script>
+    ''')
+    self.run_browser('test-subdir.html', '/report_result?0')
 
   # Similar to `test_browser_run_from_different_directory`, but
   # also also we eval the initial code, so currentScript is not present. That prevents us

test/test_other.py

@@ -6774,6 +6774,12 @@ def test_modularize_strict(self):
     output = self.run_js('run.js')
     self.assertEqual(output, 'hello, world!\n')
 
+  def test_modularize_new_misuse(self):
+    self.run_process([EMCC, test_file('hello_world.c'), '-sMODULARIZE', '-sEXPORT_NAME=Foo'])
+    create_file('run.js', 'var m = require("./a.out.js"); new m();')
+    err = self.run_js('run.js', assert_returncode=NON_ZERO)
+    self.assertContained('Foo() must not be called with new\n', err)
+
   @parameterized({
     '': ([],),
     'export_name': (['-sEXPORT_NAME=Foo'],),

tools/link.py

@@ -2385,6 +2385,16 @@ def modularize():
   if settings.EXPORT_ES6 and settings.ENVIRONMENT_MAY_BE_NODE:
     async_emit = 'async '
 
+  # TODO(sbc): Do we really need _scriptName in all cases.  Look into reducing this
+  # condition.
+  add_script_name_wrapper = not settings.MINIMAL_RUNTIME or settings.SHARED_MEMORY
+  add_assertion_wrapper = settings.ASSERTIONS and not settings.MODULARIZE == 'instance'
+
+  if add_script_name_wrapper or add_assertion_wrapper:
+    wrapper_name = settings.EXPORT_NAME + '_inner'
+  else:
+    wrapper_name = settings.EXPORT_NAME
+
   if settings.MODULARIZE == 'instance':
     wrapper_function = '''
 export default async function init(moduleArg = {}) {
@@ -2398,8 +2408,8 @@ def modularize():
       'generated_js': generated_js
     }
   else:
-    wrapper_function = '''
-%(maybe_async)sfunction(moduleArg = {}) {
+    wrapper_function = '''\
+%(maybe_async)sfunction %(wrapper_name)s(moduleArg = {}) {
   var moduleRtn;
 
 %(generated_js)s
@@ -2408,26 +2418,18 @@ def modularize():
 }
 ''' % {
       'maybe_async': async_emit,
-      'generated_js': generated_js
+      'generated_js': generated_js,
+      'wrapper_name': wrapper_name
     }
 
-  if settings.MINIMAL_RUNTIME and not settings.PTHREADS:
-    # Single threaded MINIMAL_RUNTIME programs do not need access to
-    # document.currentScript, so a simple export declaration is enough.
-    src = f'/** @nocollapse */ var {settings.EXPORT_NAME} = {wrapper_function};'
-  else:
+  if add_script_name_wrapper:
     script_url_node = ''
-    # When MODULARIZE this JS may be executed later,
-    # after document.currentScript is gone, so we save it.
-    # In EXPORT_ES6 + PTHREADS the 'thread' is actually an ES6 module
-    # webworker running in strict mode, so doesn't have access to 'document'.
-    # In this case use 'import.meta' instead.
-    if settings.EXPORT_ES6 and settings.USE_ES6_IMPORT_META:
-      script_url = 'import.meta.url'
-    else:
-      script_url = "typeof document != 'undefined' ? document.currentScript?.src : undefined"
-      if settings.ENVIRONMENT_MAY_BE_NODE:
-        script_url_node = "if (typeof __filename != 'undefined') _scriptName = _scriptName || __filename;"
+    # When MODULARIZE this JS may be executed later, after
+    # document.currentScript is gone, so we save it.  This is only
+    # needed if the program actually requires `_scriptName`
+    script_url = "typeof document != 'undefined' ? document.currentScript?.src : undefined"
+    if settings.ENVIRONMENT_MAY_BE_NODE:
+      script_url_node = "if (typeof __filename != 'undefined') _scriptName = _scriptName || __filename;"
     if settings.MODULARIZE == 'instance':
       src = '''\
   var _scriptName = %(script_url)s;
@@ -2443,14 +2445,33 @@ def modularize():
 var %(EXPORT_NAME)s = (() => {
   var _scriptName = %(script_url)s;
   %(script_url_node)s
-  return (%(wrapper_function)s);
+  return %(wrapper_function)s
 })();
 ''' % {
         'EXPORT_NAME': settings.EXPORT_NAME,
         'script_url': script_url,
         'script_url_node': script_url_node,
         'wrapper_function': wrapper_function,
       }
+  elif add_assertion_wrapper:
+    src = '''\
+var %(EXPORT_NAME)s = (() => {
+  %(wrapper_function)s
+  // Return a small, never-async wrapper around %(wrapper_name)s which
+  // checks for callers incorrectly using it with `new`.
+  return function(arg) {
+    assert(new.target, "%(EXPORT_NAME)s() should not be called with `new %(EXPORT_NAME)s()`");
+    return %(wrapper_name)s(arg);
+  }
+})();
+''' % {
+       'EXPORT_NAME': settings.EXPORT_NAME,
+       'wrapper_function': wrapper_function,
+       'wrapper_name': wrapper_name,
+      }
+  else:
+    # No wrapper required.  A simple export declaration is enough.
+    src = f'/** @nocollapse */ {wrapper_function};'
 
   # Given the async nature of how the Module function and Module object
   # come into existence in AudioWorkletGlobalScope, store the Module