-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix cycle error with existential types #62423
Changes from all commits
8ba9b10
ec62699
2f05160
2ab8d61
3600832
66b2b97
2f41962
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1268,15 +1268,43 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { | |
let opaque_defn_ty = tcx.type_of(opaque_def_id); | ||
let opaque_defn_ty = opaque_defn_ty.subst(tcx, opaque_decl.substs); | ||
let opaque_defn_ty = renumber::renumber_regions(infcx, &opaque_defn_ty); | ||
let concrete_is_opaque = infcx | ||
.resolve_vars_if_possible(&opaque_decl.concrete_ty).is_impl_trait(); | ||
|
||
debug!( | ||
"eq_opaque_type_and_type: concrete_ty={:?}={:?} opaque_defn_ty={:?}", | ||
"eq_opaque_type_and_type: concrete_ty={:?}={:?} opaque_defn_ty={:?} \ | ||
concrete_is_opaque={}", | ||
opaque_decl.concrete_ty, | ||
infcx.resolve_vars_if_possible(&opaque_decl.concrete_ty), | ||
opaque_defn_ty | ||
opaque_defn_ty, | ||
concrete_is_opaque | ||
); | ||
obligations.add(infcx | ||
.at(&ObligationCause::dummy(), param_env) | ||
.eq(opaque_decl.concrete_ty, opaque_defn_ty)?); | ||
|
||
// concrete_is_opaque is `true` when we're using an existential | ||
// type without 'revealing' it. For example, code like this: | ||
// | ||
// existential type Foo: Debug; | ||
// fn foo1() -> Foo { ... } | ||
// fn foo2() -> Foo { foo1() } | ||
// | ||
// In `foo2`, we're not revealing the type of `Foo` - we're | ||
// just treating it as the opaque type. | ||
// | ||
// When this occurs, we do *not* want to try to equate | ||
// the concrete type with the underlying defining type | ||
// of the existential type - this will always fail, since | ||
// the defining type of an existential type is always | ||
// some other type (e.g. not itself) | ||
// Essentially, none of the normal obligations apply here - | ||
// we're just passing around some unknown opaque type, | ||
// without actually looking at the underlying type it | ||
// gets 'revealed' into | ||
|
||
if !concrete_is_opaque { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the concrete type is opaque, don't we need an additional check to figure out if the opaque types are equal? Essentially what I'm asking is whether pub trait MyTrait {}
#[derive(Debug)]
pub struct MyStruct {
v: u64
}
impl MyTrait for MyStruct {}
mod foo {
pub fn bla() -> TE {
return crate::MyStruct {v:1}
}
existential type TE: crate::MyTrait;
}
mod bar {
pub fn bla2() -> TE2 {
crate::foo::bla()
}
pub existential type TE2: crate::MyTrait;
}
pub fn bla2() -> bar::TE2 {
crate::foo::bla()
} still errors after your PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @oli-obk See my reply to @nikomatsakis above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code you linked is to the master branch (or at least before your PR) right here. So we aren't always checking that as you wrote. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes - that line is unchanged in my PR - that is, we still equate the opaque types. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤦♂️ sorry, These two obligations looked so similar, I confused the two. |
||
obligations.add(infcx | ||
.at(&ObligationCause::dummy(), param_env) | ||
.eq(opaque_decl.concrete_ty, opaque_defn_ty)?); | ||
} | ||
} | ||
|
||
debug!("eq_opaque_type_and_type: equated"); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,30 +1,8 @@ | ||
error[E0391]: cycle detected when processing `Foo` | ||
error: could not find defining uses | ||
--> $DIR/existential-types-with-cycle-error.rs:3:1 | ||
| | ||
LL | existential type Foo: Fn() -> Foo; | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
note: ...which requires processing `crash`... | ||
--> $DIR/existential-types-with-cycle-error.rs:6:25 | ||
| | ||
LL | fn crash(x: Foo) -> Foo { | ||
| _________________________^ | ||
LL | | x | ||
LL | | } | ||
| |_^ | ||
= note: ...which again requires processing `Foo`, completing the cycle | ||
note: cycle used when collecting item types in top-level module | ||
--> $DIR/existential-types-with-cycle-error.rs:1:1 | ||
| | ||
LL | / #![feature(existential_type)] | ||
LL | | | ||
LL | | existential type Foo: Fn() -> Foo; | ||
LL | | | ||
... | | ||
LL | | | ||
LL | | } | ||
| |_^ | ||
|
||
error: aborting due to previous error | ||
|
||
For more information about this error, try `rustc --explain E0391`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,30 +1,8 @@ | ||
error[E0391]: cycle detected when processing `Foo` | ||
error: could not find defining uses | ||
--> $DIR/existential-types-with-cycle-error2.rs:7:1 | ||
| | ||
LL | existential type Foo: Bar<Foo, Item = Foo>; | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
note: ...which requires processing `crash`... | ||
--> $DIR/existential-types-with-cycle-error2.rs:10:25 | ||
| | ||
LL | fn crash(x: Foo) -> Foo { | ||
| _________________________^ | ||
LL | | x | ||
LL | | } | ||
| |_^ | ||
= note: ...which again requires processing `Foo`, completing the cycle | ||
note: cycle used when collecting item types in top-level module | ||
--> $DIR/existential-types-with-cycle-error2.rs:1:1 | ||
| | ||
LL | / #![feature(existential_type)] | ||
LL | | | ||
LL | | pub trait Bar<T> { | ||
LL | | type Item; | ||
... | | ||
LL | | | ||
LL | | } | ||
| |_^ | ||
|
||
error: aborting due to previous error | ||
|
||
For more information about this error, try `rustc --explain E0391`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
// check-pass | ||
|
||
#![feature(existential_type)] | ||
// Currently, the `existential_type` feature implicitly | ||
// depends on `impl_trait_in_bindings` in order to work properly. | ||
// Specifically, this line requires `impl_trait_in_bindings` to be enabled: | ||
// https://github.com/rust-lang/rust/blob/481068a707679257e2a738b40987246e0420e787/src/librustc_typeck/check/mod.rs#L856 | ||
#![feature(impl_trait_in_bindings)] | ||
//~^ WARN the feature `impl_trait_in_bindings` is incomplete and may cause the compiler to crash | ||
|
||
// Ensures that `const` items can constrain an `existential type`. | ||
|
||
use std::fmt::Debug; | ||
|
||
pub existential type Foo: Debug; | ||
|
||
const _FOO: Foo = 5; | ||
|
||
fn main() { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
warning: the feature `impl_trait_in_bindings` is incomplete and may cause the compiler to crash | ||
--> $DIR/existential_type_const.rs:8:12 | ||
| | ||
LL | #![feature(impl_trait_in_bindings)] | ||
| ^^^^^^^^^^^^^^^^^^^^^^ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// check-pass | ||
|
||
#![feature(existential_type)] | ||
|
||
// Regression test for issue #61863 | ||
|
||
pub trait MyTrait {} | ||
|
||
#[derive(Debug)] | ||
pub struct MyStruct { | ||
v: u64 | ||
} | ||
|
||
impl MyTrait for MyStruct {} | ||
|
||
pub fn bla() -> TE { | ||
return MyStruct {v:1} | ||
} | ||
|
||
pub fn bla2() -> TE { | ||
bla() | ||
} | ||
|
||
|
||
existential type TE: MyTrait; | ||
|
||
fn main() {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
// check-pass | ||
|
||
#![feature(existential_type)] | ||
#![allow(dead_code)] | ||
|
||
pub trait MyTrait {} | ||
|
||
impl MyTrait for bool {} | ||
|
||
struct Blah { | ||
my_foo: Foo, | ||
my_u8: u8 | ||
} | ||
|
||
impl Blah { | ||
fn new() -> Blah { | ||
Blah { | ||
my_foo: make_foo(), | ||
my_u8: 12 | ||
} | ||
} | ||
fn into_inner(self) -> (Foo, u8) { | ||
(self.my_foo, self.my_u8) | ||
} | ||
} | ||
|
||
fn make_foo() -> Foo { | ||
true | ||
} | ||
|
||
existential type Foo: MyTrait; | ||
|
||
fn main() {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,8 @@ | ||
error[E0391]: cycle detected when processing `Foo` | ||
error: could not find defining uses | ||
--> $DIR/no_inferrable_concrete_type.rs:6:1 | ||
| | ||
LL | existential type Foo: Copy; | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
note: ...which requires processing `bar`... | ||
--> $DIR/no_inferrable_concrete_type.rs:9:23 | ||
| | ||
LL | fn bar(x: Foo) -> Foo { x } | ||
| ^^^^^ | ||
= note: ...which again requires processing `Foo`, completing the cycle | ||
note: cycle used when collecting item types in top-level module | ||
--> $DIR/no_inferrable_concrete_type.rs:4:1 | ||
| | ||
LL | / #![feature(existential_type)] | ||
LL | | | ||
LL | | existential type Foo: Copy; | ||
LL | | | ||
... | | ||
LL | | let _: Foo = std::mem::transmute(0u8); | ||
LL | | } | ||
| |_^ | ||
|
||
error: aborting due to previous error | ||
|
||
For more information about this error, try `rustc --explain E0391`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is quite the right check, or at least it doesn't match the behavior specified by the RFC. In the RFC, it's expected that the following would compile:
In this situation, the type of the variable returned by
foo
is an impl trait variable, but since the function is within the same module as the definition of theexistential type
, it can constrain the type further and itself become a defining use. The RFC says you can even do this:I realize this isn't at all what's implemented today, but it'd be good to talk about how the behavior being implemented here interacts with the desired behavior we want to end up with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this check is still compatible with implementing the full behavior specified in the RFC. This check occurs after we've attempted to instantiate the opaque type. If we're still left with an opaque type after instantiation, then there's nothing else we can do.
Once the rest of the RFC is implemented, this check should be hit only when the opaque type is being used outside the defining module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense-- thanks for the clarification!