rustc sometimes resolves an ambiguous generic parameter instead of erroring #91369
Labels
A-inference
Area: Type inference
C-bug
Category: This is a bug.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Comments
inquisitivecrystal
added
A-inference
TheBlueMatt
added a commit
to TheBlueMatt/rust-lightning
that referenced
this issue
Jul 1, 2022
This fixes an insta-panic in `ChannelMonitor` deserialization where we always `unwrap` a previous value to determine the default value of a later field. However, because we always ran the `unwrap` before the previous field is read, we'd always panic. The fix is rather simple - use a `OptionDeserWrapper` for `default_value` fields and only fill in the default value if no value was read while walking the TLV stream. The only complexity comes from our desire to support `read_tlv_field` calls that use an explicit field rather than an `Option` of some sort, which requires some statement which can assign both an `OptionDeserWrapper<T>` variable and a `T` variable. We settle on `x = t.into()` and implement `From<T> for OptionDeserWrapper<T>` which works, though it requires users to specify types explicitly due to Rust determining expression types prior to macro execution, completely guessing with no knowlege for integer expressions (see rust-lang/rust#91369).
TheBlueMatt
added a commit
to TheBlueMatt/rust-lightning
that referenced
this issue
Jul 5, 2022
This fixes an insta-panic in `ChannelMonitor` deserialization where we always `unwrap` a previous value to determine the default value of a later field. However, because we always ran the `unwrap` before the previous field is read, we'd always panic. The fix is rather simple - use a `OptionDeserWrapper` for `default_value` fields and only fill in the default value if no value was read while walking the TLV stream. The only complexity comes from our desire to support `read_tlv_field` calls that use an explicit field rather than an `Option` of some sort, which requires some statement which can assign both an `OptionDeserWrapper<T>` variable and a `T` variable. We settle on `x = t.into()` and implement `From<T> for OptionDeserWrapper<T>` which works, though it requires users to specify types explicitly due to Rust determining expression types prior to macro execution, completely guessing with no knowlege for integer expressions (see rust-lang/rust#91369).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Apologies that I don't have a minimum reproduction but I've tried a few ways and thus far failed to reproduce outside of the original crate.
We recently had an error where we were deserializing a dummy object for backwards-compatibility, but the type being read was ambiguous, causing the code to behave unexpectedly. For some reason rustc concretized the object to an
Option<()>(guessing based on{:?}output, as I can't tell why it was concretized) when it was ambiguous (we really wanted anOption<u64>but didn't specify it).You can see the change which fixes it at lightningdevkit/rust-lightning#1195, which makes pretty clear that the object's type was, in fact, ambiguous (switching the changed declaration to
None::<()>works just as well, while changing behavior). Theread_tlv_fieldsmacro isn't all that complicated - https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/util/ser_macros.rs#L332 - it basically just calls intodecode_tlv_streamwhich is pretty straightforward - https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/util/ser_macros.rs#L168.I tried (and failed) to get a minimum reproduction which at least shows the high-level code structure at https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=c873827053afdaa20d333703d00fd16f (the actual relevant things are mostly in
util::serandutil::ser_macrosin the real code).The text was updated successfully, but these errors were encountered: