Skip to content

Fix webidl_binder so it can't handle missing assert function #21171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 25, 2024

This can happen when ASSERTIONS is not set and we are in STRICT mode or MINIMAL_RUNTIME mode.

See #20592

This can happen when ASSERTIONS is not set and we are in STRICT
mode or MINIMAL_RUNTIME mode.

See emscripten-core#20592
Comment on lines +141 to +143
if (typeof assert == "undefined") {
assert = (cond) => {}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (typeof assert == "undefined") {
assert = (cond) => {}
}
if (typeof assert === "undefined") {
var assert = () => {};
}

Copy link
Member

Choose a reason for hiding this comment

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

This may not work, if the generated code here is in a different scope than where assert is defined,

function assert() { .. }
function other() {
  if (typeof assert === "undefined") {
   // assert is always overridden here
    var assert = () => {};
  }
}

I think the WebIDL binder output here is usually added in a pre-js, so maybe that would be fine. However, it seems a little risky. Might be simplest to just define a new wassert and use that in the WebIDL binder code, perhaps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the above example would work just fine wouldn't it typeof assert === "undefined" will return false if there is an assert in the output/global scope.

webidl code certainly must be the same scope as the emscripten module code and --post-js is the canonical way to use it.

In the long term I hope to add --debug flag to webidl_binder.py but for now I think this solution should work fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because JS does function hoisting the check will always work regardless of whether this code comes after of before the assert declartion.

For example the following code always prints "good" regardless of whether it is run at the global scope or in a function:

  if (typeof assert == 'undefined') {
    console.error('oops');
  } else {
    console.error('good');
  }

  function assert() {
  }

Copy link
Member

@kripken kripken Jan 25, 2024

Choose a reason for hiding this comment

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

I think the above example would work just fine wouldn't it typeof assert === "undefined" will return false if there is an assert in the output/global scope.

No, there is a problem with scoping. Here is an extension of my example that is runnable:

function assert() {
  throw "true assert";
}

function other() {
  if (typeof assert === "undefined") {
    // assert is always overridden here
    var assert = () => {};
  }
  assert(1 == 0);
}

other();

Running that does not throw true assert, showing the assert was always overridden.

var is scoped to the function, so entering other means that the assert in the outer scope does not matter. Just the presence of the var means that assert is equal to undefined in that scope (as it is undefined until it actually reaches the var).

@sbc100 sbc100 closed this Jan 3, 2025
@sbc100 sbc100 deleted the wedidl_strict branch January 3, 2025 22:45
@sbc100 sbc100 restored the wedidl_strict branch January 4, 2025 00:27
@sbc100 sbc100 reopened this Jan 4, 2025
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