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

Make GATs object safe under generic_associated_types_extended feature #94911

Merged
merged 1 commit into from
Apr 2, 2022
Merged

Make GATs object safe under generic_associated_types_extended feature #94911

merged 1 commit into from
Apr 2, 2022

Conversation

jackh726
Copy link
Member

Based on #94869

Let's say we have

trait StreamingIterator {
    type Item<'a> where Self: 'a;
}

And dyn for<'a> StreamingIterator<Item<'a> = &'a i32>.

If we ask (dyn for<'a> StreamingIterator<Item<'a> = &'a i32>): StreamingIterator, then we have to prove that for<'x> (&'x i32): Sized. So, we generate new bound vars to subst for the GAT generics.

Importantly, this doesn't fully verify that these are usable and sound.

r? @nikomatsakis

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 13, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Mar 25, 2022

☔ The latest upstream changes (presumably #95291) made this pull request unmergeable. Please resolve the merge conflicts.

@jackh726 jackh726 mentioned this pull request Mar 29, 2022
Open
@rust-log-analyzer

This comment has been minimized.

Sign in to view
compiler-errors
compiler-errors approved these changes Mar 30, 2022
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment •
edited
Loading

Choose a reason for hiding this comment

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

these changes look good -- the logic you're using to create new bound vars for the GATs is inspired from #87900, right?

r=me once the parent PR lands (and you rebase that first commit out)

compiler/rustc_trait_selection/src/traits/object_safety.rs
ObjectSafetyViolation::GAT(ident.name, ident.span)
}),
);
if !tcx.features().generic_associated_types_extended {
Copy link
Member

Choose a reason for hiding this comment

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

I was going to say that this should also deny GATs with type parameters, but it seems we already disallow those elsewhere: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1af97ded49aa74d43f72f0c3b70658e9

@bors
Copy link
Contributor

bors commented Mar 31, 2022

☔ The latest upstream changes (presumably #95501) made this pull request unmergeable. Please resolve the merge conflicts.

@jackh726
Copy link
Member Author

jackh726 commented Apr 1, 2022

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Apr 1, 2022

📌 Commit d3fe28b has been approved by compiler-errors

@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 Apr 1, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 1, 2022
@matthiaskrgr
Rollup merge of rust-lang#94911 - jackh726:gats_extended_2, r=compile…
bd70a56 
…r-errors

Make GATs object safe under generic_associated_types_extended feature

Based on rust-lang#94869

Let's say we have
```rust
trait StreamingIterator {
    type Item<'a> where Self: 'a;
}
```
And `dyn for<'a> StreamingIterator<Item<'a> = &'a i32>`.

If we ask `(dyn for<'a> StreamingIterator<Item<'a> = &'a i32>): StreamingIterator`, then we have to prove that `for<'x> (&'x i32): Sized`. So, we generate *new* bound vars to subst for the GAT generics.

Importantly, this doesn't fully verify that these are usable and sound.

r? `@nikomatsakis`
@matthiaskrgr matthiaskrgr mentioned this pull request Apr 1, 2022
Closed
@Dylan-DPC
Copy link
Member

failed in rollup

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 2, 2022
@compiler-errors
Copy link
Member

NLL strikes again...

@jackh726
Copy link
Member Author

jackh726 commented Apr 2, 2022

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Apr 2, 2022

📌 Commit 52b00db has been approved by compiler-errors

@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 Apr 2, 2022
@bors
Copy link
Contributor

bors commented Apr 2, 2022

⌛ Testing commit 52b00db with merge 8f96ef4...

@bors
Copy link
Contributor

bors commented Apr 2, 2022

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 8f96ef4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 2, 2022
@bors bors merged commit 8f96ef4 into rust-lang:master Apr 2, 2022
@rustbot rustbot added this to the 1.61.0 milestone Apr 2, 2022
@jackh726 jackh726 deleted the gats_extended_2 branch April 2, 2022 21:25
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8f96ef4): comparison url.

Summary:

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: no relevant changes found. 1 results were found to be statistically significant but too small to be relevant.
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 1 0 0 0
mean2 N/A 0.2% N/A N/A NaN%
max N/A 0.2% N/A N/A 0.0%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@matthiaskrgr matthiaskrgr mentioned this pull request Aug 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
merged-by-bors This PR was explicitly merged by bors. 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
1.61.0
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@jackh726 @rust-log-analyzer @bors @Dylan-DPC @compiler-errors @rust-timer @nikomatsakis @rust-highfive @rustbot