Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't implement Fn* traits for #[target_feature] functions #73306

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/librustc_trait_selection/traits/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,18 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
.starts_with("std::convert::From<");
let is_unsize =
{ Some(trait_ref.def_id()) == self.tcx.lang_items().unsize_trait() };
let is_fn_trait = [
calebzulawski marked this conversation as resolved.
Show resolved Hide resolved
self.tcx.lang_items().fn_trait(),
self.tcx.lang_items().fn_mut_trait(),
self.tcx.lang_items().fn_once_trait(),
]
.contains(&Some(trait_ref.def_id()));
let is_target_feature_fn =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this a Option<DefId>, where the def_id is the inner def-id...

if let ty::FnDef(def_id, _) = trait_ref.skip_binder().self_ty().kind {
!self.tcx.codegen_fn_attrs(def_id).target_features.is_empty()
} else {
false
};
let (message, note) = if is_try && is_from {
(
Some(format!(
Expand Down Expand Up @@ -427,6 +439,13 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
);
}

if is_fn_trait && is_target_feature_fn {
err.note(&format!(
"`{}` has `#[target_feature]` and is unsafe to call",
trait_ref.skip_binder().self_ty(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and then here we can use the def-id to give an error like foo has #[target_feature], instead of fn() {foo}, which is really unclear.

Also, I think "unsafe to call" is perhaps a bit misleading. Maybe just "foo has #[target_feature] annotations and therefore does not implement the Fn traits" or something like that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the error text again, I'm not sure it's even necessary to repeat the name of the function, since it's in the help text as well (albeit with the somewhat confusing fn() {foo}). In my latest commit I've changed it to simply "#[target_feature] functions do not implement the Fn traits". Do you think it's necessary to specify the function name in the error note?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't.

));
}

// Try to report a help message
if !trait_ref.has_infer_types_or_consts()
&& self.predicate_can_apply(obligation.param_env, trait_ref)
Expand Down
16 changes: 15 additions & 1 deletion src/librustc_trait_selection/traits/select/candidate_assembly.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
candidates.ambiguous = true; // Could wind up being a fn() type.
}
// Provide an impl, but only for suitable `fn` pointers.
ty::FnDef(..) | ty::FnPtr(_) => {
ty::FnPtr(_) => {
if let ty::FnSig {
unsafety: hir::Unsafety::Normal,
abi: Abi::Rust,
Expand All @@ -317,6 +317,20 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
candidates.vec.push(FnPointerCandidate);
}
}
// Provide an impl for suitable functions, rejecting `#[target_feature]` functions (RFC 2396).
ty::FnDef(def_id, _) => {
if let ty::FnSig {
unsafety: hir::Unsafety::Normal,
abi: Abi::Rust,
c_variadic: false,
..
} = self_ty.fn_sig(self.tcx()).skip_binder()
{
if self.tcx().codegen_fn_attrs(def_id).target_features.is_empty() {
candidates.vec.push(FnPointerCandidate);
}
}
}
_ => {}
}

Expand Down
34 changes: 34 additions & 0 deletions src/test/ui/rfcs/rfc-2396-target_feature-11/fn-traits.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// only-x86_64

#![feature(target_feature_11)]

#[target_feature(enable = "avx")]
fn foo() {}

#[target_feature(enable = "avx")]
unsafe fn foo_unsafe() {}

fn call(f: impl Fn()) {
f()
}

fn call_mut(f: impl FnMut()) {
f()
}

fn call_once(f: impl FnOnce()) {
f()
}

fn main() {
call(foo); //~ ERROR expected a `std::ops::Fn<()>` closure, found `fn() {foo}`
call_mut(foo); //~ ERROR expected a `std::ops::FnMut<()>` closure, found `fn() {foo}`
call_once(foo); //~ ERROR expected a `std::ops::FnOnce<()>` closure, found `fn() {foo}`

call(foo_unsafe);
//~^ ERROR expected a `std::ops::Fn<()>` closure, found `unsafe fn() {foo_unsafe}`
call_mut(foo_unsafe);
//~^ ERROR expected a `std::ops::FnMut<()>` closure, found `unsafe fn() {foo_unsafe}`
call_once(foo_unsafe);
//~^ ERROR expected a `std::ops::FnOnce<()>` closure, found `unsafe fn() {foo_unsafe}`
calebzulawski marked this conversation as resolved.
Show resolved Hide resolved
}
81 changes: 81 additions & 0 deletions src/test/ui/rfcs/rfc-2396-target_feature-11/fn-traits.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
error[E0277]: expected a `std::ops::Fn<()>` closure, found `fn() {foo}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this error is confusing and misleading, but at the same time it is consistent with error emitted for unsafe functions. Maybe we want to change it to better explain why foo doesn’t implement Fn<()>?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a note (open to phrasing suggestions), however I didn't remove this potentially misleading note since it's applied to all closures without any arguments. I'm not sure if it's within the scope of this PR to fix the #[rustc_on_unimplemented] on the Fn traits, but it's misleading for unsafe fn() too, I think.

--> $DIR/fn-traits.rs:24:10
|
LL | fn call(f: impl Fn()) {
| ---- required by this bound in `call`
...
LL | call(foo);
| ^^^ expected an `Fn<()>` closure, found `fn() {foo}`
|
= help: the trait `std::ops::Fn<()>` is not implemented for `fn() {foo}`
= note: wrap the `fn() {foo}` in a closure with no arguments: `|| { /* code */ }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a note or change this note?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this is misleading -- but then I think I would like to make this suggestion actually work, that's what #73631 is about.

= note: `fn() {foo}` has `#[target_feature]` and is unsafe to call

error[E0277]: expected a `std::ops::FnMut<()>` closure, found `fn() {foo}`
--> $DIR/fn-traits.rs:25:14
|
LL | fn call_mut(f: impl FnMut()) {
| ------- required by this bound in `call_mut`
...
LL | call_mut(foo);
| ^^^ expected an `FnMut<()>` closure, found `fn() {foo}`
|
= help: the trait `std::ops::FnMut<()>` is not implemented for `fn() {foo}`
= note: wrap the `fn() {foo}` in a closure with no arguments: `|| { /* code */ }
= note: `fn() {foo}` has `#[target_feature]` and is unsafe to call

error[E0277]: expected a `std::ops::FnOnce<()>` closure, found `fn() {foo}`
--> $DIR/fn-traits.rs:26:15
|
LL | fn call_once(f: impl FnOnce()) {
| -------- required by this bound in `call_once`
...
LL | call_once(foo);
| ^^^ expected an `FnOnce<()>` closure, found `fn() {foo}`
|
= help: the trait `std::ops::FnOnce<()>` is not implemented for `fn() {foo}`
= note: wrap the `fn() {foo}` in a closure with no arguments: `|| { /* code */ }
= note: `fn() {foo}` has `#[target_feature]` and is unsafe to call

error[E0277]: expected a `std::ops::Fn<()>` closure, found `unsafe fn() {foo_unsafe}`
--> $DIR/fn-traits.rs:28:10
|
LL | fn call(f: impl Fn()) {
| ---- required by this bound in `call`
...
LL | call(foo_unsafe);
| ^^^^^^^^^^ expected an `Fn<()>` closure, found `unsafe fn() {foo_unsafe}`
|
= help: the trait `std::ops::Fn<()>` is not implemented for `unsafe fn() {foo_unsafe}`
= note: wrap the `unsafe fn() {foo_unsafe}` in a closure with no arguments: `|| { /* code */ }
= note: `unsafe fn() {foo_unsafe}` has `#[target_feature]` and is unsafe to call

error[E0277]: expected a `std::ops::FnMut<()>` closure, found `unsafe fn() {foo_unsafe}`
--> $DIR/fn-traits.rs:30:14
|
LL | fn call_mut(f: impl FnMut()) {
| ------- required by this bound in `call_mut`
...
LL | call_mut(foo_unsafe);
| ^^^^^^^^^^ expected an `FnMut<()>` closure, found `unsafe fn() {foo_unsafe}`
|
= help: the trait `std::ops::FnMut<()>` is not implemented for `unsafe fn() {foo_unsafe}`
= note: wrap the `unsafe fn() {foo_unsafe}` in a closure with no arguments: `|| { /* code */ }
= note: `unsafe fn() {foo_unsafe}` has `#[target_feature]` and is unsafe to call

error[E0277]: expected a `std::ops::FnOnce<()>` closure, found `unsafe fn() {foo_unsafe}`
--> $DIR/fn-traits.rs:32:15
|
LL | fn call_once(f: impl FnOnce()) {
| -------- required by this bound in `call_once`
...
LL | call_once(foo_unsafe);
| ^^^^^^^^^^ expected an `FnOnce<()>` closure, found `unsafe fn() {foo_unsafe}`
|
= help: the trait `std::ops::FnOnce<()>` is not implemented for `unsafe fn() {foo_unsafe}`
= note: wrap the `unsafe fn() {foo_unsafe}` in a closure with no arguments: `|| { /* code */ }
= note: `unsafe fn() {foo_unsafe}` has `#[target_feature]` and is unsafe to call

error: aborting due to 6 previous errors

For more information about this error, try `rustc --explain E0277`.