Skip to content

rework applying closure requirements in borrowck#102618

Merged
bors merged 2 commits into
rust-lang:masterfrom
aliemjay:simplify-closure-promote
Nov 6, 2022
Merged

rework applying closure requirements in borrowck#102618
bors merged 2 commits into
rust-lang:masterfrom
aliemjay:simplify-closure-promote

Conversation

@aliemjay

@aliemjay aliemjay commented Oct 3, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Previously the promoted closure constraints were registered under the category ConstraintCategory::ClosureBounds in type_check::prove_closure_bounds() and then mapped back their original category in regions_infer::best_blame_constraint using the complicated map closure_bounds_mapping.

Now we're registering promoted constraints under their original category and span earlier in type_check::prove_closure_bounds.

See commit messages.

Fixes #99245

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 3, 2022
@rust-highfive

rust-highfive commented Oct 3, 2022

Copy link
Copy Markdown
Contributor

r? @jackh726

(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 3, 2022
@aliemjay aliemjay marked this pull request as ready for review October 3, 2022 12:10
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@aliemjay aliemjay force-pushed the simplify-closure-promote branch 2 times, most recently from fc12ee6 to f69db63 Compare October 7, 2022 17:58
cjgillot
cjgillot reviewed Oct 8, 2022
View reviewed changes
Comment thread src/test/ui/generic-associated-types/issue-91139.rs Outdated
aliemjay
aliemjay commented Oct 8, 2022
View reviewed changes
Comment thread src/test/ui/nll/ty-outlives/projection-two-region-trait-bound-closure.stderr Outdated

@aliemjay aliemjay Oct 8, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is another minor diagnostic regression, but I believe it never was an intended behavior of the previous setup. Actually at the point of rendering diagnostics the intention was to not have a constraint with ClosureBounds category, but here we've somehow missed converting ClosureBounds constraints.

Anyway this regression only affect the niche case where there is double lifetime bounds in assoc type definition (Trait<'a, 'b>::Assoc: 'a + 'b) AND the bound X: Trait<'a, 'a> appears in param env.

@bors

bors commented Oct 12, 2022

Copy link
Copy Markdown
Collaborator

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

@aliemjay aliemjay force-pushed the simplify-closure-promote branch from f69db63 to 4ddd1eb Compare October 13, 2022 06:34
@bors

bors commented Oct 20, 2022

Copy link
Copy Markdown
Collaborator

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

@aliemjay aliemjay force-pushed the simplify-closure-promote branch from 4ddd1eb to 1e5eba9 Compare October 31, 2022 18:14
@aliemjay

aliemjay commented Nov 1, 2022

Copy link
Copy Markdown
Contributor Author

r? @compiler-errors who recently touched promoted closure constraints. Feel free to reassign.

@rustbot rustbot assigned compiler-errors and unassigned jackh726 Nov 1, 2022
@bors

bors commented Nov 4, 2022

Copy link
Copy Markdown
Collaborator

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

@compiler-errors

compiler-errors commented Nov 4, 2022

Copy link
Copy Markdown
Contributor

This change makes sense to me, and the second commit does have some good diagnostics improvements. r=me after rebasing.

@aliemjay
simplify applying closure requirements
df668b9 
Don't use `ConstraintCategory::ClosureBounds`!
Set the category and the span for the promoted constraints to that of
the original constraint earlier than before.
This eliminates the need for `closure_bounds_mapping`.
@aliemjay
use spans in TypeTest rather than mir::Location
02f78fd 
Spans are independent of the body being borrow-checked, so they don't
need remapping when promoting type-tests and they yield more specific
error spans inside bodies of closures/inline consts.
@aliemjay aliemjay force-pushed the simplify-closure-promote branch from 1e5eba9 to 02f78fd Compare November 5, 2022 04:37
@aliemjay

aliemjay commented Nov 5, 2022

Copy link
Copy Markdown
Contributor Author

@bors r=compiler-errors

@bors

bors commented Nov 5, 2022

Copy link
Copy Markdown
Collaborator

@aliemjay: 🔑 Insufficient privileges: Not in reviewers

@aliemjay

aliemjay commented Nov 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Ah no bors delegate command, then ping @compiler-errors to review.

@jackh726

jackh726 commented Nov 5, 2022

Copy link
Copy Markdown
Member

@bors r=compiler-errors

@bors

bors commented Nov 5, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit 02f78fd has been approved by compiler-errors

It is now in the queue for this repository.

@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, 2022
@bors

bors commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 02f78fd with merge e30fb6a...

@bors

bors commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 6, 2022
@bors bors merged commit e30fb6a into rust-lang:master Nov 6, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 6, 2022
@bors bors mentioned this pull request Nov 6, 2022
Merged
@rust-timer

rust-timer commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (e30fb6a): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -1.3% [-1.4%, -1.2%] 2
Improvements ✅
(secondary)		 -3.0% [-4.0%, -0.2%] 7
All ❌✅ (primary) -1.3% [-1.4%, -1.2%] 2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.1% [-0.5%, 0.4%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.4% [-0.4%, -0.4%] 1
Improvements ✅
(secondary)		 -2.4% [-2.6%, -2.0%] 5
All ❌✅ (primary) -0.4% [-0.4%, -0.4%] 1

@aliemjay aliemjay deleted the simplify-closure-promote branch November 6, 2022 10:03
@Mark-Simulacrum

Mark-Simulacrum commented Nov 8, 2022

Copy link
Copy Markdown
Member

Changes in performance are probably noise given the set of benchmarks.

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@bors
Auto merge of rust-lang#102618 - aliemjay:simplify-closure-promote, r…
a1891bf 
…=compiler-errors

rework applying closure requirements in borrowck

Previously the promoted closure constraints were registered under the category `ConstraintCategory::ClosureBounds` in `type_check::prove_closure_bounds()` and then mapped back their original category in `regions_infer::best_blame_constraint` using the complicated map `closure_bounds_mapping`.

Now we're registering promoted constraints under their original category and span earlier in `type_check::prove_closure_bounds`.

See commit messages.

Fixes rust-lang#99245
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@compiler-errors compiler-errors

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.67.0

Development

Successfully merging this pull request may close these issues.

[NLL] loss of span for type outlives errors in closures/async blocks

10 participants

@aliemjay @rust-highfive @rust-log-analyzer @bors @compiler-errors @jackh726 @rust-timer @Mark-Simulacrum @cjgillot @rustbot