Rollup merge of #129207 - GrigorenkoPV:elided-is-named, r=cjgillot

Lint that warns when an elided lifetime ends up being a named lifetime

As suggested in #48686 (comment)

Fixes #48686

Author: matthiaskrgr

compiler/rustc_ast_lowering/src/lib.rs

@@ -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 {:?}",

compiler/rustc_ast_lowering/src/lifetime_collector.rs

@@ -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 => {}

compiler/rustc_hir/src/def.rs

@@ -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.

compiler/rustc_lint/messages.ftl

@@ -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

compiler/rustc_lint/src/context/diagnostics.rs

@@ -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)
+        }
     }
 }

compiler/rustc_lint/src/lints.rs

@@ -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 {

compiler/rustc_lint_defs/src/builtin.rs

@@ -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.

compiler/rustc_lint_defs/src/lib.rs

@@ -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>,

compiler/rustc_resolve/src/late.rs

@@ -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),
                         );
 

compiler/rustc_resolve/src/late/diagnostics.rs

@@ -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 {