Treat safe target_feature functions as unsafe by default

Author: oli-obk

compiler/rustc_ast_lowering/src/delegation.rs

@@ -188,7 +188,14 @@ impl<'hir> LoweringContext<'_, 'hir> {
 ) -> hir::FnSig<'hir>{
 let header = ifletSome(local_sig_id) = sig_id.as_local(){
 matchself.resolver.delegation_fn_sigs.get(&local_sig_id){
-Some(sig) => self.lower_fn_header(sig.header, hir::Safety::Safe),
+Some(sig) => self.lower_fn_header(
+ sig.header,
+// HACK: we override the default safety instead of generating attributes from the ether.
+// We are not forwarding the attributes, as the delegation fn sigs are collected on the ast,
+// and here we need the hir attributes.
+if sig.target_feature{ hir::Safety::Unsafe}else{ hir::Safety::Safe},
+&[],
+),
 None => self.generate_header_error(),
 }
 }else{
@@ -198,7 +205,11 @@ impl<'hir> LoweringContext<'_, 'hir> {
 Asyncness::No => hir::IsAsync::NotAsync,
 };
  hir::FnHeader{
-safety: sig.safety.into(),
+safety:ifself.tcx.codegen_fn_attrs(sig_id).safe_target_features{
+ hir::HeaderSafety::SafeTargetFeatures
+}else{
+ hir::HeaderSafety::Normal(sig.safety)
+},
 constness:self.tcx.constness(sig_id),
  asyncness,
 abi: sig.abi,

compiler/rustc_ast_lowering/src/item.rs

@@ -231,7 +231,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
 });
 let sig = hir::FnSig{
  decl,
-header: this.lower_fn_header(*header, hir::Safety::Safe),
+header: this.lower_fn_header(*header, hir::Safety::Safe, attrs),
 span: this.lower_span(*fn_sig_span),
 };
  hir::ItemKind::Fn{ sig, generics,body: body_id,has_body: body.is_some()}
@@ -610,7 +610,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
 fnlower_foreign_item(&mutself,i:&ForeignItem) -> &'hir hir::ForeignItem<'hir>{
 let hir_id = hir::HirId::make_owner(self.current_hir_id_owner.def_id);
 let owner_id = hir_id.expect_owner();
-self.lower_attrs(hir_id,&i.attrs);
+let attrs = self.lower_attrs(hir_id,&i.attrs);
 let item = hir::ForeignItem{
  owner_id,
 ident:self.lower_ident(i.ident),
@@ -634,7 +634,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
 });
 
 // Unmarked safety in unsafe block defaults to unsafe.
-let header = self.lower_fn_header(sig.header, hir::Safety::Unsafe);
+let header = self.lower_fn_header(sig.header, hir::Safety::Unsafe, attrs);
 
  hir::ForeignItemKind::Fn(
  hir::FnSig{ header, decl,span:self.lower_span(sig.span)},
@@ -776,6 +776,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
  i.id,
 FnDeclKind::Trait,
  sig.header.coroutine_kind,
+ attrs,
 );
 (generics, hir::TraitItemKind::Fn(sig, hir::TraitFn::Required(names)),false)
 }
@@ -795,6 +796,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
  i.id,
 FnDeclKind::Trait,
  sig.header.coroutine_kind,
+ attrs,
 );
 (generics, hir::TraitItemKind::Fn(sig, hir::TraitFn::Provided(body_id)),true)
 }
@@ -911,6 +913,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
  i.id,
 ifself.is_in_trait_impl{FnDeclKind::Impl}else{FnDeclKind::Inherent},
  sig.header.coroutine_kind,
+ attrs,
 );
 
 (generics, hir::ImplItemKind::Fn(sig, body_id))
@@ -1339,8 +1342,9 @@ impl<'hir> LoweringContext<'_, 'hir> {
 id:NodeId,
 kind:FnDeclKind,
 coroutine_kind:Option<CoroutineKind>,
+attrs:&[hir::Attribute],
 ) -> (&'hir hir::Generics<'hir>, hir::FnSig<'hir>){
-let header = self.lower_fn_header(sig.header, hir::Safety::Safe);
+let header = self.lower_fn_header(sig.header, hir::Safety::Safe, attrs);
 let itctx = ImplTraitContext::Universal;
 let(generics, decl) = self.lower_generics(generics, id, itctx, |this| {
  this.lower_fn_decl(&sig.decl, id, sig.span, kind, coroutine_kind)
@@ -1352,6 +1356,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
 &mutself,
 h:FnHeader,
 default_safety: hir::Safety,
+attrs:&[hir::Attribute],
 ) -> hir::FnHeader{
 let asyncness = ifletSome(CoroutineKind::Async{ span, .. }) = h.coroutine_kind{
  hir::IsAsync::Async(span)
@@ -1360,7 +1365,16 @@ impl<'hir> LoweringContext<'_, 'hir> {
 };
 
 let safety = self.lower_safety(h.safety, default_safety);
-let safety = safety.into();
+
+// Treat safe `#[target_feature]` functions as unsafe, but also remember that we did so.
+let safety = if attrs.iter().any(|attr| attr.has_name(sym::target_feature))
+ && safety.is_safe()
+ && !self.tcx.sess.target.is_like_wasm
+{
+ hir::HeaderSafety::SafeTargetFeatures
+}else{
+ safety.into()
+};
 
  hir::FnHeader{
  safety,

compiler/rustc_codegen_ssa/src/codegen_attrs.rs

@@ -250,10 +250,14 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
 }
 }
  sym::target_feature => {
-if !tcx.is_closure_like(did.to_def_id())
- && letSome(fn_sig) = fn_sig()
- && fn_sig.skip_binder().safety().is_safe()
-{
+letSome(sig) = tcx.hir_node_by_def_id(did).fn_sig()else{
+ tcx.dcx().span_delayed_bug(attr.span,"target_feature applied to non-fn");
+continue;
+};
+let safe_target_features =
+matches!(sig.header.safety, hir::HeaderSafety::SafeTargetFeatures);
+ codegen_fn_attrs.safe_target_features = safe_target_features;
+if safe_target_features {
 if tcx.sess.target.is_like_wasm || tcx.sess.opts.actually_rustdoc{
 // The `#[target_feature]` attribute is allowed on
 // WebAssembly targets on all functions, including safe

compiler/rustc_hir/src/hir.rs

@@ -3762,8 +3762,18 @@ impl fmt::Display for Constness {
 }
 }
 
+/// The actualy safety specified in syntax. We may treat
+/// its safety different within the type system to create a
+/// "sound by default" system that needs checking this enum
+/// explicitly to allow unsafe operations.
 #[derive(Copy,Clone,Debug,HashStable_Generic,PartialEq,Eq)]
 pubenumHeaderSafety{
+/// A safe function annotated with `#[target_features]`.
+/// The type system treats this function as an unsafe function,
+/// but safety checking will check this enum to treat it as safe
+/// and allowing calling other safe target feature functions with
+/// the same features without requiring an additional unsafe block.
+SafeTargetFeatures,
 Normal(Safety),
 }
 
@@ -3800,6 +3810,7 @@ impl FnHeader {
 
 pubfnsafety(&self) -> Safety{
 matchself.safety{
+HeaderSafety::SafeTargetFeatures => Safety::Unsafe,
 HeaderSafety::Normal(safety) => safety,
 }
 }

compiler/rustc_hir_pretty/src/lib.rs

@@ -2424,6 +2424,10 @@ impl<'a> State<'a> {
 self.print_constness(header.constness);
 
 let safety = match header.safety{
+ hir::HeaderSafety::SafeTargetFeatures => {
+self.word_nbsp("#[target_feature]");
+ hir::Safety::Safe
+}
  hir::HeaderSafety::Normal(safety) => safety,
 };
 

compiler/rustc_hir_typeck/src/coercion.rs

@@ -932,9 +932,15 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> {
 returnErr(TypeError::ForceInlineCast);
 }
 
-// Safe `#[target_feature]` functions are not assignable to safe fn pointers (RFC 2396),
-// report a better error than a safety mismatch.
-// FIXME(target_feature): do this inside `coerce_from_safe_fn`.
+let fn_attrs = self.tcx.codegen_fn_attrs(def_id);
+ifmatches!(fn_attrs.inline,InlineAttr::Force{ .. }){
+returnErr(TypeError::ForceInlineCast);
+}
+
+// FIXME(target_feature): Safe `#[target_feature]` functions could be cast to safe fn pointers (RFC 2396),
+// as you can already write that "cast" in user code by wrapping a target_feature fn call in a closure,
+// which is safe. This is sound because you already need to be executing code that is satisfying the target
+// feature constraints..
 if b_hdr.safety.is_safe()
  && self.tcx.codegen_fn_attrs(def_id).safe_target_features
 {

compiler/rustc_mir_build/src/check_unsafety.rs

@@ -1120,6 +1120,10 @@ pub(crate) fn check_unsafety(tcx: TyCtxt<'_>, def: LocalDefId) {
 let hir_id = tcx.local_def_id_to_hir_id(def);
 let safety_context = tcx.hir().fn_sig_by_hir_id(hir_id).map_or(SafetyContext::Safe, |fn_sig| {
 match fn_sig.header.safety{
+// We typeck the body as safe, but otherwise treat it as unsafe everywhere else.
+// Call sites to other SafeTargetFeatures functions are checked explicitly and don't need
+// to care about safety of the body.
+ hir::HeaderSafety::SafeTargetFeatures => SafetyContext::Safe,
  hir::HeaderSafety::Normal(safety) => match safety {
  hir::Safety::Unsafe => SafetyContext::UnsafeFn,
  hir::Safety::Safe => SafetyContext::Safe,

src/librustdoc/clean/types.rs

@@ -668,14 +668,25 @@ impl Item {
  ty::Asyncness::Yes => hir::IsAsync::Async(DUMMY_SP),
  ty::Asyncness::No => hir::IsAsync::NotAsync,
 };
- hir::FnHeader{safety: sig.safety().into(),abi: sig.abi(), constness, asyncness }
+ hir::FnHeader{
+safety:if tcx.codegen_fn_attrs(def_id).safe_target_features{
+ hir::HeaderSafety::SafeTargetFeatures
+}else{
+ sig.safety().into()
+},
+abi: sig.abi(),
+ constness,
+ asyncness,
+}
 }
 let header = matchself.kind{
 ItemKind::ForeignFunctionItem(_, safety) => {
 let def_id = self.def_id().unwrap();
 let abi = tcx.fn_sig(def_id).skip_binder().abi();
  hir::FnHeader{
-safety:if abi == ExternAbi::RustIntrinsic{
+safety:if tcx.codegen_fn_attrs(def_id).safe_target_features{
+ hir::HeaderSafety::SafeTargetFeatures
+}elseif abi == ExternAbi::RustIntrinsic{
 intrinsic_operation_unsafety(tcx, def_id.expect_local()).into()
 }else{
  safety.into()

src/librustdoc/html/format.rs

@@ -1640,6 +1640,7 @@ impl PrintWithSpace for hir::Safety {
 implPrintWithSpacefor hir::HeaderSafety{
 fnprint_with_space(&self) -> &str{
 matchself{
+ hir::HeaderSafety::SafeTargetFeatures => "",
  hir::HeaderSafety::Normal(safety) => safety.print_with_space(),
 }
 }

tests/ui/async-await/async-closures/fn-exception-target-features.stderr

@@ -1,8 +1,8 @@
-error[E0277]: the trait bound `fn() -> Pin<Box<(dyn Future<Output = ()> + 'static)>> {target_feature}: AsyncFn()` is not satisfied
+error[E0277]: the trait bound `unsafe fn() -> Pin<Box<(dyn Future<Output = ()> + 'static)>> {target_feature}: AsyncFn()` is not satisfied
  --> $DIR/fn-exception-target-features.rs:16:10
  |
 LL | test(target_feature);
- | ---- ^^^^^^^^^^^^^^ the trait `AsyncFn()` is not implemented for fn item `fn() -> Pin<Box<(dyn Future<Output = ()> + 'static)>> {target_feature}`
+ | ---- ^^^^^^^^^^^^^^ the trait `AsyncFn()` is not implemented for fn item `unsafe fn() -> Pin<Box<(dyn Future<Output = ()> + 'static)>> {target_feature}`
  | |
  | required by a bound introduced by this call
  |