Wasmtime GitHub notifications bot (Feb 28 2025 at 18:07): Wasmtime GitHub notifications bot (Feb 28 2025 at 18:07):pchickey opened PR #10311 from bytecodealliance:pch/fix_10090 to bytecodealliance:main:
The calculation of TypeInfo only reaches types which are passed to or from a function. For types which are not reachable, default to the defining them according to the ownership setting given to bindgen.
I have my doubts that
with-reuse of bindgen types actually works properly when bindgen is set to Ownership::Borrowing but thats out of scope for this PR, which is to fix #10090<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Feb 28 2025 at 18:07):pchickey requested wasmtime-core-reviewers for a review on PR #10311.
Wasmtime GitHub notifications bot (Feb 28 2025 at 18:07):pchickey requested alexcrichton for a review on PR #10311.
Wasmtime GitHub notifications bot (Feb 28 2025 at 18:08):pchickey edited PR #10311:
Rust type definitions for wit types are created for the set returned by
modes_of, when an empty list is returned the type never gets a definition in Rust.The calculation of TypeInfo only reaches types which are passed to or from a function. For types which are not reachable, default to the defining them according to the ownership setting given to bindgen.
I have my doubts that
with-reuse of bindgen types actually works properly when bindgen is set to Ownership::Borrowing but thats out of scope for this PR, which is to fix #10090<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Feb 28 2025 at 18:10):pchickey commented on PR #10311:
ping @dicej, who last touched this logic in #6648 . Does this still work for your needs?
Wasmtime GitHub notifications bot (Feb 28 2025 at 18:21):ping @dicej, who last touched this logic in #6648 . Does this still work for your needs?
#6648 Was motivated by fuzz testing the host bindings; the nested borrows we had before made generating arbitrary test cases really difficult, so I wanted to avoid that. As long as the fuzz tests in this repo still work, then we're good :+1:
Wasmtime GitHub notifications bot (Feb 28 2025 at 18:21):dicej edited a comment on PR #10311:
ping @dicej, who last touched this logic in #6648 . Does this still work for your needs?
#6648 Was motivated by fuzz testing the host bindings and runtime component functionality; the nested borrows we had before made generating arbitrary test cases really difficult, so I wanted to avoid that. As long as the fuzz tests in this repo still work, then we're good :+1:
Wasmtime GitHub notifications bot (Feb 28 2025 at 18:52):pchickey updated PR #10311.
Wasmtime GitHub notifications bot (Feb 28 2025 at 18:55):alexcrichton submitted PR review:
This logic is originally derivative of this logic in wit-bindgen for guests, and while they were originally the same I think the guest has since moved on. IIRC that part of the guest bindings was extremely tricky and probably still isn't fully correct even today. I also think that there's various different concerns on the guest than there are for the host, which means we probably don't want them to be the exact same.
Regardless though this seems reasonable to me. I'm not actually sure if it's correct so I'm mostly with Joel where if it compiles seems reasonable to me
Wasmtime GitHub notifications bot (Feb 28 2025 at 19:01):pchickey has enabled auto merge for PR #10311.
Wasmtime GitHub notifications bot (Feb 28 2025 at 19:28):pchickey merged PR #10311.
Last updated: Apr 18 2025 at 01:31 UTC