Skip to content
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

Suggest adding a new lifetime parameter when two elided lifetimes should match up for traits and impls. #94464

Merged
merged 3 commits into from
Mar 2, 2022
Merged

Suggest adding a new lifetime parameter when two elided lifetimes should match up for traits and impls. #94464

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
abcccc9
Suggest adding a new lifetime parameter when two elided lifetimes sho…
kckeiks Feb 28, 2022
270730f
add suggestion to update trait if error is in impl
kckeiks Mar 1, 2022
1b08cba
update (bless) test results
kckeiks Mar 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 48 additions & 52 deletions compiler/rustc_infer/src/infer/error_reporting/nice_region_error/different_lifetimes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,59 +166,55 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
if lifetime_sub.name.is_elided() && lifetime_sup.name.is_elided() {
if let Some(anon_reg) = self.tcx().is_suitable_region(sub) {
let hir_id = self.tcx().hir().local_def_id_to_hir_id(anon_reg.def_id);
if let hir::Node::Item(&hir::Item {
kind: hir::ItemKind::Fn(_, ref generics, ..),
..
}) = self.tcx().hir().get(hir_id)
{
let (suggestion_param_name, introduce_new) = generics
.params
.iter()
.find(|p| matches!(p.kind, GenericParamKind::Lifetime { .. }))
.and_then(|p| self.tcx().sess.source_map().span_to_snippet(p.span).ok())
.map(|name| (name, false))
.unwrap_or_else(|| ("'a".to_string(), true));

let mut suggestions = vec![
if let hir::LifetimeName::Underscore = lifetime_sub.name {
(lifetime_sub.span, suggestion_param_name.clone())
} else {
(
lifetime_sub.span.shrink_to_hi(),
suggestion_param_name.clone() + " ",
)
},
if let hir::LifetimeName::Underscore = lifetime_sup.name {
(lifetime_sup.span, suggestion_param_name.clone())
} else {
(
lifetime_sup.span.shrink_to_hi(),
suggestion_param_name.clone() + " ",
)
},
];

if introduce_new {
let new_param_suggestion = match &generics.params {
[] => (generics.span, format!("<{}>", suggestion_param_name)),
[first, ..] => (
first.span.shrink_to_lo(),
format!("{}, ", suggestion_param_name),
),
};

suggestions.push(new_param_suggestion);
}

err.multipart_suggestion(
"consider introducing a named lifetime parameter",
suggestions,
Applicability::MaybeIncorrect,
);
err.note(
"each elided lifetime in input position becomes a distinct lifetime",
);

let generics = match self.tcx().hir().get(hir_id) {
hir::Node::Item(&hir::Item {
kind: hir::ItemKind::Fn(_, ref generics, ..),
..
})
| hir::Node::TraitItem(&hir::TraitItem { ref generics, .. })
| hir::Node::ImplItem(&hir::ImplItem { ref generics, .. }) => generics,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should handle the impl case specifically with extra wording in the suggestion to let people know that they'll have to modify the trait as well. This is likely not a problem in practice, but I'd like us to be thorough and avoid confusing people.

It might be enough to just have a boolean flag that you use to modify the suggestion description with extra wording.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I'll make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this for impl?

help: consider introducing a named lifetime parameter and update trait if needed
   |
32 |     fn foo<'a>(&'a self, x: &i32, z: &'a i32) -> Option<&i32> {
   |           ++++  ++                    ++

error: aborting due to previous error

For more information about this error, try `rustc --explain E0623`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we would have something like "consider introducing a named lifetime parameter and update trait TraitName as necessary", by getting the TraitName from the parent DefId, but that can be left as follow up work.

_ => return,
};

let (suggestion_param_name, introduce_new) = generics
.params
.iter()
.find(|p| matches!(p.kind, GenericParamKind::Lifetime { .. }))
.and_then(|p| self.tcx().sess.source_map().span_to_snippet(p.span).ok())
.map(|name| (name, false))
.unwrap_or_else(|| ("'a".to_string(), true));

let mut suggestions = vec![
if let hir::LifetimeName::Underscore = lifetime_sub.name {
(lifetime_sub.span, suggestion_param_name.clone())
} else {
(lifetime_sub.span.shrink_to_hi(), suggestion_param_name.clone() + " ")
},
if let hir::LifetimeName::Underscore = lifetime_sup.name {
(lifetime_sup.span, suggestion_param_name.clone())
} else {
(lifetime_sup.span.shrink_to_hi(), suggestion_param_name.clone() + " ")
},
];

if introduce_new {
let new_param_suggestion = match &generics.params {
[] => (generics.span, format!("<{}>", suggestion_param_name)),
[first, ..] => {
(first.span.shrink_to_lo(), format!("{}, ", suggestion_param_name))
}
};

suggestions.push(new_param_suggestion);
}

err.multipart_suggestion(
"consider introducing a named lifetime parameter",
suggestions,
Applicability::MaybeIncorrect,
);
err.note("each elided lifetime in input position becomes a distinct lifetime");
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/test/ui/issues/issue-17728.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ LL | fn attemptTraverse(&self, room: &Room, directionStr: &str) -> Result<&R
...
LL | Some(entry) => Ok(entry),
| ^^^^^^^^^ ...but data from `room` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn attemptTraverse<'a>(&'a self, room: &'a Room, directionStr: &str) -> Result<&Room, &str> {
| ++++ ++ ++

error[E0308]: `match` arms have incompatible types
--> $DIR/issue-17728.rs:109:14
Expand Down
6 changes: 6 additions & 0 deletions src/test/ui/lifetimes/lifetime-errors/ex3-both-anon-regions-return-type-is-anon.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ LL | fn foo<'a>(&self, x: &i32) -> &i32 {
| this parameter and the return type are declared with different lifetimes...
LL | x
| ^ ...but data from `x` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn foo<'a>(&'a self, x: &'a i32) -> &i32 {
| ++ ++

error: aborting due to previous error

Expand Down
6 changes: 6 additions & 0 deletions src/test/ui/lifetimes/lifetime-errors/ex3-both-anon-regions-self-is-anon.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ LL | fn foo<'a>(&self, x: &Foo) -> &Foo {
| this parameter and the return type are declared with different lifetimes...
LL | if true { x } else { self }
| ^ ...but data from `x` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn foo<'a>(&'a self, x: &'a Foo) -> &Foo {
| ++ ++

error: aborting due to previous error

Expand Down
6 changes: 6 additions & 0 deletions src/test/ui/lifetimes/lifetime-errors/ex3-both-anon-regions-using-impl-items.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ LL | fn foo(x: &mut Vec<&u8>, y: &u8) {
| --- --- these two types are declared with different lifetimes...
LL | x.push(y);
| ^ ...but data from `y` flows into `x` here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn foo<'a>(x: &mut Vec<&'a u8>, y: &'a u8) {
| ++++ ++ ++

error: aborting due to previous error

Expand Down
12 changes: 12 additions & 0 deletions src/test/ui/self/arbitrary_self_types_pin_lifetime_mismatch.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ LL | fn a(self: Pin<&Foo>, f: &Foo) -> &Foo { f }
| ---- ---- ^ ...but data from `f` is returned here
| |
| this parameter and the return type are declared with different lifetimes...
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn a<'a>(self: Pin<&'a Foo>, f: &'a Foo) -> &Foo { f }
| ++++ ++ ++
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm somewhat surprised by the suggestions I'm seeing that we don't also suggest adding 'a to the return type. Maybe an existing bug? No need to handle it in this case directly, but would we should make a note of it.

Copy link
Contributor Author

@kckeiks kckeiks Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do you want me to add a comment about that note?

I'm not 100% sure it's a bug. Seems like the code that added the hint did not take into consideration return types. To be fair though, AFAICT the diagnosis in this module is implemented very differently than what is in LifetimeContext in add_missing_lifetime_specifiers_label (which does add lifetime hints for the return type). LifetimeContext has a lot more "information", which it gathers during compilation, about the error so it can construct a better hint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just create a targeted ticket for this instead of leaving it in the code. It'd be p-low, as we already have follow up suggestions to get you to working code, although it isn't great when there's another lifetime involved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@estebank I created #94492 and made the changes you suggested.


error[E0623]: lifetime mismatch
--> $DIR/arbitrary_self_types_pin_lifetime_mismatch.rs:8:76
Expand All @@ -13,6 +19,12 @@ LL | fn c(self: Pin<&Self>, f: &Foo, g: &Foo) -> (Pin<&Foo>, &Foo) { (self,
| ---- ----------------- ^ ...but data from `f` is returned here
| |
| this parameter and the return type are declared with different lifetimes...
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn c<'a>(self: Pin<&'a Self>, f: &'a Foo, g: &Foo) -> (Pin<&Foo>, &Foo) { (self, f) }
| ++++ ++ ++

error[E0623]: lifetime mismatch
--> $DIR/arbitrary_self_types_pin_lifetime_mismatch.rs:13:58
Expand Down
36 changes: 36 additions & 0 deletions src/test/ui/self/elision/lt-ref-self.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ LL | fn ref_self(&self, f: &u32) -> &u32 {
| this parameter and the return type are declared with different lifetimes...
LL | f
| ^ ...but data from `f` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn ref_self<'a>(&'a self, f: &'a u32) -> &u32 {
| ++++ ++ ++

error[E0623]: lifetime mismatch
--> $DIR/lt-ref-self.rs:17:9
Expand All @@ -17,6 +23,12 @@ LL | fn ref_Self(self: &Self, f: &u32) -> &u32 {
| this parameter and the return type are declared with different lifetimes...
LL | f
| ^ ...but data from `f` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn ref_Self<'a>(self: &'a Self, f: &'a u32) -> &u32 {
| ++++ ++ ++

error[E0623]: lifetime mismatch
--> $DIR/lt-ref-self.rs:21:9
Expand All @@ -27,6 +39,12 @@ LL | fn box_ref_Self(self: Box<&Self>, f: &u32) -> &u32 {
| this parameter and the return type are declared with different lifetimes...
LL | f
| ^ ...but data from `f` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn box_ref_Self<'a>(self: Box<&'a Self>, f: &'a u32) -> &u32 {
| ++++ ++ ++

error[E0623]: lifetime mismatch
--> $DIR/lt-ref-self.rs:25:9
Expand All @@ -37,6 +55,12 @@ LL | fn pin_ref_Self(self: Pin<&Self>, f: &u32) -> &u32 {
| this parameter and the return type are declared with different lifetimes...
LL | f
| ^ ...but data from `f` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn pin_ref_Self<'a>(self: Pin<&'a Self>, f: &'a u32) -> &u32 {
| ++++ ++ ++

error[E0623]: lifetime mismatch
--> $DIR/lt-ref-self.rs:29:9
Expand All @@ -47,6 +71,12 @@ LL | fn box_box_ref_Self(self: Box<Box<&Self>>, f: &u32) -> &u32 {
| this parameter and the return type are declared with different lifetimes...
LL | f
| ^ ...but data from `f` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn box_box_ref_Self<'a>(self: Box<Box<&'a Self>>, f: &'a u32) -> &u32 {
| ++++ ++ ++

error[E0623]: lifetime mismatch
--> $DIR/lt-ref-self.rs:33:9
Expand All @@ -57,6 +87,12 @@ LL | fn box_pin_Self(self: Box<Pin<&Self>>, f: &u32) -> &u32 {
| this parameter and the return type are declared with different lifetimes...
LL | f
| ^ ...but data from `f` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn box_pin_Self<'a>(self: Box<Pin<&'a Self>>, f: &'a u32) -> &u32 {
| ++++ ++ ++

error: aborting due to 6 previous errors

Expand Down
36 changes: 36 additions & 0 deletions src/test/ui/self/elision/ref-mut-self.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ LL | fn ref_self(&mut self, f: &u32) -> &u32 {
| this parameter and the return type are declared with different lifetimes...
LL | f
| ^ ...but data from `f` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn ref_self<'a>(&'a mut self, f: &'a u32) -> &u32 {
| ++++ ++ ++

error[E0623]: lifetime mismatch
--> $DIR/ref-mut-self.rs:17:9
Expand All @@ -17,6 +23,12 @@ LL | fn ref_Self(self: &mut Self, f: &u32) -> &u32 {
| this parameter and the return type are declared with different lifetimes...
LL | f
| ^ ...but data from `f` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn ref_Self<'a>(self: &'a mut Self, f: &'a u32) -> &u32 {
| ++++ ++ ++

error[E0623]: lifetime mismatch
--> $DIR/ref-mut-self.rs:21:9
Expand All @@ -27,6 +39,12 @@ LL | fn box_ref_Self(self: Box<&mut Self>, f: &u32) -> &u32 {
| this parameter and the return type are declared with different lifetimes...
LL | f
| ^ ...but data from `f` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn box_ref_Self<'a>(self: Box<&'a mut Self>, f: &'a u32) -> &u32 {
| ++++ ++ ++

error[E0623]: lifetime mismatch
--> $DIR/ref-mut-self.rs:25:9
Expand All @@ -37,6 +55,12 @@ LL | fn pin_ref_Self(self: Pin<&mut Self>, f: &u32) -> &u32 {
| this parameter and the return type are declared with different lifetimes...
LL | f
| ^ ...but data from `f` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn pin_ref_Self<'a>(self: Pin<&'a mut Self>, f: &'a u32) -> &u32 {
| ++++ ++ ++

error[E0623]: lifetime mismatch
--> $DIR/ref-mut-self.rs:29:9
Expand All @@ -47,6 +71,12 @@ LL | fn box_box_ref_Self(self: Box<Box<&mut Self>>, f: &u32) -> &u32 {
| this parameter and the return type are declared with different lifetimes...
LL | f
| ^ ...but data from `f` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn box_box_ref_Self<'a>(self: Box<Box<&'a mut Self>>, f: &'a u32) -> &u32 {
| ++++ ++ ++

error[E0623]: lifetime mismatch
--> $DIR/ref-mut-self.rs:33:9
Expand All @@ -57,6 +87,12 @@ LL | fn box_pin_ref_Self(self: Box<Pin<&mut Self>>, f: &u32) -> &u32 {
| this parameter and the return type are declared with different lifetimes...
LL | f
| ^ ...but data from `f` is returned here
|
= note: each elided lifetime in input position becomes a distinct lifetime
help: consider introducing a named lifetime parameter
|
LL | fn box_pin_ref_Self<'a>(self: Box<Pin<&'a mut Self>>, f: &'a u32) -> &u32 {
| ++++ ++ ++

error: aborting due to 6 previous errors

Expand Down
Loading