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.

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

Add hidden_static_lifetime lint #10123

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4226,6 +4226,7 @@ Released 2018-09-13
[`get_first`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_first
[`get_last_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_last_with_len
[`get_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_unwrap
[`hidden_static_lifetime`]: https://rust-lang.github.io/rust-clippy/master/index.html#hidden_static_lifetime
[`identity_conversion`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_conversion
[`identity_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_op
[`if_let_mutex`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_let_mutex
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::from_raw_with_void_ptr::FROM_RAW_WITH_VOID_PTR_INFO,
crate::from_str_radix_10::FROM_STR_RADIX_10_INFO,
crate::functions::DOUBLE_MUST_USE_INFO,
crate::functions::HIDDEN_STATIC_LIFETIME_INFO,
crate::functions::MISNAMED_GETTERS_INFO,
crate::functions::MUST_USE_CANDIDATE_INFO,
crate::functions::MUST_USE_UNIT_INFO,
Expand Down
280 changes: 280 additions & 0 deletions clippy_lints/src/functions/hidden_static_lifetime.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,280 @@
use crate::rustc_lint::LintContext;
use clippy_utils::diagnostics::span_lint_and_help;
use rustc_hir::{
intravisit::FnKind, BareFnTy, FnDecl, FnRetTy, GenericArg, GenericArgs, GenericBound, GenericParam,
GenericParamKind, Generics, Lifetime, LifetimeParamKind, MutTy, ParamName, PolyTraitRef, QPath, Ty, TyKind,
TypeBindingKind, WherePredicate,
};
use rustc_lint::LateContext;
use rustc_middle::lint::in_external_macro;
use rustc_span::Span;

use super::HIDDEN_STATIC_LIFETIME;

// As a summary:
//
// A lifetime can only be changed if:
// * Used in immutable references.
// * Not behind a mutable reference.
// * Not used in function types
//
// NOTE: Struct's fields follow the same rules as types

pub(super) fn check_fn<'tcx>(cx: &LateContext<'_>, kind: FnKind<'tcx>, decl: &'tcx FnDecl<'_>, span: Span) {
if !in_external_macro(cx.sess(), span) && let FnKind::ItemFn(_, generics, _) =
kind { let mut lifetime_is_used;
for generic in generics.params.iter() {
if let GenericParamKind::Lifetime { kind } = generic.kind &&
kind != LifetimeParamKind::Elided {
lifetime_is_used = false;
// Check that inputs don't use this lifetime
for input in decl.inputs {
if lifetime_is_used {
break;
}
// If input is reference
if let TyKind::Ref(lifetime, mut_ty) = &input.kind {
if !lifetime.is_anonymous() && lifetime.ident == generic.name.ident() {
lifetime_is_used = true;
} else {
lifetime_is_used = check_if_uses_lifetime(mut_ty.ty, &generic.name);
}
} 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(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(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>.
if let GenericBound::Trait(poly_trait_ref, _) = bound {
for segment in poly_trait_ref.trait_ref.path.segments {
if let Some(gen_args) = segment.args {
for ty_binding in gen_args.bindings {
if let TypeBindingKind::Equality { .. } = ty_binding.kind {
lifetime_is_used = true;
};
};
};
};
} else {
span_lint_and_help(cx,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the same as a 'static bound.

HIDDEN_STATIC_LIFETIME,
bound.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()));
};
};
};
} else {
// Check for other lifetimes
if let WherePredicate::RegionPredicate(region_predicate) = predicate {
if region_predicate.lifetime.hir_id.owner == generic.hir_id.owner {
lifetime_is_used = true;
} else {
span_lint_and_help(cx,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the same as a 'static bound.

HIDDEN_STATIC_LIFETIME,
region_predicate.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 again.
if !lifetime_is_used &&
check_validness(ret_ty, generic, generics) &&
lifetime_not_used_in_ty(ret_ty, generic) {
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 {
if let TyKind::Path(QPath::Resolved(_, path)) = &input.kind {
for segment in path.segments {
for arg in segment.args().args {
// If input's lifetime and function's are the same.
if let GenericArg::Lifetime(lifetime) = arg {
if lifetime.is_anonymous() {
return true;
}
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::Ref(lifetime, _) = input.kind {
if lifetime.ident.name == generic.name.ident().name {
return false;
}
}
}
}

if let TyKind::Ref(_, 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::Ref(_, _) = &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;
}
}
}
}
}
};
true
}

// true = lifetime isn't used (success)
fn lifetime_not_used_in_ty(ret_ty: &Ty<'_>, generic: &GenericParam<'_>) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be merged with the previous function. While the type is a reference you're unwrapping it looking for lifetimes to change to 'static. Once you hit a type which isn't a reference you're then looking to see if the lifetime is used again.

fn ty_uses_lifetime(ty: &Ty<'_>, generic: &GenericParam<'_>) -> bool {
if let TyKind::Path(QPath::Resolved(_, path)) = ty.kind {
for segment in path.segments {
if let Some(GenericArgs { args, .. }) = segment.args {
for arg in args.iter() {
if let GenericArg::Lifetime(lifetime) = arg {
if lifetime.ident.name == generic.name.ident().name {
// generic is used
return false;
}
} else if let GenericArg::Type(ty) = arg {
return classify_ty(ty, generic);
}
}
}
}
}
true
}

#[inline]
fn barefn_uses_lifetime(barefn: &BareFnTy<'_>, generic: &GenericParam<'_>) -> bool {
for param in barefn.generic_params {
if param.def_id == generic.def_id {
return false;
}
}
true
}

#[inline]
fn tuple_uses_lifetime(tuple: &[Ty<'_>], generic: &GenericParam<'_>) -> bool {
tuple.iter().any(|ty| classify_ty(ty, generic))
}

fn opaquedef_uses_lifetime(args: &[GenericArg<'_>], generic: &GenericParam<'_>) -> bool {
for arg in args.iter() {
if let GenericArg::Lifetime(lifetime) = arg {
if lifetime.ident.name == generic.name.ident().name {
// generic is used
return false;
}
} else if let GenericArg::Type(ty) = arg {
return classify_ty(ty, generic);
}
}
true
}

#[inline]
fn traitobject_uses_lifetime(lifetime: &Lifetime, traits: &[PolyTraitRef<'_>], generic: &GenericParam<'_>) -> bool {
if lifetime.ident.name == generic.name.ident().name {
return true;
}
for PolyTraitRef {
bound_generic_params, ..
} in traits
{
if bound_generic_params.iter().any(|param| param.def_id == generic.def_id) {
return false;
};
}
true
}

#[inline]
fn classify_ty(ty: &Ty<'_>, generic: &GenericParam<'_>) -> bool {
match &ty.kind {
TyKind::Slice(ty) | TyKind::Array(ty, _) => ty_uses_lifetime(ty, generic),
TyKind::Ptr(mut_ty) => ty_uses_lifetime(mut_ty.ty, generic),
TyKind::BareFn(barefnty) => barefn_uses_lifetime(barefnty, generic),
TyKind::Tup(tuple) => tuple_uses_lifetime(tuple, generic),
TyKind::Path(_) => ty_uses_lifetime(ty, generic),
TyKind::OpaqueDef(_, genericargs, _) => opaquedef_uses_lifetime(genericargs, generic),
TyKind::TraitObject(poly_trait_ref, lifetime, _) =>
traitobject_uses_lifetime(lifetime, poly_trait_ref, generic),
TyKind::Typeof(_) // This is unused for now, this needs revising when Typeof is used.
| TyKind::Err
| TyKind::Ref(_, _) /* This will never be the case*/
| TyKind::Never => true,
TyKind::Infer => false,
}
}

// Separate refs from ty.
let mut can_change = false;
let mut final_ty = ret_ty;
while let TyKind::Ref(lifetime, MutTy { ty, .. }) = &final_ty.kind {
if lifetime.ident.name == generic.name.ident().name {
can_change = true;
};
final_ty = ty;
}

// Now final_ty is equivalent to ret_ty.peel_refs

if can_change {
return classify_ty(final_ty, generic);
}
false
}
22 changes: 22 additions & 0 deletions clippy_lints/src/functions/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
mod hidden_static_lifetime;
mod misnamed_getters;
mod must_use;
mod not_unsafe_ptr_arg_deref;
Expand Down Expand Up @@ -326,6 +327,25 @@ declare_clippy_lint! {
"getter method returning the wrong field"
}

declare_clippy_lint! {
/// ### What it does
/// Detects generic lifetimes that the compiler will always resolve to be `'static`.
/// ### Why is this bad?
/// The introduction of a lifetime parameter that can only resolve to `'static` adds unnecesary complexity.
/// ### Example
/// ```rust
/// fn foo<'a>() -> &'a str { "bar" }
/// ```
/// Use instead:
/// ```rust
/// fn foo() -> &'static str { "bar" }
/// ```
#[clippy::version = "1.68.0"]
pub HIDDEN_STATIC_LIFETIME,
pedantic,
"`'a` lifetime isn't clear to be `'static`"
}

#[derive(Copy, Clone)]
pub struct Functions {
too_many_arguments_threshold: u64,
Expand Down Expand Up @@ -353,6 +373,7 @@ impl_lint_pass!(Functions => [
RESULT_UNIT_ERR,
RESULT_LARGE_ERR,
MISNAMED_GETTERS,
HIDDEN_STATIC_LIFETIME,
]);

impl<'tcx> LateLintPass<'tcx> for Functions {
Expand All @@ -369,6 +390,7 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
too_many_lines::check_fn(cx, kind, span, body, self.too_many_lines_threshold);
not_unsafe_ptr_arg_deref::check_fn(cx, kind, decl, body, hir_id);
misnamed_getters::check_fn(cx, kind, decl, body, span, hir_id);
hidden_static_lifetime::check_fn(cx, kind, decl, span);
}

fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
Expand Down
Loading