Skip to content

Extended name section tests - #11

Merged
bvisness merged 4 commits into
mainfrom
extended-name-tests
Sep 10, 2026
Merged

bvisness merged 4 commits into
mainfrom
extended-name-tests

Conversation

@bvisness

@bvisness bvisness commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

And really for the name section generally. These do not pass yet but hopefully this will be sufficient for phase 3.

@bvisness
bvisness requested a review from rossberg September 9, 2026 18:11
And really for the name section generally.
@bvisness
bvisness force-pushed the extended-name-tests branch from 2a6c332 to 7ebcaeb Compare September 9, 2026 18:14
We don't care about instantiating them (and therefore providing
imports).

@rossberg rossberg left a comment

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 looks great! You could throw in a few more negative tests that put @name in wrong places (esp before the bound identifier, before the keyword, after an export clause, or random places inside declarations).

@bvisness
bvisness merged commit 9ddd7af into main Sep 10, 2026
1 check failed
@bvisness bvisness mentioned this pull request Sep 10, 2026
Comment on lines +125 to +135
;; An empty name map.
(module (@custom "name" (after data) "\01\01\00"))

;; An empty inner name map in an indirect name map.
(module (func) (@custom "name" (after data) "\02\03\01\00\00"))

;; An empty name.
(module (func) (@custom "name" (after data) "\01\03\01\00\00"))

;; Duplicate names.
(module (func) (func) (@custom "name" (after data) "\01\07\02\00\01a\01\01a"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question for tests like these: should a new assert_* directive be added for this? By default even with an invalid name section (e.g. all \ff bytes) these tests in theory should still pass because validators (I thought at least) shouldn't be validating the contents of custom sections by default.

In lieu of that, should a new directive like assert_valid_custom be added for tests like these?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hm, good question. It feels a bit overkill to me when you can just ensure that your test runner reports known but ill-formed custom sections as errors or warnings somehow. But it would more explicitly signal intent. I guess I will leave that decision to @rossberg.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Alex mentioned here that an assert_valid_custom directive could be useful for signaling engines to avoid round-trip tests. That seems like a pretty good use case to me.

@rossberg rossberg Sep 15, 2026

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.

When Yuri and I discussed this in the context of the branch hinting proposal, IIRC the conclusion was that it's unclear how assert_valid_custom would differ from just giving the module — engines are free to not implement individual custom sections, so they cannot be forced to actually check this. OTOH, implementations may want to provide a way to control which custom sections to recognise (that's what the reference interpreter does), but then the assertion still doesn't add any expressiveness AFAICS.

Even if the custom section doesn't round-trip, I don't understand why that would be an issue for tests like the above. Won't they still succeed just fine with the custom section removed? Sure, it doesn't check much, but no harm done. The only place where round-tripping may fall over is for negative tests on custom section, but we already have assert_invalid_custom to express those cases.

(Aside: That said, tools that implement a custom section should also handle its round-tripping. That's why we introduced annotations in the text format. Ideally, every custom section should define appropriate annotation syntax. If it doesn't, it usually should be possible to fall back to the generic @custom annotation, though this fails for ill-designed custom formats that use binary byte offsets instead of AST-level counts, since those aren't robust. Such custom formats should definitely define a text format.)

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