Auto merge of #128792 - compiler-errors:foreign-sig, r=spastorino

Use `FnSig` instead of raw `FnDecl` for `ForeignItemKind::Fn`, fix ICE for `Fn` trait error on safe foreign fn

Let's use `hir::FnSig` instead of `hir::FnDecl + hir::Safety` for `ForeignItemKind::Fn`. This consolidates some handling code between normal fns and foreign fns.

Separetly, fix an ICE where we weren't handling `Fn` trait errors for safe foreign fns.

If perf is bad for the first commit, I can rework the ICE fix to not rely on it. But if perf is good, I prefer we fix and clean up things all at once 👍

r? spastorino

Fixes #128764

compiler/rustc_ast_lowering/src/delegation.rs

@@ -189,7 +189,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
  ) -> hir::FnSig<'hir> {
  let header = if let Some(local_sig_id) = sig_id.as_local() {
  match self.resolver.delegation_fn_sigs.get(&local_sig_id) {
- Some(sig) => self.lower_fn_header(sig.header),
+ Some(sig) => self.lower_fn_header(sig.header, hir::Safety::Safe),
  None => self.generate_header_error(),
  }
  } else {

compiler/rustc_ast_lowering/src/item.rs

@@ -237,7 +237,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
  });
  let sig = hir::FnSig {
  decl,
- header: this.lower_fn_header(*header),
+ header: this.lower_fn_header(*header, hir::Safety::Safe),
  span: this.lower_span(*fn_sig_span),
  };
  hir::ItemKind::Fn(sig, generics, body_id)
@@ -668,7 +668,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
  ForeignItemKind::Fn(box Fn { sig, generics, .. }) => {
  let fdec = &sig.decl;
  let itctx = ImplTraitContext::Universal;
- let (generics, (fn_dec, fn_args)) =
+ let (generics, (decl, fn_args)) =
  self.lower_generics(generics, Const::No, false, i.id, itctx, |this| {
  (
  // Disallow `impl Trait` in foreign items.
@@ -682,9 +682,15 @@ impl<'hir> LoweringContext<'_, 'hir> {
  this.lower_fn_params_to_names(fdec),
  )
  });
- let safety = self.lower_safety(sig.header.safety, hir::Safety::Unsafe);
 
- hir::ForeignItemKind::Fn(fn_dec, fn_args, generics, safety)
+ // Unmarked safety in unsafe block defaults to unsafe.
+ let header = self.lower_fn_header(sig.header, hir::Safety::Unsafe);
+
+ hir::ForeignItemKind::Fn(
+ hir::FnSig { header, decl, span: self.lower_span(sig.span) },
+ fn_args,
+ generics,
+ )
  }
  ForeignItemKind::Static(box StaticItem { ty, mutability, expr: _, safety }) => {
  let ty = self
@@ -1390,7 +1396,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
  coroutine_kind: Option<CoroutineKind>,
  parent_constness: Const,
  ) -> (&'hir hir::Generics<'hir>, hir::FnSig<'hir>) {
- let header = self.lower_fn_header(sig.header);
+ let header = self.lower_fn_header(sig.header, hir::Safety::Safe);
  // Don't pass along the user-provided constness of trait associated functions; we don't want to
  // synthesize a host effect param for them. We reject `const` on them during AST validation.
  let constness =
@@ -1403,14 +1409,18 @@ impl<'hir> LoweringContext<'_, 'hir> {
  (generics, hir::FnSig { header, decl, span: self.lower_span(sig.span) })
  }
 
- pub(super) fn lower_fn_header(&mut self, h: FnHeader) -> hir::FnHeader {
+ pub(super) fn lower_fn_header(
+ &mut self,
+ h: FnHeader,
+ default_safety: hir::Safety,
+ ) -> hir::FnHeader {
  let asyncness = if let Some(CoroutineKind::Async { span, .. }) = h.coroutine_kind {
  hir::IsAsync::Async(span)
  } else {
  hir::IsAsync::NotAsync
  };
  hir::FnHeader {
- safety: self.lower_safety(h.safety, hir::Safety::Safe),
+ safety: self.lower_safety(h.safety, default_safety),
  asyncness: asyncness,
  constness: self.lower_constness(h.constness),
  abi: self.lower_extern(h.ext),

compiler/rustc_hir/src/hir.rs

@@ -3586,7 +3586,7 @@ impl ForeignItem<'_> {
 #[derive(Debug, Clone, Copy, HashStable_Generic)]
 pub enum ForeignItemKind<'hir> {
  /// A foreign function.
- Fn(&'hir FnDecl<'hir>, &'hir [Ident], &'hir Generics<'hir>, Safety),
+ Fn(FnSig<'hir>, &'hir [Ident], &'hir Generics<'hir>),
  /// A foreign static item (`static ext: u8`).
  Static(&'hir Ty<'hir>, Mutability, Safety),
  /// A foreign type.
@@ -3645,7 +3645,10 @@ impl<'hir> OwnerNode<'hir> {
  match self {
  OwnerNode::TraitItem(TraitItem { kind: TraitItemKind::Fn(fn_sig, _), .. })
  | OwnerNode::ImplItem(ImplItem { kind: ImplItemKind::Fn(fn_sig, _), .. })
- | OwnerNode::Item(Item { kind: ItemKind::Fn(fn_sig, _, _), .. }) => Some(fn_sig),
+ | OwnerNode::Item(Item { kind: ItemKind::Fn(fn_sig, _, _), .. })
+ | OwnerNode::ForeignItem(ForeignItem {
+ kind: ForeignItemKind::Fn(fn_sig, _, _), ..
+ }) => Some(fn_sig),
  _ => None,
  }
  }
@@ -3654,11 +3657,10 @@ impl<'hir> OwnerNode<'hir> {
  match self {
  OwnerNode::TraitItem(TraitItem { kind: TraitItemKind::Fn(fn_sig, _), .. })
  | OwnerNode::ImplItem(ImplItem { kind: ImplItemKind::Fn(fn_sig, _), .. })
- | OwnerNode::Item(Item { kind: ItemKind::Fn(fn_sig, _, _), .. }) => Some(fn_sig.decl),
- OwnerNode::ForeignItem(ForeignItem {
- kind: ForeignItemKind::Fn(fn_decl, _, _, _),
- ..
- }) => Some(fn_decl),
+ | OwnerNode::Item(Item { kind: ItemKind::Fn(fn_sig, _, _), .. })
+ | OwnerNode::ForeignItem(ForeignItem {
+ kind: ForeignItemKind::Fn(fn_sig, _, _), ..
+ }) => Some(fn_sig.decl),
  _ => None,
  }
  }
@@ -3846,11 +3848,13 @@ impl<'hir> Node<'hir> {
  match self {
  Node::TraitItem(TraitItem { kind: TraitItemKind::Fn(fn_sig, _), .. })
  | Node::ImplItem(ImplItem { kind: ImplItemKind::Fn(fn_sig, _), .. })
- | Node::Item(Item { kind: ItemKind::Fn(fn_sig, _, _), .. }) => Some(fn_sig.decl),
- Node::Expr(Expr { kind: ExprKind::Closure(Closure { fn_decl, .. }), .. })
- | Node::ForeignItem(ForeignItem {
- kind: ForeignItemKind::Fn(fn_decl, _, _, _), ..
- }) => Some(fn_decl),
+ | Node::Item(Item { kind: ItemKind::Fn(fn_sig, _, _), .. })
+ | Node::ForeignItem(ForeignItem { kind: ForeignItemKind::Fn(fn_sig, _, _), .. }) => {
+ Some(fn_sig.decl)
+ }
+ Node::Expr(Expr { kind: ExprKind::Closure(Closure { fn_decl, .. }), .. }) => {
+ Some(fn_decl)
+ }
  _ => None,
  }
  }
@@ -3874,7 +3878,10 @@ impl<'hir> Node<'hir> {
  match self {
  Node::TraitItem(TraitItem { kind: TraitItemKind::Fn(fn_sig, _), .. })
  | Node::ImplItem(ImplItem { kind: ImplItemKind::Fn(fn_sig, _), .. })
- | Node::Item(Item { kind: ItemKind::Fn(fn_sig, _, _), .. }) => Some(fn_sig),
+ | Node::Item(Item { kind: ItemKind::Fn(fn_sig, _, _), .. })
+ | Node::ForeignItem(ForeignItem { kind: ForeignItemKind::Fn(fn_sig, _, _), .. }) => {
+ Some(fn_sig)
+ }
  _ => None,
  }
  }
@@ -3949,7 +3956,7 @@ impl<'hir> Node<'hir> {
  pub fn generics(self) -> Option<&'hir Generics<'hir>> {
  match self {
  Node::ForeignItem(ForeignItem {
- kind: ForeignItemKind::Fn(_, _, generics, _), ..
+ kind: ForeignItemKind::Fn(_, _, generics), ..
  })
  | Node::TraitItem(TraitItem { generics, .. })
  | Node::ImplItem(ImplItem { generics, .. }) => Some(generics),
@@ -4039,8 +4046,8 @@ mod size_asserts {
  static_assert_size!(Expr<'_>, 64);
  static_assert_size!(ExprKind<'_>, 48);
  static_assert_size!(FnDecl<'_>, 40);
- static_assert_size!(ForeignItem<'_>, 72);
- static_assert_size!(ForeignItemKind<'_>, 40);
+ static_assert_size!(ForeignItem<'_>, 88);
+ static_assert_size!(ForeignItemKind<'_>, 56);
  static_assert_size!(GenericArg<'_>, 16);
  static_assert_size!(GenericBound<'_>, 48);
  static_assert_size!(Generics<'_>, 56);

compiler/rustc_hir/src/intravisit.rs

@@ -611,9 +611,9 @@ pub fn walk_foreign_item<'v, V: Visitor<'v>>(
  try_visit!(visitor.visit_ident(foreign_item.ident));
 
  match foreign_item.kind {
- ForeignItemKind::Fn(ref function_declaration, param_names, ref generics, _) => {
+ ForeignItemKind::Fn(ref sig, param_names, ref generics) => {
  try_visit!(visitor.visit_generics(generics));
- try_visit!(visitor.visit_fn_decl(function_declaration));
+ try_visit!(visitor.visit_fn_decl(sig.decl));
  walk_list!(visitor, visit_ident, param_names.iter().copied());
  }
  ForeignItemKind::Static(ref typ, _, _) => {

compiler/rustc_hir_analysis/src/check/check.rs

@@ -804,8 +804,8 @@ pub(crate) fn check_item_type(tcx: TyCtxt<'_>, def_id: LocalDefId) {
 
  let item = tcx.hir().foreign_item(item.id);
  match &item.kind {
- hir::ForeignItemKind::Fn(fn_decl, _, _, _) => {
- require_c_abi_if_c_variadic(tcx, fn_decl, abi, item.span);
+ hir::ForeignItemKind::Fn(sig, _, _) => {
+ require_c_abi_if_c_variadic(tcx, sig.decl, abi, item.span);
  }
  hir::ForeignItemKind::Static(..) => {
  check_static_inhabited(tcx, def_id);

compiler/rustc_hir_analysis/src/check/intrinsic.rs

@@ -30,7 +30,7 @@ fn equate_intrinsic_type<'tcx>(
  let (generics, span) = match tcx.hir_node_by_def_id(def_id) {
  hir::Node::Item(hir::Item { kind: hir::ItemKind::Fn(_, generics, _), .. })
  | hir::Node::ForeignItem(hir::ForeignItem {
- kind: hir::ForeignItemKind::Fn(.., generics, _),
+ kind: hir::ForeignItemKind::Fn(_, _, generics),
  ..
  }) => (tcx.generics_of(def_id), generics.span),
  _ => {

compiler/rustc_hir_analysis/src/check/wfcheck.rs

@@ -350,8 +350,8 @@ fn check_foreign_item<'tcx>(
  );
 
  match item.kind {
- hir::ForeignItemKind::Fn(decl, ..) => {
- check_item_fn(tcx, def_id, item.ident, item.span, decl)
+ hir::ForeignItemKind::Fn(sig, ..) => {
+ check_item_fn(tcx, def_id, item.ident, item.span, sig.decl)
  }
  hir::ForeignItemKind::Static(ty, ..) => {
  check_item_type(tcx, def_id, ty.span, UnsizedHandling::AllowIfForeignTail)

compiler/rustc_hir_analysis/src/collect.rs

@@ -1440,11 +1440,9 @@ fn fn_sig(tcx: TyCtxt<'_>, def_id: LocalDefId) -> ty::EarlyBinder<'_, ty::PolyFn
  icx.lowerer().lower_fn_ty(hir_id, header.safety, header.abi, decl, Some(generics), None)
  }
 
- ForeignItem(&hir::ForeignItem {
- kind: ForeignItemKind::Fn(fn_decl, _, _, safety), ..
- }) => {
+ ForeignItem(&hir::ForeignItem { kind: ForeignItemKind::Fn(sig, _, _), .. }) => {
  let abi = tcx.hir().get_foreign_abi(hir_id);
- compute_sig_of_foreign_fn_decl(tcx, def_id, fn_decl, abi, safety)
+ compute_sig_of_foreign_fn_decl(tcx, def_id, sig.decl, abi, sig.header.safety)
  }
 
  Ctor(data) | Variant(hir::Variant { data, .. }) if data.ctor().is_some() => {

compiler/rustc_hir_analysis/src/collect/resolve_bound_vars.rs

@@ -604,7 +604,7 @@ impl<'a, 'tcx> Visitor<'tcx> for BoundVarContext<'a, 'tcx> {
 
  fn visit_foreign_item(&mut self, item: &'tcx hir::ForeignItem<'tcx>) {
  match item.kind {
- hir::ForeignItemKind::Fn(_, _, generics, _) => {
+ hir::ForeignItemKind::Fn(_, _, generics) => {
  self.visit_early_late(item.hir_id(), generics, |this| {
  intravisit::walk_foreign_item(this, item);
  })

compiler/rustc_hir_pretty/src/lib.rs

@@ -352,16 +352,11 @@ impl<'a> State<'a> {
  self.maybe_print_comment(item.span.lo());
  self.print_outer_attributes(self.attrs(item.hir_id()));
  match item.kind {
- hir::ForeignItemKind::Fn(decl, arg_names, generics, safety) => {
+ hir::ForeignItemKind::Fn(sig, arg_names, generics) => {
  self.head("");
  self.print_fn(
- decl,
- hir::FnHeader {
- safety,
- constness: hir::Constness::NotConst,
- abi: Abi::Rust,
- asyncness: hir::IsAsync::NotAsync,
- },
+ sig.decl,
+ sig.header,
  Some(item.ident.name),
  generics,
  arg_names,

compiler/rustc_lint/src/types.rs

@@ -1734,13 +1734,16 @@ impl<'tcx> LateLintPass<'tcx> for ImproperCTypesDeclarations {
  let abi = cx.tcx.hir().get_foreign_abi(it.hir_id());
 
  match it.kind {
- hir::ForeignItemKind::Fn(decl, _, _, _) if !vis.is_internal_abi(abi) => {
- vis.check_foreign_fn(it.owner_id.def_id, decl);
+ hir::ForeignItemKind::Fn(sig, _, _) => {
+ if vis.is_internal_abi(abi) {
+ vis.check_fn(it.owner_id.def_id, sig.decl)
+ } else {
+ vis.check_foreign_fn(it.owner_id.def_id, sig.decl);
+ }
  }
  hir::ForeignItemKind::Static(ty, _, _) if !vis.is_internal_abi(abi) => {
  vis.check_foreign_static(it.owner_id, ty.span);
  }
- hir::ForeignItemKind::Fn(decl, _, _, _) => vis.check_fn(it.owner_id.def_id, decl),
  hir::ForeignItemKind::Static(..) | hir::ForeignItemKind::Type => (),
  }
  }

compiler/rustc_middle/src/hir/map/mod.rs

@@ -816,6 +816,11 @@ impl<'hir> Map<'hir> {
  })
  | Node::ImplItem(ImplItem {
  kind: ImplItemKind::Fn(sig, ..), span: outer_span, ..
+ })
+ | Node::ForeignItem(ForeignItem {
+ kind: ForeignItemKind::Fn(sig, ..),
+ span: outer_span,
+ ..
  }) => {
  // Ensure that the returned span has the item's SyntaxContext, and not the
  // SyntaxContext of the visibility.
@@ -874,10 +879,7 @@ impl<'hir> Map<'hir> {
  },
  Node::Variant(variant) => named_span(variant.span, variant.ident, None),
  Node::ImplItem(item) => named_span(item.span, item.ident, Some(item.generics)),
- Node::ForeignItem(item) => match item.kind {
- ForeignItemKind::Fn(decl, _, _, _) => until_within(item.span, decl.output.span()),
- _ => named_span(item.span, item.ident, None),
- },
+ Node::ForeignItem(item) => named_span(item.span, item.ident, None),
  Node::Ctor(_) => return self.span(self.tcx.parent_hir_id(hir_id)),
  Node::Expr(Expr {
  kind: ExprKind::Closure(Closure { fn_decl_span, .. }),

compiler/rustc_middle/src/hir/mod.rs

@@ -202,7 +202,7 @@ pub fn provide(providers: &mut Providers) {
  ..
  })
  | Node::ForeignItem(&ForeignItem {
- kind: ForeignItemKind::Fn(_, idents, _, _),
+ kind: ForeignItemKind::Fn(_, idents, _),
  ..
  }) = tcx.hir_node(tcx.local_def_id_to_hir_id(def_id))
  {

compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs

@@ -2729,6 +2729,10 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
  | Node::ImplItem(&hir::ImplItem { kind: hir::ImplItemKind::Fn(ref sig, _), .. })
  | Node::TraitItem(&hir::TraitItem {
  kind: hir::TraitItemKind::Fn(ref sig, _), ..
+ })
+ | Node::ForeignItem(&hir::ForeignItem {
+ kind: hir::ForeignItemKind::Fn(ref sig, _, _),
+ ..
  }) => (
  sig.span,
  None,

src/librustdoc/clean/mod.rs

@@ -3094,16 +3094,10 @@ fn clean_maybe_renamed_foreign_item<'tcx>(
  let def_id = item.owner_id.to_def_id();
  cx.with_param_env(def_id, |cx| {
  let kind = match item.kind {
- hir::ForeignItemKind::Fn(decl, names, generics, safety) => {
- let (generics, decl) = enter_impl_trait(cx, |cx| {
- // NOTE: generics must be cleaned before args
- let generics = clean_generics(generics, cx);
- let args = clean_args_from_types_and_names(cx, decl.inputs, names);
- let decl = clean_fn_decl_with_args(cx, decl, None, args);
- (generics, decl)
- });
- ForeignFunctionItem(Box::new(Function { decl, generics }), safety)
- }
+ hir::ForeignItemKind::Fn(sig, names, generics) => ForeignFunctionItem(
+ clean_function(cx, &sig, generics, FunctionArgs::Names(names)),
+ sig.header.safety,
+ ),
  hir::ForeignItemKind::Static(ty, mutability, safety) => ForeignStaticItem(
  Static { type_: Box::new(clean_ty(ty, cx)), mutability, expr: None },
  safety,

tests/ui/extern/extern-main-issue-86110.stderr

@@ -2,7 +2,7 @@ error: the `main` function cannot be declared in an `extern` block
  --> $DIR/extern-main-issue-86110.rs:4:5
  |
 LL | fn main();
- | ^^^^^^^^^
+ | ^^^^^^^^^^
 
 error: aborting due to 1 previous error
 

tests/ui/foreign/foreign-safe-fn-arg-mismatch.rs

@@ -0,0 +1,13 @@
+// Make sure we don't ICE when a foreign fn doesn't implement `Fn` due to arg mismatch.
+
+unsafe extern "Rust" {
+ pub safe fn foo();
+ pub safe fn bar(x: u32);
+}
+
+fn test(_: impl Fn(i32)) {}
+
+fn main() {
+ test(foo); //~ ERROR function is expected to take 1 argument, but it takes 0 arguments
+ test(bar); //~ ERROR type mismatch in function arguments
+}