Use a `Field` in `ConstraintCategory::ClosureUpvar` #94006

pierwill · 2022-02-14T22:24:19Z

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

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

pierwill · 2022-02-15T23:48:06Z

UI tests passing locally! 🎉

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

pierwill · 2022-02-16T16:28:18Z

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

@@ -421,17 +421,26 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {

        diag.span_label(*span, message);

-        // FIXME(project-rfc-2229#48): This should store a captured_place not a hir id


Should we keep this comment?

no, I think it is addressed by using the field actually

pierwill · 2022-02-16T16:37:40Z

compiler/rustc_borrowck/src/type_check/mod.rs

@@ -2530,9 +2530,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
            body,
        );
        let category = if let Some(field) = field {
-            let var_hir_id = self.borrowck_context.upvars[field.index()].place.get_root_variable();
-            // FIXME(project-rfc-2229#8): Use Place for better diagnostics


Should we keep this comment?

pierwill · 2022-02-17T17:08:20Z

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.

👍 👍

nikomatsakis · 2022-02-18T22:09:05Z

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 {


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

nikomatsakis · 2022-02-18T22:09:16Z

compiler/rustc_borrowck/src/type_check/mod.rs

@@ -2530,9 +2530,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
            body,
        );
        let category = if let Some(field) = field {
-            let var_hir_id = self.borrowck_context.upvars[field.index()].place.get_root_variable();
-            // FIXME(project-rfc-2229#8): Use Place for better diagnostics


nikomatsakis · 2022-02-18T22:11:19Z

@bors r+

bors · 2022-02-18T22:11:20Z

📌 Commit f41722a has been approved by nikomatsakis

…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

rust-highfive assigned nikomatsakis Feb 14, 2022

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 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 14, 2022

pierwill force-pushed the upvar-field branch from 1b27144 to 041ba10 Compare February 14, 2022 22:30

This comment has been minimized.

Sign in to view

pierwill force-pushed the upvar-field branch from 041ba10 to 87dbccd Compare February 15, 2022 17:24

This comment was marked as off-topic.

Sign in to view

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

pierwill commented Feb 15, 2022

View reviewed changes

compiler/rustc_borrowck/src/diagnostics/region_errors.rs Outdated Show resolved Hide resolved

pierwill force-pushed the upvar-field branch from 030d8ae to 3c499dd Compare February 15, 2022 23:52

nikomatsakis requested changes Feb 16, 2022

View reviewed changes

compiler/rustc_borrowck/src/diagnostics/region_errors.rs Outdated Show resolved Hide resolved

pierwill force-pushed the upvar-field branch from 3c499dd to 99e54c7 Compare February 16, 2022 16:05

pierwill commented Feb 16, 2022

View reviewed changes

pierwill requested a review from nikomatsakis February 16, 2022 17:57

pierwill force-pushed the upvar-field branch from 99e54c7 to ec1df02 Compare February 16, 2022 21:55

Use a Field in ConstraintCategory::ClosureUpvar

f41722a

pierwill force-pushed the upvar-field branch from ec1df02 to f41722a Compare February 16, 2022 21:57

pierwill mentioned this pull request Feb 17, 2022

Document rustc_middle::mir::Field #94025

Closed

nikomatsakis approved these changes Feb 18, 2022

View reviewed changes

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 mentioned this pull request Feb 19, 2022

Rollup of 7 pull requests #94148

Merged

bors merged commit c28940e into rust-lang:master Feb 19, 2022

rustbot added this to the 1.60.0 milestone Feb 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a `Field` in `ConstraintCategory::ClosureUpvar` #94006

Use a `Field` in `ConstraintCategory::ClosureUpvar` #94006

pierwill commented Feb 14, 2022 •

edited

Loading

This comment has been minimized.

This comment was marked as off-topic.

This comment has been minimized.

This comment was marked as outdated.

pierwill commented Feb 15, 2022

pierwill Feb 16, 2022

nikomatsakis Feb 18, 2022

pierwill Feb 16, 2022

nikomatsakis Feb 18, 2022

pierwill commented Feb 17, 2022 •

edited

Loading

nikomatsakis Feb 18, 2022

nikomatsakis Feb 18, 2022

nikomatsakis commented Feb 18, 2022

bors commented Feb 18, 2022

		@@ -421,17 +421,26 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {

		diag.span_label(*span, message);

		// FIXME(project-rfc-2229#48): This should store a captured_place not a hir id

Use a Field in ConstraintCategory::ClosureUpvar #94006

Use a Field in ConstraintCategory::ClosureUpvar #94006

Conversation

pierwill commented Feb 14, 2022 • edited Loading

This comment has been minimized.

This comment was marked as off-topic.

This comment has been minimized.

This comment was marked as outdated.

pierwill commented Feb 15, 2022

pierwill Feb 16, 2022

Choose a reason for hiding this comment

nikomatsakis Feb 18, 2022

Choose a reason for hiding this comment

pierwill Feb 16, 2022

Choose a reason for hiding this comment

nikomatsakis Feb 18, 2022

Choose a reason for hiding this comment

pierwill commented Feb 17, 2022 • edited Loading

nikomatsakis Feb 18, 2022

Choose a reason for hiding this comment

nikomatsakis Feb 18, 2022

Choose a reason for hiding this comment

nikomatsakis commented Feb 18, 2022

bors commented Feb 18, 2022

Use a `Field` in `ConstraintCategory::ClosureUpvar` #94006

Use a `Field` in `ConstraintCategory::ClosureUpvar` #94006

pierwill commented Feb 14, 2022 •

edited

Loading

pierwill commented Feb 17, 2022 •

edited

Loading