Skip to content
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

Support empty export names in wasm2js and JS mangling in general #2290

Merged
merged 5 commits into from
Aug 10, 2019
Merged

Conversation

kripken
Copy link
Member

@kripken kripken commented Aug 8, 2019

We were missing this silly validation rule, which explains the non-crash in #2288

Copy link
Contributor

@jgravelle-google jgravelle-google left a comment

Choose a reason for hiding this comment

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

Huh, I did not know that.

@kripken
Copy link
Member Author

kripken commented Aug 8, 2019

Actually this PR was totally wrong - empty exports are valid in wasm. They are just empty strings in JS, which is a valid identifier on the exports object... the spec and impls support that.

The proper fix here is then to just properly mangle such a name into something valid in JS, which I did as $.

@kripken kripken changed the title Validate that export names are not empty Support empty export names in wasm2js and JS mangling in general Aug 8, 2019
@tlively
Copy link
Member

tlively commented Aug 9, 2019

I feel like if a user is crazy enough to have an empty string import then they might also be crazy enough to have a dollar sign import. Should there be some sort of error or fallback if "$" is already present?

@kripken
Copy link
Member Author

kripken commented Aug 9, 2019

There is a runtime error for the user when they try to run the code ;) I didn't see a nice way to add an error here - asmangle has to do something for such a string, and to detect the possible problem we'd need to introduce another layer of checks on top of all the callers to it.

@kripken kripken merged commit 0cd48e6 into master Aug 10, 2019
@kripken kripken deleted the exp branch August 10, 2019 02:14
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