Borrow checking for static methods became more strict

[Andrepuel]

Code

I tried this code:

pub struct Bigger<'a> {
    _marker: &'a (),
}
impl<'a> Bigger<'a> {
    pub fn get_addr(byte_list: &'a mut Vec<u8>) -> &mut u8 {
        byte_list.iter_mut().find_map(|item| {
            Self::other(item);
            Some(())
        });

        byte_list.push(0);
        byte_list.last_mut().unwrap()
    }

    pub fn other<'b: 'a>(_value: &'b mut u8) {
        todo!()
    }
}

I expected to see this happen: I don't know what should be the behavior. Looks like it all boils down to what is going to be 'a within Bigger::<'a>::other. In stable version of Rust, the code was accepted.

Instead, this happened: Using nightly version of the compiler a borrow checker error happened.

Version it worked on

It most recently worked on: cargo 1.63.0 (fd9c4297c 2022-07-01) / rustc 1.63.0 (4b91a6e 2022-08-08)

Version with regression

rustc --version --verbose:

rustc 1.65.0-nightly (9c20b2a8c 2022-08-17)
binary: rustc
commit-hash: 9c20b2a8cc7588decb6de25ac6a7912dcef24d65
commit-date: 2022-08-17
host: x86_64-unknown-linux-gnu
release: 1.65.0-nightly
LLVM version: 15.0.0

[5225225]

searched nightlies: from nightly-2022-01-01 to nightly-2022-08-16
regressed nightly: nightly-2022-08-05
searched commit range: 1b57946...f6f9d5e
regressed commit: caee496

bisected with cargo-bisect-rustc v0.6.4

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc 2022-01-01 --preserve -- build 

Probably #98835 ?

[steffahn]

Yes, that’s definitely #98835, but an interesting new case AFAICT.

Edit: On second thought, it’s slightly less interesting/new than I initially thought. See this comment.

[steffahn]

So for some context/explanation, the compiler used to (arguably somewhat “incorrectly”) act more like as if the closure was

        byte_list.iter_mut().find_map(|item| {
            Bigger::<'_>::other(item);
            Some(())
        });

instead of

        byte_list.iter_mut().find_map(|item| {
            Bigger::<'a>::other(item); // <- expansion of the `Self::other`
            Some(())
        });

and hence accepted item to be shorter-lived than 'a.

---

To fix the code, you’ll just need to replace Self::other by Bigger::other to remove that overly-specific and constraining lifetime 'a.

[Andrepuel]

So for some context/explanation, the compiler used to (arguably somewhat “incorrectly”) act more like as if the closure was

In fact, I am glad that the new version of the compiler flagged that error. That helped me into fixing an unnecessary overly-specific lifetime constraint that slipped through the code review process.

I've reported this as the regression worried me.

[steffahn]

I've reported this as the regression worried me.

Rightfully so, better be safe than sorry.

In fact, I am glad that the new version of the compiler flagged that error.

FYI, I don’t think removing the compiler error entirely will happen, so even if this kind of regression is addressed, the usefulness of the compiler will not be reduced; as you noted it’s a useful error message, (and also the new check is there to prevent lots of new cases that would otherwise compiler successfully with the newly enabled full NLL-only borrow checking). AFAICT, the only thing still up to discussion is whether or not perhaps the new error should initially only be a lint. Possibly deny-by-default (so it’ll look a lot like an error, but not break downstream code depending on broken crates), and most definitely a future-incompatibility lint, so you (or your dependencies) cannot accidentally (i.e. without noticing it) write new code with such lifetime-annotation-inaccuracies that might be broken later if we do make it a hard error later. (Perhaps also with improved diagnostics to help fixing the issue.)

[aliemjay]

error output under #100976

  --> /tmp/tto6.rs:15:9
   |
8  | impl<'a> Bigger<'a> {
   |      -- lifetime `'a` defined here
9  |     pub fn get_addr(byte_list: &'a mut Vec<u8>) -> &mut u8 {
10 |         byte_list.iter_mut().find_map(|item| {
   |         -------------------- first mutable borrow occurs here
11 |             Self::other(item); // replace with `Bigger`
   |             ----------- type annotation requires that `*byte_list` is borrowed for `'a`
...
15 |         byte_list.push(0);
   |         ^^^^^^^^^^^^^^^^^ second mutable borrow occurs here
   |
help: you can remove unnecessary lifetime annotations here
  --> /tmp/tto6.rs:11:13
   |
11 |             Self::other(item); // replace with `Bigger`
   |             ^^^^^^^^^^^
   = help: consider replacing `Self` with `Bigger`

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

[pnkfelix]

Visited during P-high 2022 Q3 review.

We agreed that this regression was expected, and the answer here is improving the diagnostics.

Also, we note that this should be resolved when PR #100976 lands.

We're lowering the priority from P-high to P-medium.

@rustbot label: -P-high +P-medium

[pnkfelix]

Also reassigning to T-types

@rustbot label: -T-compiler +T-types