Skip to content

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
6 changes: 6 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,12 @@ lint_hidden_glob_reexport = private item shadows public glob re-export
.note_glob_reexport = the name `{$name}` in the {$namespace} namespace is supposed to be publicly re-exported here
.note_private_item = but the private item here shadows it

lint_hidden_lifetime_in_path =
paths containing hidden lifetime parameters are deprecated
Comment on lines +289 to +290
Copy link
Member

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

hidden lifetime parameters in types are deprecated

I also think we should remove this wording at least until we upgrade some lint to warn-by-default.

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

paths containing hidden lifetime parameters can be confusing


lint_hidden_lifetime_in_path_suggestion =
indicate the anonymous lifetime

lint_hidden_lifetime_parameters = hidden lifetime parameters in types are deprecated

lint_hidden_unicode_codepoints = unicode codepoint changing visible direction of text present in {$label}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ late_lint_methods!(
UnqualifiedLocalImports: UnqualifiedLocalImports,
CheckTransmutes: CheckTransmutes,
LifetimeSyntax: LifetimeSyntax,
HiddenLifetimesInTypePaths: HiddenLifetimesInTypePaths::default(),
]
]
);
Expand Down
242 changes: 240 additions & 2 deletions compiler/rustc_lint/src/lifetime_syntax.rs
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;

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Nit (wording): To make it easier to search for '_, maybe

This lint ensures that lifetime parameters are always explicitly stated, even if it is the placeholder lifetime '_.

Actually hm, this is more so trying to express

This lint ensures that lifetime parameters are explicit in types occuring as function arguments. Where lifetime elision may occur, the placeholder lifetime '_ may be used to make the presence of lifetime constraints explicit.

///
/// 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
Copy link
Member

Choose a reason for hiding this comment

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

Discussion: right, I see what you meant by having both mismatched_lifetime_syntaxes and hidden_lifetimes_in_output_paths firing against the same return position type-with-hidden-lifetime.

For reference, the discussion about what a reasonable scheme is to suppress the lower-lint-level of the two lints, or have mismatched_lifetime_syntaxes "shadow" hidden_lifetimes_in_output_paths when both are at the same lint level is at #t-compiler/diagnostics > Avoiding emitting one lint if another has been emitted.

@oli-obk suggested in that thread something among the lines of

Since we already have stashed errors we could have lint marking where we can explicitly mark spans as having linted and other lints can check if spans have been linted

Tho I'm not sure how to cleanly massage e.g. the stashed diagnostics mechanism and StashKey to indicate how to resolve if both lints are to be emitted (since they'd be ran in separate lint passes). I'll hand the review over to oli for this part.

Copy link
Contributor

Choose a reason for hiding this comment

The 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)]
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

The 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

hidden lifetime parameters in types outside function signatures can be confusing

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;
}
}
21 changes: 21 additions & 0 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3284,3 +3284,24 @@ impl Subdiagnostic for MismatchedLifetimeSyntaxesSuggestion {
}
}
}

#[derive(LintDiagnostic)]
#[diag(lint_hidden_lifetime_in_path)]
pub(crate) struct HiddenLifetimeInPath {
#[subdiagnostic]
pub suggestions: HiddenLifetimeInPathSuggestion,
}

pub(crate) struct HiddenLifetimeInPathSuggestion {
pub suggestions: Vec<(Span, String)>,
}

impl Subdiagnostic for HiddenLifetimeInPathSuggestion {
fn add_to_diag<G: EmissionGuarantee>(self, diag: &mut Diag<'_, G>) {
diag.multipart_suggestion_verbose(
fluent::lint_hidden_lifetime_in_path_suggestion,
self.suggestions,
Applicability::MachineApplicable,
);
}
}