Skip to content

Add missing ids to getExpressionInfo and fix existing bugs - #7507

Merged
kripken merged 3 commits into
WebAssembly:mainfrom
GulgDev:js-expression-info
Apr 18, 2025
Merged

kripken merged 3 commits into
WebAssembly:mainfrom
GulgDev:js-expression-info

Conversation

@GulgDev

@GulgDev GulgDev commented Apr 16, 2025

Copy link
Copy Markdown
Contributor

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

kripken commented Apr 16, 2025

Copy link
Copy Markdown
Member

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

GulgDev commented Apr 16, 2025

Copy link
Copy Markdown
Contributor Author

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

kripken commented Apr 16, 2025

Copy link
Copy Markdown
Member

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

GulgDev commented Apr 16, 2025

Copy link
Copy Markdown
Contributor Author

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

@kripken

kripken commented Apr 16, 2025

Copy link
Copy Markdown
Member

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

@GulgDev

GulgDev commented Apr 16, 2025

Copy link
Copy Markdown
Contributor Author

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

kripken commented Apr 16, 2025

Copy link
Copy Markdown
Member

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

GulgDev commented Apr 17, 2025

Copy link
Copy Markdown
Contributor Author

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

'id': id,
'type': type,
'name': UTF8ToString(Module['_BinaryenBlockGetName'](expr)),
'name': name ? UTF8ToString(name) : null,

Copy link
Copy Markdown
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
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