Implement Fn traits for safe functions of any ABI

[Jules-Bertholet]

View all comments

This brings the behavior of the language in line with its documentation:

rust/library/core/src/primitive_docs.rs

Lines 1930 to 1931 in 1a833e1

	/// In addition, all *safe* function pointers implement [`Fn`], [`FnMut`], and [`FnOnce`], because
	/// these traits are specially known to the compiler.

ABIs that are not callable, or that are internal implementation details, are excluded.

Reference PR: rust-lang/reference#2383

@rustbot label T-lang needs-fcp A-ABI A-extern-fn I-lang-nominated

[rustbot]

miri is developed in its own repository. If the Miri part of this change can be broken out, consider making this change to rust-lang/miri instead. However, if Miri needs adjusting for rustc changes, just ignore this message.

cc @rust-lang/miri

[rustbot]

r? @jackh726

rustbot has assigned @jackh726.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 74 candidates
• Random selection from 16 candidates

[ChayimFriedman2]

Don't you need to generate shims or Happy Things will happen?

[Jules-Bertholet]

I don't think so? The basic tests I added work fine, and Miri doesn't complain. But I don't really know that part of the codebase, am relying on Cunningham's Law

[ChayimFriedman2]

I suggest you check with &dyn Fn(), things are more likely to break there.

[Jules-Bertholet]

Nothing broke…

[ChayimFriedman2]

🤷 Maybe it is fine...

[ChayimFriedman2]

Although the practical ABI for &isize and isize is identical for C and Rust, so it will compile fine and I don't know if Miri can detect a mismatch - maybe try something like [usize; 2], that appears to have a different ABI between Rust and C?

[ds84182]

Probably should make sure the function that calls the &dyn Fn is not inlined.

[RalfJung]

Yeah I'd expect the rust-call / argument (un)tupling to go wrong here as that needs the Rust ABI.

Does this actually solve a problem? If not I'd rather update the docs.

[Jules-Bertholet]

Modified the tests to use a function that I confirmed with Godbolt has a completely different "Rust" vs. "C" ABI. And everything still works

[Jules-Bertholet]

Does this actually solve a problem?

With new ABIs like "tail", extern is not just for FFI anymore.

[RalfJung]

I guess what happens is that we synthesize a call/call_once body for the traits when implementing them for a fn item, which discards the self argument, and which in this case also serves as an ABI adapter?

But someone should dig into where that happens.

[Jules-Bertholet]

Suggested change

	!self.c_variadic() && self.safety().is_safe() && self.abi().is_fn_trait_compatible()
	self.safety().is_safe() && self.abi().is_fn_trait_compatible()

We could try to remove the !self.c_variadic() condition. But there's not much point, as any actual use of variadics is necessarily unsafe. It would also be a bunch of work, as the trait solver currently assumes no type has more than one impl of each Fn trait.

View changes since the review

[teor2345]

This looks reasonable to me, just some notes on missing comments and a missing test for AsyncFn for extern "C" fn (…) -> impl Future

View changes since this review

[teor2345]

We might need a comment explaining that RustCall implements the Fn traits, but not this way

Suggested change

	ExternAbi::RustCall
	// RustCall is callable, but it is used as part of implementing the Fn traits for closures,
	// so it can't implement the Fn traits via this specific mechanism.
	ExternAbi::RustCall

[Jules-Bertholet]

There is actually no reason why we couldn't make rust-call true here. There just also isn't any reason why we should

[teor2345]

This seems like something worth saying explicitly?

Suggested change

	ExternAbi::RustCall
	// RustCall is callable, but it is also used as part of implementing the Fn traits for closures.
	// We could make it implement the Fn traits directly, but we have no reason to right now.
	ExternAbi::RustCall

[Jules-Bertholet]

We have a comment at the top of the function saying that we make all internal ABIs false. I think that is enough, "rust-call" is no different from the other two internal ABIs.

[teor2345]

I guess what happens is that we synthesize a call/call_once body for the traits when implementing them for a fn item, which discards the self argument, and which in this case also serves as an ABI adapter?

But someone should dig into where that happens.

Trait selection happens here:

included for context, legacy solver impacts

For AsyncFn/Fn in the next solver:

rust/compiler/rustc_next_trait_solver/src/solve/assembly/structural_traits.rs

Line 283 in c1bf6e1

// keep this in sync with assemble_fn_pointer_candidates until the old solver is removed.

For Fn in the legacy solver:

rust/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs

Line 582 in c1bf6e1

// Provide an impl, but only for suitable `fn` pointers.

For AsyncFn in the legacy solver:

rust/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs

Line 521 in c1bf6e1

// Provide an impl, but only for suitable `fn` pointers.

But the actual shim is created here:

rust/compiler/rustc_mir_transform/src/shim.rs

Lines 41 to 55 in c1bf6e1

	ty::ShimKind::FnPtr(def_id, ty) => {
	let trait_ = tcx.parent(def_id);
	// Supports `Fn` or `async Fn` traits.
	let adjustment = match tcx
	.fn_trait_kind_from_def_id(trait_)
	.or_else(|| tcx.async_fn_trait_kind_from_def_id(trait_))
	{
	Some(ty::ClosureKind::FnOnce) => Adjustment::Identity,
	Some(ty::ClosureKind::Fn) => Adjustment::Deref { source: DerefSource::ImmRef },
	Some(ty::ClosureKind::FnMut) => Adjustment::Deref { source: DerefSource::MutRef },
	None => bug!("fn pointer {:?} is not an fn", ty),
	};
	build_call_shim(tcx, shim, Some(adjustment), CallKind::Indirect(ty))
	}

rust/compiler/rustc_mir_transform/src/shim.rs

Lines 770 to 1001 in c1bf6e1

	/// Builds a "call" shim for `instance`. The shim calls the function specified by `call_kind`,
	/// first adjusting its first argument according to `rcvr_adjustment`.
	#[instrument(level = "debug", skip(tcx), ret)]
	fn build_call_shim<'tcx>(
	tcx: TyCtxt<'tcx>,
	shim: ty::ShimKind<'tcx>,
	rcvr_adjustment: Option<Adjustment>,
	call_kind: CallKind<'tcx>,
	) -> Body<'tcx> {
	// `FnPtrShim` contains the fn pointer type that a call shim is being built for - this is used
	// to instantiate into the signature of the shim. It is not necessary for users of this
	// MIR body to perform further instantiations (see `InstanceKind::has_polymorphic_mir_body`).
	let (sig_args, untuple_args) = if let ty::ShimKind::FnPtr(_, ty) = shim {
	let sig = tcx.instantiate_bound_regions_with_erased(ty.fn_sig(tcx));
	let untuple_args = sig.inputs();
	// Create substitutions for the `Self` and `Args` generic parameters of the shim body.
	let arg_tup = Ty::new_tup(tcx, untuple_args);
	(Some([ty.into(), arg_tup.into()]), Some(untuple_args))
	} else {
	(None, None)
	};
	let def_id = shim.def_id();
	let sig = tcx.fn_sig(def_id);
	let sig = sig.map_bound(|sig| tcx.instantiate_bound_regions_with_erased(sig));
	assert_eq!(sig_args.is_some(), !shim.has_polymorphic_mir_body());
	let mut sig = if let Some(sig_args) = sig_args {
	sig.instantiate(tcx, &sig_args).skip_norm_wip()
	} else {
	sig.instantiate_identity().skip_norm_wip()
	};
	if let CallKind::Indirect(fnty) = call_kind {
	// `sig` determines our local decls, and thus the callee type in the `Call` terminator. This
	// can only be an `FnDef` or `FnPtr`, but currently will be `Self` since the types come from
	// the implemented `FnX` trait.
	// Apply the opposite adjustment to the MIR input.
	let mut inputs_and_output = sig.inputs_and_output.to_vec();
	// Initial signature is `fn(&? Self, Args) -> Self::Output` where `Args` is a tuple of the
	// fn arguments. `Self` may be passed via (im)mutable reference or by-value.
	assert_eq!(inputs_and_output.len(), 3);
	// `Self` is always the original fn type `ty`. The MIR call terminator is only defined for
	// `FnDef` and `FnPtr` callees, not the `Self` type param.
	let self_arg = &mut inputs_and_output[0];
	*self_arg = match rcvr_adjustment.unwrap() {
	Adjustment::Identity => fnty,
	Adjustment::Deref { source } => match source {
	DerefSource::ImmRef => Ty::new_imm_ref(tcx, tcx.lifetimes.re_erased, fnty),
	DerefSource::MutRef => Ty::new_mut_ref(tcx, tcx.lifetimes.re_erased, fnty),
	DerefSource::MutPtr => Ty::new_mut_ptr(tcx, fnty),
	},
	Adjustment::RefMut => bug!("`RefMut` is never used with indirect calls: {shim:?}"),
	};
	sig.inputs_and_output = tcx.mk_type_list(&inputs_and_output);
	}
	// FIXME: Avoid having to adjust the signature both here and in
	// `fn_sig_for_fn_abi`.
	if let ty::ShimKind::VTable(..) = shim {
	// Modify fn(self, ...) to fn(self: *mut Self, ...)
	let mut inputs_and_output = sig.inputs_and_output.to_vec();
	let self_arg = &mut inputs_and_output[0];
	debug_assert!(tcx.generics_of(def_id).has_self && *self_arg == tcx.types.self_param);
	*self_arg = Ty::new_mut_ptr(tcx, *self_arg);
	sig.inputs_and_output = tcx.mk_type_list(&inputs_and_output);
	}
	let span = tcx.def_span(def_id);
	debug!(?sig);
	let mut local_decls = local_decls_for_sig(&sig, span);
	let source_info = SourceInfo::outermost(span);
	let destination = Place::return_place();
	let rcvr_place = || {
	assert!(rcvr_adjustment.is_some());
	Place::from(Local::arg(0))
	};
	let mut statements = vec![];
	let rcvr = rcvr_adjustment.map(|rcvr_adjustment| match rcvr_adjustment {
	Adjustment::Identity => Operand::Move(rcvr_place()),
	Adjustment::Deref { source: _ } => Operand::Move(tcx.mk_place_deref(rcvr_place())),
	Adjustment::RefMut => {
	// let rcvr = &mut rcvr;
	let ref_rcvr = local_decls.push(
	LocalDecl::new(
	Ty::new_mut_ref(tcx, tcx.lifetimes.re_erased, sig.inputs()[0]),
	span,
	)
	.immutable(),
	);
	let borrow_kind = BorrowKind::Mut { kind: MutBorrowKind::Default };
	statements.push(Statement::new(
	source_info,
	StatementKind::Assign(Box::new((
	Place::from(ref_rcvr),
	Rvalue::Ref(tcx.lifetimes.re_erased, borrow_kind, rcvr_place()),
	))),
	));
	Operand::Move(Place::from(ref_rcvr))
	}
	});
	let (callee, mut args) = match call_kind {
	// `FnPtr` call has no receiver. Args are untupled below.
	CallKind::Indirect(_) => (rcvr.unwrap(), vec![]),
	// `FnDef` call with optional receiver.
	CallKind::Direct(def_id) => {
	let ty = tcx.type_of(def_id).instantiate_identity().skip_norm_wip();
	(
	Operand::Constant(Box::new(ConstOperand {
	span,
	user_ty: None,
	const_: Const::zero_sized(ty),
	})),
	rcvr.into_iter().collect::<Vec<_>>(),
	)
	}
	};
	let mut arg_range = 0..sig.inputs().len();
	// Take the `self` ("receiver") argument out of the range (it's adjusted above).
	if rcvr_adjustment.is_some() {
	arg_range.start += 1;
	}
	// Take the last argument, if we need to untuple it (handled below).
	if untuple_args.is_some() {
	arg_range.end -= 1;
	}
	// Pass all of the non-special arguments directly.
	args.extend(arg_range.map(|i| Operand::Move(Place::from(Local::arg(i)))));
	// Untuple the last argument, if we have to.
	if let Some(untuple_args) = untuple_args {
	let tuple_arg = Local::arg(sig.inputs().len() - 1);
	args.extend(untuple_args.iter().enumerate().map(|(i, ity)| {
	Operand::Move(tcx.mk_place_field(Place::from(tuple_arg), FieldIdx::new(i), *ity))
	}));
	}
	let n_blocks = if let Some(Adjustment::RefMut) = rcvr_adjustment { 5 } else { 2 };
	let mut blocks = IndexVec::with_capacity(n_blocks);
	let block = |blocks: &mut IndexVec<_, _>, statements, kind, is_cleanup| {
	blocks.push(BasicBlockData::new_stmts(
	statements,
	Some(Terminator { source_info, kind, loop_hint_attrs: ThinVec::new() }),
	is_cleanup,
	))
	};
	// BB #0
	let args = args.into_iter().map(|a| Spanned { node: a, span: DUMMY_SP }).collect();
	block(
	&mut blocks,
	statements,
	TerminatorKind::Call {
	func: callee,
	args,
	destination,
	target: Some(BasicBlock::new(1)),
	unwind: if let Some(Adjustment::RefMut) = rcvr_adjustment {
	UnwindAction::Cleanup(BasicBlock::new(3))
	} else {
	UnwindAction::Continue
	},
	call_source: CallSource::Misc,
	fn_span: span,
	},
	false,
	);
	if let Some(Adjustment::RefMut) = rcvr_adjustment {
	// BB #1 - drop for Self
	block(
	&mut blocks,
	vec![],
	TerminatorKind::Drop {
	place: rcvr_place(),
	target: BasicBlock::new(2),
	unwind: UnwindAction::Continue,
	replace: false,
	drop: None,
	},
	false,
	);
	}
	// BB #1/#2 - return
	let stmts = vec![];
	block(&mut blocks, stmts, TerminatorKind::Return, false);
	if let Some(Adjustment::RefMut) = rcvr_adjustment {
	// BB #3 - drop if closure panics
	block(
	&mut blocks,
	vec![],
	TerminatorKind::Drop {
	place: rcvr_place(),
	target: BasicBlock::new(4),
	unwind: UnwindAction::Terminate(UnwindTerminateReason::InCleanup),
	replace: false,
	drop: None,
	},
	/* is_cleanup */ true,
	);
	// BB #4 - resume
	block(&mut blocks, vec![], TerminatorKind::UnwindResume, true);
	}
	let mut body =
	new_body(MirSource::from_shim(shim), blocks, local_decls, sig.inputs().len(), span);
	if let ExternAbi::RustCall = sig.abi() {
	body.spread_arg = Some(Local::new(sig.inputs().len()));
	}
	body
	}

[Jules-Bertholet]

Can you also test that this behaves sensibly with rustc_intrinsic FnDefs?

I don't know what there is to test here?

[theemathas]

TIL that rustc_intrinsic function items implement the Fn traits, even if they can't be converted to fn pointers. I guess there's nothing special to test here, then.

[theemathas]

Crater rollup in #164148

@craterbot abort

[craterbot]

🗑️ Experiment pr-160186 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[teor2345]

Does this need an update to the Rust Reference, which currently says:

Fn, FnMut, and FnOnce are implemented unless the function has any of the following:
…

• an ABI other than "Rust"

https://doc.rust-lang.org/stable/reference/types/function-item.html#r-type.fn-item.traits

Or do we do those kinds of updates later?

[Jules-Bertholet]

Reference PR: rust-lang/reference#2383

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[tmandry]

@rfcbot fcp merge lang

We discussed this in the lang meeting today, and we were in agreement that this is a positive change. We should do it if we can get away with it (pending crater results).

@rfcbot concern wait for crater results

[rust-rfcbot]

@tmandry has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• wait for crater results (Implement Fn traits for safe functions of any ABI #160186 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

Makes sense to me. Thanks @Jules-Bertholet (and thanks for filing the Reference PR).

@rfcbot reviewed

[joshtriplett]

@rfcbot reviewed

[joshtriplett]

I remember we talked about tracking this in some fashion, but I don't think that applies to the Fn traits. We want it for lowercase fn types, to distinguish "this is a pointer to a cdecl function" vs "this is a pointer to a stdcall function". But for capital-F Fn, we only care whether Rust knows how to call it.

[lcnr]

I see some discussion about whether this causes ABI compatability problems. Why does it not cause any such issues, can this reasoning be written down somewhere?

[Jules-Bertholet]

Why does it not cause any such issues, can this reasoning be written down somewhere?

@teor2345 tracked it down in #160186 (comment). Essentially, nothing in build_call_shim() relies on a specific ABI

[RalfJung]

Or put differently, IIUC this ends up generating a shim that serves as an ABI conversion trampoline.