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.

Sign up for GitHub

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

Jump to bottom

fix trait objects with a Self-containing projection values #56863

Merged
merged 2 commits into from
Dec 18, 2018
Merged

fix trait objects with a Self-containing projection values #56863

merged 2 commits into from
Dec 18, 2018

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Dec 15, 2018 •
edited
Loading

Fixes #56288.

This follows ALT2 in the issue.

beta-nominating since this is a regression.

r? @nikomatsakis

@arielb1 arielb1 added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 15, 2018
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 15, 2018
@arielb1 arielb1 changed the title fix trait objects with a Self-having projection va fix trait objects with a Self-having projection values Dec 15, 2018
@arielb1 arielb1 changed the title fix trait objects with a Self-having projection values fix trait objects with a Self-containing projection values Dec 15, 2018
@ExpHP
Copy link
Contributor

ExpHP commented Dec 17, 2018

cc #56865

nikomatsakis
nikomatsakis approved these changes Dec 17, 2018
View reviewed changes
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.

Left some comments for the tests, but this seems reasonable.

One thing I found myself wondering:

What will happen if you have

trait Helper: Base<Output = <Self as Helper>::Target> + Base<Output = u32> { ... }

maybe worth testing that? I'm kind of "OK" with any outcome =) My expectation is that dyn Helper will not require Output to be specified based on the code though, but I could easily be missing something.

r=me with nits addressed + new test

src/test/ui/traits/trait-object-with-self-in-projection-output-bad.rs
@@ -0,0 +1,22 @@
trait Base {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a comment to this test:

Regression test for #56288. Checks that if a supertrait defines an associated type projection that references Self, then that associated type must still be explicitly specified in the dyn Trait variant, since we don't know what Self is anymore.

src/test/ui/traits/trait-object-with-self-in-projection-output-good.rs
@@ -0,0 +1,23 @@
// compile-pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly a comment here would be good:

Regression test related to #56288. Check that a supertrait projection (of Output) that references Self can be ok if it is referencing a projection (of Self::Target, in this case). Note that we still require the user to manually specify both Target and Output for now.

@nikomatsakis nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 17, 2018
@arielb1
Copy link
Contributor Author

arielb1 commented Dec 17, 2018 •
edited
Loading

What will happen if you have

trait Helper: Base<Output = <Self as Helper>::Target> + Base<Output = u32> { ... }

In that case, you will get an E0282 while WF-checking the trait (even if it is not used anywhere), unless rustc manages to normalize the projection and finds that it is equal to u32, in that case the compiler treats it as if it didn't contain Self.

Actually, I did manage to get a test to work. Will add that.

@arielb1
Copy link
Contributor Author

arielb1 commented Dec 17, 2018

@nikomatsakis

Done.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 18, 2018

📌 Commit 1fd23f5 has been approved by nikomatsakis

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 18, 2018
@nikomatsakis
Copy link
Contributor

@bors p=1 -- regression fix

@bors
Copy link
Contributor

bors commented Dec 18, 2018

⌛ Testing commit 1fd23f5 with merge d99a320...

bors added a commit that referenced this pull request Dec 18, 2018
@bors
Auto merge of #56863 - arielb1:supertrait-self-4, r=nikomatsakis
d99a320 
fix trait objects with a Self-containing projection values

Fixes #56288.

This follows ALT2 in the issue.

beta-nominating since this is a regression.

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Dec 18, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing d99a320 to master...

@bors bors merged commit 1fd23f5 into rust-lang:master Dec 18, 2018
@pnkfelix
Copy link
Member

discussed at T-compiler meeting. beta-accepting.

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Dec 20, 2018
@pietroalbini pietroalbini mentioned this pull request Dec 31, 2018
Merged
@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 31, 2018
bors added a commit that referenced this pull request Dec 31, 2018
@bors
Auto merge of #57236 - pietroalbini:beta-backports, r=pietroalbini
bab921d 
[beta] Rollup backports

* #56919: Remove a wrong multiplier on relocation offset computation
* #56916: Fix mutable references in `static mut`
* #56863: fix trait objects with a Self-containing projection values
* #56850: Fixed issue with using `Self` ctor in typedefs

r? @ghost
bors added a commit that referenced this pull request Jan 1, 2019
@bors
Auto merge of #57236 - pietroalbini:beta-backports, r=pietroalbini
fa64cc8 
[beta] Rollup backports

* #56919: Remove a wrong multiplier on relocation offset computation
* #56916: Fix mutable references in `static mut`
* #56863: fix trait objects with a Self-containing projection values
* #56850: Fixed issue with using `Self` ctor in typedefs

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@arielb1 @ExpHP @nikomatsakis @bors @pnkfelix @pietroalbini @rust-highfive