Fix cycle error with existential types #62423

Aaron1011 · 2019-07-05T21:21:53Z

We now allow uses of existential type's that aren't defining uses - that is, uses which don't constrain the underlying concrete type.

To make this work correctly, we also modify eq_opaque_type_and_type to not try to apply additional constraints to an opaque type. If we have code like this:

existential type Foo;
fn foo1() -> Foo { ... }
fn foo2() -> Foo { foo1() }

then foo2 doesn't end up constraining Foo, which means that foo2 will end up using the type Foo internally - that is, an actual TyKind::Opaque. We don't want to equate this to the underlying concrete type - we just need to enforce the basic equality constraint between the two types (here, the return type of foo1 and the return type of foo2)

rust-highfive · 2019-07-05T21:21:57Z

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

Aaron1011 · 2019-07-05T21:22:09Z

r? @oli-obk

Centril · 2019-07-05T22:41:31Z

cc @rust-lang/lang

Aaron1011 · 2019-07-05T22:52:45Z

My initial solution didn't work properly when an existential type was used inside another type (e.g. fn foo() -> (Foo, bool) where Foo is an existential type). I've modified my fix to handle that properly, and added an additional test case.

src/test/run-pass/existential_type_const.rs

src/librustc_mir/borrow_check/nll/type_check/mod.rs

src/librustc_typeck/check/writeback.rs

Centril · 2019-07-05T22:54:57Z

Since you added a commit mid my review some suggestions are sorta outdated now but the basic idea isn't so you can "translate" the changes to the new location... ;)

Aaron1011 · 2019-07-06T00:15:59Z

@Centril: I've addressed your comments

Fixes rust-lang#61863 We now allow uses of 'existential type's that aren't defining uses - that is, uses which don't constrain the underlying concrete type. To make this work correctly, we also modify eq_opaque_type_and_type to not try to apply additional constraints to an opaque type. If we have code like this: ``` existential type Foo; fn foo1() -> Foo { ... } fn foo2() -> Foo { foo1() } ``` then 'foo2' doesn't end up constraining 'Foo', which means that 'foo2' will end up using the type 'Foo' internally - that is, an actual 'TyKind::Opaque'. We don't want to equate this to the underlying concrete type - we just need to enforce the basic equality constraint between the two types (here, the return type of 'foo1' and the return type of 'foo2')

Previously, types like (Foo, u8) would not be handled correctly (where Foo is an 'existential type')

Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

cramertj · 2019-07-09T00:34:13Z

src/librustc_mir/borrow_check/nll/type_check/mod.rs

@@ -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


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.

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.

That makes sense-- thanks for the clarification!

eddyb · 2019-07-11T12:48:13Z

r? @nikomatsakis

nikomatsakis

I've got a local build and I'm taking a look. I'm concerned that the current check doesn't feel sufficient -- i.e., just testing that you have some opaque type doesn't mean you have the same opaque type, for starters... and obviously this isn't as flexible as we might like, as @cramertj pointed out. I'd like to put a bit of thought into whether we can implement the "proper" semantics -- not necessarily in this PR, though. Once my build completes and I finish a few other things I'll do some testing and try to give a bit more detailed feedback! Thanks @Aaron1011 for looking into this!

nikomatsakis · 2019-07-11T17:06:16Z

src/librustc_mir/borrow_check/nll/type_check/mod.rs

+                    // not to the opaque type itself. Therefore, it's enough
+                    // to simply equate the output and opque 'revealed_type',
+                    // as we do above
+                    if revealed_ty_is_opaque {


just testing that the revealed type is opaque doesn't seem like enough

Aaron1011 · 2019-07-12T23:03:22Z

@nikomatsakis: We always check that the opaque type is equal to the 'output' type here. We only skip applying the obligations that should only apply to the underlying revealed type.

As I mentioned to @cramertj, I believe this change is fully forwards-compatible with 'revealing' opaque types under more circumstances (e.g. within the defining module). My checks runs during MIR typecheck - at this stage of compilation, any opaque type usages that have not been revealed are never going to be revealed (i.e. usages outside the defining module). Future changes may affect under what circumstances my check is hit, but I believe that it's always correct to skip the 'revealed' obligations for an opaque type that we cannot reveal.

Aaron1011 · 2019-07-22T22:38:00Z

@nikomatsakis: Are there any changes that you'd like me to make?

oli-obk · 2019-07-23T06:23:59Z

r? @oli-obk

oli-obk · 2019-07-23T06:42:38Z

src/librustc_mir/borrow_check/nll/type_check/mod.rs

+                        // without actually looking at the underlying type it
+                        // gets 'revealed' into
+
+                        if !concrete_is_opaque {


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

@oli-obk See my reply to @nikomatsakis above

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.

Yes - that line is unchanged in my PR - that is, we still equate the opaque types.

🤦‍♂️ sorry, These two obligations looked so similar, I confused the two.

oli-obk · 2019-07-27T15:32:17Z

@bors r+

bors · 2019-07-27T15:32:18Z

📌 Commit 2f41962 has been approved by oli-obk

…oli-obk Fix cycle error with existential types Fixes rust-lang#61863 We now allow uses of `existential type`'s that aren't defining uses - that is, uses which don't constrain the underlying concrete type. To make this work correctly, we also modify `eq_opaque_type_and_type` to not try to apply additional constraints to an opaque type. If we have code like this: ```rust existential type Foo; fn foo1() -> Foo { ... } fn foo2() -> Foo { foo1() } ``` then `foo2` doesn't end up constraining `Foo`, which means that `foo2` will end up using the type `Foo` internally - that is, an actual `TyKind::Opaque`. We don't want to equate this to the underlying concrete type - we just need to enforce the basic equality constraint between the two types (here, the return type of `foo1` and the return type of `foo2`)

@ghost

Rollup of 6 pull requests Successful merges: - #62423 (Fix cycle error with existential types) - #62979 (Cleanup save-analysis JsonDumper) - #62982 (Don't access a static just for its size and alignment) - #63013 (add `repr(transparent)` to `IoSliceMut` where missing) - #63014 (Stop bare trait lint applying to macro call sites) - #63036 (Add lib section to rustc_lexer's Cargo.toml) Failed merges: r? @ghost

rust-highfive assigned cramertj Jul 5, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 5, 2019

rust-highfive assigned oli-obk and unassigned cramertj Jul 5, 2019

Centril reviewed Jul 5, 2019

View reviewed changes

Aaron1011 and others added 7 commits July 7, 2019 17:22

Fix bug when opaque type was nested in another type.

ec62699

Previously, types like (Foo, u8) would not be handled correctly (where Foo is an 'existential type')

Remove unecessary doc comment

2f05160

s/consts/const items/

2ab8d61

Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

Address review comments

3600832

Fix test stderr

66b2b97

Add explanation to 'existential_type_const' test

2f41962

Aaron1011 force-pushed the fix/existential-cycle branch from 90fa14f to 2f41962 Compare July 7, 2019 21:22

cramertj reviewed Jul 9, 2019

View reviewed changes

rust-highfive assigned nikomatsakis and unassigned oli-obk Jul 11, 2019

nikomatsakis reviewed Jul 12, 2019

View reviewed changes

rust-highfive assigned oli-obk and unassigned nikomatsakis Jul 23, 2019

oli-obk reviewed Jul 23, 2019

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2019

Centril mentioned this pull request Jul 27, 2019

Rollup of 6 pull requests #63043

Merged

bors merged commit 2f41962 into rust-lang:master Jul 27, 2019

Aaron1011 deleted the fix/existential-cycle branch July 27, 2019 20:42

oli-obk mentioned this pull request Jul 28, 2019

Existental type, const fn, and generator ICE #53678

Closed

lqd mentioned this pull request Oct 25, 2019

ICE when using 'impl Trait' with an infinity recursive function #65561

Closed

oli-obk mentioned this pull request Jun 30, 2023

Tait must be constrained if in sig #113169

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cycle error with existential types #62423

Fix cycle error with existential types #62423

Aaron1011 commented Jul 5, 2019 •

edited by Centril

rust-highfive commented Jul 5, 2019

Aaron1011 commented Jul 5, 2019

Centril commented Jul 5, 2019

Aaron1011 commented Jul 5, 2019

Centril commented Jul 5, 2019 •

edited

Aaron1011 commented Jul 6, 2019

cramertj Jul 9, 2019

Aaron1011 Jul 9, 2019

cramertj Jul 9, 2019

eddyb commented Jul 11, 2019

nikomatsakis left a comment

nikomatsakis Jul 11, 2019

Aaron1011 commented Jul 12, 2019 •

edited

Aaron1011 commented Jul 22, 2019

oli-obk commented Jul 23, 2019

oli-obk Jul 23, 2019

Aaron1011 Jul 23, 2019

oli-obk Jul 24, 2019

Aaron1011 Jul 24, 2019 •

edited

oli-obk Jul 27, 2019

oli-obk commented Jul 27, 2019

bors commented Jul 27, 2019

Fix cycle error with existential types #62423

Fix cycle error with existential types #62423

Conversation

Aaron1011 commented Jul 5, 2019 • edited by Centril

rust-highfive commented Jul 5, 2019

Aaron1011 commented Jul 5, 2019

Centril commented Jul 5, 2019

Aaron1011 commented Jul 5, 2019

Centril commented Jul 5, 2019 • edited

Aaron1011 commented Jul 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Jul 11, 2019

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaron1011 commented Jul 12, 2019 • edited

Aaron1011 commented Jul 22, 2019

oli-obk commented Jul 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaron1011 Jul 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jul 27, 2019

bors commented Jul 27, 2019

Aaron1011 commented Jul 5, 2019 •

edited by Centril

Centril commented Jul 5, 2019 •

edited

Aaron1011 commented Jul 12, 2019 •

edited

Aaron1011 Jul 24, 2019 •

edited