Rollup merge of rust-lang#159780 - folkertdev:extern-custom-check-fn-ptrs, r=WaffleLapkin

check `extern "custom"` function pointers

tracking issue: rust-lang#140829
related RFC: rust-lang/rfcs#3980

Best reviewed commit-by-commit.

This PR makes 3 changes

- extend the ABI checks we already performed on function definitions and trait and foreign declarations to function pointer types. This touches the various `interupt` ABIs and `extern "custom"`.
- remove the ability for `extern "custom"` to return `!`
- improve the suggestion when `safe` is used in a function pointer type

Author: jhpratt

compiler/rustc_ast/src/ast.rs

@@ -2339,27 +2339,7 @@ pub struct FnSig {
 impl FnSig {
     /// Return a span encompassing the header, or where to insert it if empty.
     pub fn header_span(&self) -> Span {
-        match self.header.ext {
-            Extern::Implicit(span) | Extern::Explicit(_, span) => {
-                return self.span.with_hi(span.hi());
-            }
-            Extern::None => {}
-        }
-
-        match self.header.safety {
-            Safety::Unsafe(span) | Safety::Safe(span) => return self.span.with_hi(span.hi()),
-            Safety::Default => {}
-        };
-
-        if let Some(coroutine_kind) = self.header.coroutine_kind {
-            return self.span.with_hi(coroutine_kind.span().hi());
-        }
-
-        if let Const::Yes(span) = self.header.constness {
-            return self.span.with_hi(span.hi());
-        }
-
-        self.span.shrink_to_lo()
+        self.header.span().unwrap_or(self.span.shrink_to_lo())
     }
 
     /// The span of the header's safety, or where to insert it if empty.
@@ -2382,6 +2362,19 @@ impl FnSig {
     pub fn extern_span(&self) -> Span {
         self.header.ext.span().unwrap_or(self.safety_span().shrink_to_hi())
     }
+
+    pub fn as_borrowed(&self) -> BorrowedFnSig<'_> {
+        BorrowedFnSig { header: self.header, decl: &self.decl, span: self.span }
+    }
+}
+
+/// A borrowed version of `FnSig`, used to share logic between function declarations and function
+/// pointer types.
+#[derive(Clone, Debug)]
+pub struct BorrowedFnSig<'a> {
+    pub header: FnHeader,
+    pub decl: &'a FnDecl,
+    pub span: Span,
 }
 
 /// A constraint on an associated item.
@@ -2487,6 +2480,16 @@ pub struct FnPtrTy {
     pub decl_span: Span,
 }
 
+impl FnPtrTy {
+    pub fn header(&self) -> FnHeader {
+        FnHeader { constness: Const::No, coroutine_kind: None, safety: self.safety, ext: self.ext }
+    }
+
+    pub fn as_borrowed_fn_sig<'a>(&'a self) -> BorrowedFnSig<'a> {
+        BorrowedFnSig { header: self.header(), decl: &self.decl, span: self.decl_span }
+    }
+}
+
 #[derive(Clone, Encodable, Decodable, Debug, Walkable)]
 pub struct UnsafeBinderTy {
     pub generic_params: ThinVec<GenericParam>,
@@ -3845,6 +3848,30 @@ impl FnHeader {
             || matches!(constness, Const::Yes(_))
             || !matches!(ext, Extern::None)
     }
+
+    pub fn span(&self) -> Option<Span> {
+        let mut spans = smallvec::SmallVec::<[Span; 4]>::new();
+
+        match self.ext {
+            Extern::Implicit(span) | Extern::Explicit(_, span) => spans.push(span),
+            Extern::None => {}
+        }
+
+        match self.safety {
+            Safety::Unsafe(span) | Safety::Safe(span) => spans.push(span),
+            Safety::Default => {}
+        };
+
+        if let Some(coroutine_kind) = self.coroutine_kind {
+            spans.push(coroutine_kind.span());
+        }
+
+        if let Const::Yes(span) = self.constness {
+            spans.push(span)
+        }
+
+        spans.into_iter().reduce(Span::to)
+    }
 }
 
 impl Default for FnHeader {

compiler/rustc_ast_passes/src/ast_validation.rs

@@ -546,7 +546,13 @@ impl<'a> AstValidator<'a> {
     }
 
     /// Check that the signature of this function does not violate the constraints of its ABI.
-    fn check_extern_fn_signature(&self, abi: ExternAbi, ctxt: FnCtxt, ident: &Ident, sig: &FnSig) {
+    fn check_extern_fn_signature(
+        &self,
+        abi: ExternAbi,
+        ctxt: FnCtxt,
+        opt_function_name: Option<&Ident>, // None for function pointers
+        sig: &BorrowedFnSig<'_>,
+    ) {
         match AbiMap::from_target(&self.sess.target).canonize_abi(abi, false) {
             AbiMapping::Direct(canon_abi) | AbiMapping::Deprecated(canon_abi) => {
                 match canon_abi {
@@ -569,13 +575,13 @@ impl<'a> AstValidator<'a> {
 
                     CanonAbi::Custom => {
                         // An `extern "custom"` function must be unsafe.
-                        self.reject_safe_fn(abi, ctxt, sig);
+                        self.reject_safe_fn(abi, ctxt, sig, opt_function_name.is_none());
 
                         // An `extern "custom"` function cannot be `async` and/or `gen`.
                         self.reject_coroutine(abi, sig);
 
                         // An `extern "custom"` function must have type `fn()`.
-                        self.reject_params_or_return(abi, ident, sig);
+                        self.reject_params_or_return(abi, opt_function_name, sig);
                     }
 
                     CanonAbi::Interrupt(interrupt_kind) => {
@@ -600,7 +606,7 @@ impl<'a> AstValidator<'a> {
                             self.reject_return(abi, sig);
                         } else {
                             // An `extern "interrupt"` function must have type `fn()`.
-                            self.reject_params_or_return(abi, ident, sig);
+                            self.reject_params_or_return(abi, opt_function_name, sig);
                         }
                     }
                 }
@@ -609,18 +615,27 @@ impl<'a> AstValidator<'a> {
         }
     }
 
-    fn reject_safe_fn(&self, abi: ExternAbi, ctxt: FnCtxt, sig: &FnSig) {
+    fn reject_safe_fn(
+        &self,
+        abi: ExternAbi,
+        ctxt: FnCtxt,
+        sig: &BorrowedFnSig<'_>,
+        is_fn_ptr: bool,
+    ) {
         let dcx = self.dcx();
 
         match sig.header.safety {
             Safety::Unsafe(_) => { /* all good */ }
             Safety::Safe(safe_span) => {
-                let source_map = self.sess.psess.source_map();
-                let safe_span = source_map.span_until_non_whitespace(safe_span.to(sig.span));
-                dcx.emit_err(diagnostics::AbiCustomSafeForeignFunction {
-                    span: sig.span,
-                    safe_span,
-                });
+                // Function pointers already error when `safe` is used.
+                if !is_fn_ptr {
+                    let source_map = self.sess.psess.source_map();
+                    let safe_span = source_map.span_until_non_whitespace(safe_span.to(sig.span));
+                    dcx.emit_err(diagnostics::AbiCustomSafeForeignFunction {
+                        span: sig.span,
+                        safe_span,
+                    });
+                }
             }
             Safety::Default => match ctxt {
                 FnCtxt::Foreign => { /* all good */ }
@@ -635,7 +650,7 @@ impl<'a> AstValidator<'a> {
         }
     }
 
-    fn reject_coroutine(&self, abi: ExternAbi, sig: &FnSig) {
+    fn reject_coroutine(&self, abi: ExternAbi, sig: &BorrowedFnSig<'_>) {
         if let Some(coroutine_kind) = sig.header.coroutine_kind {
             let coroutine_kind_span = self
                 .sess
@@ -652,7 +667,7 @@ impl<'a> AstValidator<'a> {
         }
     }
 
-    fn reject_return(&self, abi: ExternAbi, sig: &FnSig) {
+    fn reject_return(&self, abi: ExternAbi, sig: &BorrowedFnSig<'_>) {
         if let FnRetTy::Ty(ref ret_ty) = sig.decl.output
             && match &ret_ty.kind {
                 TyKind::Never => false,
@@ -664,29 +679,41 @@ impl<'a> AstValidator<'a> {
         }
     }
 
-    fn reject_params_or_return(&self, abi: ExternAbi, ident: &Ident, sig: &FnSig) {
+    fn reject_params_or_return(
+        &self,
+        abi: ExternAbi,
+        opt_function_name: Option<&Ident>, // None for function pointers
+        sig: &BorrowedFnSig<'_>,
+    ) {
         let mut spans: Vec<_> = sig.decl.inputs.iter().map(|p| p.span).collect();
+
+        let allowed_return = |ret_ty: &Ty| match &ret_ty.kind {
+            TyKind::Never if abi != ExternAbi::Custom => true,
+            TyKind::Tup(tup) if tup.is_empty() => true,
+            _ => false,
+        };
+
         if let FnRetTy::Ty(ref ret_ty) = sig.decl.output
-            && match &ret_ty.kind {
-                TyKind::Never => false,
-                TyKind::Tup(tup) if tup.is_empty() => false,
-                _ => true,
-            }
+            && !allowed_return(ret_ty)
         {
             spans.push(ret_ty.span);
         }
 
         if !spans.is_empty() {
-            let header_span = sig.header_span();
+            let header_span = sig.header.span().unwrap_or(sig.span.shrink_to_lo());
             let suggestion_span = header_span.shrink_to_hi().to(sig.decl.output.span());
             let padding = if header_span.is_empty() { "" } else { " " };
 
             self.dcx().emit_err(diagnostics::AbiMustNotHaveParametersOrReturnType {
                 spans,
-                symbol: ident.name,
+                abi,
+
                 suggestion_span,
                 padding,
-                abi,
+                symbol: match opt_function_name {
+                    Some(ident) => format!(" {}", ident.name),
+                    None => String::new(),
+                },
             });
         }
     }
@@ -717,8 +744,12 @@ impl<'a> AstValidator<'a> {
     }
 
     fn check_fn_ptr_safety(&self, span: Span, safety: Safety) {
-        if matches!(safety, Safety::Safe(_)) {
-            self.dcx().emit_err(diagnostics::InvalidSafetyOnFnPtr { span });
+        if let Safety::Safe(safe_span) = safety {
+            let remove_span = self.sess.source_map().span_until_non_whitespace(span);
+            self.dcx().emit_err(diagnostics::InvalidSafetyOnFnPtr {
+                span: safe_span,
+                safe_span: remove_span,
+            });
         }
     }
 
@@ -1138,6 +1169,24 @@ impl<'a> AstValidator<'a> {
                 if let Extern::Implicit(extern_span) = bfty.ext {
                     self.handle_missing_abi(extern_span, ty.id);
                 }
+
+                let ext = match bfty.ext {
+                    Extern::None => None,
+                    Extern::Implicit(_) => Some(ExternAbi::FALLBACK),
+                    Extern::Explicit(str_lit, _) => {
+                        ExternAbi::from_str(str_lit.symbol.as_str()).ok()
+                    }
+                };
+
+                // Some ABIs impose special restrictions on the signature.
+                if let Some(extern_abi) = ext {
+                    self.check_extern_fn_signature(
+                        extern_abi,
+                        FnCtxt::Free,
+                        None,
+                        &bfty.as_borrowed_fn_sig(),
+                    );
+                }
             }
             TyKind::TraitObject(bounds, ..) => {
                 let mut any_lifetime_bounds = false;
@@ -1664,8 +1713,8 @@ impl Visitor<'_> for AstValidator<'_> {
                 self.check_extern_fn_signature(
                     self.extern_mod_abi.unwrap_or(ExternAbi::FALLBACK),
                     FnCtxt::Foreign,
-                    ident,
-                    sig,
+                    Some(ident),
+                    &sig.as_borrowed(),
                 );
 
                 if let Some(attr) = attr::find_by_name(fi.attrs(), sym::track_caller)
@@ -1870,7 +1919,12 @@ impl Visitor<'_> for AstValidator<'_> {
 
             if let Some((extern_abi, extern_abi_span)) = ext {
                 // Some ABIs impose special restrictions on the signature.
-                self.check_extern_fn_signature(extern_abi, ctxt, &fun.ident, &fun.sig);
+                self.check_extern_fn_signature(
+                    extern_abi,
+                    ctxt,
+                    Some(&fun.ident),
+                    &fun.sig.as_borrowed(),
+                );
 
                 // #[track_caller] can only be used with the rust ABI.
                 if let Some(attr) = attr::find_by_name(attrs, sym::track_caller)

compiler/rustc_ast_passes/src/diagnostics.rs

@@ -382,6 +382,13 @@ pub(crate) struct InvalidSafetyOnItem {
 pub(crate) struct InvalidSafetyOnFnPtr {
     #[primary_span]
     pub span: Span,
+    #[suggestion(
+        "remove the `safe` qualifier",
+        code = "",
+        applicability = "machine-applicable",
+        style = "verbose"
+    )]
+    pub safe_span: Span,
 }
 
 #[derive(Diagnostic)]
@@ -1115,11 +1122,11 @@ pub(crate) struct AbiMustNotHaveParametersOrReturnType {
     #[suggestion(
         "remove the parameters and return type",
         applicability = "maybe-incorrect",
-        code = "{padding}fn {symbol}()",
+        code = "{padding}fn{symbol}()",
         style = "verbose"
     )]
     pub suggestion_span: Span,
-    pub symbol: Symbol,
+    pub symbol: String,
     pub padding: &'static str,
 }