Add a new lint for catching unbound lifetimes in return values #4908
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,185 @@ | ||
| use if_chain::if_chain; | ||
| use rustc::declare_lint_pass; | ||
| use rustc::hir::map::Map; | ||
| use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; | ||
| use rustc_hir::intravisit::*; | ||
| use rustc_hir::*; | ||
| use rustc_session::declare_tool_lint; | ||
|
|
||
| use crate::utils::span_lint; | ||
|
|
||
| declare_clippy_lint! { | ||
| /// **What it does:** Checks for lifetime annotations on return values | ||
| /// which might not always get bound. | ||
| /// | ||
| /// **Why is this bad?** If the function contains unsafe code which | ||
| /// transmutes to the lifetime, the resulting lifetime may live longer | ||
| /// than was intended. | ||
| /// | ||
| /// **Known problems*: None. | ||
| /// | ||
| /// **Example:** | ||
| /// ```rust | ||
| /// struct WrappedStr(str); | ||
| /// | ||
| /// // Bad: unbound return lifetime causing unsoundness (e.g. when x is String) | ||
| /// fn unbound<'a>(x: impl AsRef<str> + 'a) -> &'a WrappedStr { | ||
| /// let s = x.as_ref(); | ||
| /// unsafe { &*(s as *const str as *const WrappedStr) } | ||
| /// } | ||
| /// | ||
| /// // Good: bound return lifetime is sound | ||
| /// fn bound<'a>(x: &'a str) -> &'a WrappedStr { | ||
| /// unsafe { &*(x as *const str as *const WrappedStr) } | ||
| /// } | ||
| /// ``` | ||
| pub UNBOUND_RETURN_LIFETIMES, | ||
| correctness, | ||
| "unbound lifetimes in function return values" | ||
| } | ||
|
|
||
| declare_lint_pass!(UnboundReturnLifetimes => [UNBOUND_RETURN_LIFETIMES]); | ||
|
|
||
| impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnboundReturnLifetimes { | ||
| fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item<'tcx>) { | ||
| if let ItemKind::Fn(ref sig, ref generics, _id) = item.kind { | ||
| check_fn_inner(cx, &sig.decl, generics, None); | ||
| } | ||
| } | ||
|
|
||
| fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx ImplItem<'tcx>) { | ||
| if let ImplItemKind::Method(ref sig, _) = item.kind { | ||
| let parent_generics = impl_generics(cx, item.hir_id); | ||
| check_fn_inner(cx, &sig.decl, &item.generics, parent_generics); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn check_fn_inner<'a, 'tcx>( | ||
| cx: &LateContext<'a, 'tcx>, | ||
| decl: &'tcx FnDecl<'tcx>, | ||
| generics: &'tcx Generics<'tcx>, | ||
| parent_data: Option<(&'tcx Generics<'tcx>, &'tcx Ty<'tcx>)>, | ||
|
There was a problem hiding this comment. Can inner functions refer to the lifetimes of outer functions? If yes, going one level up the stack might not be enough. |
||
| ) { | ||
| let output_type = if let FunctionRetTy::Return(ref output_type) = decl.output { | ||
| output_type | ||
| } else { | ||
| return; | ||
| }; | ||
|
|
||
| let lt = if let TyKind::Rptr(ref lt, _) = output_type.kind { | ||
| lt | ||
| } else { | ||
| return; | ||
|
There was a problem hiding this comment. What about lifetimes in the output type generics, e.g. Also what about arrays or tuples containing refs? |
||
| }; | ||
|
|
||
| if matches!(lt.name, LifetimeName::Param(_)) { | ||
| let target_lt = lt; | ||
|
|
||
| // check function generics | ||
| // the target lifetime parameter must appear on the left of some outlives relation | ||
| if lifetime_outlives_something(target_lt, generics) { | ||
| return; | ||
| } | ||
|
|
||
| // check parent generics | ||
| // the target lifetime parameter must appear on the left of some outlives relation | ||
| if let Some((ref parent_generics, _)) = parent_data { | ||
| if lifetime_outlives_something(target_lt, parent_generics) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // check type generics | ||
| // the target lifetime parameter must be included in the type | ||
| if let Some((_, ref parent_ty)) = parent_data { | ||
| if TypeVisitor::type_contains_lifetime(parent_ty, target_lt) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // check arguments the target lifetime parameter must be included as a | ||
| // lifetime of a reference, either directly or through the gneric | ||
| // parameters of the argument type. | ||
| for input in decl.inputs.iter() { | ||
| if TypeVisitor::type_contains_lifetime(input, target_lt) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| span_lint( | ||
| cx, | ||
| UNBOUND_RETURN_LIFETIMES, | ||
| target_lt.span, | ||
| "lifetime is unconstrained", | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| struct TypeVisitor<'tcx> { | ||
| found: bool, | ||
| target_lt: &'tcx Lifetime, | ||
| } | ||
|
|
||
| impl<'tcx> TypeVisitor<'tcx> { | ||
| fn type_contains_lifetime(ty: &Ty<'_>, target_lt: &'tcx Lifetime) -> bool { | ||
| let mut visitor = TypeVisitor { | ||
| found: false, | ||
| target_lt, | ||
| }; | ||
| walk_ty(&mut visitor, ty); | ||
| visitor.found | ||
| } | ||
| } | ||
|
|
||
| impl<'tcx> Visitor<'tcx> for TypeVisitor<'tcx> { | ||
| type Map = Map<'tcx>; | ||
|
|
||
| fn visit_lifetime(&mut self, lt: &'tcx Lifetime) { | ||
| if lt.name == self.target_lt.name { | ||
| self.found = true; | ||
| } | ||
| } | ||
|
|
||
| fn visit_ty(&mut self, ty: &'tcx Ty<'_>) { | ||
| match ty.kind { | ||
| TyKind::Rptr(ref lt, _) => { | ||
| if lt.name == self.target_lt.name { | ||
| self.found = true; | ||
| } | ||
| }, | ||
| TyKind::Path(ref qpath) => { | ||
| if !self.found { | ||
| walk_qpath(self, qpath, ty.hir_id, ty.span); | ||
| } | ||
| }, | ||
| _ => (), | ||
| } | ||
| } | ||
|
|
||
| fn nested_visit_map(&mut self) -> NestedVisitorMap<'_, Self::Map> { | ||
| NestedVisitorMap::None | ||
| } | ||
| } | ||
|
|
||
| fn lifetime_outlives_something<'tcx>(target_lt: &'tcx Lifetime, generics: &'tcx Generics<'tcx>) -> bool { | ||
| if let Some(param) = generics.get_named(target_lt.name.ident().name) { | ||
| if param.bounds.iter().any(|b| matches!(b, GenericBound::Outlives(_))) { | ||
| return true; | ||
| } | ||
| } | ||
| false | ||
| } | ||
|
|
||
| fn impl_generics<'tcx>(cx: &LateContext<'_, 'tcx>, hir_id: HirId) -> Option<(&'tcx Generics<'tcx>, &'tcx Ty<'tcx>)> { | ||
| let parent_impl = cx.tcx.hir().get_parent_item(hir_id); | ||
| if_chain! { | ||
| if parent_impl != CRATE_HIR_ID; | ||
| if let Node::Item(item) = cx.tcx.hir().get(parent_impl); | ||
| if let ItemKind::Impl(_, _, _, ref parent_generics, _, ref ty, _) = item.kind; | ||
| then { | ||
| return Some((parent_generics, ty)) | ||
| } | ||
| } | ||
| None | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move
WrappedStrabove theBadcomment and use it in bad function as return type for more symmetry between the two examples? Also could you please separate the}and//with a newline?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also say that when the bad foo takes a
String,'acan be equal to'static?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an example note.