Skip to content

Make anyfunc an alias of funcref #34

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

Merged
merged 8 commits into from
Nov 18, 2021
Merged

Conversation

gahaas
Copy link
Contributor

@gahaas gahaas commented Oct 29, 2021

As discussed in WebAssembly/reference-types#85 (review), this PR turns anyfunc in the js-api into an alias of funcref. Both anyfunc and funcref are valid strings in the js-api. I renamed most uses of anyfunc into funcref but kept a test for anyfunc.

@gahaas
Copy link
Contributor Author

gahaas commented Nov 8, 2021

ping

"anyfunc",
"funcref",
Copy link
Contributor

Choose a reason for hiding this comment

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

You should keep both, and test that new Global({ type: "funcref" }) works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would it work if we keep both? Wouldn't that mean that they are different types? How should I define then what the result of type() is?
My idea was that there is only one type, funcref, but the string anyfunc is accepted as an alias in the constructor. Internally, in the non-observable parts of the spec, only funcref is used. Then type() could remain mostly unchanged, because we don't have to merge the return value for funcref and anyfunc somehow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Internally, we only use the "core" types, and the IDL enum here is only a way to expose them to JS. ToValueType then maps both enum values to [=funcref=], and FromValueType only creates the "funcref" value. (Those are both already correct in the PR.) Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are saying that this is the set of strings that is allowed to be used in a GlobalType, and not the internal type. Fair enough, I added anyfunc back. PTAL

@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 8, 2021

My understanding is that these cases should be handled in spec and tests:

Inputs (should support both spellings):

  • Global constructor
  • Table constructor

Outputs (should use "funcref"):

  • Table type()
  • Global type()
  • Module imports() (both directly and in function signatures)
  • Module exports() (both directly and in function signatures)

@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 8, 2021

Ref #25

Copy link
Contributor Author

@gahaas gahaas left a comment

Choose a reason for hiding this comment

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

I added more tests now. I'm not sure about the tests in module.exports and module.imports that you requested. Please tell me if you meant more that this.

I used the wpt tests as a base, because the tests in this repository seem to be outdated.

"anyfunc",
"funcref",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would it work if we keep both? Wouldn't that mean that they are different types? How should I define then what the result of type() is?
My idea was that there is only one type, funcref, but the string anyfunc is accepted as an alias in the constructor. Internally, in the non-observable parts of the spec, only funcref is used. Then type() could remain mostly unchanged, because we don't have to merge the return value for funcref and anyfunc somehow.

Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

Thanks! It seems like you missed ToTableKind in the spec, but lgtm otherwise.

@gahaas
Copy link
Contributor Author

gahaas commented Nov 17, 2021

Done.

assert_true(type.writable, 'type: writable');
assert_true(type.enumerable, 'type: enumerable');
assert_true(type.configurable, 'type: configurable');
if (expected.type.parameters) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do !== undefined here as well

@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 18, 2021

@rossberg could you merge?

@rossberg rossberg merged commit 5ad0278 into WebAssembly:main Nov 18, 2021
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