False positive "field is never read" warnings

[emilio]

Here's a reduced test-case from Firefox source code:

struct Data {
    pub field: bool,
}

struct Foo<'a> {
    data: &'a mut Data,
}

impl<'a> Foo<'a> {
    fn parse_data(&mut self) {
        macro_rules! parse {
            () => {
                self.data.field = true;
            }
        }
        parse!()
    }
}

fn main() {
    let mut data = Data { field: false };
    {
        let mut foo = Foo { data: &mut data };
        foo.parse_data();
    }
    println!("{}", data.field);
}

Compiling that program warns with:

warning: field is never read: `data`
 --> t.rs:6:5
  |
6 |     data: &'a mut Data,
  |     ^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

Which is obviously not true.

Meta

rustc --version --verbose:

rustc 1.51.0-nightly (04caa632d 2021-01-30)
binary: rustc
commit-hash: 04caa632dd10c2bf64b69524c7f9c4c30a436877
commit-date: 2021-01-30
host: x86_64-unknown-linux-gnu
release: 1.51.0-nightly
LLVM version: 11.0.1

[emilio]

Seems like this is a regression from d3c69a4.

[emilio]

cc @sanxiyn

[sanxiyn]

Oops, sorry! Technically, Foo.data field is, in fact, never read. What went wrong is Foo.data is written and read through other ways.

I think restricting the lint to non-reference fields should work, and will submit a PR to do so quickly. Please double check whether my thinking is correct.

[emilio]

Ah, yeah, I somehow thought that the macro was needed to trigger the bug but it is not. That sounds good. Writes to mutable references have side effects so doing that seems ok.

[djc]

Would that also cover this case here, in the idna crate? (The Mapper::errors field is a &mut reference to an Errors type, which is in fact returned to the caller of processing().)

https://github.com/servo/rust-url/pull/674/checks?check_run_id=1855869353

[emilio]

Yeah, I think it's the same.

[pnkfelix]

Ah, this was closed, but the bug leaked into beta (as mentioned in #81658) while the fix is only in nightly.

Reopening and tagging as stable-to-beta regression

[JohnTitor]

Assigning P-medium as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

[pnkfelix]

As noted on #81473 (comment) , we're going to resolve the beta issue by reverting PR #81473 on beta (which I have assigned myself to take care of).

[pietroalbini]

This was a regression affecting beta 1.51, closing this.