Possible false-negative with tail-expr-drop-order

[ehuss]

There may be a false-negative with tail-expr-drop-order. Reproduction:

1. curl -L https://crates.io/api/v1/crates/faer-core/0.17.1/download | tar -xz
2. cd faer-core-0.17.1
3. Set up cargo-features and a git repo: git init . && sed -i '1i cargo-features = ["edition2024"]' Cargo.toml && git add . && git commit -m test
4. cargo fix --edition

On current nightly (2024-11-09), this generates a large number of warnings about tail-expr-drop-order. However, none of them are in the offending function swap_cols. Additionally, after #131326, there are no warnings at all.

After switching to 2024, this yields an error:

error[E0716]: temporary value dropped while borrowed
     --> src/lib.rs:10425:69
      |
10425 |           $crate::zip::ZipEq::new($crate::zip::ViewMut::view_mut(&mut { $head }), $crate::zipped!($($tail,)*))
      |                                                                       ^^^^^^^^^                              - temporary value is freed at the end of this statement
      |                                                                       |
      |                                                                       creates a temporary value which is freed while still in use
      |
     ::: src/permutation.rs:304:9
      |
304   | /         zipped!(
305   | |             mat_a.const_cast().as_2d_mut(),
306   | |             mat_b.const_cast().as_2d_mut(),
307   | |         )
      | |_________- in this macro invocation
308   |       }
309   |       .for_each(|unzipped!(mut a, mut b)| {
      |        -------- borrow later used by call
      |
      = note: consider using a `let` binding to create a longer lived value
      = note: this error originates in the macro `zipped` (in Nightly builds, run with -Z macro-backtrace for more info)

Unfortunately that crate is a maze of types and generics, so it is a bit difficult to minimize.

I'm just assuming this is a tail expression drop change due to the shape of the error.

It does not seem related to the macro, since manually expanding it also does not change anything.

I would expect tail-expr-drop-order to at least warn something here.

cc @dingxiangfei2009

Meta

rustc --version --verbose:

rustc 1.84.0-nightly (59cec72a5 2024-11-08)
binary: rustc
commit-hash: 59cec72a57af178767a7b8e7f624b06cc50f1087
commit-date: 2024-11-08
host: aarch64-unknown-linux-gnu
release: 1.84.0-nightly
LLVM version: 19.1.3

[ehuss]

fltk@1.4.35 exhibits similar behavior. This function:

pub fn event_dx() -> MouseWheel {
    match 0.cmp(unsafe { &fl::Fl_event_dx() }) {
        cmp::Ordering::Greater => MouseWheel::Right,
        cmp::Ordering::Equal => MouseWheel::None,
        cmp::Ordering::Less => MouseWheel::Left,
    }
}

does not generate any warnings. However, after migrating there is an error:

error[E0716]: temporary value dropped while borrowed
   --> src/app/event.rs:127:27
    |
127 |     match 0.cmp(unsafe { &fl::Fl_event_dx() }) {
    |             ---           ^^^^^^^^^^^^^^^^-
    |             |             |               |
    |             |             |               temporary value is freed at the end of this statement
    |             |             creates a temporary value which is freed while still in use
    |             borrow later used by call
    |
    = note: consider using a `let` binding to create a longer lived value

error[E0716]: temporary value dropped while borrowed
   --> src/app/event.rs:138:27
    |
138 |     match 0.cmp(unsafe { &fl::Fl_event_dy() }) {
    |             ---           ^^^^^^^^^^^^^^^^-
    |             |             |               |
    |             |             |               temporary value is freed at the end of this statement
    |             |             creates a temporary value which is freed while still in use
    |             borrow later used by call
    |
    = note: consider using a `let` binding to create a longer lived value

For more information about this error, try `rustc --explain E0716`.

[dingxiangfei2009]

I agree the explanation is rather weak without a focus on the edition change. Technically tail-expr-drop-order does not lint against this case, but rather capture implicit changes in drop order that does not fail the borrow checker.

I am looking into improving the diagnostic here.

@rustbot claim

[ehuss]

Another example is gix@0.67.0:

    pub fn is_active(&self) -> Result<bool, is_active::Error> {
        let (mut platform, mut attributes) = self.state.active_state_mut()?;
        let is_active = platform.is_active(&self.state.repo.config.resolved, self.name.as_ref(), {
            &mut |relative_path, case, is_dir, out| {
                attributes
                    .set_case(case)
                    .at_entry(relative_path, Some(is_dir_to_mode(is_dir)), &self.state.repo.objects)
                    .map_or(false, |platform| platform.matching_attributes(out))
            }
        })?;
        Ok(is_active)
    }

doesn't generate warnings, but fails in 2024:

error[E0716]: temporary value dropped while borrowed
   --> src/submodule/mod.rs:154:18
    |
154 |                &mut |relative_path, case, is_dir, out| {
    |   _____________-    ^
    |  |__________________|
155 | ||                 attributes
156 | ||                     .set_case(case)
157 | ||                     .at_entry(relative_path, Some(is_dir_to_mode(is_dir)), &self.state.repo.objects)
158 | ||                     .map_or(false, |platform| platform.matching_attributes(out))
159 | ||             }
    | ||             ^
    | ||             |
    | ||_____________temporary value is freed at the end of this statement
    |  |_____________creates a temporary value which is freed while still in use
    |                borrow later used here
    |
    = note: consider using a `let` binding to create a longer lived value

[ehuss]

Additional reports are probably not helpful, but here are some anyway:

glib@0.25.0:
https://crater-reports.s3.amazonaws.com/pr-132712/try%23da25749bf5e6ba5ed862ff361c19afff2a986b2d/reg/glib-0.20.5/log.txt

gstreamer@0.23.3
https://crater-reports.s3.amazonaws.com/pr-132712/try%23da25749bf5e6ba5ed862ff361c19afff2a986b2d/reg/gstreamer-0.23.3/log.txt

half@2.4.1
https://crater-reports.s3.amazonaws.com/pr-132712/try%23da25749bf5e6ba5ed862ff361c19afff2a986b2d/reg/half-2.4.1/log.txt

hongfuzz@0.5.56
https://crater-reports.s3.amazonaws.com/pr-132712/try%23da25749bf5e6ba5ed862ff361c19afff2a986b2d/reg/honggfuzz-0.5.56/log.txt

ibc-relayer-types@0.29.3
https://crater-reports.s3.amazonaws.com/pr-132712/try%23da25749bf5e6ba5ed862ff361c19afff2a986b2d/reg/ibc-relayer-types-0.29.3/log.txt

imageproc@0.25.0
https://crater-reports.s3.amazonaws.com/pr-132712/try%23da25749bf5e6ba5ed862ff361c19afff2a986b2d/reg/imageproc-0.25.0/log.txt

inferno@0.11.21
https://crater-reports.s3.amazonaws.com/pr-132712/try%23da25749bf5e6ba5ed862ff361c19afff2a986b2d/reg/inferno-0.11.21/log.txt

io-extras@0.18.3
https://crater-reports.s3.amazonaws.com/pr-132712/try%23da25749bf5e6ba5ed862ff361c19afff2a986b2d/reg/io-extras-0.18.3/log.txt

minijinja@2.4.0
https://crater-reports.s3.amazonaws.com/pr-132712/try%23da25749bf5e6ba5ed862ff361c19afff2a986b2d/reg/minijinja-2.4.0/log.txt

safecoin-sdk@1.14.17
https://crater-reports.s3.amazonaws.com/pr-132712/try%23da25749bf5e6ba5ed862ff361c19afff2a986b2d/reg/safecoin-sdk-1.14.17/log.txt

shuttle@0.8.0
https://crater-reports.s3.amazonaws.com/pr-132712/try%23da25749bf5e6ba5ed862ff361c19afff2a986b2d/reg/shuttle-0.8.0/log.txt

[dingxiangfei2009]

@ehuss Thanks for the data! I will use it for writing another machine applicable suggestion in the borrowck.

[ehuss]

This example from the RFC is another example:

#![warn(rust_2024_compatibility)]
fn main() {
    let zero = { String::new().as_str() }.len();
}

[ehuss]

@dingxiangfei2009 One thing I wanted to mention is that the use of MIR (here) for generating the lint is incompatible with the way cargo fix --edition works. It does the equivalent of cargo check which is --emit=metadata which circumvents all the codegen side of things including this code path.

For example:

#![deny(tail_expr_drop_order)]

struct PrintOnDrop(&'static str);
impl Drop for PrintOnDrop {
    fn drop(&mut self) {
        println!("{}", self.0);
    }
}

impl PrintOnDrop {
    fn get(&self) -> Option<u8> {
        None
    }
}

fn foo() -> Option<u8> {
    let x = PrintOnDrop("x");
    PrintOnDrop("temp").get()
}

This does not fire the lint with cargo check.

[compiler-errors]

This example from the RFC is another example:

#![warn(rust_2024_compatibility)]
fn main() {
    let zero = { String::new().as_str() }.len();
}

@ehuss @dingxiangfei2009:

The reason why this lint doesn't fire is because the current logic explicitly ignores things with "insignificant dtors". That is, String wraps Vec<u8> which is marked with #[rustc_insignificant_dtor].

This is because originally this lint was intended to lint in cases where we were shortening the lifetime of temporaries such as mutex guards, which would silently fail and possibly cause things like races or UB.

We probably should think if there's a way to detect when we're leaking references to things even if they have insignificant dtors, since that results in the class of borrowck errors you see in that example. That will likely require a significant reworking of the lint reporting code, though :/

[dingxiangfei2009]

cc @nikomatsakis

I still have a draft to detect the false positive cases, in other words to detect the potential borrowck error in the new edition. Let me get it posted in a few hours.

[compiler-errors]

Also for the record, #134493 should fix the --emit metadata/cargo check problem.