Prevent opaque types being instantiated twice with different regions within the same function #116935
Conversation
rustbot
added
S-waiting-on-review
tests/ui/impl-trait/issue-86465.rs
Outdated
There was a problem hiding this comment.
This test was exactly duplicated with fn foo in tests/ui/type-alias-impl-trait/multiple-def-uses-in-one-fn-lifetimes.rs
|
This should probably work. Can you check that? type Tait<'x> = impl Sized;
fn define<'a: 'b, 'b: 'a>(x: &'a u8, y: &'b u8) -> (Tait<'a>, Tait<'b>) {
((), ())
} |
|
This technically could break RPIT code, so I'd feel comfortable only after a crater run. // doesnt work
// (bounds there to make params early-bound)
fn rpit<'a: 'a, 'b: 'b>() -> impl Sized {
let () = rpit::<'b, 'a>();
}
// should work
fn rpit<'a: 'b, 'b: 'a>() -> impl Sized {
let () = rpit::<'b, 'a>();
}@bors try |
compiler-errors
added
T-types
…same_sig, r=<try> Prevent opaque types being instantiated twice with different regions within the same function addresses https://github.com/orgs/rust-lang/projects/22/views/1?pane=issue&itemId=41329537 r? `@compiler-errors`
|
☀️ Try build successful - checks-actions |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
Almost everything in the root regressions list is spurious disk failures (and the rest is unrelated ICEs, possibly due to disk space issues), though had me stumped for a moment until I realized that |
|
I'm gonna block this on T-lang approval of the new Min-TAIT plan. Otherwise implementation looks great to me. @rustbot blocked |
rustbot
added
S-blocked
traviscross
added
the
T-lang
|
@rustbot labels +I-lang-nominated Nominating for T-lang to decide whether to adopt the following rule for Rust:
This is called the "once modulo regions restriction". This rule was described and proposed in the 2023-11-08 T-lang Mini-TAIT design meeting (minutes): rust-lang/lang-team#233. For a full discussion of this question, see: The effect of this rule would be to reject the following RPIT code which is currently accepted: pub trait Trait<'x> {}
impl<'x, T> Trait<'x> for T {}
pub fn test<'a, 'b>(a: &'a (), b: &'b ()) -> impl Trait<'a> {
// ~^ ERROR lifetime may not live long enough
// ~| NOTE argument requires that `'b` must outlive `'a`
if false {
let _ = test(b, b);
}
a
}And to reject similar code under TAIT: #![feature(type_alias_impl_trait)]
type Tait<'x> = impl Sized;
fn test<'a, 'b>(a: &'a (), b: &'b ()) -> Tait<'a> {
// ~^ ERROR lifetime may not live long enough
// ~| NOTE requires that `'a` must outlive `'b`
// ~| NOTE requires that `'b` must outlive `'a`
if false {
let _: Tait<'b> = b;
}
a
}And to also reject, e.g.: #![feature(type_alias_impl_trait)]
type Tait<'x> = impl Sized;
fn test<'a, 'b>(a: &'a (), b: &'b ()) -> (Tait<'a>, Tait<'b>) {
// ~^ ERROR lifetime may not live long enough
// ~| NOTE requires that `'a` must outlive `'b`
// ~| NOTE requires that `'b` must outlive `'a`
(a, b)
} |
rustbot
added
the
I-lang-nominated
traviscross
added
A-impl-trait
|
This proposal worries me. It would reject code that:
Perhaps I just need to learn more about the internals of the new solver to understand the motivation. But from an outside perspective, this does not inspire confidence. |
|
Let me break down this list:
There's lots of sound code that doesn't work in the old trait solver or the new trait solver. Especially when it comes to things that are equal modulo regions, we already have a lot of limitations when it comes to the trait solver distinguishing choices (e.g.
I haven't seen any examples of code that rely on this property. I agree that it's plausible, but it's clear that our guestimates of the probability of this being an issue that is hit in practice differ. My understanding from discussing this briefly with @oli-obk is that he only wrote the tests that exercise this behavior because they could work, not because they were motivated by any real-life examples (though I may be misremembering, so any paraphrasing is my own).
Again, lots of things that intuitively should work don't due to real, concrete limitations to the compiler, especially ones that enable other code to work, and for the trait solver to be sound. I go into more detail below, but this limitation exists to make space for lazy normalization and region uniquification.
You can use type or const parameters to work around this, but again, a fix would likely be motivated by first understanding what is trying to be achieved at a high level.
The purpose of this limitation being put in place is precisely to level out the differences between the new trait solver and the old trait solver. See below.
We are not technically committing to this behavior until the new trait solver lands, but yes, I don't think there's a way to make this work soundly in a language that doesn't do borrow-checking in tandem with type-checking. Hopefully at the point of stabilization of the new solver, we will be able to more precisely motivate the reasons why the trait solver requires this to work the way it does. but TL;DR is "lazy normalization" and "region uniquification". The former is a property that we need to have to make the trait solver sound, and the latter is one that gives us better caching and is a requirement to make the trait solver work properly between typeck and borrowck. The side-effect of this limitation is that we can treat defining instantiations of TAITs as being defined anywhere in the body. For example (https://godbolt.org/z/v9GjnP769) (though don't go trying it with region args yet, that still fails due to bugs with aliases in the new trait solver). |
lcnr
removed
the
T-bootstrap
|
While the concern of higher-ranked-lifetimes is still applicable and may block the new capture rules (#117587), I don't see a reason for it to block ATPIT stabilization since we have a hard error now when opaque types mention higher ranked regions (#121386) and I believe that's forward compatible with any future decision. For this reason I am going to resolve my concern here and mention it as a blocker in #117587. Thanks to @traviscross for bringing this to my attention. |
rfcbot
added
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rustbot
removed
the
I-lang-nominated
oli-obk
removed
the
S-blocked
rfcbot
added
finished-final-comment-period
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
oli-obk
added
the
S-waiting-on-review
…within the same function
oli-obk
force-pushed
the
different_lifetime_taits_in_same_sig
branch
from
3ef7f68
to
be9317d
Compare
rustbot
added
the
T-bootstrap
|
@bors r+ rollup=never (bisectability) |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (a7e4de1): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 670.459s -> 668.419s (-0.30%) |
addresses https://github.com/orgs/rust-lang/projects/22/views/1?pane=issue&itemId=41329537
r? @compiler-errors