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

Change opaque types to not be structural match. #72156

Closed
lcnr opened this issue May 12, 2020 · 8 comments
Closed

Change opaque types to not be structural match. #72156

lcnr opened this issue May 12, 2020 · 8 comments
Labels
C-bug Category: This is a bug. F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented May 12, 2020

The following currently passes the structural match check and causes an ICE later on:
(now that #72153 has landed, this emits an error)

#![feature(const_fn, type_alias_impl_trait)]

type F = impl Send;

// This does not implement structural match
struct A;

const fn value(a: A) -> F {
    a
}

const V: F = value(A);

fn main() {
    
    match value(A) {
        V => println!("hey"),
    }
}

I believe we have to either leak the structural match property of opaque types,
(similar to Send and Sync afaict) or forbid this entirely.

I am in favor of the second option for now. It should not be a breaking change to go from the
second to the first later on.

This issue concerns the implementation in #72153

@eddyb @pnkfelix @RalfJung

@lcnr lcnr changed the title Do not implement structural match of opaque types. Do not implement structural match for opaque types. May 12, 2020
@jonas-schievink jonas-schievink added C-bug Category: This is a bug. F-const_fn F-impl_trait_in_bindings `#![feature(impl_trait_in_bindings)]` F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 12, 2020
@lcnr lcnr removed the F-impl_trait_in_bindings `#![feature(impl_trait_in_bindings)]` label May 12, 2020
@lcnr
Copy link
Contributor Author

lcnr commented May 12, 2020

The same problem probably also exists for Generator and GeneratorWitness, which are currently implicitly accepted.

@RalfJung
Copy link
Member

I believe we have to either leak the structural match property of opaque types,
(similar to Send and Sync afaict)

I don't know what "leak" means.

But forbidding it for now seems best. :D

@lcnr lcnr changed the title Do not implement structural match for opaque types. Change opaque types to not be structural match. May 13, 2020
@lcnr
Copy link
Contributor Author

lcnr commented May 13, 2020

This is what I've meant with "leak":

fn test<T: Sync>(_: T) {}

fn ok() -> impl Eq {
    7
}

fn not_ok() -> impl Eq {
    std::cell::Cell::new(7)
}

fn main() {
    test(ok());
    // This works, `ok` returns a type implementing `Sync`.
    //
    // Note that we do not explicitly mention `Send` in the return type of `ok`.
    
    test(not_ok());
    // This fails, `not_ok` returns a type not implementing `Sync`.
}

@RalfJung
Copy link
Member

Ah I see -- auto traits are "leaked" through opaque ("impl Trait") types.

Structural(Partial)Eq are not auto traits though and likely can't be...

@lcnr
Copy link
Contributor Author

lcnr commented May 13, 2020

Structural(Partial)Eq are not auto traits though and likely can't be...

I don't see the connection between auto traits and leaking through opaque types rn.
At least to my understanding we theoretically could completely leak the actual type and allow stuff like

fn int() -> impl Send {
    7
}

fn main() {
    let _x: i32 = int().wrapping_add(7);
}

@RalfJung
Copy link
Member

RalfJung commented May 13, 2020

I don't see the connection between auto traits and leaking through opaque types rn.

AFAIK all auto traits currently leak through opaque types (Send/Sync are not special) and nothing else does. That seems like a pretty direct connection to me?^^

@lcnr
Copy link
Contributor Author

lcnr commented May 13, 2020

Didn't know we leak Unpin 👀

@lcnr
Copy link
Contributor Author

lcnr commented Jun 1, 2020

#72153 now explicitly changes opaque types to not be structural match.

In case there is a use case where we want to change this, we can open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Development

No branches or pull requests

3 participants