-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Split elided_lifetime_in_paths into finer-grained lints #120808
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
79216b6
2089a29
662b848
640abd7
1bfae6e
788db3e
b0e622e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,10 @@ | ||
use std::slice; | ||
|
||
use rustc_data_structures::fx::FxIndexMap; | ||
use rustc_hir::intravisit::{self, Visitor}; | ||
use rustc_hir::{self as hir, LifetimeSource}; | ||
use rustc_session::{declare_lint, declare_lint_pass}; | ||
use rustc_session::lint::Lint; | ||
use rustc_session::{declare_lint, declare_lint_pass, impl_lint_pass}; | ||
use rustc_span::Span; | ||
use tracing::instrument; | ||
|
||
|
@@ -71,7 +74,84 @@ declare_lint! { | |
"detects when a lifetime uses different syntax between arguments and return values" | ||
} | ||
|
||
declare_lint_pass!(LifetimeSyntax => [MISMATCHED_LIFETIME_SYNTAXES]); | ||
declare_lint! { | ||
/// The `hidden_lifetimes_in_input_paths` lint detects the use of | ||
/// hidden lifetime parameters in types occurring as a function | ||
/// argument. | ||
/// | ||
/// ### Example | ||
/// | ||
/// ```rust,compile_fail | ||
/// #![deny(hidden_lifetimes_in_input_paths)] | ||
/// | ||
/// struct ContainsLifetime<'a>(&'a i32); | ||
/// | ||
/// fn foo(x: ContainsLifetime) {} | ||
/// ``` | ||
/// | ||
/// {{produces}} | ||
/// | ||
/// ### Explanation | ||
/// | ||
/// Hidden lifetime parameters can make it difficult to see at a | ||
/// glance that borrowing is occurring. | ||
/// | ||
/// This lint ensures that lifetime parameters are always | ||
/// explicitly stated, even if it is the `'_` [placeholder | ||
/// lifetime]. | ||
Comment on lines
+99
to
+100
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (wording): To make it easier to search for
Actually hm, this is more so trying to express
|
||
/// | ||
/// This lint is "allow" by default as function arguments by | ||
/// themselves do not usually cause much confusion. | ||
/// | ||
/// [placeholder lifetime]: https://doc.rust-lang.org/reference/lifetime-elision.html#lifetime-elision-in-functions | ||
pub HIDDEN_LIFETIMES_IN_INPUT_PATHS, | ||
Allow, | ||
"hidden lifetime parameters in types in function arguments may be confusing" | ||
} | ||
|
||
declare_lint! { | ||
/// The `hidden_lifetimes_in_output_paths` lint detects the use | ||
/// of hidden lifetime parameters in types occurring as a function | ||
/// return value. | ||
/// | ||
/// ### Example | ||
/// | ||
/// ```rust,compile_fail | ||
/// #![deny(hidden_lifetimes_in_output_paths)] | ||
/// | ||
/// struct ContainsLifetime<'a>(&'a i32); | ||
/// | ||
/// fn foo(x: &i32) -> ContainsLifetime { | ||
/// ContainsLifetime(x) | ||
/// } | ||
/// ``` | ||
/// | ||
/// {{produces}} | ||
/// | ||
/// ### Explanation | ||
/// | ||
/// Hidden lifetime parameters can make it difficult to see at a | ||
/// glance that borrowing is occurring. This is especially true | ||
/// when a type is used as a function's return value: lifetime | ||
/// elision will link the return value's lifetime to an argument's | ||
/// lifetime, but no syntax in the function signature indicates | ||
/// that. | ||
Comment on lines
+132
to
+136
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussion: right, I see what you meant by having both For reference, the discussion about what a reasonable scheme is to suppress the lower-lint-level of the two lints, or have @oli-obk suggested in that thread something among the lines of
Tho I'm not sure how to cleanly massage e.g. the stashed diagnostics mechanism and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my intention was that we'd add a new system, but I do realize now it's not incremental friendly (which isn't an issue for errors). |
||
/// | ||
/// This lint ensures that lifetime parameters are always | ||
/// explicitly stated, even if it is the `'_` [placeholder | ||
/// lifetime]. | ||
/// | ||
/// [placeholder lifetime]: https://doc.rust-lang.org/reference/lifetime-elision.html#lifetime-elision-in-functions | ||
pub HIDDEN_LIFETIMES_IN_OUTPUT_PATHS, | ||
Allow, | ||
"hidden lifetime parameters in types in function return values are deprecated" | ||
} | ||
|
||
declare_lint_pass!(LifetimeSyntax => [ | ||
MISMATCHED_LIFETIME_SYNTAXES, | ||
HIDDEN_LIFETIMES_IN_INPUT_PATHS, | ||
HIDDEN_LIFETIMES_IN_OUTPUT_PATHS, | ||
]); | ||
|
||
impl<'tcx> LateLintPass<'tcx> for LifetimeSyntax { | ||
#[instrument(skip_all)] | ||
|
@@ -123,6 +203,8 @@ fn check_fn_like<'tcx>(cx: &LateContext<'tcx>, fd: &'tcx hir::FnDecl<'tcx>) { | |
} | ||
|
||
report_mismatches(cx, &input_map, &output_map); | ||
report_hidden_in_paths(cx, &input_map, HIDDEN_LIFETIMES_IN_INPUT_PATHS); | ||
report_hidden_in_paths(cx, &output_map, HIDDEN_LIFETIMES_IN_OUTPUT_PATHS); | ||
} | ||
|
||
#[instrument(skip_all)] | ||
|
@@ -433,6 +515,50 @@ fn build_mismatch_suggestion( | |
} | ||
} | ||
|
||
fn report_hidden_in_paths<'tcx>( | ||
cx: &LateContext<'tcx>, | ||
info_map: &LifetimeInfoMap<'tcx>, | ||
lint: &'static Lint, | ||
) { | ||
let relevant_lifetimes = info_map | ||
.iter() | ||
.filter(|&(&&res, _)| reportable_lifetime_resolution(res)) | ||
.flat_map(|(_, info)| info) | ||
.filter(|info| { | ||
matches!(info.lifetime.source, LifetimeSource::Path { .. }) | ||
&& info.lifetime.is_implicit() | ||
}); | ||
|
||
let mut reporting_spans = Vec::new(); | ||
let mut suggestions = Vec::new(); | ||
|
||
for info in relevant_lifetimes { | ||
reporting_spans.push(info.reporting_span()); | ||
suggestions.push(info.suggestion("'_")); | ||
} | ||
|
||
if reporting_spans.is_empty() { | ||
return; | ||
} | ||
|
||
cx.emit_span_lint( | ||
lint, | ||
reporting_spans, | ||
lints::HiddenLifetimeInPath { | ||
suggestions: lints::HiddenLifetimeInPathSuggestion { suggestions }, | ||
}, | ||
); | ||
} | ||
|
||
/// We don't care about errors, nor do we care about the lifetime | ||
/// inside of a trait object. | ||
fn reportable_lifetime_resolution(kind: hir::LifetimeKind) -> bool { | ||
matches!( | ||
kind, | ||
hir::LifetimeKind::Param(..) | hir::LifetimeKind::Infer | hir::LifetimeKind::Static | ||
) | ||
} | ||
|
||
#[derive(Debug)] | ||
struct Info<'tcx> { | ||
type_span: Span, | ||
|
@@ -527,3 +653,115 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeInfoCollector<'a, 'tcx> { | |
self.referenced_type_span = old_referenced_type_span; | ||
} | ||
} | ||
|
||
declare_lint! { | ||
/// The `hidden_lifetimes_in_type_paths` lint detects the use of | ||
/// hidden lifetime parameters in types not part of a function's | ||
/// arguments or return values. | ||
/// | ||
/// ### Example | ||
/// | ||
/// ```rust,compile_fail | ||
/// #![deny(hidden_lifetimes_in_type_paths)] | ||
/// | ||
/// struct ContainsLifetime<'a>(&'a i32); | ||
/// | ||
/// static FOO: ContainsLifetime = ContainsLifetime(&42); | ||
/// ``` | ||
/// | ||
/// {{produces}} | ||
/// | ||
/// ### Explanation | ||
/// | ||
/// Hidden lifetime parameters can make it difficult to see at a | ||
/// glance that borrowing is occurring. | ||
/// | ||
/// This lint ensures that lifetime parameters are always | ||
/// explicitly stated, even if it is the `'_` [placeholder | ||
/// lifetime]. | ||
/// | ||
/// [placeholder lifetime]: https://doc.rust-lang.org/reference/lifetime-elision.html#lifetime-elision-in-functions | ||
pub HIDDEN_LIFETIMES_IN_TYPE_PATHS, | ||
Allow, | ||
"hidden lifetime parameters in types outside function signatures are discouraged" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion [WORDING-STRENGTH 2/2]: same here, maybe lessen the "strength" conveyed in this initial PR adding this lint as allow-by-default, and defer the lint level bump + wording strengthening (to "deprecate" and "discourage") in the follow-up upgrade-to-warn-by-default PR. I.e. maybe
But I don't feel too strongly about this. |
||
} | ||
|
||
#[derive(Default)] | ||
pub(crate) struct HiddenLifetimesInTypePaths { | ||
inside_fn_signature: bool, | ||
} | ||
|
||
impl_lint_pass!(HiddenLifetimesInTypePaths => [HIDDEN_LIFETIMES_IN_TYPE_PATHS]); | ||
|
||
impl<'tcx> LateLintPass<'tcx> for HiddenLifetimesInTypePaths { | ||
#[instrument(skip(self, cx))] | ||
fn check_ty(&mut self, cx: &LateContext<'tcx>, ty: &'tcx hir::Ty<'tcx, hir::AmbigArg>) { | ||
if self.inside_fn_signature { | ||
return; | ||
} | ||
|
||
// Do not lint about usages like `ContainsLifetime::method` or | ||
// `ContainsLifetimeAndType::<SomeType>::method`. | ||
if ty.source == hir::TySource::ImplicitSelf { | ||
return; | ||
} | ||
|
||
let hir::TyKind::Path(path) = ty.kind else { return }; | ||
|
||
let path_segments = match path { | ||
hir::QPath::Resolved(_ty, path) => path.segments, | ||
|
||
hir::QPath::TypeRelative(_ty, path_segment) => slice::from_ref(path_segment), | ||
|
||
hir::QPath::LangItem(..) => &[], | ||
}; | ||
|
||
let mut suggestions = Vec::new(); | ||
|
||
for path_segment in path_segments { | ||
for arg in path_segment.args().args { | ||
if let hir::GenericArg::Lifetime(lifetime) = arg | ||
&& lifetime.is_implicit() | ||
&& reportable_lifetime_resolution(lifetime.kind) | ||
{ | ||
suggestions.push(lifetime.suggestion("'_")) | ||
} | ||
} | ||
} | ||
|
||
if suggestions.is_empty() { | ||
return; | ||
} | ||
|
||
cx.emit_span_lint( | ||
HIDDEN_LIFETIMES_IN_TYPE_PATHS, | ||
ty.span, | ||
lints::HiddenLifetimeInPath { | ||
suggestions: lints::HiddenLifetimeInPathSuggestion { suggestions }, | ||
}, | ||
); | ||
} | ||
|
||
#[instrument(skip_all)] | ||
fn check_fn( | ||
&mut self, | ||
_: &LateContext<'tcx>, | ||
_: hir::intravisit::FnKind<'tcx>, | ||
_: &'tcx hir::FnDecl<'tcx>, | ||
_: &'tcx hir::Body<'tcx>, | ||
_: rustc_span::Span, | ||
_: rustc_span::def_id::LocalDefId, | ||
) { | ||
// We make the assumption that we will visit the function | ||
// declaration first, before visiting the body. | ||
self.inside_fn_signature = true; | ||
} | ||
|
||
// This may be a function's body, which would indicate that we are | ||
// no longer in the signature. Even if it's not, a body cannot | ||
// occur inside a function signature. | ||
#[instrument(skip_all)] | ||
fn check_body(&mut self, _: &LateContext<'tcx>, _: &hir::Body<'tcx>) { | ||
self.inside_fn_signature = false; | ||
} | ||
} |
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.
Suggestion [WORDING-STRENGTH 1/2]: I think I agree with @tmandry's comment earlier
where if this set of 3 lints are initially allow-by-default in this PR, we should not use "deprecated" in this PR, but instead add "deprecated" in the follow-up PR that bumps these three lints and the
hidden_lifetimes_in_paths
to warn-by-default, so that the deprecation is part of the follow-up decision to explicitly deprecate this form.In this PR, I would reword this less strong initially, maybe something like