Skip to content

Commit

Permalink
Adress review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
smoelius committed Oct 30, 2022
1 parent 10b7fab commit 2c44398
Show file tree
Hide file tree
Showing 8 changed files with 258 additions and 118 deletions.
81 changes: 31 additions & 50 deletions clippy_lints/src/lifetimes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,6 @@ fn explicit_self_type<'tcx>(cx: &LateContext<'tcx>, func: &FnDecl<'tcx>, ident:
}
}

#[expect(clippy::too_many_lines)]
fn could_use_elision<'tcx>(
cx: &LateContext<'tcx>,
func: &'tcx FnDecl<'_>,
Expand Down Expand Up @@ -331,50 +330,32 @@ fn could_use_elision<'tcx>(
}
}

// no input lifetimes? easy case!
if input_lts.is_empty() {
None
} else if output_lts.is_empty() {
// no output lifetimes, check distinctness of input lifetimes
// A lifetime can be newly elided if:
// - It occurs only once among the inputs.
// - If there are multiple input lifetimes, then the newly elided lifetime does not occur among the
// outputs (because eliding such an lifetime would create an ambiguity).
let elidable_lts = named_lifetime_occurrences(&input_lts)
.into_iter()
.filter_map(|(def_id, occurrences)| {
if occurrences <= 1 && (input_lts.len() <= 1 || !output_lts.contains(&RefLt::Named(def_id))) {
Some((
def_id,
input_visitor
.lifetime_generic_arg_spans
.get(&def_id)
.or_else(|| output_visitor.lifetime_generic_arg_spans.get(&def_id))
.copied(),
))
} else {
None
}
})
.collect::<Vec<_>>();

// only unnamed and static, ok
let unnamed_and_static = input_lts.iter().all(|lt| *lt == RefLt::Unnamed || *lt == RefLt::Static);
if unnamed_and_static {
return None;
}
// we have no output reference, so we can elide explicit lifetimes that occur at most once
let elidable_lts = named_lifetime_occurrences(&input_lts)
.into_iter()
.filter_map(|(def_id, occurrences)| {
if occurrences <= 1 {
Some((def_id, input_visitor.sample_generic_arg_span.get(&def_id).copied()))
} else {
None
}
})
.collect::<Vec<_>>();
if elidable_lts.is_empty() {
None
} else {
Some(elidable_lts)
}
if elidable_lts.is_empty() {
None
} else {
// we have output references, so we need one input reference,
// and all output lifetimes must be the same
if input_lts.len() == 1 {
match (&input_lts[0], &output_lts[0]) {
(&RefLt::Named(n1), &RefLt::Named(n2)) if n1 == n2 => {
Some(vec![(n1, input_visitor.sample_generic_arg_span.get(&n1).copied())])
},
(&RefLt::Named(n), &RefLt::Unnamed) => {
Some(vec![(n, input_visitor.sample_generic_arg_span.get(&n).copied())])
},
_ => None, /* already elided, different named lifetimes
* or something static going on */
}
} else {
None
}
Some(elidable_lts)
}
}

Expand All @@ -397,11 +378,11 @@ fn named_lifetime_occurrences(lts: &[RefLt]) -> Vec<(LocalDefId, usize)> {
let mut occurrences = Vec::new();
for lt in lts {
if let &RefLt::Named(curr_def_id) = lt {
if let Some(i) = occurrences
.iter()
.position(|&(prev_def_id, _)| prev_def_id == curr_def_id)
if let Some(pair) = occurrences
.iter_mut()
.find(|(prev_def_id, _)| *prev_def_id == curr_def_id)
{
occurrences[i].1 += 1;
pair.1 += 1;
} else {
occurrences.push((curr_def_id, 1));
}
Expand All @@ -416,7 +397,7 @@ const CLOSURE_TRAIT_BOUNDS: [LangItem; 3] = [LangItem::Fn, LangItem::FnMut, Lang
struct RefVisitor<'a, 'tcx> {
cx: &'a LateContext<'tcx>,
lts: Vec<RefLt>,
sample_generic_arg_span: FxHashMap<LocalDefId, Span>,
lifetime_generic_arg_spans: FxHashMap<LocalDefId, Span>,
nested_elision_site_lts: Vec<RefLt>,
unelided_trait_object_lifetime: bool,
}
Expand All @@ -426,7 +407,7 @@ impl<'a, 'tcx> RefVisitor<'a, 'tcx> {
Self {
cx,
lts: Vec::new(),
sample_generic_arg_span: FxHashMap::default(),
lifetime_generic_arg_spans: FxHashMap::default(),
nested_elision_site_lts: Vec::new(),
unelided_trait_object_lifetime: false,
}
Expand Down Expand Up @@ -525,7 +506,7 @@ impl<'a, 'tcx> Visitor<'tcx> for RefVisitor<'a, 'tcx> {
if let GenericArg::Lifetime(l) = generic_arg
&& let LifetimeName::Param(def_id, _) = l.name
{
self.sample_generic_arg_span.entry(def_id).or_insert(l.span);
self.lifetime_generic_arg_spans.entry(def_id).or_insert(l.span);
}
// Replace with `walk_generic_arg` if/when https://github.com/rust-lang/rust/pull/103692 lands.
// walk_generic_arg(self, generic_arg);
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/matches/significant_drop_in_scrutinee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ fn set_diagnostic<'tcx>(diag: &mut Diagnostic, cx: &LateContext<'tcx>, expr: &'t

/// If the expression is an `ExprKind::Match`, check if the scrutinee has a significant drop that
/// may have a surprising lifetime.
fn has_significant_drop_in_scrutinee<'tcx, 'a>(
cx: &'a LateContext<'tcx>,
fn has_significant_drop_in_scrutinee<'tcx>(
cx: &LateContext<'tcx>,
scrutinee: &'tcx Expr<'tcx>,
source: MatchSource,
) -> Option<(Vec<FoundSigDrop>, &'static str)> {
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/mut_from_ref.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![allow(unused)]
#![allow(unused, clippy::needless_lifetimes)]
#![warn(clippy::mut_from_ref)]

struct Foo;
Expand Down
90 changes: 79 additions & 11 deletions tests/ui/needless_lifetimes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,20 @@ fn multiple_in_and_out_1<'a>(x: &'a u8, _y: &'a u8) -> &'a u8 {
x
}

// No error; multiple input refs.
fn multiple_in_and_out_2<'a, 'b>(x: &'a u8, _y: &'b u8) -> &'a u8 {
// Error; multiple input refs, but the output lifetime is not elided, i.e., the following is valid:
// fn multiple_in_and_out_2a<'a>(x: &'a u8, _y: &u8) -> &'a u8
// ^^^
fn multiple_in_and_out_2a<'a, 'b>(x: &'a u8, _y: &'b u8) -> &'a u8 {
x
}

// Error; multiple input refs, but the output lifetime is not elided, i.e., the following is valid:
// fn multiple_in_and_out_2b<'b>(_x: &u8, y: &'b u8) -> &'b u8
// ^^^
fn multiple_in_and_out_2b<'a, 'b>(_x: &'a u8, y: &'b u8) -> &'b u8 {
y
}

// No error; multiple input refs
async fn func<'a>(args: &[&'a str]) -> Option<&'a str> {
args.get(0).cloned()
Expand All @@ -44,11 +53,20 @@ fn in_static_and_out<'a>(x: &'a u8, _y: &'static u8) -> &'a u8 {
x
}

// No error.
fn deep_reference_1<'a, 'b>(x: &'a u8, _y: &'b u8) -> Result<&'a u8, ()> {
// Error; multiple input refs, but the output lifetime is not elided, i.e., the following is valid:
// fn deep_reference_1a<'a>(x: &'a u8, _y: &u8) -> Result<&'a u8, ()>
// ^^^
fn deep_reference_1a<'a, 'b>(x: &'a u8, _y: &'b u8) -> Result<&'a u8, ()> {
Ok(x)
}

// Error; multiple input refs, but the output lifetime is not elided, i.e., the following is valid:
// fn deep_reference_1b<'b>(_x: &u8, y: &'b u8) -> Result<&'b u8, ()>
// ^^^
fn deep_reference_1b<'a, 'b>(_x: &'a u8, y: &'b u8) -> Result<&'b u8, ()> {
Ok(y)
}

// No error; two input refs.
fn deep_reference_2<'a>(x: Result<&'a u8, &'a u8>) -> &'a u8 {
x.unwrap()
Expand Down Expand Up @@ -129,11 +147,20 @@ impl X {
&self.x
}

// No error; multiple input refs.
fn self_and_in_out<'s, 't>(&'s self, _x: &'t u8) -> &'s u8 {
// Error; multiple input refs, but the output lifetime is not elided, i.e., the following is valid:
// fn self_and_in_out_1<'s>(&'s self, _x: &u8) -> &'s u8
// ^^^
fn self_and_in_out_1<'s, 't>(&'s self, _x: &'t u8) -> &'s u8 {
&self.x
}

// Error; multiple input refs, but the output lifetime is not elided, i.e., the following is valid:
// fn self_and_in_out_2<'t>(&self, x: &'t u8) -> &'t u8
// ^^^^^
fn self_and_in_out_2<'s, 't>(&'s self, x: &'t u8) -> &'t u8 {
x
}

fn distinct_self_and_in<'s, 't>(&'s self, _x: &'t u8) {}

// No error; same lifetimes on two params.
Expand Down Expand Up @@ -167,8 +194,19 @@ fn struct_with_lt3<'a>(_foo: &Foo<'a>) -> &'a str {
unimplemented!()
}

// No warning; two input lifetimes.
fn struct_with_lt4<'a, 'b>(_foo: &'a Foo<'b>) -> &'a str {
// Warning; two input lifetimes, but the output lifetime is not elided, i.e., the following is
// valid:
// fn struct_with_lt4a<'a>(_foo: &'a Foo<'_>) -> &'a str
// ^^
fn struct_with_lt4a<'a, 'b>(_foo: &'a Foo<'b>) -> &'a str {
unimplemented!()
}

// Warning; two input lifetimes, but the output lifetime is not elided, i.e., the following is
// valid:
// fn struct_with_lt4b<'b>(_foo: &Foo<'b>) -> &'b str
// ^^^^
fn struct_with_lt4b<'a, 'b>(_foo: &'a Foo<'b>) -> &'b str {
unimplemented!()
}

Expand Down Expand Up @@ -203,8 +241,19 @@ fn alias_with_lt3<'a>(_foo: &FooAlias<'a>) -> &'a str {
unimplemented!()
}

// No warning; two input lifetimes.
fn alias_with_lt4<'a, 'b>(_foo: &'a FooAlias<'b>) -> &'a str {
// Warning; two input lifetimes, but the output lifetime is not elided, i.e., the following is
// valid:
// fn alias_with_lt4a<'a>(_foo: &'a FooAlias<'_>) -> &'a str
// ^^
fn alias_with_lt4a<'a, 'b>(_foo: &'a FooAlias<'b>) -> &'a str {
unimplemented!()
}

// Warning; two input lifetimes, but the output lifetime is not elided, i.e., the following is
// valid:
// fn alias_with_lt4b<'b>(_foo: &FooAlias<'b>) -> &'b str
// ^^^^^^^^^
fn alias_with_lt4b<'a, 'b>(_foo: &'a FooAlias<'b>) -> &'b str {
unimplemented!()
}

Expand Down Expand Up @@ -419,12 +468,31 @@ mod issue7296 {
}
}

mod false_negative {
mod pr_9743_false_negative_fix {
#![allow(unused)]

fn foo<'a>(x: &'a u8, y: &'_ u8) {}

fn bar<'a>(x: &'a u8, y: &'_ u8, z: &'_ u8) {}
}

mod pr_9743_output_lifetime_checks {
#![allow(unused)]

// lint: only one input
fn one_input<'a>(x: &'a u8) -> &'a u8 {
unimplemented!()
}

// lint: multiple inputs, output would not be elided
fn multiple_inputs_output_not_elided<'a, 'b>(x: &'a u8, y: &'b u8, z: &'b u8) -> &'b u8 {
unimplemented!()
}

// don't lint: multiple inputs, output would be elided (which would create an ambiguity)
fn multiple_inputs_output_would_be_elided<'a, 'b>(x: &'a u8, y: &'b u8, z: &'b u8) -> &'a u8 {
unimplemented!()
}
}

fn main() {}
Loading

0 comments on commit 2c44398

Please sign in to comment.