Skip to content

[test] Account for one-pass implementations in more places#1904

Merged
rossberg merged 1 commit into
WebAssembly:mainfrom
YerinAlexey:streaming-test-fix
May 13, 2025
Merged

[test] Account for one-pass implementations in more places#1904
rossberg merged 1 commit into
WebAssembly:mainfrom
YerinAlexey:streaming-test-fix

Conversation

@YerinAlexey
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Hm, to be honest, I don't follow the reason for most of the changes in this PR. Can you elaborate why you think they're needed and how they relate to streaming compilation?

Comment thread test/core/data.wast Outdated
Comment on lines +415 to +420
(module
(global (export "global-i32") i32 (i32.const 0))
(global (export "global-mut-i32") (mut i32) (i32.const 0))
)
(register "test")

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.

This shouldn't be needed. Validation has to happen before linking, so it mustn't matter for any of the tests whether the imported module actually exists.

Same with the other tests.

@YerinAlexey
Copy link
Copy Markdown
Contributor Author

Sorry, I mixed up the terminology in the title - I was referring to implementations that do decoding and validation in one pass.

I was generally following the idea that failure tests should only contain one type of error.

This test change specifically permits implementations to immediately resolve imports rather than doing that during instantiation. I think this is reasonable and can even be beneficial for some of them. If this addition is undesirable, I can remove that part.

@YerinAlexey YerinAlexey force-pushed the streaming-test-fix branch from 1416df2 to 23f9c8d Compare May 12, 2025 21:35
@YerinAlexey YerinAlexey changed the title [test] Account for streaming implementations in more places [test] Account for one-pass implementations in more places May 12, 2025
@rossberg
Copy link
Copy Markdown
Member

With Wasm's model of separate compilation, imports are not available during validation or compilation. Providing them, i.e., linking, is part of instantiation, and a compiled module can be instantiated multiple times with different imports. Even for API that combines the two phases, an invalid module must be treated as if linking was never started.

Because of that, the presence of module imports should never matter for pure validation tests, and we in fact want to test that validation fails regardless.

@YerinAlexey YerinAlexey force-pushed the streaming-test-fix branch from 23f9c8d to cca5a5e Compare May 13, 2025 14:16
@YerinAlexey
Copy link
Copy Markdown
Contributor Author

Thanks for the clarification! I've removed all linking-related changes.

Copy link
Copy Markdown
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Thanks!

@rossberg rossberg merged commit 8d6792e into WebAssembly:main May 13, 2025
1 check 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.

2 participants