Relate existential associated types with variance Invariant

[spastorino]

Fixes #71550 #72315

r? @nikomatsakis

The test case reported in that issue now errors with the following message ...

error[E0495]: cannot infer an appropriate lifetime for lifetime parameter 'a in function call due to conflicting requirements
  --> /tmp/test.rs:25:5
   |
25 |     bad(&Bar(PhantomData), x)
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: first, the lifetime cannot outlive the lifetime `'a` as defined on the function body at 24:11...
  --> /tmp/test.rs:24:11
   |
24 | fn extend<'a, T>(x: &'a T) -> &'static T {
   |           ^^
note: ...so that reference does not outlive borrowed content
  --> /tmp/test.rs:25:28
   |
25 |     bad(&Bar(PhantomData), x)
   |                            ^
   = note: but, the lifetime must be valid for the static lifetime...
note: ...so that the types are compatible
  --> /tmp/test.rs:25:9
   |
25 |     bad(&Bar(PhantomData), x)
   |         ^^^^^^^^^^^^^^^^^
   = note: expected  `&'static T`
              found  `&T`

error: aborting due to previous error

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

I could also add that test case if we want to have a weaponized one too.

[nikomatsakis]

It seems certainly possible that this will cause regressions in people's code on crater. I wonder if a crater run is appropriate, or if we should just move forward.

@Mark-Simulacrum -- weren't you saying that the crater queue is very long at present?

[spastorino]

Just pushed some minor stderr files fixes.

[nikomatsakis]

r=me but the only question is whether we should do a crater run -- see zulip topic

[Mark-Simulacrum]

@bors try

Let's prepare a try build for a potential crater run

[bors]

⌛ Trying commit 4a376a9ca7c5c0d6f3bb12ef241ace06641963f9 with merge 96b3c169cd2d8959926fa240f10342f1659f8ae3...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: 96b3c169cd2d8959926fa240f10342f1659f8ae3 (96b3c169cd2d8959926fa240f10342f1659f8ae3)

[Mark-Simulacrum]

@craterbot check p=1

[craterbot]

👌 Experiment pr-71896 created and queued.
🤖 Automatically detected try build 96b3c169cd2d8959926fa240f10342f1659f8ae3
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-71896 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚨 Experiment pr-71896 has encountered an error: some threads returned an error
🛠️ If the error is fixed use the retry command.

🆘 Can someone from the infra team check in on this? @rust-lang/infra
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[Mark-Simulacrum]

@craterbot retry

I can't find anything in logs...

[craterbot]

🛠️ Experiment pr-71896 queued again.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-71896 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-71896 is completed!
📊 21 regressed and 1 fixed (103152 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[bjorn3]

Spurious

• Ziqul.vulkano-training.5e4a0f210c93c69cc7a6741ba09aa4c9bcadec9e
• denoland.rusty_v8.36bec8cd2ba766b9428cc0fb3f2038863e7c21ff
• electron-0.1.0
• radish-database-0.1.0
• rusty_v8-0.4.1

Regressions

• ezksd.combinator.a2989e2e8fc92c9c8b1b4c949d9d8cc0b4de1986
• githubaccount624.rocket-rooms.181ae13300244f79e7295eff84b3d07f7b28bdd5
• gribbet.rust_api_example.6c2086f619b7e3f591cae6a7aa1deed91a504824
• mich101mich.game_rs.f5e2796d3f8437e9630777e745ed7b8b2a513d30
• spacemeowx2.slp-server-rust.2aaf02c7f0a352c6c826c45b6f12f5978604b089
• sstanfield.slsh.b3068a910f91a5bd35b2e2b61a18b5ea18071785
• attr-0.1.0
• dcmdump-0.1.0
• dicom-object-0.1.0
  • dicom-0.1.0
• happy-0.1.1
• symbolic-debuginfo-7.3.0
  • getsentry.sentry-cli.121f1b75e468f21378b2993e369b762f2c05ddf1
  • mozilla.dump_syms.2603fb01a18e4e11d95b95fc08d9dacad9076f43
  • symbolic-minidump-7.3.0
  • symbolic-7.3.0

[nikomatsakis]

Fewer regressions that I feared, though more than I would like. =)

[nikomatsakis]

So I'm pondering a few things:

• We could plausibly try to accept this code.
• We could plausible try a future-compatibility warning, but I don't quite know how to do that.
• We could ping crate authors and try to land this change.

[bors]

💥 Test timed out

[nikomatsakis]

Uh...

@bors retry

[bors]

⌛ Testing commit c99164e with merge f33d195b1d8402ecdbbc7492d2b9fc61868f0c9e...

[Dylan-DPC-zz]

@bors retry yield

[bors]

⌛ Testing commit c99164e with merge 3d9aab01630687424b650880ce3cbc1338c2ce02...

[bors]

💔 Test failed - checks-azure

[nikomatsakis]

I failed somehow to find the logs in that output above... what the heck =)

[spastorino]

@bors retry

[bors]

⌛ Testing commit c99164e with merge 3ddf480...

[bors]

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing 3ddf480 to master...

[coolreader18]

Here's a piece of code that broke with this:

fn c_string_array_to_rust<'a>(
    arr: *const *const ::std::os::raw::c_char,
    count: usize,
) -> Box<dyn Iterator<Item = &'a str>> {
    //assert!(!arr.is_null());
    if arr.is_null() {
        Box::new(iter::empty::<&'a str>())
        // ~^ ERROR cannot infer an appropriate lifetime due to conflicting requirements [E0495]
    } else {
        let slice_of_pointers = unsafe { slice::from_raw_parts(arr, count) };
        Box::new(
            slice_of_pointers.iter()
                .map(|s| unsafe { CStr::from_ptr(*s) }.to_str().unwrap()),
        )
    }
}

I'm pretty sure this is sound? Since iter::empty never actually produces a value of its T

full message:

error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
  --> src/discovery.rs:76:18
   |
76 |         Box::new(iter::empty::<&'a str>())
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: first, the lifetime cannot outlive the lifetime `'a` as defined on the function body at 70:27...
  --> src/discovery.rs:70:27
   |
70 | fn c_string_array_to_rust<'a>(
   |                           ^^
note: ...so that the expression is assignable
  --> src/discovery.rs:76:18
   |
76 |         Box::new(iter::empty::<&'a str>())
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^
   = note: expected `std::iter::Empty<&str>`
              found `std::iter::Empty<&'a str>`
   = note: but, the lifetime must be valid for the static lifetime...
note: ...so that the expression is assignable
  --> src/discovery.rs:76:9
   |
76 |         Box::new(iter::empty::<&'a str>())
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: expected `std::boxed::Box<(dyn std::iter::Iterator<Item = &'a str> + 'static)>`
              found `std::boxed::Box<dyn std::iter::Iterator<Item = &'a str>>`

[kennytm]

@coolreader18 try using Box<dyn Iterator<Item = &'a str> + 'a> as the return type of c_string_array_to_rust?

[coolreader18]

I resolved it another way, just wanted to bring this to attention. Thanks for the suggestion though @kennytm, that does fix it as well.

[nikomatsakis]

Just to clarify the problem:

This is expected. The cause is the same as I explained for ezksd.combinator. In fact, i'm going to inline the hackmd for posterity's sake, since it has a lot more detailed that never made it into a github comment.

regressions for #71896

Crater report: https://crater-reports.s3.amazonaws.com/pr-71896/index.html

summary

package	sound-ish?	trait
ezksd	❌	dyn Parser<Input = X>, but interacts with outlives
rocket-rooms	✔️	response::ResultFuture<'r>
gribbet.rust_api_example	✔️	ResultFuture<'static>
mich101mich.game_rs	✔️	Iterator
spacemeowx2.slp-server-rust	✔️	dyn Stream
sstanfield.slsh	✔️	dyn Iterator
attr-1.0	🤷	unclear
dcmdump-0.1.0	🤷	unclear
dicom-object-0.1.0	🤷	unclear, same as previous?
happy-0.1.0	❌	dyn Parser<Input = X>, maybe interacts with outlives
symbolic-debuginfo-7.3.0	✔️	dyn Iterator

Key:

• ✔️ -- "straightforwardly sound"
• ❌ -- not sound
• 🤷 didn't quite figure it out yet

ezksd.combinator.a2989e2e8fc92c9c8b1b4c949d9d8cc0b4de1986

This one is tricky. The actual interaction of subtyping is unsound. The code is
relying on Box<dyn Parser<Input = &'static str>> being a subtype of Box<dyn Parser<Input = &'a str>>, which is incorrect, as Self::Input appears in a
contravariant position. In other words, effectively, given a fn that requires
'static inputs, we are "upcasting" it to a fn that takes any input.

However, the tricky bit is in the reason that a Box<dyn Parser<Input = &'static str>>
type is required in the first place. This comes about because there is a type like
so used in the flat_map combinator:

Box<dyn Parser<Input = &'a str, ...>>

The problem here is that there is an implicit bound of Box<dyn Parser<..> + 'static> going on. The underlying type in this case is something like
Empty<&'? str>, where the '? is an inference variable. Because of our
outlives rules, that '? gets inferred to 'static. The clincher is that the
Empty types does't actually contain any data, it's just holding types. We
don't really have a good way to express this today given Rust's outlives rules.

Moreover, if we used the invariant rule that this PR puts in place, but we had better
outlives rules, then the code would be accepted.

githubaccount624.rocket-rooms.181ae13300244f79e7295eff84b3d07f7b28bdd5

The actual error occurs in rocket, which is a dependency of rocket-rooms:

https://github.com/SergioBenitez/Rocket/blob/e7afd0d1f464b85392051be3a2f382185960ccb8/core/lib/src/response/responder.rs#L218

The relevant code is:

pub trait Responder<'r> {
    fn respond_to(self, request: &'r Request<'_>) -> response::ResultFuture<'r>;
}


impl Responder<'_> for String {
    fn respond_to(self, _: &Request<'_>) -> response::ResultFuture<'static> {
        Box::pin(async move {
            Response::build()
                .header(ContentType::Plain)
                .sized_body(Cursor::new(self))
                .ok()
        })
    }
}

The subtyping is occuring in a covariant position (the return type), so accepting response::ResultFuture<'static> <: response::ResultFuture<'r> would be sound if we inferred the variance instead of using Invariant.

gribbet.rust_api_example.6c2086f619b7e3f591cae6a7aa1deed91a504824

The errors are ResultFuture<'static> -- the associated type in future is an output that appears in covariant position.

mich101mich.game_rs.f5e2796d3f8437e9630777e745ed7b8b2a513d30

Box<dyn Iterator<Item = T>>, as above

spacemeowx2.slp-server-rust.2aaf02c7f0a352c6c826c45b6f12f5978604b089

Not a root regression, but related to dyn futures::Stream<Item = GraphQLResponse<'a, S>
Not a root regression, but it looks like maybe this comes from Future? Related to juniper_subscriptions

https://github.com/spacemeowx2/juniper/blob/95686b7ce530538d14bd42aafd47e07989dc9c2a/juniper_subscriptions/src/lib.rs#L149
log

sstanfield.slsh.b3068a910f91a5bd35b2e2b61a18b5ea18071785

Box<dyn Iterator<Item = &Expression>>

attr-0.1.0

Not totally sure what's going on here but I think it's related to Box<dyn Iterator<...>> and the implicit 'static bound. The error occurs in a test:

[INFO] [stderr] error[E0597]: `path` does not live long enough
[INFO] [stderr]    --> tests/mapping.rs:197:18
[INFO] [stderr]     |
[INFO] [stderr] 197 |     let result = path.traverse(&top).unwrap().map(std::result::Result::unwrap).collect::<Vec<_>>();
[INFO] [stderr]     |                  ^^^^---------------
[INFO] [stderr]     |                  |
[INFO] [stderr]     |                  borrowed value does not live long enough
[INFO] [stderr]     |                  argument requires that `path` is borrowed for `'static`
[INFO] [stderr] 198 |     assert_eq!(result, vec!["foo", "bla"]);
[INFO] [stderr] 199 | }
[INFO] [stderr]     | - `path` dropped here while still borrowed

dcmdump-0.1.0

Not very clear

[INFO] [stderr] error[E0310]: the parameter type `S` may not live long enough
[INFO] [stderr]    --> /opt/rustwide/cargo-home/registry/src/github.com-1ecc6299db9ec823/dicom-object-0.1.0/src/mem.rs:183:18
[INFO] [stderr]     |
[INFO] [stderr] 166 |     pub fn from_reader_with_dict<S>(src: S, dict: D) -> Result<Self>
[INFO] [stderr]     |                                  - help: consider adding an explicit lifetime bound...: `S: 'static`
[INFO] [stderr] ...
[INFO] [stderr] 183 |             obj: InMemDicomObject::build_object(&mut dataset, dict, false, Length::UNDEFINED)?,
[INFO] [stderr]     |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[INFO] [stderr]     |
[INFO] [stderr] note: ...so that the type `std::io::BufReader<S>` will meet its required lifetime bounds
[INFO] [stderr]    --> /opt/rustwide/cargo-home/registry/src/github.com-1ecc6299db9ec823/dicom-object-0.1.0/src/mem.rs:183:18
[INFO] [stderr]     |
[INFO] [stderr] 183 |             obj: InMemDicomObject::build_object(&mut dataset, dict, false, Length::UNDEFINED)?,
[INFO] [stderr]     |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[INFO] [stderr] 
[INFO] [stderr] error: aborting due to previous error

happy

really derives from a use of combine v2.4.0:

[INFO] [stderr] error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
[INFO] [stderr]   --> src/parse_route.rs:86:26
[INFO] [stderr]    |
[INFO] [stderr] 86 |     optional(token('/')).and(many1(segment())).map(|(_, v)| v).boxed()
[INFO] [stderr]    |                          ^^^
[INFO] [stderr]    |
[INFO] [stderr] note: first, the lifetime cannot outlive the lifetime `'a` as defined on the function body at 85:8...
[INFO] [stderr]   --> src/parse_route.rs:85:8
[INFO] [stderr]    |
[INFO] [stderr] 85 | fn url<'a>() -> Box<StrParser<'a, Vec<Segment>>> {
[INFO] [stderr]    |        ^^

type StrParser<'a, R> = combine::Parser<Output=R, Input=&'a str>;

looks unsound given Parser trait, possibly a similar pattern to the first

symbolic-debuginfo-7.3.0

 DynIterator<'_, Symbol<'_>>