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

Remove support for defaulted trait methods with async or return-position impl Trait #108142

Closed

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Feb 16, 2023

Unfortunately, default methods on async fn and return-position impl Trait in traits are unsound and pretty unworkable in their current state. To put it in perspective, given the code:

trait Foo {
  async fn foo();
}

The above code cannot bbe desugared like:

#![feature(type_alias_impl_trait, associated_type_defaults)]

use std::future::Future;

trait Foo {
    type FooRPIT = impl Future<Output = ()>;
    
    fn foo() -> Self::FooRPIT {
        async {}
    }
}

because we shouldn't be able to observe the Self::FooRPIT -> impl Future projection.

Ideally, we could observe this inside the body of Foo::foo, but the way that our projection logic implements this is not correct, leading to issues like #107002. Fixing it leads to weird cycles or committing to really nasty hacks in HIR and MIR typeck (#107013).


To discourage users from relying on this behavior and hitting issues like #107002, let's just deny this behavior for now. Maybe I can revisit this feature in the future with a clearer mind. I may have some tricks up my sleeve to fix this, but for now, let's not commit to supporting this.

cc @rust-lang/wg-async and @rust-lang/types who may have thoughts about this.

r? @lcnr

Fixes #107002 (not really, but at least closes it)

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 16, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2023

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

@eholk
Copy link
Contributor

eholk commented Feb 17, 2023

I think denying default impls on async methods sounds reasonable for now, but I think it's something we should try to support in the future.

@compiler-errors
Copy link
Member Author

Alternatively, lcnr and I discussed we should be able to both fix these default trait methods without removing them with #108203.

Let's wait a bit to see if that PR has any fatal flaws, otherwise I'll land this.

@rustbot blocked

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 18, 2023
@compiler-errors
Copy link
Member Author

Closing in favor of #108203.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 19, 2023
…s-2, r=jackh726

Fix RPITITs in default trait methods (by assuming projection predicates in param-env)

Instead of having special projection logic that allows us to turn `ProjectionTy(RPITIT, [Self#0, ...])` into `OpaqueTy(RPITIT, [Self#0, ...])`, we can instead augment the param-env of default trait method bodies to assume these as projection predicates. This should allow us to only project where we're allowed to!

In order to make this work without introducing a bunch of cycle errors, we additionally tweak the `OpaqueTypeExpander` used by `ParamEnv::with_reveal_all_normalized` to not normalize the right-hand side of projection predicates. This should be fine, because if we use the projection predicate to normalize some other projection type, we'll continue to normalize the opaque that it gets projected to.

This also makes it possible to support default trait methods with RPITITs in an associated-type based RPITIT lowering strategy without too much extra effort.

Fixes rust-lang#107002
Alternative to rust-lang#108142
@compiler-errors compiler-errors deleted the rpitit-fix-defaults branch August 11, 2023 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected behaviour when calling associated async function of a trait with default implementations
5 participants