Ideas how to deal with new Closure warnings?

[Floh]

I'm getting a lot of Closure warnings in my EM_JS() functions since updating to 3.1.24.

For instance, when passing an XMLHttpRequest array buffer response to new Uint8Array, Closure can't figure out that this is indeed an ArrayBuffer.

var u8_array = new Uint8Array(req.response);

This results in a warning 

[JSC_TYPE_MISMATCH] actual parameter 1 of Uint8Array does not match formal parameter
found   : (Object|null|string)
required: (Array<number>|ArrayBuffer|ArrayBufferView|SharedArrayBuffer|null|number)
  768|     var u8_array = new Uint8Array(req.response);
                                             ^^^^^^^^

I googled around and found out that Closure takes type hints like this:

var u8_array = new Uint8Array(/** @type {!ArrayBuffer} */(req.response));

Any ideas how to best deal with this?

Cheers,

-Floh.

[Floh]

PS: I forgot an

"But this doesn't work"

after 

I googled around and found out that Closure takes type hints like this:

var u8_array = new Uint8Array(/** @type {!ArrayBuffer} */(req.response));

:)

[Sam Clegg]

Without addressing that particular closure warning, I should explain why you started seeing these warnings in 3.1.24.

I landed a change that make closure warnings controlled via the normal `-W` flags: https://github.com/emscripten-core/emscripten/pull/17878.  Those warnings were always there but emscripten was hiding them from you.   We still don't enable closure warnings by default (by default we still hide them), but users who pass `-Wall` will now see them by default.

I could land another change to make `-Wall` not enable `-Wclosure`, depending on how many folks are impacted by this change.  In the short term you can pass `-Wno-closure` to revert the old behaviour.  In the long term we need to decide if `-Wclosure` being part of `-Wall` makes sense?  Are these kinds of warnings useful to you, or would you rather not see them at all?

cheers,

sam

--
You received this message because you are subscribed to the Google Groups "emscripten-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to emscripten-disc...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/emscripten-discuss/b3eece1f-d8aa-433d-9a7c-c7d16a761062n%40googlegroups.com.

[Floh]

Enabling -Wclosure as part of -Wall, and using -Wno-closure to suppress those warnings totally makes sense to me, I don't know how bad it is in much bigger projects though (in terms of warning spam).

Some of the closure warnings were definitely useful, they weren't bugs, but definitely code smells which I have fixed now. The rest look like false positives to me. Ideally I would like to suppress individual false positives right in the source, but I guess the 'official' way of placing Closure hints into comments doesn't work because the C preprocessor already removes those. But this isn't important enough to justify an awkward workaround in Emscripten...

I think I will keep the closure warnings actived when compiling in debug mode, but for release mode, use -Wno-closure to suppress them.

Cheers!

-Floh.

[Sam Clegg]

On Tue, Oct 18, 2022 at 2:00 AM Floh <flo...@gmail.com> wrote:

Enabling -Wclosure as part of -Wall, and using -Wno-closure to suppress those warnings totally makes sense to me, I don't know how bad it is in much bigger projects though (in terms of warning spam).

Some of the closure warnings were definitely useful, they weren't bugs, but definitely code smells which I have fixed now. The rest look like false positives to me. Ideally I would like to suppress individual false positives right in the source, but I guess the 'official' way of placing Closure hints into comments doesn't work because the C preprocessor already removes those. But this isn't important enough to justify an awkward workaround in Emscripten...

You can add docs/hints, it's just a little more ugly in JS library code. e.g.: https://github.com/emscripten-core/emscripten/blob/7b9747ee3293029cd7884f6629f885337cfeec9f/src/library.js#L61

To view this discussion on the web visit https://groups.google.com/d/msgid/emscripten-discuss/5776e40c-e38d-480a-a204-eeb724738de3n%40googlegroups.com.

[Floh]

Hmm... but this would not work inside EM_JS() right? Because the C preprocessor already removes the comments before Closure even sees the code?

I've tried all sorts of things to 'escape' the comments (e.g. \/*/*/* bla bla bla \*\/) but that just generated lots of 'unknown escape sequence' warnings in the C compiler, and then still didn't arrive at the Closure compiler :)

[Floh]

Hrmpf, I feel that I'm very close to the solution by formatting the Closure hints like this:

\x2F\x2A\x2A @suppress {missingProperties} \x2A\x2F

...which sneaks the /** and */ past the C preprocessor ...this then results in a warning like this:

WARNING - [JSC_MISPLACED_SUPPRESS] @suppress annotation not allowed here. See https://github.com/google/closure-compiler/wiki/@suppress-annotations
  638|  /** @suppress {missingProperties} */ if ((index < 0) || (index >= Module.sokol_dropped_files.length)) {
                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  639|   return 0;
       ^^^^^^^^^^^
...
  641|   return Module.sokol_dropped_files[index].size;
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  642|  }

...but at least the /** ... */ makes it through to Closure. I will continue tinkering a bit with this, but not a big deal if I can't find a solution.

[Floh]

Ok I got it working with the above idea to sneak the Closure hint past the C preprocessor, I just had to restructure the code a bit and move the property access out of the if() statement, the whole EM_JS block looks like this now and doesn't produce a Closure warning:

EM_JS(uint32_t, sapp_js_dropped_file_size, (int index), {

    \x2F\x2A\x2A @suppress {missingProperties} \x2A\x2F

    const files = Module.sokol_dropped_files;
    if ((index < 0) || (index >= files.length)) {
        return 0;
    }
    else {
        return files[index].size;
    }
});

...might look a bit weird for somebody reading the code, but works for me :)