Skip to content

Add missing ids to getExpressionInfo and fix existing bugs #7507

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 3 commits into from
Apr 18, 2025

Conversation

GulgDev
Copy link
Contributor

@GulgDev GulgDev commented Apr 16, 2025

This PR adds cases for GC expressions in getExpressionInfo and updates CallIndirectId (fixes #7490), TryId. In addition, it also adds tests for getExpressionInfo results, which were not checked before.

Also, getExpressionInfo is very similar to the expression wrappers, so maybe we should modify this function to just return a copy of the wrapper? If we do so, should we either return the corresponding wrapper or return an object which has all the required properties of the wrapper (should they be value properties or getter properties?)?

@GulgDev GulgDev marked this pull request as draft April 16, 2025 17:32
@kripken
Copy link
Member

kripken commented Apr 16, 2025

Also, getExpressionInfo is very similar to the expression wrappers, so maybe we should modify this function to just return a copy of the wrapper? If we do so, should we either return the corresponding wrapper or return an object which has all the required properties of the wrapper (should they be value properties or getter properties?)?

Yes, that does seem redundant...

Implementing getExpressionInfo using the wrappers might be good. I think we can just return the corresponding wrapper - it's an API change but seems strictly better, and low risk.

@GulgDev
Copy link
Contributor Author

GulgDev commented Apr 16, 2025

Implementing getExpressionInfo using the wrappers might be good. I think we can just return the corresponding wrapper - it's an API change but seems strictly better, and low risk.

If we return the wrapper instead of an info object this would not only break some code (they are not strictly equal), but that would also mean that info objects now mutate the state of the expression. Is this change wanted? Maybe it would be better to re-define all properties of the wrapper on a new object, with the needed changes to maintain compatibility with the previous API. Also, should getExpressionInfo be deprecated? Additionally, if the aforementioned function is not required anymore, there should be a way to create a wrapper from an unknown expression id (that is, the end user should be able to construct a wrapper of an expression, the id of which is unknown, just like it was possible with getExpressionInfo).

@kripken
Copy link
Member

kripken commented Apr 16, 2025

Yes, being able to get info for an expression of unknown class was probably the main motivation for getExpressionInfo. I agree we can deprecate it if we add such a method that creates a wrapper. Seems safer than a breaking change, good point.

@GulgDev
Copy link
Contributor Author

GulgDev commented Apr 16, 2025

Should this be implemented in a separate PR or is it ok to repurpose this one?

@kripken
Copy link
Member

kripken commented Apr 16, 2025

Maybe leave that for a later PR? The code in this PR looks good to land, unless it is incomplete somehow?

@GulgDev
Copy link
Contributor Author

GulgDev commented Apr 16, 2025

It is complete, but I am working on changing implicit assertInfoEqual to explicit property equality assertions. This would guarantee that any further changes will not break existing API.

@kripken
Copy link
Member

kripken commented Apr 16, 2025

Sounds good, let me know when it's ready here.

@GulgDev GulgDev marked this pull request as ready for review April 17, 2025 16:13
@GulgDev
Copy link
Contributor Author

GulgDev commented Apr 17, 2025

@kripken Done! I've also fixed some bugs that appeared after updating the tests.

return {
'id': id,
'type': type,
'name': UTF8ToString(Module['_BinaryenBlockGetName'](expr)),
'name': name ? UTF8ToString(name) : null,
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we could have a UTF8ToStringOrNull helper for this pattern, but we can consider that separately.

@kripken kripken merged commit b5a4e36 into WebAssembly:main Apr 18, 2025
14 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.

getExpressionInfo returns a string pointer instead of a JS string when called on a CallIndirect
2 participants