Remove support for defaulted trait methods with async or return-position impl Trait#108142
Closed
compiler-errors wants to merge 2 commits intorust-lang:masterfrom
Closed
Remove support for defaulted trait methods with async or return-position impl Trait#108142compiler-errors wants to merge 2 commits intorust-lang:masterfrom
async or return-position impl Trait#108142compiler-errors wants to merge 2 commits intorust-lang:masterfrom
Conversation
Collaborator
|
|
26fd458 to
7147449
Compare
oli-obk
approved these changes
Feb 17, 2023
Contributor
|
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. |
Contributor
Author
Contributor
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unfortunately, default methods on
async fnand return-positionimpl Traitin traits are unsound and pretty unworkable in their current state. To put it in perspective, given the code:The above code cannot bbe desugared like:
because we shouldn't be able to observe the
Self::FooRPIT->impl Futureprojection.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)