Adress review comments

Author: smoelius

clippy_lints/src/lifetimes.rs

@@ -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<'_>,
@@ -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)
     }
 }
 
@@ -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));
             }
@@ -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,
 }
@@ -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,
         }
@@ -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);

clippy_lints/src/matches/significant_drop_in_scrutinee.rs

@@ -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)> {

tests/ui/mut_from_ref.rs

@@ -1,4 +1,4 @@
-#![allow(unused)]
+#![allow(unused, clippy::needless_lifetimes)]
 #![warn(clippy::mut_from_ref)]
 
 struct Foo;

tests/ui/needless_lifetimes.rs

@@ -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()
@@ -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()
@@ -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.
@@ -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!()
 }
 
@@ -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!()
 }
 
@@ -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() {}