Skip to content
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

Merged
merged 7 commits into from
Jul 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 33 additions & 5 deletions src/librustc_mir/borrow_check/nll/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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:

existential type Foo: Debug;

fn foo() -> Foo {
    5u32
}

fn bar(cond: bool) -> Foo {
    if cond { foo() } else { 5u32 }
}

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 the existential type, it can constrain the type further and itself become a defining use. The RFC says you can even do this:

existential type Foo: Debug;

fn foo() -> Foo {
    5u32
}

fn bar() -> Foo {
    let x: u32 = foo();
    x + 5
}

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.

Copy link
Member Author

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.

Copy link
Member

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!

.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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oli-obk See my reply to @nikomatsakis above

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@Aaron1011 Aaron1011 Jul 24, 2019

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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");
Expand Down
55 changes: 31 additions & 24 deletions src/librustc_typeck/check/writeback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -576,36 +576,43 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> {
})
};

let mut skip_add = false;

if let ty::Opaque(defin_ty_def_id, _substs) = definition_ty.sty {
if def_id == defin_ty_def_id {
// Concrete type resolved to the existential type itself.
// Force a cycle error.
// FIXME(oli-obk): we could just not insert it into `concrete_existential_types`
// which simply would make this use not a defining use.
self.tcx().at(span).type_of(defin_ty_def_id);
debug!("Skipping adding concrete definition for opaque type {:?} {:?}",
opaque_defn, defin_ty_def_id);
skip_add = true;
}
}

if !opaque_defn.substs.has_local_value() {
let new = ty::ResolvedOpaqueTy {
concrete_type: definition_ty,
substs: opaque_defn.substs,
};

let old = self.tables
.concrete_existential_types
.insert(def_id, new);
if let Some(old) = old {
if old.concrete_type != definition_ty || old.substs != opaque_defn.substs {
span_bug!(
span,
"visit_opaque_types tried to write \
different types for the same existential type: {:?}, {:?}, {:?}, {:?}",
def_id,
definition_ty,
opaque_defn,
old,
);
// We only want to add an entry into `concrete_existential_types`
// if we actually found a defining usage of this existential type.
// Otherwise, we do nothing - we'll either find a defining usage
// in some other location, or we'll end up emitting an error due
// to the lack of defining usage
if !skip_add {
Centril marked this conversation as resolved.
Show resolved Hide resolved
let new = ty::ResolvedOpaqueTy {
concrete_type: definition_ty,
substs: opaque_defn.substs,
};

let old = self.tables
.concrete_existential_types
.insert(def_id, new);
if let Some(old) = old {
if old.concrete_type != definition_ty || old.substs != opaque_defn.substs {
span_bug!(
span,
"visit_opaque_types tried to write different types for the same \
existential type: {:?}, {:?}, {:?}, {:?}",
def_id,
definition_ty,
opaque_defn,
old,
);
}
}
}
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#![feature(existential_type)]

existential type Foo: Fn() -> Foo;
//~^ ERROR: cycle detected when processing `Foo`
//~^ ERROR: could not find defining uses

fn crash(x: Foo) -> Foo {
x
Expand Down
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
Expand Up @@ -5,7 +5,7 @@ pub trait Bar<T> {
}

existential type Foo: Bar<Foo, Item = Foo>;
//~^ ERROR: cycle detected when processing `Foo`
//~^ ERROR: could not find defining uses

fn crash(x: Foo) -> Foo {
x
Expand Down
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`.
20 changes: 20 additions & 0 deletions src/test/ui/existential_types/existential_type_const.rs
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() {
}
6 changes: 6 additions & 0 deletions src/test/ui/existential_types/existential_type_const.stderr
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)]
| ^^^^^^^^^^^^^^^^^^^^^^

27 changes: 27 additions & 0 deletions src/test/ui/existential_types/existential_type_fns.rs
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() {}
33 changes: 33 additions & 0 deletions src/test/ui/existential_types/existential_type_tuple.rs
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() {}
6 changes: 3 additions & 3 deletions src/test/ui/existential_types/no_inferrable_concrete_type.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
// Issue 52985: Cause cycle error if user code provides no use case that allows an existential type
// to be inferred to a concrete type. This results in an infinite cycle during type normalization.
// Issue 52985: user code provides no use case that allows an existential type
// We now emit a 'could not find defining uses' error

#![feature(existential_type)]

existential type Foo: Copy; //~ cycle detected
existential type Foo: Copy; //~ could not find defining uses

// make compiler happy about using 'Foo'
fn bar(x: Foo) -> Foo { x }
Expand Down
21 changes: 1 addition & 20 deletions src/test/ui/existential_types/no_inferrable_concrete_type.stderr
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`.