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

Normalize obligations for closure confirmation #88441

Merged
merged 2 commits into from
Nov 6, 2021
Merged

Normalize obligations for closure confirmation #88441

merged 2 commits into from
Nov 6, 2021

Conversation

jackh726
Copy link
Member

@jackh726 jackh726 commented Aug 28, 2021 •
edited
Loading

Based on #90017

Fixes #74261
Fixes #71955
Fixes #88459

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 28, 2021
jackh726
jackh726 commented Aug 28, 2021
View reviewed changes
...test/ui/higher-rank-trait-bounds/normalize-under-binder/normalization-under-binders-2.stderr Outdated
@@ -0,0 +1,92 @@
error: implementation of `Parser` is not general enough
Copy link
Member Author

Choose a reason for hiding this comment

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

The duplication here is not nice, but I'd rather leave this for a followup PR.

jackh726
jackh726 commented Aug 28, 2021
View reviewed changes
src/test/ui/higher-rank-trait-bounds/normalize-under-binder/normalization-under-binders-2.rs Outdated
Comment on lines 1 to 2
// ignore-compare-mode-nll
// fails in migrate, passes in nll
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit weird, not sure

Copy link
Contributor

Choose a reason for hiding this comment

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

I spent some time digging into this but I'm not sure what's going on yet.

@camelid camelid added the A-closures Area: Closures (`|…| { … }`) label Sep 11, 2021
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 13, 2021
@bors
Copy link
Contributor

bors commented Sep 17, 2021

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

This was referenced Sep 26, 2021
Open
Closed
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2021
@nikomatsakis
Copy link
Contributor

@bors r+

In our last discussion, @jackh726 and I figured out what was going wrong here, but we also decided it's a pre-existing bug that we should deal with separately. The problem is that, in the case of an impl with type parameters whose value is derived from an associated type binding like U in T: Iterator<Item = U>, you can wind up with a "normalized result" that is an unresolved type parameter (?U) that will be determined by the resulting obligations. We fail to map the placeholders in that value back to their bound form. This bug results from @jackh726 and I having overlooked this possibility and erroneously concluding that there would never be unbound type parameters in the resulting type that didn't arise from the input. D'oh!

@bors
Copy link
Contributor

bors commented Nov 5, 2021

📌 Commit cacc3ee 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 Nov 5, 2021
@bors
Copy link
Contributor

bors commented Nov 6, 2021

⌛ Testing commit cacc3ee with merge 18cae26...

@bors
Copy link
Contributor

bors commented Nov 6, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 18cae26 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 6, 2021
@bors bors merged commit 18cae26 into rust-lang:master Nov 6, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 6, 2021
@jackh726 jackh726 deleted the closure_norm branch November 6, 2021 05:43
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (18cae26): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

@aliemjay aliemjay mentioned this pull request Nov 14, 2021
Closed
@jackh726 jackh726 restored the closure_norm branch March 12, 2022 18:30
@jackh726 jackh726 deleted the closure_norm branch March 12, 2022 18:34
@jackh726 jackh726 restored the closure_norm branch March 12, 2022 18:42
@jackh726 jackh726 deleted the closure_norm branch March 12, 2022 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis left review comments

Assignees

@nikomatsakis nikomatsakis

Labels
A-closures Area: Closures (`|…| { … }`) 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.58.0
8 participants
@jackh726 @bors @nikomatsakis @rust-timer @apiraino @rust-highfive @camelid @rustbot