rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.82k stars 12.66k forks source link

Doc comment is shown as snippet for trait not implemented instead of field #89877

Closed koivunej closed 1 year ago

koivunej commented 3 years ago

Given the following code: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=fc1473e4afb90d6ab12751d3cc0cb6a8

fn main() {
    todo!("does not compile")
}

#[derive(Debug, serde::Deserialize)]
pub struct Root {
    /// doc comment included as the snippet
    foo: Bar,
    // this has a regular comment, not shown as snippet
    car: Bar,
}

/// this one didn't have deserialize derived by accident
#[derive(Debug)]
pub struct Bar {
    bar: u32
}

The current output is:

   Compiling playground v0.0.1 (/playground)
error[E0277]: the trait bound `Bar: Deserialize<'_>` is not satisfied
    --> src/main.rs:7:5
     |
7    |     /// doc comment included as the snippet
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `Bar`
     |
note: required by `next_element`
    --> /playground/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.130/src/de/mod.rs:1703:5
     |
1703 | /     fn next_element<T>(&mut self) -> Result<Option<T>, Self::Error>
1704 | |     where
1705 | |         T: Deserialize<'de>,
     | |____________________________^

error[E0277]: the trait bound `Bar: Deserialize<'_>` is not satisfied
    --> src/main.rs:10:5
     |
10   |     car: Bar,
     |     ^^^ the trait `Deserialize<'_>` is not implemented for `Bar`
     |
note: required by `next_element`
    --> /playground/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.130/src/de/mod.rs:1703:5
     |
1703 | /     fn next_element<T>(&mut self) -> Result<Option<T>, Self::Error>
1704 | |     where
1705 | |         T: Deserialize<'de>,
     | |____________________________^

error[E0277]: the trait bound `Bar: Deserialize<'_>` is not satisfied
    --> src/main.rs:7:5
     |
7    |     /// doc comment included as the snippet
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `Bar`
     |
note: required by `next_value`
    --> /playground/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.130/src/de/mod.rs:1842:5
     |
1842 | /     fn next_value<V>(&mut self) -> Result<V, Self::Error>
1843 | |     where
1844 | |         V: Deserialize<'de>,
     | |____________________________^

error[E0277]: the trait bound `Bar: Deserialize<'_>` is not satisfied
    --> src/main.rs:10:5
     |
10   |     car: Bar,
     |     ^^^ the trait `Deserialize<'_>` is not implemented for `Bar`
     |
note: required by `next_value`
    --> /playground/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.130/src/de/mod.rs:1842:5
     |
1842 | /     fn next_value<V>(&mut self) -> Result<V, Self::Error>
1843 | |     where
1844 | |         V: Deserialize<'de>,
     | |____________________________^

For more information about this error, try `rustc --explain E0277`.
error: could not compile `playground` due to 4 previous errors

Ideally the output should look like:

Instead of:

7    |     /// doc comment included as the snippet
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `Bar`

It should have like it already has with nightly 2021-10-13 (not beta):

7    | /     /// doc comment included as the snippet
8    | |     foo: Bar,
     | |____________^ the trait `Deserialize<'_>` is not implemented for `Bar`

Rationale: Comment is the whole snippet above. Nightlys version is much better with both the doc comment and the field shown, but at minimum the field should be always shown.

The field with plain // comment didn't trigger this, so it's related to handling of fields with doc comments.

Also plain #[derive(Debug)] being missing from struct Bar will not interestingly enough trigger this, but I didn't narrow it down further, just happened to find this while using serde.

Versions:

Found with stable 1.55.0. Looking at playground, beta has the same bug. nightly 2021-10-13 in the playground does not. I couldn't find an issue for this problem, so reporting it to remind to backport the fix to beta.

koivunej commented 3 years ago

Tried to manually bisect the nightly versions which would have had the span of only line 7 in the snippet but going back even 2020-01-01 (1.42.0) has the both lines 7 and 8 (doc comment and field). Maybe this is caused by something other being enabled or disabled in stable and beta channels?

Edition switch to 2015 doesn't affect this.

Trying to bisect stable channel, 1.31.0 gives (after changing the todo! back into unimplemented!):

error[E0277]: the trait bound `Bar: _IMPL_DESERIALIZE_FOR_Root::_serde::Deserialize<'_>` is not satisfied
 --> src/main.rs:7:5
  |
7 |     /// doc comment included as the snippet
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `_IMPL_DESERIALIZE_FOR_Root::_serde::Deserialize<'_>` is not implemented for `Bar`
  |
  = note: required by `_IMPL_DESERIALIZE_FOR_Root::_serde::de::SeqAccess::next_element`

I don't know how to look into this further.

GuillaumeGomez commented 3 years ago

I think this is a problem coming from serde directly.

estebank commented 3 years ago

I think there might be 1) a small mistake in serde and 2) some improvements we could make in our end to minimize the likelihood of people making that mistake. That being said, a priori can't tell if that is the case. Could you file a ticket for serde as well so they can also investigate? I'm sure that even if it is a mistake in our end, they might be able to work around it to get a fix out for users faster than our 12 weeks cadence.

koivunej commented 3 years ago

Oki thank you both, having now read a bit more on what syn et al. do, this is sounding more like a serde/syn bug for me as well at least for the span. I'll report it in serde.

Compiling the expanded version as standalone crate without any proc macros puts the error in the usual places (the point of use):

error[E0277]: the trait bound `Bar: Deserialize<'_>` is not satisfied
    --> src/main.rs:123:37
     |
123  |                         match match _serde::de::SeqAccess::next_element::<Bar>(&mut __seq) {
     |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `Bar`
     |
note: required by `next_element`
    --> /home/joonas/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.130/src/de/mod.rs:1703:5
     |
1703 | /     fn next_element<T>(&mut self) -> Result<Option<T>, Self::Error>
1704 | |     where
1705 | |         T: Deserialize<'de>,
     | |____________________________^
estebank commented 1 year ago

The current output is much better nowadays:

error[[E0277]](https://doc.rust-lang.org/stable/error_codes/E0277.html): the trait bound `Bar: Deserialize<'_>` is not satisfied
    --> src/main.rs:8:10
     |
8    |     foo: Bar,
     |          ^^^ the trait `Deserialize<'_>` is not implemented for `Bar`
     |
     = help: the following other types implement trait `Deserialize<'de>`:
               &'a Path
               &'a [u8]
               &'a str
               ()
               (T0, T1)
               (T0, T1, T2)
               (T0, T1, T2, T3)
               (T0, T1, T2, T3, T4)
             and 130 others
note: required by a bound in `next_element`
    --> /playground/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.156/src/de/mod.rs:1729:12
     |
1729 |         T: Deserialize<'de>,
     |            ^^^^^^^^^^^^^^^^ required by this bound in `SeqAccess::next_element`
...

I believe this can be considered fixed (likely by serde itself).

koivunej commented 1 year ago

Confirmed it looks better with today's nightly, not yet with stable with playground. Note that this is the same behaviour as I noted when reporting:

Found with stable 1.55.0. Looking at playground, beta has the same bug. nightly 2021-10-13 in the playground does not. I couldn't find an issue for this problem, so reporting it to remind to backport the fix to beta.

EDIT: However current beta on playground looks now better.