wasmparser: Refactor canonical option validation and func type lowering #2163
Conversation
Rather than repeatedly searching the list of canonical options, create a `struct CanonicalOptions` that summarizes their meaning when taken altogether. This `struct` is returned from `check_options`, where some initial, light validation occurs. Then, additional methods like `options.require_memory()?` can be called on the resulting `struct` depending on the use site (e.g. lifting vs. lowering vs. `task.return` vs. etc...). This also makes it so that `check_options` does not require a `LoweringInfo` as a parameter. That presupposes that we already know the core function signature we will be lowering into. That will no longer be true with the combination of Wasm GC and the component model, where there will be a canonical option to specify the desired core function type, and this controls whether GC is used or not, among other things. Finally, the `ComponentFuncType::lower` method is now invoked after canonical options are checked and takes a `CanonicalOptions` argument. Rather than setting `info.requires_memory = true` when it determines that a memory is required (for example) which was then validated in `check_options` later on, it has become fallible and calls `options.require_memory()?` itself to validate that a memory is declared. Again, this fallibility will be required for Wasm GC and component model integration in the future.
| let ty = types[self.core_function_at(*idx, offset)?].unwrap_func(); | ||
| if ty.params() | ||
| let ty_id = self.core_function_at(*idx, offset)?; | ||
| let func_ty = types[ty_id].unwrap_func(); | ||
| if func_ty.params() | ||
| != [ValType::I32, ValType::I32, ValType::I32, ValType::I32] | ||
| || ty.results() != [ValType::I32] | ||
| || func_ty.results() != [ValType::I32] | ||
| { |
There was a problem hiding this comment.
Do you think it might make sense to defer this type-check until CanonicalOptions::check_{lower,lift}? That's how post-return and callback options work for somewhat unrelated reasons, but in the future with memory64 integration it might be required for realloc as well
There was a problem hiding this comment.
I was trying to validate as much up front as possible, so that it is harder to forget to add things in the implementation of subsequent methods and so that it can be shared as much as possible. That said, I don't feel super strongly about it. My attitude is probably best summarized as
maybe? but it isn't clear to me. let's wait and cross that bridge when we get to it
|
Going to put this in the merge queue, if you feel more strongly about when/where we validate the |
Rather than repeatedly searching the list of canonical options, create a
struct CanonicalOptionsthat summarizes their meaning when taken altogether. Thisstructis returned fromcheck_options, where some initial, light validation occurs. Then, additional methods likeoptions.require_memory()?can be called on the resultingstructdepending on the use site (e.g. lifting vs. lowering vs.task.returnvs. etc...).This also makes it so that
check_optionsdoes not require aLoweringInfoas a parameter. That presupposes that we already know the core function signature we will be lowering into. That will no longer be true with the combination of Wasm GC and the component model, where there will be a canonical option to specify the desired core function type, and this controls whether GC is used or not, among other things.Finally, the
ComponentFuncType::lowermethod is now invoked after canonical options are checked and takes aCanonicalOptionsargument. Rather than settinginfo.requires_memory = truewhen it determines that a memory is required (for example) which was then validated incheck_optionslater on, it has become fallible and callsoptions.require_memory()?itself to validate that a memory is declared. Again, this fallibility will be required for Wasm GC and component model integration in the future.