Skip to content

Conversation

juj
Copy link
Collaborator

@juj juj commented Aug 30, 2025

This enables Closure spam free wasm2js test suite runs.

Even though the checkTypes annotation doesn't do anything in wasm2js.js file for WebAssembly, I'm still adding it there because otherwise in six months me or someone else would look at that Closure warning and have a first impression "oh, that's just missing a checkTypes suppress."

Muting these warnings globally in Closure for WASM2JS runs isn't that bad, since any developer who is targeting WASM2JS will also very likely target regular Wasm runs, and there they will see any checkTypes/uselessCode related diagnostics, giving them the needed coverage.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add a test confirming no warnings remain, somewhere?

@juj
Copy link
Collaborator Author

juj commented Sep 2, 2025

Perhaps add a test confirming no warnings remain, somewhere?

Closure prints its warnings in all caps WARNING - .... In my CI setup I use that to highlight the run results in orange, which is how I noticed this. So in that perspective we do have coverage.

(I presume we'd have to run all tests to ensure they really don't warn? so that would be a test to run all tests..)

@kripken
Copy link
Member

kripken commented Sep 2, 2025

Well, I just mean a single test, for basic coverage. E.g. like this existing test:

  @with_env_modify({'EMCC_LOGGING': '0'})  # this test assumes no emcc output
  def test_no_warnings(self):
    # build once before to make sure system libs etc. exist
    self.run_process([EMXX, test_file('hello_libcxx.cpp')])
    # check that there is nothing in stderr for a regular compile
    err = self.run_process([EMXX, test_file('hello_libcxx.cpp')], stderr=PIPE).stderr
    self.assertEqual(err, '')

But maybe it's not worth it, lgtm either way.

@juj
Copy link
Collaborator Author

juj commented Sep 2, 2025

But maybe it's not worth it, lgtm either way.

Hmm, I think probably better to work towards running the test suite with -Werror=closure altogether, so that all compilation with Closure would check against warnings.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 2, 2025

We already run with -Werror by default which applies to closure warnings. So as long as a test using the default flags we already cover this case, so I don't think we should need any special testing.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are a bunch of tests that have -Wno-closure to avoid these issues.

Those should be removed as part of this PR, right?

# s(0) | 0;
# ^^^^^^^^
# Turn off this check in Closure to allow clean Closure output.
cmd += ['--jscomp_off=uselessCode']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does closure compile run on the generated wasm2js output code? I guess it must otherwise this would not be needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it does, I think this is because the WASM2JS output is so huge that it needs Closure to help make it half-reasonable.

@juj
Copy link
Collaborator Author

juj commented Sep 3, 2025

I think there are a bunch of tests that have -Wno-closure to avoid these issues.

Those should be removed as part of this PR, right?

Indeed, looks like these aren't needed anymore. Removed.

@juj juj merged commit 57dda9a into emscripten-core:main Sep 3, 2025
22 of 30 checks passed
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

Successfully merging this pull request may close these issues.

3 participants