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
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
@@ -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
@@ -1656,7 +1656,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
}

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

@@ -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 {:?}",
2 changes: 1 addition & 1 deletion compiler/rustc_ast_lowering/src/lifetime_collector.rs
Original file line number Diff line number Diff line change
@@ -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 => {}
9 changes: 7 additions & 2 deletions compiler/rustc_hir/src/def.rs
Original file line number Diff line number Diff line change
@@ -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.
4 changes: 4 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
@@ -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
13 changes: 13 additions & 0 deletions compiler/rustc_lint/src/context/diagnostics.rs
Original file line number Diff line number Diff line change
@@ -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;

@@ -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
@@ -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 {
33 changes: 33 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
@@ -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,
@@ -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.
7 changes: 7 additions & 0 deletions compiler/rustc_lint_defs/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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>,
79 changes: 67 additions & 12 deletions compiler/rustc_resolve/src/late.rs
Original file line number Diff line number Diff line change
@@ -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();
@@ -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);
@@ -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;
@@ -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))]
@@ -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,
@@ -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));
}
@@ -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.
@@ -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),
);

11 changes: 9 additions & 2 deletions compiler/rustc_resolve/src/late/diagnostics.rs
Original file line number Diff line number Diff line change
@@ -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.
@@ -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 {
@@ -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 {
2 changes: 1 addition & 1 deletion library/alloc/src/string.rs
Original file line number Diff line number Diff line change
@@ -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>,
{
Loading
Loading