Reduce scope of wasmparser::BinaryReaderError in the codebase #1962
Conversation
webmaster128
requested review from
Hywan,
losfair,
MarkMcCaskey,
nlewycky and
syrusakbary
as code owners
webmaster128
force-pushed
the
handle-BinaryReaderError
branch
from
c1cc1a2
to
572fa7e
Compare
1963: Deprecate to_wasm_error r=syrusakbary a=webmaster128 # Description This is a code style improvement that came up when working on #1962 but is independent of that. Should I remove `to_wasm_error` or keep it as deprecated? # Review - [ ] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Simon Warta <simon@warta.it>
1963: Remove to_wasm_error r=jubianchi a=webmaster128 # Description This is a code style improvement that came up when working on #1962 but is independent of that. Should I remove `to_wasm_error` or keep it as deprecated? # Review - [ ] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Simon Warta <simon@warta.it>
webmaster128
force-pushed
the
handle-BinaryReaderError
branch
from
572fa7e
to
3a06e3e
Compare
|
Thank you for the feedback @syrusakbary. I'm having compile issues right now. I'll let you know when I have a demo available showing the integration of this PR in a project. |
webmaster128
force-pushed
the
handle-BinaryReaderError
branch
from
4c1d039
to
3b856c7
Compare
| @@ -113,7 +112,7 @@ impl Compiler for SinglepassCompiler { | |||
|
|
|||
| while generator.has_control_frames() { | |||
| generator.set_srcloc(reader.original_position() as u32); | |||
| let op = reader.read_operator().map_err(to_compile_error)?; | |||
| let op = reader.read_operator()?; | |||
There was a problem hiding this comment.
Before this PR, BinaryReaderError was converted to CompileError::Codegen at this place. Now it is converted to CompileError::Wasm. Is this change correct?
There was a problem hiding this comment.
Hmm, it's a fuzzy line for sure. Perhaps the idea for calling it "codegen" was that we only read more operators here based on state of the code generator... whereas calling it a "wasm" error suggests that if the invariant here isn't met, then it's because of a problem with the Wasm...
I would defer to the types of errors that this function (and functions like it) are returning... if they're mostly returning codegen errors, then I'd expect that to be what we want here, likewise with Wasm
There was a problem hiding this comment.
Alright, reader.read_operator() now returns WasmError, so as long as this is fine, I leave it to the default type converter.
One thought I had about this code section: Maybe to_compile_error should be renamed to to_codegen_error if it should always produces CompileError::Codegen, independent of the source type.
There was a problem hiding this comment.
Hmm, it looks like to_compile_error comes from the trait / method, so it probably makes sense to keep it is as used if that's a widely used abstraction here -- I'm not super familiar with this part of wasmer's code
|
Alright, I'm happy now. Demo here: https://github.com/CosmWasm/cosmwasm/pull/675/files. |
|
bors r+ |
1962: Reduce scope of wasmparser::BinaryReaderError in the codebase r=MarkMcCaskey a=webmaster128 ~Based on #1963~ Closes #1950 # Description There is an error conversion pileline `BinaryReaderError` -> `WasmError`. In this PR, the same conversion happens earlier, such that `WasmError`/`WasmResult` can be used in middlewares. Questions: - [ ] Should middlewares be allowed to produce all `WasmError` cases? Or should we introduce a `WasmError::MiddlewareError(String)`? - [x] ~Should `to_wasm_error` be removed as part of this PR?~ Extracted to #1963 # Review - [ ] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Simon Warta <simon@warta.it>
|
Build failed: |
webmaster128
force-pushed
the
handle-BinaryReaderError
branch
from
3b856c7
to
4c351a7
Compare
webmaster128
force-pushed
the
handle-BinaryReaderError
branch
from
4c351a7
to
df74a48
Compare
|
build fixed |
|
bors r+ |
Based on #1963Closes #1950
Description
There is an error conversion pileline
BinaryReaderError->WasmError. In this PR, the same conversion happens earlier, such thatWasmError/WasmResultcan be used in middlewares.Questions:
WasmErrorcases? Or should we introduce aWasmError::MiddlewareError(String)?ShouldExtracted to Remove to_wasm_error #1963to_wasm_errorbe removed as part of this PR?Review