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

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Jul 5, 2019

Fixes #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)

@rust-highfive
Copy link
Collaborator

r? @cramertj

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

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

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned cramertj Jul 5, 2019
@Centril
Copy link
Contributor

Centril commented Jul 5, 2019

cc @rust-lang/lang

@Aaron1011
Copy link
Member Author

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 Outdated Show resolved Hide resolved
src/test/run-pass/existential_type_const.rs Outdated Show resolved Hide resolved
src/test/run-pass/existential_type_const.rs Outdated Show resolved Hide resolved
src/librustc_mir/borrow_check/nll/type_check/mod.rs Outdated Show resolved Hide resolved
src/librustc_mir/borrow_check/nll/type_check/mod.rs Outdated Show resolved Hide resolved
src/librustc_mir/borrow_check/nll/type_check/mod.rs Outdated Show resolved Hide resolved
src/librustc_mir/borrow_check/nll/type_check/mod.rs Outdated Show resolved Hide resolved
src/librustc_mir/borrow_check/nll/type_check/mod.rs Outdated Show resolved Hide resolved
src/librustc_mir/borrow_check/nll/type_check/mod.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/writeback.rs Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Jul 5, 2019

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
Copy link
Member Author

@Centril: I've addressed your comments

Aaron1011 and others added 7 commits July 7, 2019 17:22
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>
@@ -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!

@eddyb
Copy link
Member

eddyb commented Jul 11, 2019

r? @nikomatsakis

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

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!

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

Choose a reason for hiding this comment

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

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

@Aaron1011
Copy link
Member Author

Aaron1011 commented Jul 12, 2019

@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
Copy link
Member Author

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

@oli-obk
Copy link
Contributor

oli-obk commented Jul 23, 2019

r? @oli-obk

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

@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Jul 27, 2019

📌 Commit 2f41962 has been approved by oli-obk

@bors 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 added a commit to Centril/rust that referenced this pull request Jul 27, 2019
…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`)
bors added a commit that referenced this pull request Jul 27, 2019
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
@bors bors merged commit 2f41962 into rust-lang:master Jul 27, 2019
@Aaron1011 Aaron1011 deleted the fix/existential-cycle branch July 27, 2019 20:42
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 12, 2024
…in_sig, r=lcnr

Tait must be constrained if in sig

r? `@compiler-errors`

kind of reverts rust-lang#62423, but that PR only removed cycles in cases that we want to forbid now anyway.

see https://rust-lang.zulipchat.com/#narrow/stream/315482-t-compiler.2Fetc.2Fopaque-types/topic/lcnr.20oli.20meeting/near/370712246 for related discussion and motivating example

a TAIT showing up in a signature doesn't just mean it can register a hidden type, but that it must.

This is the [design the types team decided upon](https://hackmd.io/QOsEaEJtQK-XDS_xN4UyQA#Proposal-preferred) for the following reasons

* avoids a hypothetical situation where getting smarter in trait solving could cause new cycle errors or new inference errors to show up, where today something is treated as opaque.
* avoids having the situation where a (minor?) change to a function body causes an error, because its TAIT usage suddenly isn't "opaque enough" anymore.
* avoids having to explain why in some cases you need to put a Tait into a tiny module together with its defining usages. Now you basically gotta always do this, the moment a Tait is in a signature that isn't in the defining scope.
* avoids false-cycle errors
* anything diverging from this pattern needs to either
    * move the TAIT declaration and defining function into their own helper sub module
    * use the attributes we'll provide in the future to explicitly opt in or out of being in the defining scope

fixes rust-lang#117861

reverts rust-lang#125362 cc `@Nilstrieb` `@joboet`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trying to use same existential type for two functions cause cycle dependency
8 participants