wit-bindgen: generate only one StreamPayload or FuturePayload implementation per type alias set #1482
Conversation
danielvallance
force-pushed
the
duplicate_stream_impl_fix
branch
from
81ba43d to
4cac3fc
Compare
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Thanks for the PR! I'm a bit wary of adding yet-more-layers-of-maps though, especially when there's already a map of stream_payloads. Would you be up for doing some extra refactoring to remove the need for these maps? For example naively I'd expect that the stream_payloads map could be refactored to be indexed by TypeId rather than String. Callers would then be sure to call dealias to ensure that the only types inserted/looked up would be un-aliased forms to solve the original bug
|
hello, thanks for taking a look! yeah I agree that does sound like a cleaner solution, it sounds quite similar to a previous PR I made for this issue (#1478) in which I index the stream_payloads map with an Option < Type > however I declined this due to CI failures those CI failures happened because the same wit Type/TypeId was referred to at different paths, and since in this case it was not a TypeDefKind::Type, it resulted in the generation of identical structs at different paths which the compiler considered different types. and since the wit Type was used to index the stream_payloads map, it could only contain a reference to one of the several identical struct types generated from that wit Type, and in some cases it was the wrong one that sort of led me to think I had to create this new map to handle the cases where a type alias was generated, and in the other cases, use stream_payloads as before i did also have the thought of generating type aliases instead of regenerating multiple identical struct types at different paths for the same wit Type, however I felt that was quite a large change which could have unforseen consequences. however i may mark this as a draft and investigate this option further for now, do you think that sounds feasible? |
|
Ah yeah ok I can see where that would add complications. I believe that's due to the fact that if an interface is both imported and exported that uses the same What you describe sounds like it might work yeah, although another possible solution would be to invert things a bit. The trait implementation could be generated when the type is defined, for example. This would mean that a pass would be done over the WIT to determine the set of types that are used in futures/streams, and then when generating types if the type is in that set the trait impl is generated. That I think would work in the fact of import/export and avoid the need to work with aliases too much? (insertion into the set of futures/streams would be where the de-aliasing happens) |
|
actually yeah that's probably a better way of going about it - thanks for the pointer! i will mark this as draft while i look into doing it this way |
danielvallance
force-pushed
the
duplicate_stream_impl_fix
branch
from
4cac3fc to
1fbf224
Compare
…tations WIT 'use' statements are represented as Type::Id(_) Types in this codebase, and get translated to Rust type aliases in the generated Rust bindings. Since these aliases may be located at different module paths, creating a StreamPayload or FuturePayload implementation for more than one of these paths will cause the Rust compiler to complain about conflicting trait implementations for the same type. This commit solves this issue by dealiasing payload types of the form Type::Id(_) when generating a key for the future_payloads and stream_payloads maps. This means each alias set will have at most one implementation of these traits. Fixes issue 1432
danielvallance
force-pushed
the
duplicate_stream_impl_fix
branch
from
1fbf224 to
157459f
Compare
This change dealiases inside print_ty during recursion, so it catches aliases at every nesting depth. PR bytecodealliance#1482 only dealiases the top-level payload type. That works when the payload itself is an alias (e.g., future<error-code>), but not when the payload is an anonymous type that contains aliases (e.g., future<result<_, error-code>>). Fixes bytecodealliance#1523 Resolves bytecodealliance#1432
2 participants
WIT 'use' statements translate to Rust type aliases in the generated Rust bindings.
Since these aliases may be located at different module paths, creating a StreamPayload or FuturePayload implementation for more than one of these paths will cause the Rust compiler to complain about conflicting trait implementations for the same type.
This commit allows only one StreamPayload or FuturePayload implementation to be generated for a WIT type which will get transpiled into a Rust type alias, and it records mappings of the WIT type to the canonical module path to the type alias, which allows equivalent type aliases to reuse the implementation.
Fixes issue #1432