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.

[apiraino]

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

[weiznich]

As a heads up: This has hit beta now. I would really prefer to have this warning fixed before it hits a stable release. As already mentioned multiple times above: I believe the correct place for lints with emit false positives is clippy and not rustc. If rustc warns about something it should mention a clear way how to fix the warning . This lint only suggests that something is uneeded and can be removed. My minimal example above demonstrate just removing the field (as implicitly suggested by the warning) would lead to undefined behaviour and introduce unsoundness. Given these facts I'm in doubt that P-medium is the right choice here. Probably also I-unsound could be appropriate. (@apiraino )

I'm not saying that such a lint should not be included in rustc, but only that it should be much more precise before it's enabled by default + it should error on the side of missing a warning instead of issuing warnings on code that it cannot reason about.

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.

My main point here is that this not only happens with drop implementations but also can happen as soon a unsafe code is involved. Missing a drop implementation "only" changes the behaviour of the program, while the example above demonstrates that this lint can lead to an unsound implementation.

[Mark-Simulacrum]

Nominating for T-lang to decide what the correct behavior of this lint is. The change was made in #81473, to fix #49256 (but perhaps only partially? Unclear why it doesn't close that issue).

[estebank]

Looking at the fix PR, it seems to me that the only reason the existing code didn't trigger before was because we ignored assignments. This tells me that there might be other as of yet unreported false positives lurking in this lint. That being said, if in the next 5 weeks we can't find a way to make the lint account for unsafe usages in other items, we can always revert the PR in beta.

[tmiasko]

My main point here is that this not only happens with drop implementations but also can happen as soon a unsafe code is involved.

The drop implementation is necessary as otherwise the behaviour of both programs would be equivalent.

The same issue exists for unused variable lint, so it probably would make consistent policy decision whether a warning should be produced or not: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=a19dafb0b131fb33225ac4c4e2e15dfa (the same program with unused variable removed would have undefined behaviour).

[pnkfelix]

We discussed this at today's lang design team (T-lang)'s triage meeting.

My takeaways from the discussion there and the comment thread here follow:

T-lang thinks that the right answer here is to prepend underscore to the field name. A leading underscore does not necessarily denote "unused" or "dead code." It is a general purpose way to flag a field or variable as having some purpose, despite it not being explicitly read by expressions visible in the control flow of the program.

T-lang did briefly discuss trying to make the lint narrower in scope, i.e. trying to catch the cases where the field ends up being passed to FFI, or at least has its address taken. However, we generally felt like those could only be done on a "best effort" basis; we cannot catch all such cases, and therefore we still need to have some answer for the cases that do arise. (And that answer is in the previous bullet: use a field name with a prepended underscore.)

The real bug here, and its fix, in my personal opinion, is implied by what @weiznich noted in an earlier comment (though I admit this solution is probably not what @weiznich was asking for): The solution that that we think is the correct one (prepend an underscore) is not being suggested by the compiler. Instead, all the compiler is emitting is the following diagnostic text:

warning: field is never read: `input_binds`
  --> src/main.rs:26:5
   |
26 |     input_binds: Option<Binds>,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

So the user is being told implicitly that this is "dead_code" (even though it is not), and that could well lead them to remove this field (even though it serves an important purpose).

My own suggestion for how to resolve this: Expand the diagnostic text in some manner, perhaps the following:

   = note: If the `input_binds` field serves some purpose (e.g. it has a drop effect, 
      or is used by foreign code), then you can prepend underscore to its name:

26 |   _input_binds: Option<Binds>,

    The leading underscore is a signal to a reader that the field might not be read 
    explicitly by any of the Rust code, but still serves a purpose.

[pietroalbini]

My understanding of this comment is that the PR causing this regression was reverted on beta 1.51 but was not reverted on master, as it was assumed that #83004 would land before 1.52.0 is released. Is that correct @pnkfelix?

If that's so, we need to either merge #83004 or revert that PR again on beta.

[pnkfelix]

PR #84864 embedded 381f131 which (again) reverts PR #81473

[pietroalbini]

A revert for the PR affecting this is being included in 1.53.0 beta.

This regression is only present in 1.54.0 nightly right now (current master). If #83004 is not merged before 1.54.0 branches off to beta we'll need to keep reverting the PR causing this.

[Mark-Simulacrum]

I am inclined to revert on 1.54 (master) to avoid continuing to need to revert each cycle.

[pnkfelix]

I am inclined to revert on 1.54 (master) to avoid continuing to need to revert each cycle.

I agree with this and have filed PR #86212 to accomplish it.

[pietroalbini]

The revert has landed on all the supported branches. Closing the regression issue.