Add validness checks

rust-lang · Jan 12, 2023 · b523269 · b523269
1 parent bf6fd45
commit b523269
Show file tree

Hide file tree

Showing 3 changed files with 132 additions and 34 deletions.
diff --git a/clippy_lints/src/functions/hidden_static_lifetime.rs b/clippy_lints/src/functions/hidden_static_lifetime.rs
@@ -1,8 +1,8 @@
 use crate::rustc_lint::LintContext;
 use clippy_utils::diagnostics::span_lint_and_help;
 use rustc_hir::{
-    intravisit::FnKind, FnDecl, FnRetTy, GenericArg, GenericBound, GenericParamKind, LifetimeParamKind, ParamName,
-    QPath, Ty, TyKind, TypeBindingKind, WherePredicate,
+    intravisit::FnKind, FnDecl, FnRetTy, GenericArg, GenericBound, GenericParam, GenericParamKind, Generics,
+    LifetimeParamKind, ParamName, QPath, Ty, TyKind, TypeBindingKind, WherePredicate,
 };
 use rustc_lint::LateContext;
 use rustc_middle::lint::in_external_macro;
@@ -31,18 +31,21 @@ pub(super) fn check_fn<'tcx>(cx: &LateContext<'_>, kind: FnKind<'tcx>, decl: &'t
 						}
 					} else {
 						lifetime_is_used = check_if_uses_lifetime(input, &generic.name);
-					}
-				}
+					};
+				};
 
 				if !lifetime_is_used {
 					// Check that lifetime is used in return type.
-					if let FnRetTy::Return(_) = decl.output {
+					if let FnRetTy::Return(ret_ty) = decl.output {
 						// Check that it isn't used in `where` predicate
 						for predicate in generics.predicates {
 							// Check for generic types: `where A: 'a`
 							if let WherePredicate::BoundPredicate(bound_predicate) = predicate {
 								for bound in bound_predicate.bounds {
-									if let GenericBound::Outlives(_) = bound {
+									if let GenericBound::Outlives(lifetime) = bound {
+										if lifetime.ident.name == rustc_span::symbol::kw::StaticLifetime {
+											continue;
+										}
 										lifetime_is_used = true;
 									} else {
 										// Check that generic isn't X<A = B>.
@@ -64,9 +67,9 @@ pub(super) fn check_fn<'tcx>(cx: &LateContext<'_>, kind: FnKind<'tcx>, decl: &'t
 												None,
 								&format!("try removing the lifetime parameter `{}` and changing references to `'static`", generic.name.ident().as_str()),
 											);
-										}
-									}
-								}
+										};
+									};
+								};
 							} else {
 								// Check for other lifetimes
 								if let WherePredicate::RegionPredicate(region_predicate) = predicate {
@@ -80,27 +83,29 @@ pub(super) fn check_fn<'tcx>(cx: &LateContext<'_>, kind: FnKind<'tcx>, decl: &'t
 											None,
 								&format!("try removing the lifetime parameter `{}` and changing references to `'static`", generic.name.ident().as_str()),
 										);
-									}
-								}
-							}
-						}
+									};
+								};
+							};
+						};
 
 						// Check again.
 						if !lifetime_is_used {
-							span_lint_and_help(cx,
-								HIDDEN_STATIC_LIFETIME,
-								generic.span,
-								"this lifetime can be changed to `'static`",
-								None,
-								&format!("try removing the lifetime parameter `{}` and changing references to `'static`", generic.name.ident().as_str()),
-							);
-						}
-					}
-				}
-
-			}
-        }
-    }
+							// Check validness
+							if check_validness(ret_ty, generic, generics) {
+								span_lint_and_help(cx,
+									HIDDEN_STATIC_LIFETIME,
+									generic.span,
+									"this lifetime can be changed to `'static`",
+									None,
+									&format!("try removing the lifetime parameter `{}` and changing references to `'static`", generic.name.ident().as_str()),
+								);
+							};
+						};
+					};
+				};
+			};
+        };
+    };
 }
 
 fn check_if_uses_lifetime(input: &Ty<'_>, generic_name: &ParamName) -> bool {
@@ -115,11 +120,49 @@ fn check_if_uses_lifetime(input: &Ty<'_>, generic_name: &ParamName) -> bool {
                     if let ParamName::Plain(ident) = generic_name {
                         if lifetime.ident.name == ident.name {
                             return true;
+                        };
+                    };
+                };
+            }
+        }
+    };
+    false
+}
+
+fn check_validness(ret_ty: &Ty<'_>, generic: &GenericParam<'_>, generics: &Generics<'_>) -> bool {
+    // (#10123) Comment by @Jarcho explains what "valid" means.
+    // The lint doesn't check invalid return types, because not every lifetime can be changed to 'static
+    // without problems.
+    if let TyKind::BareFn(barefn) = ret_ty.peel_refs().kind {
+        for input in barefn.decl.inputs {
+            if let TyKind::Rptr(lifetime, _) = input.kind {
+                if lifetime.ident.name == generic.name.ident().name {
+                    return false;
+                }
+            }
+        }
+    }
+
+    if let TyKind::Rptr(_, mut_ty) = &ret_ty.kind {
+        // Check for &'a mut &'b T
+        if mut_ty.mutbl.is_mut() {
+            // This path diverges:
+            // * &'a mut &'b T : Not valid
+            // * &'a mut T : Only if T: 'static (Checked before)
+            if let TyKind::Rptr(_, _) = &mut_ty.ty.kind {
+                return false;
+            };
+
+            for predicate in generics.predicates {
+                for bound in predicate.bounds() {
+                    if let GenericBound::Outlives(lifetime) = bound {
+                        if !lifetime.is_static() {
+                            return false;
                         }
                     }
                 }
             }
         }
-    }
-    false
+    };
+    true
 }
diff --git a/tests/ui/hidden_static_lifetime.rs b/tests/ui/hidden_static_lifetime.rs
@@ -12,7 +12,7 @@ mod module {
     }
 }
 
-// Should warn
+// ============= Should warn =============
 
 fn a<'a>() -> &'a str {
     ""
@@ -21,7 +21,20 @@ fn h<'h>() -> S<'h> {
     S { s: &1 }
 }
 
-// Should not warn
+// Valid
+fn o<'o, T>() -> &'o mut T
+where
+    T: 'static,
+{
+    unsafe { std::ptr::null::<&mut T>().read() }
+}
+
+// Only 'm1
+fn n<'m1, 'm2, T>() -> &'m1 fn(&'m2 T) {
+    unsafe { std::ptr::null::<&'m1 fn(&'m2 T)>().read() }
+}
+
+// ============= Should not warn =============
 fn b<'b>(_: &'b str) -> &'b str {
     ""
 }
@@ -43,8 +56,26 @@ fn i() -> S<'static> {
     S { s: &1 }
 }
 
-fn j<'a>(s: &module::S<'a>) -> S<'a> {
-    todo!()
+fn j<'j>(s: &module::S<'j>) -> S<'j> {
+    S { s: &1 }
+}
+
+// Invalid
+fn k<'k1, 'k2, T>() -> &'k1 mut &'k2 T {
+    unsafe { std::ptr::null::<&mut &T>().read() }
+}
+
+// Invalid
+fn m<'m, T>() -> fn(&'m T) {
+    unsafe { std::ptr::null::<fn(&'m T)>().read() }
+}
+
+// Valid
+fn l<'l, T>() -> &'l mut T
+where
+    T: 'static,
+{
+    unsafe { std::ptr::null::<&mut T>().read() }
 }
 
 fn main() {}
diff --git a/tests/ui/hidden_static_lifetime.stderr b/tests/ui/hidden_static_lifetime.stderr
@@ -15,5 +15,29 @@ LL | fn h<'h>() -> S<'h> {
    |
    = help: try removing the lifetime parameter `'h` and changing references to `'static`
 
-error: aborting due to 2 previous errors
+error: this lifetime can be changed to `'static`
+  --> $DIR/hidden_static_lifetime.rs:25:6
+   |
+LL | fn o<'o, T>() -> &'o mut T
+   |      ^^
+   |
+   = help: try removing the lifetime parameter `'o` and changing references to `'static`
+
+error: this lifetime can be changed to `'static`
+  --> $DIR/hidden_static_lifetime.rs:33:6
+   |
+LL | fn n<'m1, 'm2, T>() -> &'m1 fn(&'m2 T) {
+   |      ^^^
+   |
+   = help: try removing the lifetime parameter `'m1` and changing references to `'static`
+
+error: this lifetime can be changed to `'static`
+  --> $DIR/hidden_static_lifetime.rs:74:6
+   |
+LL | fn l<'l, T>() -> &'l mut T
+   |      ^^
+   |
+   = help: try removing the lifetime parameter `'l` and changing references to `'static`
+
+error: aborting due to 5 previous errors