eval_order_dependence shouldn't fire for struct initializers

[CAD97]

It's become idiomatic for syn-based parsers to parse members of each AST struct directly into the struct initializer, including e.g. bracketed! which initializes a place outside of the initializer. The bracketed! example:

use proc_macro2::TokenStream;
use syn::parse::{Parse, ParseStream};
use syn::{bracketed, token, Result, Token};

// Parse an outer attribute like:
//
//     #[repr(C, packed)]
struct OuterAttribute {
    pound_token: Token![#],
    bracket_token: token::Bracket,
    content: TokenStream,
}

impl Parse for OuterAttribute {
    fn parse(input: ParseStream) -> Result<Self> {
        let content;
        Ok(OuterAttribute {
            pound_token: input.parse()?,
            bracket_token: bracketed!(content in input),
            content: content.parse()?,
        })
    }
}

clippy gives a eval_order_dependence warning here:

warning: unsequenced read of a variable
  --> src/lib.rs:20:22
   |
20 |             content: content.parse()?,
   |                      ^^^^^^^
   |
   = note: `#[warn(clippy::eval_order_dependence)]` on by default
note: whether read occurs before this write depends on evaluation order
  --> src/lib.rs:19:28
   |
19 |             bracket_token: bracketed!(content in input),
   |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#eval_order_dependence
   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

I believe that struct initializer syntax is in fact strongly specified to evaluate the member expressions in source order. And in this case, the order of the write and read can only be this, as the place assigned is both non-mut and not yet initialized before the write.

(Note that ParseStream is &'_ ParseBuffer<'_> which does include internal mutability but does so through &Cell, so the false negative on internal mutability ordering here is expected and likely unpreventable.)

[CAD97]

Other than special casing struct initializers, the other potential change is to not fire eval_order_dependence when the write is to a non-mut let binding, as the place can only ever be assigned once and must be written before the read, so only one interpretation of the ordering is sound.

[hellow554]

It hit me today. Here's a simplified version:

async fn one() -> u32 { 1 }
async fn two() -> u32 { 2 }

pub struct TwoInts {
    a: u32,
    b: u32,
}

pub async fn ff() -> TwoInts {
    TwoInts {
        a: one().await,
        b: two().await,
    }
}

gives

warning: unsequenced read of a variable
  --> src/lib.rs:12:12
   |
12 |         b: two().await,
   |            ^^^^^^^^^^^
   |
   = note: `#[warn(clippy::eval_order_dependence)]` on by default
note: whether read occurs before this write depends on evaluation order
  --> src/lib.rs:11:12
   |
11 |         a: one().await,
   |            ^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#eval_order_dependence

warning: unsequenced read of a variable
  --> src/lib.rs:11:12
   |
11 |         a: one().await,
   |            ^^^^^^^^^^^
   |
note: whether read occurs before this write depends on evaluation order
  --> src/lib.rs:12:12
   |
12 |         b: two().await,
   |            ^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#eval_order_dependence

IMHO this is safe and should not be linted.

@flip1995 Could you add the correct labels as well as maybe give an instruction on how to fix this? I would work on this, but currently have no plan to start where :)

[flip1995]

@hellow554 I added the labels. For the instruction on how to fix this, it is a bit more complicated. This is probably a really easy fix, the question is, where it has to be done. You probably have to just add if some_span.from_expansion() { return } somewhere in the linting code. If you are unable to find the place where this should be added, just ping me again and I'll take a look at the code.

[hellow554]

I tried a few things yesterday, but they either broke existing tests (not sure if legitmate) oder hadn't the desirede effect. I don't think that this should be too hard 😅 But yeah, a look would be great @flip1995

[flip1995]

Uh, I think this is harder than I thought. For once, the lint is triggered inside a visitor, which is a bad idea most of the time. This is also the reason why just adding a from_expansion won't work. Fixing this may even mean rewriting some of the linting code... (At least I think so)

[Jarcho]

The lint has been renamed to mixed_read_write_expression as of #8621.