Repository navigation
Implement Fn traits for safe functions of any ABI - #160186
Conversation
|
cc @rust-lang/miri |
|
r? @jackh726 rustbot has assigned @jackh726. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Don't you need to generate shims or Happy Things will happen? |
|
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 |
|
I suggest you check with |
|
Nothing broke… |
|
🤷 Maybe it is fine... |
|
Although the practical ABI for |
|
Probably should make sure the function that calls the |
|
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. |
32a4f77 to
2403142
Compare
|
Modified the tests to use a function that I confirmed with Godbolt has a completely different |
With new ABIs like |
|
I guess what happens is that we synthesize a But someone should dig into where that happens. |
|
|
||
| pub fn is_fn_trait_compatible(self) -> bool { | ||
| !self.c_variadic() && self.safety().is_safe() && self.abi() == ExternAbi::Rust | ||
| !self.c_variadic() && self.safety().is_safe() && self.abi().is_fn_trait_compatible() |
There was a problem hiding this comment.
| !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.
There was a problem hiding this comment.
Yes. I am pretty happy with this PR right now and I would have been pretty annoyed if it tried to bundle that change.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
|
The title made me ponder potential future things like @rfcbot reviewed |
( |
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. |
There was a problem hiding this comment.
I have this weird "I feel like we want more tests" feeling, but we can always benefit from some fuzzing, so that's not different from anything else in the compiler. This seems good. It does strengthen the promise you make inside unsafe extern with safe from "yeah, safe, anyone deliberately calling this is okay" to "No, really, you can call this as an anonymized closure three crates over, hope you were right that it's fine to call this context-free!" but that's consistent with unsafe elsewhere: that code has got some mighty fine print.
|
I haven't seen any activity from the randomly-assigned reviewer on this PR, and it has received quite a bit of scrutiny, including now my own. r? @workingjubilee |
|
📋 This PR cannot be approved because it currently has the following label: |
|
Has reference PR: rust-lang/reference#2383 |
|
This has unanimous approval by lang, FCP has closed, and the reference PR was presumably reviewed (as it was certainly viewable) during discussion of this feature. In the interest of avoiding a "Who's on first?" skit, I'm gonna r+ this again but if I should've waited until the ref PR was formally merged or something please let me know. @bors r+ |
Rollup of 10 pull requests Successful merges: - #160186 (Implement `Fn` traits for safe functions of any ABI) - #160370 (rustdoc: notable trait badge and popover enhancements) - #165077 (Fix alloctests for panic=abort) - #163623 (Make docblock `<code>` elements inline so they behave better) - #163833 (Fix RISC-V inline ASM `clobber_abi` when extra target features are enabled) - #165067 (cg_llvm: Use an invariant lifetime brand for LLVM modules ) - #165227 (Respect `--color=never` when printing `--explain` output to the terminal) - #165254 (Cleanup `rustdoc-html` tests) - #165258 (impl Eq for AllocRange) - #165259 (rustc-dev-guide subtree update)
Rollup merge of #160186 - Jules-Bertholet:fn-traits-any-abi, r=workingjubilee Implement `Fn` traits for safe functions of any ABI This brings the behavior of the language in line with its documentation: https://github.com/rust-lang/rust/blob/1a833e16546c2eb012758ddd499964fd8afee29e/library/core/src/primitive_docs.rs#L1930-L1931 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
Rollup of 10 pull requests Successful merges: - rust-lang/rust#160186 (Implement `Fn` traits for safe functions of any ABI) - rust-lang/rust#160370 (rustdoc: notable trait badge and popover enhancements) - rust-lang/rust#165077 (Fix alloctests for panic=abort) - rust-lang/rust#163623 (Make docblock `<code>` elements inline so they behave better) - rust-lang/rust#163833 (Fix RISC-V inline ASM `clobber_abi` when extra target features are enabled) - rust-lang/rust#165067 (cg_llvm: Use an invariant lifetime brand for LLVM modules ) - rust-lang/rust#165227 (Respect `--color=never` when printing `--explain` output to the terminal) - rust-lang/rust#165254 (Cleanup `rustdoc-html` tests) - rust-lang/rust#165258 (impl Eq for AllocRange) - rust-lang/rust#165259 (rustc-dev-guide subtree update)
Rollup of 10 pull requests Successful merges: - rust-lang/rust#160186 (Implement `Fn` traits for safe functions of any ABI) - rust-lang/rust#160370 (rustdoc: notable trait badge and popover enhancements) - rust-lang/rust#165077 (Fix alloctests for panic=abort) - rust-lang/rust#163623 (Make docblock `<code>` elements inline so they behave better) - rust-lang/rust#163833 (Fix RISC-V inline ASM `clobber_abi` when extra target features are enabled) - rust-lang/rust#165067 (cg_llvm: Use an invariant lifetime brand for LLVM modules ) - rust-lang/rust#165227 (Respect `--color=never` when printing `--explain` output to the terminal) - rust-lang/rust#165254 (Cleanup `rustdoc-html` tests) - rust-lang/rust#165258 (impl Eq for AllocRange) - rust-lang/rust#165259 (rustc-dev-guide subtree update)
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
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