Skip to content

Use a Field in ConstraintCategory::ClosureUpvar #94006

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
Merged
merged 1 commit into from
Feb 19, 2022
Merged

Use a Field in ConstraintCategory::ClosureUpvar #94006

merged 1 commit into from
Feb 19, 2022

Conversation

@pierwill
Copy link
Contributor

@pierwill pierwill commented Feb 14, 2022 •
edited
Loading

As part of #90317, we do not want HirId to implement Ord, PartialOrd. This line of code has made that difficult

https://github.com/rust-lang/rust/blob/1b27144afc77031ba9c05d86c06c64485589775a/compiler/rustc_borrowck/src/region_infer/mod.rs#L2184

since it sorts a ConstraintCategory::ClosureUpvar(HirId).

This PR makes that variant take a Field instead.

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

This comment has been minimized.

Sign in to view
@pierwill

This comment was marked as off-topic.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pierwill

This comment was marked as outdated.

Sign in to view
pierwill
pierwill commented Feb 15, 2022
View reviewed changes
@pierwill
Copy link
Contributor Author

pierwill commented Feb 15, 2022

UI tests passing locally! 🎉
nikomatsakis
nikomatsakis requested changes Feb 16, 2022
View reviewed changes
pierwill
pierwill commented Feb 16, 2022
View reviewed changes
compiler/rustc_borrowck/src/diagnostics/region_errors.rs Outdated
Copy link
Contributor Author

@pierwill pierwill Feb 16, 2022

Choose a reason for hiding this comment

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

Should we keep this comment?
Copy link
Contributor

@nikomatsakis nikomatsakis Feb 18, 2022

Choose a reason for hiding this comment

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

no, I think it is addressed by using the field actually
pierwill
pierwill commented Feb 16, 2022
View reviewed changes
compiler/rustc_borrowck/src/type_check/mod.rs Outdated
Copy link
Contributor Author

@pierwill pierwill Feb 16, 2022

Choose a reason for hiding this comment

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

Should we keep this comment?
Copy link
Contributor

@nikomatsakis nikomatsakis Feb 18, 2022

Choose a reason for hiding this comment

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

no
@pierwill
Copy link
Contributor Author

pierwill commented Feb 17, 2022 •
edited
Loading

Conversation regarding correctness of this PR on Zulip:

@pierwill: Another PR related to #90317 (ordering): #94006. I was so eager to clear away sorting of HirIds that I didn't fully think through whether the type introduced here would have the same problems. Is rustc_middle::mir::Field safe for incr. comp. in the sense of #90317?

@cgillot: Field represents the position of a data field inside an datatype. This type exists for documentation and type-safety purposes. It derives HashStable in a straightforward manner: the index is hashed, without any elaborate hashing scheme like DefId and ExpnId do. So Field is fine for incr comp.

👍 👍
@pierwill pierwill mentioned this pull request Feb 17, 2022
Closed
nikomatsakis
nikomatsakis approved these changes Feb 18, 2022
View reviewed changes
compiler/rustc_borrowck/src/diagnostics/region_errors.rs
diag.span_label(upvar_def_span, "variable defined here");
diag.span_label(upvar_span, "variable captured here");
let captured_place = &self.upvars[upvar_field.index()].place;
let defined_hir = match captured_place.place.base {
Copy link
Contributor

@nikomatsakis nikomatsakis Feb 18, 2022

Choose a reason for hiding this comment

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

to some extent that comment here to this now, because the thing we've captured is really a full place, not just the base of the place
compiler/rustc_borrowck/src/type_check/mod.rs Outdated
Copy link
Contributor

@nikomatsakis nikomatsakis Feb 18, 2022

Choose a reason for hiding this comment

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

no
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 18, 2022

@bors r+
@bors
Copy link
Collaborator

bors commented Feb 18, 2022

📌 Commit f41722a 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 Feb 18, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 19, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2022
@bors
Auto merge of rust-lang#94148 - matthiaskrgr:rollup-jgea68f, r=matthi…
e08d569 
…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#92902 (Improve the documentation of drain members) - rust-lang#93658 (Stabilize `#[cfg(panic = "...")]`) - rust-lang#93954 (rustdoc-json: buffer output) - rust-lang#93979 (Add debug assertions to validate NUL terminator in c strings) - rust-lang#93990 (pre rust-lang#89862 cleanup) - rust-lang#94006 (Use a `Field` in `ConstraintCategory::ClosureUpvar`) - rust-lang#94086 (Fix ScalarInt to char conversion) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
@bors bors merged commit c28940e into rust-lang:master Feb 19, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

6 participants

@pierwill @rust-log-analyzer @nikomatsakis @bors @rust-highfive @rustbot