Invalid `field is never read: ` lint warning

[weiznich]

Code

Diesel CI begins to show warnings that some field is never read, while the field is actually read at some point.
For reference the field is read here via a ffi function.

I expected to see this happen: No warning shown as the field is read.
Instead, this happened: We've got a warning field is never read: input_binds``

Version it worked on

It most recently worked on: last stable/beta and nightlies from a few days back

Version with regression

cargo 1.51.0-nightly (c3abcfe8a 2021-01-25)

[matthiaskrgr]

Probably related to #81310 ?

[tmiasko]

An example below (makes no attempt to reproduce FFI aspect since as far as I can see FFI doesn't read the field per se), warns that field f is never read; behaviour changed in rustc 1.51.0-nightly (04caa632d 2021-01-30):

#![crate_type = "lib"]

pub struct S {
    f: Option<String>
}

impl S {
    pub fn f(&mut self, f: Option<String>) {
        self.f = f;
    }
}

In this case there is an implied read of the field by the drop implementation, so there should be no warning. If the type of field were Copy, the warning would be appropriate, I think? @sanxiyn via #81473.

[sanxiyn]

I think I would want a warning for @tmiasko's example, but maybe that's not appropriate for a default-on lint in the compiler (as opposed to Clippy). I can certainly change it to Copy fields.

[sanxiyn]

Note that Drop is not considered in the lint already in stable:

struct S {
    i: i32,
    s: String,
}

fn main() {
    let s = S { i: 0, s: String::new() };
    println!("{}", s.i);
}

S.s is reported dead in stable. Should I fix it together?

[tmiasko]

Based on what you wrote, I feel convinced that it would be useful to warn about those cases. Consistently checking for drop in the fields is probably too conservative. Additionally if the field exists only for the side effect of executing its drop implementation, there is a simple escape hatch available, prepend underscore to the field name.

@rust-lang/wg-diagnostics

[weiznich]

To give a bit more information about the case linked above. The issue there is not that this field is only read by drop, but that we pass a pointer to some internal part of this field via a ffi function. Now we must keep that field in the same location as long as the underlying part of the ffi data structure is in place (in this case, till we read all values returned by this prepared database statement).
Regarding to the mentioned workarounds: I'm aware that we could just put a #[allow(dead_code)] there or mark that field as unused, but that would not be true and therefore misleading for potential contributors.

[tmiasko]

we pass a pointer to some internal part of this field via a ffi function

Maybe I am misreading the code, but as far as I can see that happens before the value is even assigned to the field, so that operation doesn't read the field at all. A smaller example demonstrating the issue would probably help.

This is how I see the case linked above (and another simple solution to avoid warning is to actually read the field, instead of the argument, which potentially also avoids UB in the case of an indirection through box which invalidates raw pointers after move):

#![crate_type = "lib"]

use std::rc::Rc;

pub struct A {
    s: Rc<String>,
}

extern {
    fn escape(s: *const u8, n: usize);
}

impl A {
    pub fn f(&mut self, s: Rc<String>) {
        // The address of allocation escapes. Function might read the 
        // content of the string, but that doesn't mean that it reads the `A::s` field.
        unsafe { escape(s.as_ptr(), s.len()) };
        self.s = s;
    }
}

[weiznich]

Let me unroll the code in diesel a bit:

struct Statement {
    stmt: NotNull<ffi::MYSQL_STMT>,
    input_binds: Option<Binds>
}

pub struct Binds {
    data: Vec<BindData>,
}

pub struct BindData {
    tpe: ffi::enum_field_types,
    bytes: Vec<u8>,
    length: libc::c_ulong,
    is_null: ffi::my_bool,
    is_truncated: Option<ffi::my_bool>,
}

impl BindData {
    unsafe fn mysql_bind(&mut self) -> ffi::MYSQL_BIND {
        let mut bind: ffi::MYSQL_BIND = mem::zeroed();
        bind.buffer_type = self.tpe;
        bind.buffer = self.bytes.as_mut_ptr() as *mut libc::c_void;
        bind.buffer_length = self.bytes.capacity() as libc::c_ulong;
        bind.length = &mut self.length;
        bind.is_null = &mut self.is_null;

        if let Some(ref mut is_truncated) = self.is_truncated {
            bind.error = is_truncated;
        }

        bind
    }
}

impl Statement {
    pub(super) fn input_bind(&mut self, mut input_binds: Binds) -> QueryResult<()> {
        {
                    let mut binds = self
                       .data
                       .iter_mut()
                       .map(|x| unsafe { x.mysql_bind() })
                      .collect::<Vec<_>>();
                   let bind_ptr = binds.as_mut_ptr(); // you cannot assume if `input_binds` is  read or not after this point anymore
                  some_function_that_reads_this_pointer_at_a_later_point_in_time(bind_ptr)
        }
        self.input_binds = Some(input_binds);
        self.did_an_error_occur()
    }
}

Now the warning points at the input_binds field in the Statement struct. Removing this field as the warning suggests would lead to undefined behaviour as the pointer now points to uninitialized memory. I believe no warning issued by rustc should ever lead to undefined behaviour.

Now it's up to discussion if this code should better be written in another way or not, but in that case rustc should emit an error message telling me that, but it's pointing in the completely wrong direction.

[weiznich]

As this is marked as needs a minimal example: I've put some playground together that reproduces this issue without depending on diesel or on any ffi. As another note: If you try to follow the implicit suggestion here and remove that field miri will report a error about a read after free error, while this does not happen if the field stays the same.
I have nearly no knowledge of how much information are available for lints, but I would suggest to disable this lint as soon as a raw pointer to the field itself or any sub field is involved. That's the point where you cannot reason anymore if the field (or any subfield) is read anywhere.

[tmiasko]

I agree completely that the lint should be conservative when the field address is taken. Though, I disagree with factual statement that the field has its address taken in the example.

The field has a drop implementation, so the behaviour of the program is clearly different with the field removed. Maybe the lint should indicate that?. The situation is the same as with scope guards which when unused will be warned about, even though removing them might change the behaviour. I suggested earlier the possibility of looking if the type needs drop, but that would essentially disable the lint.