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

Do not propose to elide lifetimes if this causes an ambiguity #13929

Merged
merged 1 commit into from
Jan 10, 2025
Merged

Do not propose to elide lifetimes if this causes an ambiguity #13929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
39 changes: 39 additions & 0 deletions clippy_lints/src/lifetimes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,11 +482,13 @@ fn has_where_lifetimes<'tcx>(cx: &LateContext<'tcx>, generics: &'tcx Generics<'_
false
}

#[allow(clippy::struct_excessive_bools)]
struct Usage {
lifetime: Lifetime,
in_where_predicate: bool,
in_bounded_ty: bool,
in_generics_arg: bool,
lifetime_elision_impossible: bool,
}

struct LifetimeChecker<'cx, 'tcx, F> {
Expand All @@ -495,6 +497,7 @@ struct LifetimeChecker<'cx, 'tcx, F> {
where_predicate_depth: usize,
bounded_ty_depth: usize,
generic_args_depth: usize,
lifetime_elision_impossible: bool,
phantom: std::marker::PhantomData<F>,
}

Expand All @@ -519,6 +522,7 @@ where
where_predicate_depth: 0,
bounded_ty_depth: 0,
generic_args_depth: 0,
lifetime_elision_impossible: false,
xFrednet marked this conversation as resolved.
Show resolved Hide resolved
phantom: std::marker::PhantomData,
}
}
Expand Down Expand Up @@ -560,6 +564,7 @@ where
in_where_predicate: self.where_predicate_depth != 0,
in_bounded_ty: self.bounded_ty_depth != 0,
in_generics_arg: self.generic_args_depth != 0,
lifetime_elision_impossible: self.lifetime_elision_impossible,
});
}
}
Expand All @@ -586,11 +591,44 @@ where
self.generic_args_depth -= 1;
}

fn visit_fn_decl(&mut self, fd: &'tcx FnDecl<'tcx>) -> Self::Result {
self.lifetime_elision_impossible = !is_candidate_for_elision(fd);
samueltardieu marked this conversation as resolved.
Show resolved Hide resolved
walk_fn_decl(self, fd);
self.lifetime_elision_impossible = false;
}

fn nested_visit_map(&mut self) -> Self::Map {
self.cx.tcx.hir()
}
}

/// Check if `fd` supports function elision with an anonymous (or elided) lifetime,
/// and has a lifetime somewhere in its output type.
fn is_candidate_for_elision(fd: &FnDecl<'_>) -> bool {
struct V;

impl Visitor<'_> for V {
type Result = ControlFlow<bool>;

fn visit_lifetime(&mut self, lifetime: &Lifetime) -> Self::Result {
ControlFlow::Break(lifetime.is_elided() || lifetime.is_anonymous())
Copy link
Member

Choose a reason for hiding this comment

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

This currently breaks on the first lifetime it finds. Meaning that (x: Cx<'_, 'tcx>) would say it's anonymous, right?

It feels like this should only break if it finds a user-defined lifetime 🤔

Could you also add a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but:

  • when checking the output type, we don't care about the lifetime being anonymous (we just want to know if we have at least one lifetime)
  • we check the input types only when the function supports lifetime elision, which is not the case here because we have at least two input lifetimes ('_ and 'tcx)

Does that make sense? I've added a test, let me know if this is what you had in mind.

Copy link
Member

Choose a reason for hiding this comment

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

It make sense, I didn't know that two LTs implied that they can't be elapsed

}
}

if fd.lifetime_elision_allowed
&& let Return(ret_ty) = fd.output
&& walk_ty(&mut V, ret_ty).is_break()
{
// The first encountered input lifetime will either be one on `self`, or will be the only lifetime.
fd.inputs
.iter()
.find_map(|ty| walk_ty(&mut V, ty).break_value())
.unwrap()
} else {
false
}
}

fn report_extra_lifetimes<'tcx>(cx: &LateContext<'tcx>, func: &'tcx FnDecl<'_>, generics: &'tcx Generics<'_>) {
let mut checker = LifetimeChecker::<hir_nested_filter::None>::new(cx, generics);

Expand Down Expand Up @@ -656,6 +694,7 @@ fn report_elidable_impl_lifetimes<'tcx>(
Usage {
lifetime,
in_where_predicate: false,
lifetime_elision_impossible: false,
..
},
] = usages.as_slice()
Expand Down
81 changes: 81 additions & 0 deletions tests/ui/needless_lifetimes.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,4 +576,85 @@ mod issue13749bis {
impl<'a, T: 'a> Generic<T> {}
}

mod issue13923 {
struct Py<'py> {
data: &'py str,
}

enum Content<'t, 'py> {
Py(Py<'py>),
T1(&'t str),
T2(&'t str),
}

enum ContentString<'t> {
T1(&'t str),
T2(&'t str),
}

impl<'t, 'py> ContentString<'t> {
// `'py` cannot be elided
fn map_content1(self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, 'py> {
match self {
Self::T1(content) => Content::T1(f(content)),
Self::T2(content) => Content::T2(f(content)),
}
}
}

impl<'t> ContentString<'t> {
// `'py` can be elided because of `&self`
fn map_content2(&self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, '_> {
match self {
Self::T1(content) => Content::T1(f(content)),
Self::T2(content) => Content::T2(f(content)),
}
}
}

impl<'t> ContentString<'t> {
// `'py` can be elided because of `&'_ self`
fn map_content3(&'_ self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, '_> {
match self {
Self::T1(content) => Content::T1(f(content)),
Self::T2(content) => Content::T2(f(content)),
}
}
}

impl<'t, 'py> ContentString<'t> {
// `'py` should not be elided as the default lifetime, even if working, could be named as `'t`
fn map_content4(self, f: impl FnOnce(&'t str) -> &'t str, o: &'t str) -> Content<'t, 'py> {
match self {
Self::T1(content) => Content::T1(f(content)),
Self::T2(_) => Content::T2(o),
}
}
}

impl<'t> ContentString<'t> {
// `'py` can be elided because of `&Self`
fn map_content5(
self: std::pin::Pin<&Self>,
f: impl FnOnce(&'t str) -> &'t str,
o: &'t str,
) -> Content<'t, '_> {
match *self {
Self::T1(content) => Content::T1(f(content)),
Self::T2(_) => Content::T2(o),
}
}
}

struct Cx<'a, 'b> {
a: &'a u32,
b: &'b u32,
}

// `'c` cannot be elided because we have several input lifetimes
fn one_explicit<'b>(x: Cx<'_, 'b>) -> &'b u32 {
x.b
}
}

fn main() {}
81 changes: 81 additions & 0 deletions tests/ui/needless_lifetimes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,4 +576,85 @@ mod issue13749bis {
impl<'a, T: 'a> Generic<T> {}
}

mod issue13923 {
struct Py<'py> {
data: &'py str,
}

enum Content<'t, 'py> {
Py(Py<'py>),
T1(&'t str),
T2(&'t str),
}

enum ContentString<'t> {
T1(&'t str),
T2(&'t str),
}

impl<'t, 'py> ContentString<'t> {
// `'py` cannot be elided
fn map_content1(self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, 'py> {
match self {
Self::T1(content) => Content::T1(f(content)),
Self::T2(content) => Content::T2(f(content)),
}
}
}

impl<'t, 'py> ContentString<'t> {
// `'py` can be elided because of `&self`
fn map_content2(&self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, 'py> {
match self {
Self::T1(content) => Content::T1(f(content)),
Self::T2(content) => Content::T2(f(content)),
}
}
}

impl<'t, 'py> ContentString<'t> {
// `'py` can be elided because of `&'_ self`
fn map_content3(&'_ self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, 'py> {
match self {
Self::T1(content) => Content::T1(f(content)),
Self::T2(content) => Content::T2(f(content)),
}
}
}

impl<'t, 'py> ContentString<'t> {
// `'py` should not be elided as the default lifetime, even if working, could be named as `'t`
fn map_content4(self, f: impl FnOnce(&'t str) -> &'t str, o: &'t str) -> Content<'t, 'py> {
match self {
Self::T1(content) => Content::T1(f(content)),
Self::T2(_) => Content::T2(o),
}
}
}

impl<'t, 'py> ContentString<'t> {
// `'py` can be elided because of `&Self`
fn map_content5(
self: std::pin::Pin<&Self>,
f: impl FnOnce(&'t str) -> &'t str,
o: &'t str,
) -> Content<'t, 'py> {
match *self {
Self::T1(content) => Content::T1(f(content)),
Self::T2(_) => Content::T2(o),
}
}
}

struct Cx<'a, 'b> {
a: &'a u32,
b: &'b u32,
}

// `'c` cannot be elided because we have several input lifetimes
fn one_explicit<'b>(x: Cx<'_, 'b>) -> &'b u32 {
&x.b
}
}

fn main() {}
61 changes: 60 additions & 1 deletion tests/ui/needless_lifetimes.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,5 +576,64 @@ LL - fn one_input<'a>(x: &'a u8) -> &'a u8 {
LL + fn one_input(x: &u8) -> &u8 {
|

error: aborting due to 48 previous errors
error: the following explicit lifetimes could be elided: 'py
--> tests/ui/needless_lifetimes.rs:605:14
|
LL | impl<'t, 'py> ContentString<'t> {
| ^^^
LL | // `'py` can be elided because of `&self`
LL | fn map_content2(&self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, 'py> {
| ^^^
|
help: elide the lifetimes
|
LL ~ impl<'t> ContentString<'t> {
LL | // `'py` can be elided because of `&self`
LL ~ fn map_content2(&self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, '_> {
|

error: the following explicit lifetimes could be elided: 'py
--> tests/ui/needless_lifetimes.rs:615:14
|
LL | impl<'t, 'py> ContentString<'t> {
| ^^^
LL | // `'py` can be elided because of `&'_ self`
LL | fn map_content3(&'_ self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, 'py> {
| ^^^
|
help: elide the lifetimes
|
LL ~ impl<'t> ContentString<'t> {
LL | // `'py` can be elided because of `&'_ self`
LL ~ fn map_content3(&'_ self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, '_> {
|

error: the following explicit lifetimes could be elided: 'py
--> tests/ui/needless_lifetimes.rs:635:14
|
LL | impl<'t, 'py> ContentString<'t> {
| ^^^
...
LL | ) -> Content<'t, 'py> {
| ^^^
|
help: elide the lifetimes
|
LL ~ impl<'t> ContentString<'t> {
LL | // `'py` can be elided because of `&Self`
...
LL | o: &'t str,
LL ~ ) -> Content<'t, '_> {
|

error: this expression creates a reference which is immediately dereferenced by the compiler
--> tests/ui/needless_lifetimes.rs:656:9
|
LL | &x.b
| ^^^^ help: change this to: `x.b`
|
= note: `-D clippy::needless-borrow` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::needless_borrow)]`

error: aborting due to 52 previous errors

Loading