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

Handle type errors in closure/generator upvar_tys #78432

Merged
merged 2 commits into from
Oct 30, 2020
Merged

Handle type errors in closure/generator upvar_tys #78432

merged 2 commits into from
Oct 30, 2020

Conversation

arora-aman
Copy link
Member

Fixes #77993

@arora-aman @roxelo
Handle type errors in closure/generator upvar_tys
f0ae24e 
Co-authored-by: Roxane Fruytier <roxane.fruytier@hotmail.com>
@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 27, 2020
@arora-aman
Copy link
Member Author

r? @nikomatsakis

@arora-aman arora-aman mentioned this pull request Oct 27, 2020
Closed
hameerabbasi
hameerabbasi reviewed Oct 27, 2020
View reviewed changes
compiler/rustc_middle/Cargo.toml Outdated
@@ -8,6 +8,7 @@ edition = "2018"
doctest = false

[dependencies]
either = "1.5.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note, this is an extra dependency, don't know how willing we are to accept that.

nikomatsakis
nikomatsakis requested changes Oct 27, 2020
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.

This seems pretty reasonable, but I have a suggestion to avoid the Either dependency.

compiler/rustc_middle/src/ty/sty.rs Outdated
#[inline]
pub fn upvar_tys(self) -> impl Iterator<Item = Ty<'tcx>> + 'tcx {
self.tupled_upvars_ty().tuple_fields()
match self.tupled_upvars_ty().kind() {
TyKind::Error(_) => Either::Left(std::iter::empty()),
Copy link
Contributor

Choose a reason for hiding this comment

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

May I suggest returning None here...

Copy link
Member

Choose a reason for hiding this comment

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

Fixed

compiler/rustc_middle/src/ty/sty.rs Outdated
self.tupled_upvars_ty().tuple_fields()
match self.tupled_upvars_ty().kind() {
TyKind::Error(_) => Either::Left(std::iter::empty()),
TyKind::Tuple(..) => Either::Right(self.tupled_upvars_ty().tuple_fields()),
Copy link
Contributor

Choose a reason for hiding this comment

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

...and Some(self.tupled_upvars_ty().tuple_fields()) here...

Copy link
Member

Choose a reason for hiding this comment

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

Fixed

compiler/rustc_middle/src/ty/sty.rs
TyKind::Tuple(..) => Either::Right(self.tupled_upvars_ty().tuple_fields()),
TyKind::Infer(_) => bug!("upvar_tys called before capture types are inferred"),
ty => bug!("Unexpected representation of upvar types tuple {:?}", ty),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

and then calling .into_iter().flatten() here, then you can avoid the Either dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed

compiler/rustc_middle/src/ty/sty.rs Outdated
#[inline]
pub fn upvar_tys(self) -> impl Iterator<Item = Ty<'tcx>> + 'tcx {
self.tupled_upvars_ty().tuple_fields()
match self.tupled_upvars_ty().kind() {
TyKind::Error(_) => Either::Left(std::iter::empty()),
Copy link
Contributor

Choose a reason for hiding this comment

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

As above.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed

arora-aman
arora-aman commented Oct 27, 2020
View reviewed changes
Copy link
Member Author

@arora-aman arora-aman left a comment

Choose a reason for hiding this comment

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

I think we can get rid of the last Either as well

compiler/rustc_middle/src/ty/sty.rs Outdated Show resolved Hide resolved
@roxelo roxelo mentioned this pull request Oct 27, 2020
Closed
@arora-aman arora-aman mentioned this pull request Oct 28, 2020
Closed
@nikomatsakis
Copy link
Contributor

@bors r+ p=1

Giving p=1 because fixes a P-high regression.

@bors
Copy link
Contributor

bors commented Oct 28, 2020

📌 Commit 5229571 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 28, 2020
@jonas-schievink jonas-schievink mentioned this pull request Oct 28, 2020
Closed
@bors
Copy link
Contributor

bors commented Oct 29, 2020

⌛ Testing commit 5229571 with merge 794cbe28f8b07229c3ad9a8e05849706276bc6c4...

@bors
Copy link
Contributor

bors commented Oct 29, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 29, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 29, 2020

@bors retry

Unclear what causes this error, but it seems to be spurious. https://zulip-archive.rust-lang.org/242791tinfra/66128applex8664ghachecks.html

@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 Oct 29, 2020
@bors
Copy link
Contributor

bors commented Oct 29, 2020

⌛ Testing commit 5229571 with merge 9aa73f5280f6909336dcfba74e35c9000444c7d0...

@bors
Copy link
Contributor

bors commented Oct 29, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 29, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 30, 2020

@bors retry

GitHub internal error :/

@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 Oct 30, 2020
@camelid camelid added A-closures Area: Closures (`|…| { … }`) A-coroutines Area: Coroutines labels Oct 30, 2020
@bors
Copy link
Contributor

bors commented Oct 30, 2020

⌛ Testing commit 5229571 with merge 0d33ab7...

@bors
Copy link
Contributor

bors commented Oct 30, 2020

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 0d33ab7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 30, 2020
@bors bors merged commit 0d33ab7 into rust-lang:master Oct 30, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 30, 2020
@arora-aman arora-aman deleted the fix-77993-take3 branch October 30, 2020 05:35
@Mark-Simulacrum
Copy link
Member

This seems to be a win on the match-stress benchmark -- up to 6% -- though this is relatively surprising, and it does not appear to be noise. Did we expect such an effect? A cursory look over the PR doesn't suggest an obvious reason, so maybe we missed something in the implementation? CPU cycles and wall times also improved.

@jyn514 jyn514 mentioned this pull request Nov 4, 2020
Closed
@nikomatsakis
Copy link
Contributor

Somewhat surprising indeed.

@Mark-Simulacrum
Copy link
Member

This was discussed briefly in compiler triage and settled as likely fine, though no particular explanations arose either.

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

@hameerabbasi hameerabbasi hameerabbasi left review comments

@roxelo roxelo roxelo left review comments

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
A-closures Area: Closures (`|…| { … }`) A-coroutines Area: Coroutines 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.
Projects
None yet
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.

ICE: tuple_fields called on non-tuple: async fn with unknown macro