Anonymous bound region BrAnon(0) in return but not args #47511
Comments
dtolnay
added
I-ICE
|
cc @nikomatsakis Looks like |
|
triage: P-high |
|
rust/src/librustc_typeck/astconv.rs Line 1207 in ab7abfc Is changing My understanding is that |
|
OK, looked a bit closer. I think the code is correct, but the I thought this was impossible because the elision code (which kicks in in such a case) ought to have detected that and repored an error. For example, But I overlooked the fact that there could be a lifetime which appeared in the parameters but was not constrained by the parameters. In particular, type and lifetime inputs to an associated type are not constrained (which is of course what we have here, though the use of a type-alias kind of hides it). So I think we basically just want to tweak the error message code in this case. |
|
Hmm, actually, while we can simply fix the error message, I'm now second guessing if this is the right place to detect an error, or if this should be an error at all. We could certainly handle it (alternatively) by treating the anonymous lifetime as an early-bound region. |
|
Well, I opened a PR to fix the ICE, anyway. |
|
Regarding the regression, it was caused by this bug fix #33685, for which we already underwent the warning cycle etc. I'm not sure why however things were working until 1.21. Seems like a bug. |
|
The bug was fixed by @petrochenkov in 80cf3f9, if I'm not mistaken. This moved a check that previously appears to only have been in place for |
This bug prevented me from implementing SCARY assignments for
In the next breaking release of Syn I plan to expose simply A proof of concept works great for use std::marker::PhantomData;
pub struct Punctuated<T, P> {
t: PhantomData<T>,
p: PhantomData<P>,
}
impl<T, P> IntoIterator for Punctuated<T, P> {
type Item = T;
type IntoIter = IntoIter<T, P>;
fn into_iter(self) -> Self::IntoIter {
unimplemented!()
}
}
pub type IntoIter<T, P = ()> = <Punctuated<T, P> as private::IntoIterator>::IntoIter;
mod private {
use super::Punctuated;
use std::marker::PhantomData;
pub trait IntoIterator {
type IntoIter;
}
impl<T, P> IntoIterator for Punctuated<T, P> {
type IntoIter = IntoIter<T>;
}
pub struct IntoIter<T> {
t: PhantomData<T>,
}
impl<T> Iterator for IntoIter<T> {
type Item = T;
fn next(&mut self) -> Option<Self::Item> {
unimplemented!()
}
}
}
struct A;
struct B;
struct C;
fn f(arg: IntoIter<A, B>) -> IntoIter<A, C> {
// SCARY assignment
arg
}But it falls apart if we attempt exactly the same thing for use std::marker::PhantomData;
pub struct Punctuated<T, P> {
t: PhantomData<T>,
p: PhantomData<P>,
}
impl<'a, T, P> IntoIterator for &'a Punctuated<T, P> {
type Item = T;
type IntoIter = Iter<'a, T, P>;
fn into_iter(self) -> Self::IntoIter {
unimplemented!()
}
}
pub type Iter<'a, T, P = ()> = <&'a Punctuated<T, P> as private::IntoIterator>::IntoIter;
mod private {
use super::Punctuated;
use std::marker::PhantomData;
pub trait IntoIterator {
type IntoIter;
}
impl<'a, T, P> IntoIterator for &'a Punctuated<T, P> {
type IntoIter = Iter<'a, T>;
}
pub struct Iter<'a, T: 'a> {
t: PhantomData<&'a T>,
}
impl<'a, T> Iterator for Iter<'a, T> {
type Item = &'a T;
fn next(&mut self) -> Option<Self::Item> {
unimplemented!()
}
}
}
struct A;
struct B;
struct C;
fn f(arg: Iter<A, B>) -> Iter<A, C> {
// SCARY assignment
arg
} |
Hmm, so, I had in mind issuing a future compatibility warning, which means this code would break eventually. However, an alternative, with perhaps a bit more effort, might be to promote |
|
I guess I am unclear where this is going wrong. The second code sample does not immediately look like #33685 to me because every lifetime in the code also appears in the trait input type.
Is there an easy way to sum up the rules for whether a type alias with generic parameters is valid and behaves in a sensible way? Does every lifetime parameter and type parameter need to be mentioned in the Associated type? type Iter<'a, T, P> = <&'a Punctuated<T, P> as Trait>::Associated;If it is possible to relax the restrictions by promoting |
The point is that the type of the argument is not the trait inputs, it's the projection. i.e., the type is Still, as I said, the ordinary thing for us to do is to make lifetimes that meet this criteria early-bound. For example, if you do |
|
I'll try to prep a full fix, I think, @dtolnay's example convinces me that this is a bug. |
|
Huh. It appears that the code is trying to do the right thing here, actually. The problem is that the |
|
(Or, better, make some kind of query that we can use for a def-id to tell which of its substs are constrained; for a type definition, this would be all of them, but for a type alias it would check the definition.) |
|
Wait, why would |
|
Ugh. Solving this properly is a horrible pain in the neck.
Yes it is.
Maybe, I'm going to investigate that tomorrow I suppose, because unfolding the type aliases basically amounts to a specialized re-implementation of lifetime resolution (perhaps factoring out into the "map lifetime name to def-id" part that we've talked about would help here, not sure, but that's its own job). |
|
However, deferring that would be also quite annoying. It's very nice the way it is now =) |
|
Specifically, the problem is this code: rust/src/librustc/middle/resolve_lifetime.rs Lines 2284 to 2291 in def3269 which treats all type parameters to something like |
|
triage: P-medium This is not a burning problem right now, but it's a definite bug. I'm thinking about trying to mentor out (or do) some refactorings that would make a proper fix more feasible. |
|
Current output no longer ICEs but is still rejected: |
estebank
removed
the
I-ICE
|
Is this issue the reason why in this playground, line 45 compiles but line 52 produces an error? |
|
Hi! impl ValueOperationWithGat for Box<dyn for<'e> Fn(&'e ExecutionContext<'e>) -> Value<'e>> {
#[inline]
// the 'e: 'e works around a compiler bug
fn execute<'e: 'e>(&self, ctx: ExecutionContextOf<'e, Self>) -> Value<'e> {
(self)(ctx)
}
}We believe it's another instance of this bug! If you just introduce the Is there a path forward to fix it? |
…r=jackh726 Add known-bug directive to issue rust-lang#47511 test case
…r=jackh726 Add known-bug directive to issue rust-lang#47511 test case
Rollup of 6 pull requests Successful merges: - rust-lang#94446 (UNIX `remove_dir_all()`: Try recursing first on the slow path) - rust-lang#94460 (Reenable generator drop tracking tests and fix mutation handling) - rust-lang#94620 (Edit docs on consistency of `PartialOrd` and `PartialEq`) - rust-lang#94624 (Downgrade `#[test]` on macro call to warning) - rust-lang#94626 (Add known-bug directive to issue rust-lang#47511 test case) - rust-lang#94631 (Fix typo in c-variadic) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
jackh726
added
the
S-bug-has-test
…d, r=nikomatsakis avoid making substs of type aliases late bound when used as fn args fixes rust-lang#47511 fixes rust-lang#85533 (although I did not know theses issues existed when i was working on this 🙃) currently `Alias<...>` is treated the same as `Struct<...>` when deciding if generics should be late bound or early bound but this is not correct as `Alias` might normalize to a projection which does not constrain the generics. I think this needs more tests before merging more explanation of PR [here](https://hackmd.io/v44a-QVjTIqqhK9uretyQg?view) Hackmd inline for future readers: --- This assumes reader is familiar with the concept of early/late bound lifetimes. There's a section on rustc-dev-guide if not (although i think some details are a bit out of date) ## problem & background Not all lifetimes on a fn can be late bound: ```rust fn foo<'a>() -> &'a (); impl<'a> Fn<()> for FooFnDef { type Output = &'a (); // uh oh unconstrained lifetime } ``` so we make make them early bound ```rust fn foo<'a>() -> &'a (); impl<'a> Fn<()> for FooFnDef<'a> {// wow look at all that lifetimey type Output = &'a (); } ``` (Closures have the same constraint however it is not enforced leading to soundness bugs, [rust-lang#84385](rust-lang#84385) implements this "downgrading late bound to early bound" for closures) lifetimes on fn items are only late bound when they are "constrained" by the fn args: ```rust fn foo<'a>(_: &'a ()) -> &'a (); // late bound, not present on `FooFnItem` // vv impl<'a> Trait<(&'a (),)> for FooFnItem { type Output = &'a (); } // projections do not constrain inputs fn bar<'a, T: Trait>(_: <T as Trait<'a>>::Assoc) -> &'a (); // early bound // vv impl<'a, T: Trait> Fn<(<T as Trait<'a>>::Assoc,)> for BarFnItem<'a, T> { type Output = &'a (); } ``` current logic for determining if inputs "constrain" a lifetime works off of HIR so does not normalize aliases. It also assumes that any path with no self type constrains all its substs (i.e. `Foo<'a, u32>` has no self type but `T::Assoc` does). This falls apart for top level type aliases (see linked issues): ```rust type Alias<'a, T> = <T as Trait<'a>>::Assoc; // wow look its a path with no self type uwu // i bet that constrains `'a` so it should be latebound // vvvvvvvvvvv fn foo<'a, T: Trait>(_: Alias<'a, T>) -> &'a (); // `Alias` normalized to make things clearer // vvvvvvvvvvvvvvvvvvvvvvv impl<'a, T: Trait> Fn<(<T as Trait<'a>>::Assoc,)> for FooFnDef<T> { type Output = &'a (); // oh no `'a` isnt constrained wah wah waaaah *trumbone noises* // i think, idk what musical instrument that is } ``` ## solution The PR solves this by having the hir visitor that checks for lifetimes in constraining uses check if the path is a `DefKind::Alias`. If it is we ""normalize"" it by calling `type_of` and walking the returned type. This is a bit hacky as it requires a mapping between the substs on the path in hir, and the generics of the `type Alias<...>` which is on the ty layer. Alternative solutions may involve calculating the "late boundness" of lifetimes after/during astconv rather than relying on hir at all. We already have code to determine whether a lifetime SHOULD be late bound or not as this is currently how the error for `fn foo<'a, T: Trait>(_: Alias<'a, T>) -> &'a ();` gets emitted. It is probably not possible to do this right now, late boundness is used by `generics_of` and `gather_explicit_predicates_of` as we currently do not put late bound lifetimes in `Generics`. Although this seems sus to me as the long term goal is to make all generics late bound which would result in `generics_of(function)` being empty? [rust-lang#103448](rust-lang#103448) places all lifetimes in `Generics` regardless of late boundness so that may be a good step towards making this possible.
…d, r=nikomatsakis avoid making substs of type aliases late bound when used as fn args fixes rust-lang#47511 fixes rust-lang#85533 (although I did not know theses issues existed when i was working on this 🙃) currently `Alias<...>` is treated the same as `Struct<...>` when deciding if generics should be late bound or early bound but this is not correct as `Alias` might normalize to a projection which does not constrain the generics. I think this needs more tests before merging more explanation of PR [here](https://hackmd.io/v44a-QVjTIqqhK9uretyQg?view) Hackmd inline for future readers: --- This assumes reader is familiar with the concept of early/late bound lifetimes. There's a section on rustc-dev-guide if not (although i think some details are a bit out of date) ## problem & background Not all lifetimes on a fn can be late bound: ```rust fn foo<'a>() -> &'a (); impl<'a> Fn<()> for FooFnDef { type Output = &'a (); // uh oh unconstrained lifetime } ``` so we make make them early bound ```rust fn foo<'a>() -> &'a (); impl<'a> Fn<()> for FooFnDef<'a> {// wow look at all that lifetimey type Output = &'a (); } ``` (Closures have the same constraint however it is not enforced leading to soundness bugs, [rust-lang#84385](rust-lang#84385) implements this "downgrading late bound to early bound" for closures) lifetimes on fn items are only late bound when they are "constrained" by the fn args: ```rust fn foo<'a>(_: &'a ()) -> &'a (); // late bound, not present on `FooFnItem` // vv impl<'a> Trait<(&'a (),)> for FooFnItem { type Output = &'a (); } // projections do not constrain inputs fn bar<'a, T: Trait>(_: <T as Trait<'a>>::Assoc) -> &'a (); // early bound // vv impl<'a, T: Trait> Fn<(<T as Trait<'a>>::Assoc,)> for BarFnItem<'a, T> { type Output = &'a (); } ``` current logic for determining if inputs "constrain" a lifetime works off of HIR so does not normalize aliases. It also assumes that any path with no self type constrains all its substs (i.e. `Foo<'a, u32>` has no self type but `T::Assoc` does). This falls apart for top level type aliases (see linked issues): ```rust type Alias<'a, T> = <T as Trait<'a>>::Assoc; // wow look its a path with no self type uwu // i bet that constrains `'a` so it should be latebound // vvvvvvvvvvv fn foo<'a, T: Trait>(_: Alias<'a, T>) -> &'a (); // `Alias` normalized to make things clearer // vvvvvvvvvvvvvvvvvvvvvvv impl<'a, T: Trait> Fn<(<T as Trait<'a>>::Assoc,)> for FooFnDef<T> { type Output = &'a (); // oh no `'a` isnt constrained wah wah waaaah *trumbone noises* // i think, idk what musical instrument that is } ``` ## solution The PR solves this by having the hir visitor that checks for lifetimes in constraining uses check if the path is a `DefKind::Alias`. If it is we ""normalize"" it by calling `type_of` and walking the returned type. This is a bit hacky as it requires a mapping between the substs on the path in hir, and the generics of the `type Alias<...>` which is on the ty layer. Alternative solutions may involve calculating the "late boundness" of lifetimes after/during astconv rather than relying on hir at all. We already have code to determine whether a lifetime SHOULD be late bound or not as this is currently how the error for `fn foo<'a, T: Trait>(_: Alias<'a, T>) -> &'a ();` gets emitted. It is probably not possible to do this right now, late boundness is used by `generics_of` and `gather_explicit_predicates_of` as we currently do not put late bound lifetimes in `Generics`. Although this seems sus to me as the long term goal is to make all generics late bound which would result in `generics_of(function)` being empty? [rust-lang#103448](rust-lang#103448) places all lifetimes in `Generics` regardless of late boundness so that may be a good step towards making this possible.
Looks just like #43567. Mentioning @petrochenkov and @eddyb who were involved in that issue. The following code compiles with 1.21.0 but ICE as of 1.22.0.
The text was updated successfully, but these errors were encountered: