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

calebzulawski · 2020-06-13T06:15:46Z

Closes #72012.

rust-highfive · 2020-06-13T06:15:48Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @varkor (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

LeSeulArtichaut · 2020-06-13T08:32:30Z

src/test/ui/rfcs/rfc-2396-target_feature-11/fn-traits.stderr

@@ -0,0 +1,39 @@
+error[E0277]: expected a `std::ops::Fn<()>` closure, found `fn() {foo}`


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<()>?

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.

LeSeulArtichaut · 2020-06-13T08:38:40Z

src/test/ui/rfcs/rfc-2396-target_feature-11/fn-traits.stderr

+   |          ^^^ 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 */ }


Maybe add a note or change this note?

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

varkor · 2020-06-21T13:27:48Z

cc @rust-lang/lang on the right approach for addressing #72012.

src/test/ui/rfcs/rfc-2396-target_feature-11/fn-traits.rs

src/librustc_trait_selection/traits/error_reporting/mod.rs

nikomatsakis · 2020-06-26T20:09:42Z

On #72012 I think there was general agreement with this approach. So this PR is "cleared for review". I left a nit but generally it looks ok, but I'll leave it to @varkor for the real review.

varkor

Looks good to me. Just one minor comment.

src/librustc_trait_selection/traits/error_reporting/mod.rs

nikomatsakis

re-read, one suggestion to improve the quality of the error message

nikomatsakis · 2020-06-29T17:02:41Z

src/librustc_trait_selection/traits/error_reporting/mod.rs

+                            self.tcx.lang_items().fn_once_trait(),
+                        ]
+                        .contains(&Some(trait_ref.def_id()));
+                        let is_target_feature_fn =


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

nikomatsakis · 2020-06-29T17:03:40Z

src/librustc_trait_selection/traits/error_reporting/mod.rs

+                        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(),


...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?

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?

No, I don't.

nikomatsakis · 2020-06-29T17:04:36Z

r? @nikomatsakis (since @varkor approved, and I left a nit, I'll take on the assignment)

nikomatsakis · 2020-07-01T15:50:01Z

@bors r+

bors · 2020-07-01T15:50:06Z

📌 Commit 51858da has been approved by nikomatsakis

…trait-soundness, r=nikomatsakis Don't implement Fn* traits for #[target_feature] functions Closes rust-lang#72012.

@ghost

…arth Rollup of 16 pull requests Successful merges: - rust-lang#72569 (Remove legacy InnoSetup GUI installer) - rust-lang#73306 (Don't implement Fn* traits for #[target_feature] functions) - rust-lang#73345 (expand: Stop using nonterminals for passing tokens to attribute and derive macros) - rust-lang#73449 (Provide more information on duplicate lang item error.) - rust-lang#73569 (Handle `macro_rules!` tokens consistently across crates) - rust-lang#73803 (Recover extra trailing angle brackets in struct definition) - rust-lang#73839 (Split and expand nonstandard-style lints unicode unit test.) - rust-lang#73841 (Remove defunct `-Z print-region-graph`) - rust-lang#73848 (Fix markdown rendering in librustc_lexer docs) - rust-lang#73865 (Fix Zulip topic format) - rust-lang#73892 (Clean up E0712 explanation) - rust-lang#73898 (remove duplicate test for rust-lang#61935) - rust-lang#73906 (Add missing backtick in `ty_error_with_message`) - rust-lang#73909 (`#[deny(unsafe_op_in_unsafe_fn)]` in libstd/fs.rs) - rust-lang#73910 (Rewrite a few manual index loops with while-let) - rust-lang#73929 (Fix comment typo) Failed merges: r? @ghost

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

144206e

rust-highfive assigned varkor Jun 13, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2020

LeSeulArtichaut reviewed Jun 13, 2020

View reviewed changes

Add error note when trying fn as Fn trait

c98b4c8

varkor added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jun 21, 2020

LeSeulArtichaut added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jun 21, 2020

jonkislin reviewed Jun 25, 2020

View reviewed changes

src/test/ui/rfcs/rfc-2396-target_feature-11/fn-traits.rs Show resolved Hide resolved

nikomatsakis requested changes Jun 26, 2020

View reviewed changes

src/librustc_trait_selection/traits/error_reporting/mod.rs Outdated Show resolved Hide resolved

nikomatsakis removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jun 26, 2020

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

8e899b1

varkor approved these changes Jun 29, 2020

View reviewed changes

src/librustc_trait_selection/traits/error_reporting/mod.rs Outdated Show resolved Hide resolved

nikomatsakis requested changes Jun 29, 2020

View reviewed changes

rust-highfive assigned nikomatsakis and unassigned varkor Jun 29, 2020

Make #[target_feature] Fn trait error message less confusing

51858da

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 1, 2020

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 1, 2020

Rollup merge of rust-lang#73306 - calebzulawski:target-feature-11-fn-…

88e068b

…trait-soundness, r=nikomatsakis Don't implement Fn* traits for #[target_feature] functions Closes rust-lang#72012.

Manishearth mentioned this pull request Jul 1, 2020

Rollup of 16 pull requests #73939

Closed

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2020

Rollup merge of rust-lang#73306 - calebzulawski:target-feature-11-fn-…

d7e8c0c

…trait-soundness, r=nikomatsakis Don't implement Fn* traits for #[target_feature] functions Closes rust-lang#72012.

Manishearth mentioned this pull request Jul 2, 2020

Rollup of 16 pull requests #73944

Closed

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2020

Rollup merge of rust-lang#73306 - calebzulawski:target-feature-11-fn-…

094f24a

…trait-soundness, r=nikomatsakis Don't implement Fn* traits for #[target_feature] functions Closes rust-lang#72012.

Manishearth mentioned this pull request Jul 2, 2020

Rollup of 17 pull requests #73946

Closed

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2020

Rollup merge of rust-lang#73306 - calebzulawski:target-feature-11-fn-…

aacf886

…trait-soundness, r=nikomatsakis Don't implement Fn* traits for #[target_feature] functions Closes rust-lang#72012.

Manishearth mentioned this pull request Jul 2, 2020

Rollup of 17 pull requests #73947

Closed

Manishearth mentioned this pull request Jul 2, 2020

Rollup of 16 pull requests #73950

Merged

bors merged commit 3d391d2 into rust-lang:master Jul 2, 2020

cuviper added this to the 1.46 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

calebzulawski commented Jun 13, 2020

rust-highfive commented Jun 13, 2020

LeSeulArtichaut Jun 13, 2020

calebzulawski Jun 13, 2020

LeSeulArtichaut Jun 13, 2020

nikomatsakis Jul 1, 2020

varkor commented Jun 21, 2020

nikomatsakis commented Jun 26, 2020

varkor left a comment

nikomatsakis left a comment

nikomatsakis Jun 29, 2020

nikomatsakis Jun 29, 2020

calebzulawski Jun 30, 2020

nikomatsakis Jul 1, 2020

nikomatsakis commented Jun 29, 2020

nikomatsakis commented Jul 1, 2020

bors commented Jul 1, 2020

		@@ -0,0 +1,39 @@
		error[E0277]: expected a `std::ops::Fn<()>` closure, found `fn() {foo}`

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

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

Conversation

calebzulawski commented Jun 13, 2020

rust-highfive commented Jun 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varkor commented Jun 21, 2020

nikomatsakis commented Jun 26, 2020

varkor left a comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Jun 29, 2020

nikomatsakis commented Jul 1, 2020

bors commented Jul 1, 2020