Refactor `region_name`: add `RegionNameHighlight` #74661

SNCPlay42 · 2020-07-22T23:33:54Z

This PR does not change any diagnostics itself, rather it enables further code changes, but I would like to get approval for the refactoring first before making use of it.

In rustc_mir::borrow_check::diagnostics::region_name, there is code that allows for, when giving a synthesized name like '1 to an anonymous lifetime, pointing at e.g. the exact '&' that introduces the lifetime.

This PR decouples that code from the specific case of arguments, adding a new enum RegionNameHighlight, enabling future changes to use it in other places.

This allows:

We could change the other AnonRegionFrom* variants to use RegionNameHighlight to precisely point at where lifetimes are introduced in other locations when they have type annotations, e.g. a closure return |...| -> &i32.
- Because of how async functions are lowered this affects async functions as well, see Error message in async fn refers to lifetime '1 without defining it #74072
for Explain elision when it is relevant to lifetime errors #74597, we could add a second, optional RegionNameHighlight to the AnonRegionFromArgument variant that highlights a lifetime in the return type of a function when, due to elision, this is the same as the argument lifetime.
in Unclear lifetime error in closure producing a future #74497 (comment) I noticed that a diagnostic was trying to introduce a lifetime '2 in the opaque type impl std::future::Future. The code for the case of arguments has code to handle cases like this but not the others. This refactoring would allow the same code path to handle this.
- It might be appropriate to add another variant of RegionNameHighlight to say something like lifetime '1 appears in the opaque type impl std::future::Future.

These are quite a few changes so I thought I would make sure the refactoring is OK before I start making changes that rely on it. :)

out of highlight_if_we_can_match_hir_ty_from_argument, which is then renamed

rust-highfive · 2020-07-22T23:33:58Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

SNCPlay42 · 2020-07-22T23:43:41Z

src/librustc_mir/borrow_check/diagnostics/region_errors.rs

-        match self.give_region_a_name(*outlived_fr).unwrap().source {
-            RegionNameSource::NamedEarlyBoundRegion(fr_span)
-            | RegionNameSource::NamedFreeRegion(fr_span)
-            | RegionNameSource::SynthesizedFreeEnvRegion(fr_span, _)
-            | RegionNameSource::CannotMatchHirTy(fr_span, _)
-            | RegionNameSource::MatchedHirTy(fr_span)
-            | RegionNameSource::MatchedAdtAndSegment(fr_span)
-            | RegionNameSource::AnonRegionFromUpvar(fr_span, _)
-            | RegionNameSource::AnonRegionFromOutput(fr_span, _, _) => {
-                diag.span_label(fr_span, "inferred to be a `FnMut` closure");
-            }
-            _ => {}


This match was missing a few variants that have spans (AnonRegionFromYieldTy/AnonRegionFromAsyncFn). I don't think that was deliberate, just that they were allowed to be absent by the underscore pattern. On the other hand I'm not sure that they would be used in this code.

SNCPlay42 · 2020-07-22T23:55:03Z

src/librustc_mir/borrow_check/diagnostics/region_name.rs

+            .and_then(|arg_hir_ty| self.highlight_if_we_can_match_hir_ty(fr, arg_ty, arg_hir_ty))
+            .or_else(|| {
+                // `highlight_if_we_cannot_match_hir_ty` needs to know the number we will give to
+                // the anonymous region. If it succeeds, the `synthesize_region_name` call below
+                // will increment the counter, "reserving" the number we just used.
+                let counter = *self.next_region_name.try_borrow().unwrap();
+                self.highlight_if_we_cannot_match_hir_ty(fr, arg_ty, span, counter)
+            })


The idea is that this bit (from the highlight_if_we_can_match_hir_ty call down) can be repeated to create RegionNameHighlights in other places. I would have extracted this into its own function in this PR, but I'm a bit concerned about this business with counter, and if increasing the syntactic distance from the synthesize_region_name call will make things unclear.

estebank · 2020-07-23T23:31:19Z

@bors r+

These look like they will be quite useful. Really glad to see these changes.

bors · 2020-07-23T23:31:21Z

📌 Commit 8a776ee has been approved by estebank

…r=estebank Refactor `region_name`: add `RegionNameHighlight` This PR does not change any diagnostics itself, rather it enables further code changes, but I would like to get approval for the refactoring first before making use of it. In `rustc_mir::borrow_check::diagnostics::region_name`, there is code that allows for, when giving a synthesized name like `'1` to an anonymous lifetime, pointing at e.g. the exact '`&`' that introduces the lifetime. This PR decouples that code from the specific case of arguments, adding a new enum `RegionNameHighlight`, enabling future changes to use it in other places. This allows: * We could change the other `AnonRegionFrom*` variants to use `RegionNameHighlight` to precisely point at where lifetimes are introduced in other locations when they have type annotations, e.g. a closure return `|...| -> &i32`. * Because of how async functions are lowered this affects async functions as well, see rust-lang#74072 * for rust-lang#74597, we could add a second, optional `RegionNameHighlight` to the `AnonRegionFromArgument` variant that highlights a lifetime in the return type of a function when, due to elision, this is the same as the argument lifetime. * in rust-lang#74497 (comment) I noticed that a diagnostic was trying to introduce a lifetime `'2` in the opaque type `impl std::future::Future`. The code for the case of arguments has [code to handle cases like this](https://github.com/rust-lang/rust/blob/bbebe7351fcd29af1eb9a35e315369b15887ea09/src/librustc_mir/borrow_check/diagnostics/region_name.rs#L365) but not the others. This refactoring would allow the same code path to handle this. * It might be appropriate to add another variant of `RegionNameHighlight` to say something like `lifetime '1 appears in the opaque type impl std::future::Future`. These are quite a few changes so I thought I would make sure the refactoring is OK before I start making changes that rely on it. :)

@ghost

…arth Rollup of 8 pull requests Successful merges: - rust-lang#72954 (revise RwLock for HermitCore) - rust-lang#74367 (Rearrange the pipeline of `pow` to gain efficiency) - rust-lang#74491 (Optimize away BitAnd and BitOr when possible) - rust-lang#74639 (Downgrade glibc to 2.11.1 for ppc, ppc64 and s390x) - rust-lang#74661 (Refactor `region_name`: add `RegionNameHighlight`) - rust-lang#74692 (delay_span_bug instead of silent ignore) - rust-lang#74698 (fixed error reporting for mismatched traits) - rust-lang#74715 (Add a system for creating diffs across multiple mir optimizations.) Failed merges: r? @ghost

SNCPlay42 added 8 commits July 22, 2020 23:52

add RegionName::span

c276334

extract RegionNameHighlight

51af5af

change returns to RegionNameHighlight

601518e

rename functions

723ea90

clean up give_name_if_anonymous_region_appears_in_arguments

b56f5b9

decouple highlight_if_we_cannot_match_hir_ty

a7450b7

move highlight_if_we_can_match_hir_ty call

ebb4aba

out of highlight_if_we_can_match_hir_ty_from_argument, which is then renamed

rename arguments to highlight_if_we_can_match_hir_ty

8a776ee

rust-highfive assigned estebank Jul 22, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2020

SNCPlay42 commented Jul 22, 2020

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 Jul 23, 2020

Dylan-DPC-zz mentioned this pull request Jul 24, 2020

Rollup of 10 pull requests #74719

Closed

Manishearth mentioned this pull request Jul 24, 2020

Rollup of 8 pull requests #74724

Merged

bors merged commit ceaef73 into rust-lang:master Jul 24, 2020

SNCPlay42 deleted the lifetime-names-refactor branch July 24, 2020 20:12

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `region_name`: add `RegionNameHighlight` #74661

Refactor `region_name`: add `RegionNameHighlight` #74661

SNCPlay42 commented Jul 22, 2020 •

edited

Loading

rust-highfive commented Jul 22, 2020

SNCPlay42 Jul 22, 2020

SNCPlay42 Jul 22, 2020

estebank commented Jul 23, 2020

bors commented Jul 23, 2020

Refactor region_name: add RegionNameHighlight #74661

Refactor region_name: add RegionNameHighlight #74661

Conversation

SNCPlay42 commented Jul 22, 2020 • edited Loading

rust-highfive commented Jul 22, 2020

SNCPlay42 Jul 22, 2020

Choose a reason for hiding this comment

SNCPlay42 Jul 22, 2020

Choose a reason for hiding this comment

estebank commented Jul 23, 2020

bors commented Jul 23, 2020

Refactor `region_name`: add `RegionNameHighlight` #74661

Refactor `region_name`: add `RegionNameHighlight` #74661

SNCPlay42 commented Jul 22, 2020 •

edited

Loading