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 1 commit
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
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
24 changes: 24 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,24 @@
// only-x86_64

#![feature(target_feature_11)]

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

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}`
}
39 changes: 39 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,39 @@
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:21: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.


error[E0277]: expected a `std::ops::FnMut<()>` closure, found `fn() {foo}`
--> $DIR/fn-traits.rs:22: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 */ }

error[E0277]: expected a `std::ops::FnOnce<()>` closure, found `fn() {foo}`
--> $DIR/fn-traits.rs:23: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 */ }

error: aborting due to 3 previous errors

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