Add annotations proposal implementation#322
Conversation
MaxDesiatov
force-pushed
the
maxd/annotations-support
branch
2 times, most recently
from
34d122b to
abf4c6c
Compare
kateinoigakukun
left a comment
kateinoigakukun
left a comment
There was a problem hiding this comment.
I feel like this change is too vibey 😞
| "spec": { | ||
| "repository": "https://github.com/WebAssembly/spec.git", | ||
| "revision": "1c9a04e7b9d6998b9a30c6edf40b42d4c2a13c7b", | ||
| "categories": ["spec"] | ||
| }, | ||
| "annotations": { | ||
| "repository": "https://github.com/WebAssembly/annotations.git", | ||
| "revision": "20a8e4f125d5d856aac392105c35055af9ed49f7", | ||
| "sparse-checkout": "test/custom", | ||
| "categories": ["default"] | ||
| }, |
There was a problem hiding this comment.
Do we need those repositories as dependencies?
There was a problem hiding this comment.
Only these repositories have relevant WAST tests for the proposal. We could use a sparse checkout to pull in just the few WAST files if you prefer that?
There was a problem hiding this comment.
Ok, the root problem here is that those wast files are not included in testsuite repository. I've submitted a fix now WebAssembly/testsuite#155
Also do we still need spec repository?
There was a problem hiding this comment.
The testsuite PR has been merged
There was a problem hiding this comment.
Unfortunately, the PR has been merged post Wasm-3.0 additions, which merged all proposals into the main test suite directory, which means we can't easily exclude GC tests directory wholesale if we bump to the commit where this PR is merged.
I'm vendoring the new tests for now.
| } | ||
|
|
||
| static func wastFiles(include: [String] = [], exclude: [String] = ["annotations.wast"]) -> [URL] { | ||
| static func wastFiles(include: [String] = [], exclude: [String] = ["id.wast"]) -> [URL] { |
There was a problem hiding this comment.
Why do we need to add id.wast as a new excluded test case?
There was a problem hiding this comment.
id.wast needed to be excluded because this function runs against WABT/WasmTools baseline, and neither of those support this WAST file, it's tested separately in func annotationProposalSpectest(), and that also has an explanation comment
| emitCustomSections(placement: .after(.import), encoder: &encoder) | ||
|
|
||
| // Section 3: Function section | ||
| emitCustomSections(placement: .before(.func), encoder: &encoder) |
There was a problem hiding this comment.
It's very error-prune to enforce inserting emitCustomSections before/after each section manually. Can we make it as a part of Encoder.section?
| let range: Range<Lexer.Index> | ||
| let kind: TokenKind | ||
| /// For quoted identifiers ($"..."), the decoded string bytes (not including the `$` prefix). | ||
| let quotedIdBytes: [UInt8]? |
There was a problem hiding this comment.
Why do we need to hold it here? Can't we simply interpret it in Parser?
There was a problem hiding this comment.
Thanks, that's cleaned up.
| } | ||
|
|
||
| /// Recognized annotation IDs that the parser needs to handle. | ||
| private static let recognizedAnnotations: Set<String> = ["name", "custom"] |
There was a problem hiding this comment.
I don't think Lexer needs to know about them. We can treat all custom section annotation items as opaque in Lexer, and parse known custom sections in Parser when needed. We can remove skipAnnotationBody.
There was a problem hiding this comment.
That's addressed on the Parser level now.
MaxDesiatov
force-pushed
the
maxd/annotations-support
branch
2 times, most recently
from
fe6353b to
37d96ee
Compare
MaxDesiatov
force-pushed
the
maxd/annotations-support
branch
from
37d96ee to
fe0f8de
Compare
# Conflicts: # README.md # Sources/WAT/BinaryEncoding/Encoder.swift
MaxDesiatov
force-pushed
the
maxd/annotations-support
branch
from
5e5dd36 to
920bca2
Compare
2 participants
wasm-tools printincludes custom annotations on certain Wasm components, which WasmKit is currently unable to read.Annotations proposal enables usage of custom sections and names in WAT that weren't previously expressible as WAT ids or strings. See proposal overview for more details.