Suggest adding named lifetime when the return contains value borrowed from more than one lifetimes of function inputs #105805

yanchen4791 · 2022-12-16T23:59:53Z

The problem: The suggestion of adding an explicit '_ lifetime bound is incorrect when the function's return type contains a value which could be borrowed from more than one lifetimes of the function's inputs. Instead, a named lifetime parameter can be introduced in such a case.

The solution: Checking the number of elided lifetimes in the function signature. If more than one lifetimes found in the function inputs when the suggestion of adding explicit '_ lifetime, change it to using named lifetime parameter 'a instead.

rustbot · 2022-12-17T00:00:00Z

r? @estebank

(rustbot has picked a reviewer for you, use r? to override)

est31

Thanks for the great fix, but the test as it is right now does not prevent regressing the issue I filed: it might still error. Yes, the output is in an stderr file, but often people run --bless when doing refactors or similar, and don't really read what is being changed.

Ideally, the test would run rustfix, ~~but it seems that there is no option for rustfix to apply suggestions that are maybe incorrect: https://github.com/rust-lang/rustfix/issues/200~~ edit: rustfix applies all suggestions, you just have to tell it to apply suggestions. see e.g. src/test/ui/cast/issue-89497.rs

src/test/ui/lifetimes/issue-105227.rs

estebank

r=me after addressing the unwrap and let chain nitpicks

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/static_impl_trait.rs

estebank · 2022-12-21T17:12:07Z

src/test/ui/lifetimes/issue-105227.stderr

+error[E0700]: hidden type for `impl Iterator<Item = char>` captures lifetime that does not appear in bounds
+  --> $DIR/issue-105227.rs:8:5
+   |
+LL | fn chars0(v :(& str, &str)) -> impl Iterator<Item = char> {
+   |                      ---- hidden type `std::iter::Chain<Chars<'_>, Chars<'_>>` captures the anonymous lifetime defined here
+...
+LL |     v.0.chars().chain(v.1.chars())
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: to declare that `impl Iterator<Item = char>` captures `'_`, you can consider introducing a named lifetime parameter `'a`
+   |
+LL | fn chars0<'a>(v :(&'a  str, &'a str)) -> impl Iterator<Item = char> + 'a  {
+   |          ++++      ++        ++                                     ++++


It'd be amazing if we could deduplicate these E0700s based on the primary span, precisely so that we can turn this test into a // run-rustfix test (and reduce verbosity to boot. We might even go further and not just hide the later one but rather incorporate all the spans pointing at the params capturing the hidden lifetimes.

This should be follow up work.

We already have a buffering mechanism, but it occurs at the Diagnostic level. I wonder if we should move these to operate on a higher level enum or struct so that we have more info available to merge multiple errors into one.

rust/compiler/rustc_borrowck/src/diagnostics/region_errors.rs

Lines 215 to 226 in a5cd3bd

RegionErrorKind::UnexpectedHiddenRegion { span, hidden_ty, key, member_region } => {

let named_ty = self.regioncx.name_regions(self.infcx.tcx, hidden_ty);

let named_key = self.regioncx.name_regions(self.infcx.tcx, key);

let named_region = self.regioncx.name_regions(self.infcx.tcx, member_region);

self.buffer_error(unexpected_hidden_region_diagnostic(

self.infcx.tcx,

span,

named_ty,

named_region,

named_key,

));

}

Seeing this I wonder if the deduplication logic is not triggering because of the secondary spans only.

The deduplication logic in fn emit_diagnostic(&mut self, diagnostic: &mut Diagnostic) will not be triggered because the two spans in span_lables field of two diagnostic records are different:

Diagnostic {.., span: MultiSpan { primary_spans: [issue-105227.rs:8:5: 8:35 (#0)], span_labels: [(issue-105227.rs:5:15: 5:20 (#0), Str("hidden type `std::iter::Chain<Chars<'_>, Chars<'_>>` captures the anonymous lifetime defined here"))] }..}

vs

Diagnostic {.., span: MultiSpan { primary_spans: [issue-105227.rs:8:5: 8:35 (#0)], span_labels: [(issue-105227.rs:5:22: 5:26 (#0), Str("hidden type `std::iter::Chain<Chars<'_>, Chars<'_>>` captures the anonymous lifetime defined here"))] }..}

Although the primary_spans and label field of span_labels are the same.
We may take out the span in span_labels when doing hash for diagnostic records to mark them duplicates for the case, but need to take care of any side effects.

I've tried the following code change to deduplicate these E0700s based on the (primary_span, named_ty and named_key) tuple, seems working as expected without side effects:

pub(crate) fn report_region_errors(&mut self, nll_errors: RegionErrors<'tcx>) { // Iterate through all the errors, producing a diagnostic for each one. The diagnostics are // buffered in the `MirBorrowckCtxt`. let mut outlives_suggestion = OutlivesSuggestionBuilder::default(); + let mut last_unexpected_hidden_region: Option<(Span, Ty<'_>, ty::OpaqueTypeKey<'tcx>)> = None; for nll_error in nll_errors.into_iter() { match nll_error { ... ... RegionErrorKind::UnexpectedHiddenRegion { span, hidden_ty, key, member_region } => { let named_ty = self.regioncx.name_regions(self.infcx.tcx, hidden_ty); let named_key = self.regioncx.name_regions(self.infcx.tcx, key); let named_region = self.regioncx.name_regions(self.infcx.tcx, member_region); + if last_unexpected_hidden_region != Some((span, named_ty, named_key)) { self.buffer_error(unexpected_hidden_region_diagnostic( self.infcx.tcx, span, named_ty, named_region, named_key, )); + last_unexpected_hidden_region = Some((span, named_ty, named_key)); + }

All tests passed and including this test with run-rustfix enabled. Is this a reasonable solution for fixing the issue?

The only thing I'd change is in the else branch to still construct the error but call .delay_as_bug() on it. That way, if there's some fluke caused by a future refactor where no errors are emitted we still won't emit an invalid binary.

Thanks for the advice. Added .delay_as_bug() in the else branch.

src/test/ui/lifetimes/issue-105227.rs

estebank · 2022-12-22T02:07:07Z

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/static_impl_trait.rs

+                        spans_suggs
+                            .push((generics.span.shrink_to_hi(), "<'a>".to_string()));


I suspect that with the new requested test this logic will no longer be good enough. Have you looked at the existing suggestion machinery? You might be able to call a function to get this information, instead of having to replicate it here.

The closest function I could found for suggesting adding named lifetime is fn add_missing_lifetime_specifiers_label in compiler/rustc_resolve/src/late/diagnostics.rs, but it cannot be used directly for this purpose. The logic of adding a new lifetime properly without conflicting with existing lifetime in this function could be borrowed here to solve the issue. It seems to require quite a bit of code and probably needs to create a separate function for this, unless such a function already exists which I'm not aware of.

Why can't it be used? Because of the crate it is in? We have freedom to move logic from each crate to an earlier one, if it would still work (an example of something that wouldn't work is if the logic operates on the HIR but all we have available is the AST).

You can use generics.span_for_lifetime_suggestion().
This PR replaces all anonymous lifetimes by a named lifetime. Can we pass the ty::Region corresponding to the hidden region? This could allow to only replace a single lifetime.

Thanks for the advice. I used the logic in generics.span_for_lifetime_suggestion() to identify if the fn contains a lifetime parameter and what the parameter name is. If there are more than one lifetime parameters, I will use the first one.

cjgillot · 2023-01-04T18:37:02Z

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/static_impl_trait.rs

+                        spans_suggs
+                            .push((generics.span.shrink_to_hi(), "<'a>".to_string()));


You can use generics.span_for_lifetime_suggestion().
This PR replaces all anonymous lifetimes by a named lifetime. Can we pass the ty::Region corresponding to the hidden region? This could allow to only replace a single lifetime.

cjgillot · 2023-01-04T18:45:32Z

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/static_impl_trait.rs

+                            .params
+                            .iter()
+                            .filter(|p| p.is_elided_lifetime())
+                            .map(|p| (p.span.shrink_to_hi(), "'a ".to_string()))


This is incorrect in general. If p is '_, this will suggest '_'a. If p is in an elided path like T or T<()>, we'd get weirder suggestions. For hir::Lifetime, there is suggestion_position which allows to chose which case. The same logic applies to GenericParam.

Resolved the '_'a issue by conditionally use span of span.shrink_to_hi() based on the span area size. Since it is elided lifetime in the function inputs, there seems only two possibilities: with or without '_, which can be identified by the span area size. Are there any possibilities to having T or T<()> cases in function input parameters? If there is, could you provide me a test case for it? Thanks!

estebank

Left a few nitpicks, but I'm r=me on this after addressing those.

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/static_impl_trait.rs

src/test/ui/lifetimes/issue-105227.rs

…d from more than one lifetimes of the function's inputs

estebank · 2023-01-06T06:00:48Z

@bors r+

bors · 2023-01-06T06:00:50Z

📌 Commit 523fe7a has been approved by estebank

It is now in the queue for this repository.

bors · 2023-01-06T12:08:52Z

⌛ Testing commit 523fe7a with merge 7bbbaab...

bors · 2023-01-06T15:20:51Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 7bbbaab to master...

rust-timer · 2023-01-06T16:39:31Z

Finished benchmarking commit (7bbbaab): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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)	3.2%	[3.0%, 3.3%]	2
Regressions ❌ (secondary)	2.7%	[1.0%, 3.8%]	40
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.2%	[3.0%, 3.3%]	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
Improvements ✅ (secondary)	-3.0%	[-5.3%, -1.9%]	3
All ❌✅ (primary)	-	-	0

rustbot assigned estebank Dec 16, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 17, 2022

est31 requested changes Dec 20, 2022

View reviewed changes

src/test/ui/lifetimes/issue-105227.rs Outdated Show resolved Hide resolved

est31 mentioned this pull request Dec 20, 2022

cargo fix: Improve handling of MaybeIncorrect suggestions rust-lang/cargo#13028

Open

yanchen4791 force-pushed the issue-105227-fix branch from ffc7685 to 8297c20 Compare December 20, 2022 19:11

estebank reviewed Dec 21, 2022

View reviewed changes

estebank added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 21, 2022

yanchen4791 force-pushed the issue-105227-fix branch from 8297c20 to 20dae2f Compare December 21, 2022 23:13

estebank reviewed Dec 22, 2022

View reviewed changes

src/test/ui/lifetimes/issue-105227.rs Show resolved Hide resolved

estebank reviewed Dec 22, 2022

View reviewed changes

estebank approved these changes Dec 22, 2022

View reviewed changes

cjgillot reviewed Jan 4, 2023

View reviewed changes

yanchen4791 force-pushed the issue-105227-fix branch from 20dae2f to ec4390c Compare January 5, 2023 23:19

estebank approved these changes Jan 6, 2023

View reviewed changes

Suggests adding named lifetime when the return contains value borrowe…

523fe7a

…d from more than one lifetimes of the function's inputs

yanchen4791 force-pushed the issue-105227-fix branch from ec4390c to 523fe7a Compare January 6, 2023 05:11

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 6, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 6, 2023

bors merged commit 7bbbaab into rust-lang:master Jan 6, 2023

rustbot added this to the 1.68.0 milestone Jan 6, 2023

bors mentioned this pull request Jan 6, 2023

Add hint for missing lifetime bound on trait object when type alias is used #105345

Merged

yanchen4791 deleted the issue-105227-fix branch January 13, 2023 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest adding named lifetime when the return contains value borrowed from more than one lifetimes of function inputs #105805

Suggest adding named lifetime when the return contains value borrowed from more than one lifetimes of function inputs #105805

yanchen4791 commented Dec 16, 2022

rustbot commented Dec 17, 2022

est31 left a comment •

edited

Loading

estebank left a comment

estebank Dec 21, 2022

estebank Dec 21, 2022

yanchen4791 Dec 21, 2022

yanchen4791 Dec 22, 2022

estebank Jan 4, 2023

yanchen4791 Jan 5, 2023

estebank Dec 22, 2022

yanchen4791 Dec 22, 2022

estebank Jan 4, 2023

cjgillot Jan 4, 2023

yanchen4791 Jan 5, 2023

cjgillot Jan 4, 2023

cjgillot Jan 4, 2023

yanchen4791 Jan 6, 2023

estebank left a comment

estebank commented Jan 6, 2023

bors commented Jan 6, 2023

bors commented Jan 6, 2023

bors commented Jan 6, 2023

rust-timer commented Jan 6, 2023

	RegionErrorKind::UnexpectedHiddenRegion { span, hidden_ty, key, member_region } => {
	let named_ty = self.regioncx.name_regions(self.infcx.tcx, hidden_ty);
	let named_key = self.regioncx.name_regions(self.infcx.tcx, key);
	let named_region = self.regioncx.name_regions(self.infcx.tcx, member_region);
	self.buffer_error(unexpected_hidden_region_diagnostic(
	self.infcx.tcx,
	span,
	named_ty,
	named_region,
	named_key,
	));
	}

		spans_suggs
		.push((generics.span.shrink_to_hi(), "<'a>".to_string()));

Suggest adding named lifetime when the return contains value borrowed from more than one lifetimes of function inputs #105805

Suggest adding named lifetime when the return contains value borrowed from more than one lifetimes of function inputs #105805

Conversation

yanchen4791 commented Dec 16, 2022

rustbot commented Dec 17, 2022

est31 left a comment • edited Loading

Choose a reason for hiding this comment

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank left a comment

Choose a reason for hiding this comment

estebank commented Jan 6, 2023

bors commented Jan 6, 2023

bors commented Jan 6, 2023

bors commented Jan 6, 2023

rust-timer commented Jan 6, 2023

Overall result: no relevant changes - no action needed

est31 left a comment •

edited

Loading