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

Lint that warns when an elided lifetime ends up being a named lifetime (elided_named_lifetimes) #129207

Merged
merged 3 commits into from
Sep 1, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 3 additions & 3 deletions compiler/rustc_ast_lowering/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -837,7 +837,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {

(hir::ParamName::Fresh, hir::LifetimeParamKind::Elided(kind))
}
LifetimeRes::Static | LifetimeRes::Error => return None,
LifetimeRes::Static { .. } | LifetimeRes::Error => return None,
res => panic!(
"Unexpected lifetime resolution {:?} for {:?} at {:?}",
res, ident, ident.span
Expand Down Expand Up @@ -1656,7 +1656,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
}

// Opaques do not capture `'static`
LifetimeRes::Static | LifetimeRes::Error => {
LifetimeRes::Static { .. } | LifetimeRes::Error => {
continue;
}

Expand Down Expand Up @@ -2069,7 +2069,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
hir::LifetimeName::Param(param)
}
LifetimeRes::Infer => hir::LifetimeName::Infer,
LifetimeRes::Static => hir::LifetimeName::Static,
LifetimeRes::Static { .. } => hir::LifetimeName::Static,
LifetimeRes::Error => hir::LifetimeName::Error,
res => panic!(
"Unexpected lifetime resolution {:?} for {:?} at {:?}",
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ast_lowering/src/lifetime_collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ impl<'ast> LifetimeCollectVisitor<'ast> {
self.collected_lifetimes.insert(lifetime);
}
}
LifetimeRes::Static | LifetimeRes::Error => {
LifetimeRes::Static { .. } | LifetimeRes::Error => {
self.collected_lifetimes.insert(lifetime);
}
LifetimeRes::Infer => {}
Expand Down
9 changes: 7 additions & 2 deletions compiler/rustc_hir/src/def.rs
Original file line number Diff line number Diff line change
Expand Up @@ -863,8 +863,13 @@ pub enum LifetimeRes {
/// This variant is used for anonymous lifetimes that we did not resolve during
/// late resolution. Those lifetimes will be inferred by typechecking.
Infer,
/// Explicit `'static` lifetime.
Static,
/// `'static` lifetime.
Static {
/// We do not want to emit `elided_named_lifetimes`
/// when we are inside of a const item or a static,
/// because it would get too annoying.
suppress_elision_warning: bool,
},
/// Resolution failure.
Error,
/// HACK: This is used to recover the NodeId of an elided lifetime.
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,10 @@ lint_duplicate_macro_attribute =

lint_duplicate_matcher_binding = duplicate matcher binding

lint_elided_named_lifetime = elided lifetime has a name
.label_elided = this elided lifetime gets resolved as `{$name}`
.label_named = lifetime `{$name}` declared here

lint_enum_intrinsics_mem_discriminant =
the return value of `mem::discriminant` is unspecified when called with a non-enum type
.note = the argument to `discriminant` should be a reference to an enum, but it was passed a reference to a `{$ty_param}`, which is not an enum
Expand Down
13 changes: 13 additions & 0 deletions compiler/rustc_lint/src/context/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use rustc_errors::{
use rustc_middle::middle::stability;
use rustc_session::lint::BuiltinLintDiag;
use rustc_session::Session;
use rustc_span::symbol::kw;
use rustc_span::BytePos;
use tracing::debug;

Expand Down Expand Up @@ -441,5 +442,17 @@ pub(super) fn decorate_lint(sess: &Session, diagnostic: BuiltinLintDiag, diag: &
BuiltinLintDiag::UnexpectedBuiltinCfg { cfg, cfg_name, controlled_by } => {
lints::UnexpectedBuiltinCfg { cfg, cfg_name, controlled_by }.decorate_lint(diag)
}
BuiltinLintDiag::ElidedIsStatic { elided } => {
lints::ElidedNamedLifetime { elided, name: kw::StaticLifetime, named_declaration: None }
.decorate_lint(diag)
}
BuiltinLintDiag::ElidedIsParam { elided, param: (param_name, param_span) } => {
lints::ElidedNamedLifetime {
elided,
name: param_name,
named_declaration: Some(param_span),
}
.decorate_lint(diag)
}
}
}
10 changes: 10 additions & 0 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2611,6 +2611,16 @@ pub(crate) struct ElidedLifetimesInPaths {
pub subdiag: ElidedLifetimeInPathSubdiag,
}

#[derive(LintDiagnostic)]
#[diag(lint_elided_named_lifetime)]
pub(crate) struct ElidedNamedLifetime {
#[label(lint_label_elided)]
pub elided: Span,
pub name: Symbol,
#[label(lint_label_named)]
pub named_declaration: Option<Span>,
}

#[derive(LintDiagnostic)]
#[diag(lint_invalid_crate_type_value)]
pub(crate) struct UnknownCrateTypes {
Expand Down
33 changes: 33 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ declare_lint_pass! {
DUPLICATE_MACRO_ATTRIBUTES,
ELIDED_LIFETIMES_IN_ASSOCIATED_CONSTANT,
ELIDED_LIFETIMES_IN_PATHS,
ELIDED_NAMED_LIFETIMES,
EXPLICIT_BUILTIN_CFGS_IN_FLAGS,
EXPORTED_PRIVATE_DEPENDENCIES,
FFI_UNWIND_CALLS,
Expand Down Expand Up @@ -1862,6 +1863,38 @@ declare_lint! {
"hidden lifetime parameters in types are deprecated"
}

declare_lint! {
/// The `elided_named_lifetimes` lint detects when an elided
/// lifetime ends up being a named lifetime, such as `'static`
/// or some lifetime parameter `'a`.
///
/// ### Example
///
/// ```rust,compile_fail
/// #![deny(elided_named_lifetimes)]
/// struct Foo;
/// impl Foo {
/// pub fn get_mut(&'static self, x: &mut u8) -> &mut u8 {
/// unsafe { &mut *(x as *mut _) }
/// }
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Lifetime elision is quite useful, because it frees you from having
/// to give each lifetime its own name, but sometimes it can produce
/// somewhat surprising resolutions. In safe code, it is mostly okay,
/// because the borrow checker prevents any unsoundness, so the worst
/// case scenario is you get a confusing error message in some other place.
/// But with `unsafe` code, such unexpected resolutions may lead to unsound code.
pub ELIDED_NAMED_LIFETIMES,
Warn,
"detects when an elided lifetime gets resolved to be `'static` or some named parameter"
}

declare_lint! {
/// The `bare_trait_objects` lint suggests using `dyn Trait` for trait
/// objects.
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_lint_defs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,13 @@ pub enum BuiltinLintDiag {
},
MacroExpandedMacroExportsAccessedByAbsolutePaths(Span),
ElidedLifetimesInPaths(usize, Span, bool, Span),
ElidedIsStatic {
elided: Span,
},
ElidedIsParam {
elided: Span,
param: (Symbol, Span),
},
UnknownCrateTypes {
span: Span,
candidate: Option<Symbol>,
Expand Down
79 changes: 67 additions & 12 deletions compiler/rustc_resolve/src/late.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1557,14 +1557,14 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
if ident.name == kw::StaticLifetime {
self.record_lifetime_res(
lifetime.id,
LifetimeRes::Static,
LifetimeRes::Static { suppress_elision_warning: false },
LifetimeElisionCandidate::Named,
);
return;
}

if ident.name == kw::UnderscoreLifetime {
return self.resolve_anonymous_lifetime(lifetime, false);
return self.resolve_anonymous_lifetime(lifetime, lifetime.id, false);
}

let mut lifetime_rib_iter = self.lifetime_ribs.iter().rev();
Expand Down Expand Up @@ -1667,13 +1667,23 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
}

#[instrument(level = "debug", skip(self))]
fn resolve_anonymous_lifetime(&mut self, lifetime: &Lifetime, elided: bool) {
fn resolve_anonymous_lifetime(
&mut self,
lifetime: &Lifetime,
id_for_lint: NodeId,
elided: bool,
) {
debug_assert_eq!(lifetime.ident.name, kw::UnderscoreLifetime);

let kind =
if elided { MissingLifetimeKind::Ampersand } else { MissingLifetimeKind::Underscore };
let missing_lifetime =
MissingLifetime { id: lifetime.id, span: lifetime.ident.span, kind, count: 1 };
let missing_lifetime = MissingLifetime {
id: lifetime.id,
span: lifetime.ident.span,
kind,
count: 1,
id_for_lint,
};
let elision_candidate = LifetimeElisionCandidate::Missing(missing_lifetime);
for (i, rib) in self.lifetime_ribs.iter().enumerate().rev() {
debug!(?rib.kind);
Expand All @@ -1697,7 +1707,8 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
if lifetimes_in_scope.is_empty() {
self.record_lifetime_res(
lifetime.id,
LifetimeRes::Static,
// We are inside a const item, so do not warn.
LifetimeRes::Static { suppress_elision_warning: true },
elision_candidate,
);
return;
Expand Down Expand Up @@ -1800,7 +1811,7 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
LifetimeRes::ElidedAnchor { start: id, end: NodeId::from_u32(id.as_u32() + 1) },
LifetimeElisionCandidate::Ignore,
);
self.resolve_anonymous_lifetime(&lt, true);
self.resolve_anonymous_lifetime(&lt, anchor_id, true);
}

#[instrument(level = "debug", skip(self))]
Expand Down Expand Up @@ -1916,6 +1927,7 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
};
let missing_lifetime = MissingLifetime {
id: node_ids.start,
id_for_lint: segment_id,
span: elided_lifetime_span,
kind,
count: expected_lifetimes,
Expand Down Expand Up @@ -2039,8 +2051,44 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
if let Some(prev_res) = self.r.lifetimes_res_map.insert(id, res) {
panic!("lifetime {id:?} resolved multiple times ({prev_res:?} before, {res:?} now)")
}

match candidate {
LifetimeElisionCandidate::Missing(missing @ MissingLifetime { .. }) => {
debug_assert_eq!(id, missing.id);
match res {
LifetimeRes::Static { suppress_elision_warning } => {
if !suppress_elision_warning {
self.r.lint_buffer.buffer_lint(
lint::builtin::ELIDED_NAMED_LIFETIMES,
missing.id_for_lint,
missing.span,
BuiltinLintDiag::ElidedIsStatic { elided: missing.span },
);
}
}
LifetimeRes::Param { param, binder: _ } => {
let tcx = self.r.tcx();
self.r.lint_buffer.buffer_lint(
lint::builtin::ELIDED_NAMED_LIFETIMES,
missing.id_for_lint,
missing.span,
BuiltinLintDiag::ElidedIsParam {
elided: missing.span,
param: (tcx.item_name(param.into()), tcx.source_span(param)),
},
);
}
LifetimeRes::Fresh { .. }
| LifetimeRes::Infer
| LifetimeRes::Error
| LifetimeRes::ElidedAnchor { .. } => {}
}
}
LifetimeElisionCandidate::Ignore | LifetimeElisionCandidate::Named => {}
}

match res {
LifetimeRes::Param { .. } | LifetimeRes::Fresh { .. } | LifetimeRes::Static => {
LifetimeRes::Param { .. } | LifetimeRes::Fresh { .. } | LifetimeRes::Static { .. } => {
if let Some(ref mut candidates) = self.lifetime_elision_candidates {
candidates.push((res, candidate));
}
Expand Down Expand Up @@ -2558,9 +2606,14 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {

ItemKind::Static(box ast::StaticItem { ref ty, ref expr, .. }) => {
self.with_static_rib(def_kind, |this| {
this.with_lifetime_rib(LifetimeRibKind::Elided(LifetimeRes::Static), |this| {
this.visit_ty(ty);
});
this.with_lifetime_rib(
LifetimeRibKind::Elided(LifetimeRes::Static {
suppress_elision_warning: true,
}),
|this| {
this.visit_ty(ty);
},
);
if let Some(expr) = expr {
// We already forbid generic params because of the above item rib,
// so it doesn't matter whether this is a trivial constant.
Expand Down Expand Up @@ -2589,7 +2642,9 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
this.visit_generics(generics);

this.with_lifetime_rib(
LifetimeRibKind::Elided(LifetimeRes::Static),
LifetimeRibKind::Elided(LifetimeRes::Static {
suppress_elision_warning: true,
}),
|this| this.visit_ty(ty),
);

Expand Down
11 changes: 9 additions & 2 deletions compiler/rustc_resolve/src/late/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,13 @@ fn import_candidate_to_enum_paths(suggestion: &ImportSuggestion) -> (String, Str
pub(super) struct MissingLifetime {
/// Used to overwrite the resolution with the suggestion, to avoid cascading errors.
pub id: NodeId,
/// As we cannot yet emit lints in this crate and have to buffer them instead,
/// we need to associate each lint with some `NodeId`,
/// however for some `MissingLifetime`s their `NodeId`s are "fake",
/// in a sense that they are temporary and not get preserved down the line,
/// which means that the lints for those nodes will not get emitted.
/// To combat this, we can try to use some other `NodeId`s as a fallback option.
pub id_for_lint: NodeId,
/// Where to suggest adding the lifetime.
pub span: Span,
/// How the lifetime was introduced, to have the correct space and comma.
Expand Down Expand Up @@ -3028,7 +3035,7 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
maybe_static = true;
in_scope_lifetimes = vec![(
Ident::with_dummy_span(kw::StaticLifetime),
(DUMMY_NODE_ID, LifetimeRes::Static),
(DUMMY_NODE_ID, LifetimeRes::Static { suppress_elision_warning: false }),
)];
}
} else if elided_len == 0 {
Expand All @@ -3040,7 +3047,7 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
maybe_static = true;
in_scope_lifetimes = vec![(
Ident::with_dummy_span(kw::StaticLifetime),
(DUMMY_NODE_ID, LifetimeRes::Static),
(DUMMY_NODE_ID, LifetimeRes::Static { suppress_elision_warning: false }),
)];
}
} else if num_params == 1 {
Expand Down
2 changes: 1 addition & 1 deletion library/alloc/src/string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2313,7 +2313,7 @@ impl<'b> Pattern for &'b String {
}

#[inline]
fn strip_suffix_of<'a>(self, haystack: &'a str) -> Option<&str>
fn strip_suffix_of<'a>(self, haystack: &'a str) -> Option<&'a str>
where
Self::Searcher<'a>: core::str::pattern::ReverseSearcher<'a>,
{
Expand Down
Loading
Loading