Backwards-incompatible lifetime change (regression)

[staktrace]

I tried this:

rustup update nightly
git clone https://github.com/rust-lang/git2-rs && cd git2-rs
cargo +nightly build

I expected to see this happen: build completes

Instead, this happened: build failed with this output:

error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
  --> src/attr.rs:74:15
   |
74 |         match (self, other) {
   |               ^^^^^^^^^^^^^
   |
note: first, the lifetime cannot outlive the anonymous lifetime defined on the method body at 73:26...
  --> src/attr.rs:73:26
   |
73 |     fn eq(&self, other: &AttrValue<'_>) -> bool {
   |                          ^^^^^^^^^^^^^
note: ...so that the types are compatible
  --> src/attr.rs:74:15
   |
74 |         match (self, other) {
   |               ^^^^^^^^^^^^^
   = note: expected `(&AttrValue<'_>, &AttrValue<'_>)`
              found `(&AttrValue<'_>, &AttrValue<'_>)`
note: but, the lifetime must be valid for the lifetime `'_` as defined on the impl at 72:30...
  --> src/attr.rs:72:30
   |
72 | impl PartialEq for AttrValue<'_> {
   |                              ^^
note: ...so that the types are compatible
  --> src/attr.rs:79:16
   |
79 |             | (Self::Bytes(bytes), AttrValue::String(string)) => string.as_bytes() == *bytes,
   |                ^^^^^^^^^^^^^^^^^^
   = note: expected `AttrValue<'_>`
              found `AttrValue<'_>`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0495`.
error: could not compile `git2`

To learn more, run the command again with --verbose.

Meta

rustc +nightly --version --verbose:

rustc 1.54.0-nightly (5dc8789e3 2021-05-21)
binary: rustc
commit-hash: 5dc8789e300930751a78996da0fa906be5a344a2
commit-date: 2021-05-21
host: x86_64-unknown-linux-gnu
release: 1.54.0-nightly
LLVM version: 12.0.1

This was working with the rustc nightly from rustc 1.54.0-nightly (f94942d84 2021-05-19) so this is a very recent regression.

[Mark-Simulacrum]

Might be #85511, but would be good to bisect.

[ehuss]

Bisect confirmed it is #85511. This is blocking cargo's CI. Let me know if there's any way I can help.

[ehuss]

Here's a reproduction:

pub enum Foo<'string> {
    String(&'string str),
}

impl Foo<'_> {
    fn f(&self, other: &Foo<'_>) {
        match (self, other) {
            // Important: a and b swap between the two patterns, and one pattern uses `Self`
            (Foo::String(a), Foo::String(b))
            | (Self::String(b), Foo::String(a)) => {}
        }
    }
}

I couldn't get it smaller than that.

I don't know if this is helpful, but the following will ICE on stable/beta, but returns E0495 on nightly:

enum Foo<'a> {
    Var(&'a str)
}

impl Foo<'_> {
    fn f(other: Foo<'_>) {
        match other {
            Self::Var(..) => {}
        }
    }
}

[matthiaskrgr]

Note for those also affected by this:
git2 0.12 still builds so you might be able to temporarily downgrade your git2 dependency as a workaround until there is a solution.

[Mark-Simulacrum]

Cc @nikomatsakis

I'd lean towards reverting for now (and potentially landing as future compat or something). I'm not sure if this is technically a soundness fix, Niko can likely give a better reading there. I think the problem is that Self::String as a constructor is now (I believe arguably correctly, but not sure if it has any soundness implications) requiring the argument to outlive the lifetime on Self - but of course this isn't necessarily true of the other branch.

This means that changing Self to the actual name of the type will fix this, so maybe patching git2 quickly is a good idea; it'll fix things before the next nightly at least for Cargo.

[staktrace]

This means that changing Self to the actual name of the type will fix this, so maybe patching git2 quickly is a good idea; it'll fix things before the next nightly at least for Cargo.

Replacing the <'_> with <'a> in src/attr.rs in git2 seems to fix the error (with appropriate decorations on the impl)

[staktrace]

Note for those also affected by this:
git2 0.12 still builds so you might be able to temporarily downgrade your git2 dependency as a workaround until there is a solution.

You can actually just downgrade to 0.13.17 and that will work too. The src/attr.rs code is new in 0.13.18.

[Mark-Simulacrum]

I haven't been able to come up with an example that uses the lifetime mismatch to do anything bad yet, but judging by the fact that other similar code seems to demand equality, it seems likely that this is indeed a bug fix.

Filed #85584 to revert the PR on nightly for now.

[nikomatsakis]

I believe the new behavior is correct, yes. In the minimized example, the variable b is being forced to have both types. I'm not sure if one can create an unsoundness from pattern matching specifically, but @Mark-Simulacrum we already found one soundness hole that was closed by the PR.

[nikomatsakis]

Region errors like this are traditionally the sort of thing that is hard to manage with a warning period. We could potentially swing it but it'll take some effort.

[Mark-Simulacrum]

Hm. Well, so far I think the only breakage we know of is in git2 and an easy fix -- and a patch release is already out there.

Let's not land the revert for now then since we do recognize it as a soundness bug "in theory" if not necessarily in practice, and we can see what beta crater (in the next cycle, this is in nightly for now) reveals.

[arturoc]

I'm getting a similar problem on some rust code after updating stable from 1.53 to 1.54. Will try to find a small example to post but it seems related to use for<'a> ... to specify bounds on a trait implementation

[nbdd0121]

A slightly modified version of @ehuss's mcve:

pub enum Foo<'string> {
    String(&'string str),
}

impl<'a> Foo<'a> {
    fn f<'b>(&self, other: &Foo<'b>) {
        match (self, other) {
            // Important: a and b swap between the two patterns, and one pattern uses `Self`
            (Foo::String(a), Foo::String(b))
            | (Foo::<'a>::String(b), Foo::String(a)) => {}
        }
    }
}

This wouldn't compile on 1.53. The only change is to spell Self out explicitly (with Foo::<'a>). So I would say this should indeed be considered as a bugfix. I guess the issue should be closed? (at least it shouldn't be P-critical)

Also the effected version is 1.54+, so retag:
@rustbot label -regression-from-stable-to-beta +regression-from-stable-to-stable

[rustbot]

Error: Parsing relabel command in comment failed: ...'-to-beta +' | error: empty label at >| ' regressio'...

Please let @rust-lang/release know if you're having trouble with this bot.

[apiraino]

Demoting to P-high after reading the latest comments

@rustbot label -P-critical +P-high

[apiraino]

I think this issue can be closed since the release notes were amended to also include this compatibility note in commit 9fc2bfa

@rustbot label +T-release