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

Implement Debug, Pointer, etc on function pointers for all calling conventions #92964

Closed
wants to merge 6 commits into from

Conversation

Kampfkarren
Copy link
Contributor

Extends the default function pointer implementations, such as Debug, to all stable calling conventions.

I would like to extend this to the unstable calling conventions as well, but cannot find a way to do so that appeases ineffective_unstable_trait_impl.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 16, 2022
@rust-highfive
Copy link
Collaborator

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 16, 2022
@Kampfkarren Kampfkarren marked this pull request as draft January 16, 2022 10:44
@dtolnay
Copy link
Member

dtolnay commented Mar 18, 2022

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2022
@Kampfkarren
Copy link
Contributor Author

Going to be completely out next week--will try to finish up reviews the week after, apologies.

@Kampfkarren
Copy link
Contributor Author

Will finish tomorrow, promise, apologies

@Kampfkarren Kampfkarren marked this pull request as ready for review April 2, 2022 02:07
@Kampfkarren
Copy link
Contributor Author

CC @yaahc

@yaahc
Copy link
Member

yaahc commented Apr 2, 2022

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 2, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2022
@apiraino apiraino added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 28, 2022
@yaahc
Copy link
Member

yaahc commented Apr 29, 2022

r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs

@rust-highfive rust-highfive assigned dtolnay and unassigned yaahc Apr 29, 2022
@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 29, 2022
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

The changes look good so far, but I don't think this can be merged as currently implemented without further work in rustdoc. This PR changes doc/core/primitive.fn.html from 1.4 MB to 14 MB which is beyond what is reasonable and getting into crashy territory for browsers.

@Kampfkarren could you please look into making rustdoc render all these repetitive impls in some more compact way?

library/core/src/ptr/mod.rs Show resolved Hide resolved
@dtolnay dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2022
@Kampfkarren Kampfkarren changed the title Implement Debug, Pointer, etc on function pointers for all stable calling conventions Implement Debug, Pointer, etc on function pointers for all calling conventions May 28, 2022
@RalfJung
Copy link
Member

RalfJung commented Jun 1, 2022

This overlaps with #96513.

@RalfJung
Copy link
Member

RalfJung commented Jun 1, 2022

The changes look good so far, but I don't think this can be merged as currently implemented without further work in rustdoc. This PR changes doc/core/primitive.fn.html from 1.4 MB to 14 MB which is beyond what is reasonable and getting into crashy territory for browsers.

Another potential concern is the size of the rlib. How much bugger does compiled libcore become through all these extra instances?

@lcnr
Copy link
Contributor

lcnr commented Jun 1, 2022

As an alternative solution we could (and should imo) add a builtin trait:

trait FnPtr {
    // Alternatively `-> usize` idc
    fn as_ptr(self) -> *const ();
    // Other additions, like `const VARIADIC: bool` or whatever can be added if needed
}

This trait can then be used to implement Debug, Pointer and so on for all function pointers at once. I think even the existing number of impls in https://doc.rust-lang.org/nightly/std/primitive.fn.html is already far from ideal. I would generally be available to mentor/push that effort.

@RalfJung
Copy link
Member

RalfJung commented Jun 1, 2022

// Alternatively -> usize idc

Cc #95489 -- I am not sure which option is more portable, but #97420 seems to prefer usize.

@CAD97
Copy link
Contributor

CAD97 commented Jun 3, 2022

[trait FnPtr] can then be used to implement Debug, Pointer and so on for all function pointers at once.

If this is done via a blanket impl<F: FnPtr> Debug for F, doing so will require a coherence-impacting #[sealed] (or hacking that onto FnPtr directly) to avoid prohibiting downstream impls of the traits IIUC.

@lcnr
Copy link
Contributor

lcnr commented Jun 3, 2022

the coherence implications should fall out directly from the compiler internal impl of trait FnPtr. I don't expect there to be any issues here.

@dtolnay
Copy link
Member

dtolnay commented Jun 17, 2022

Another potential concern is the size of the rlib. How much bigger does compiled libcore become through all these extra instances?

On my machine, comparing 764b861...8efec95, this PR increases the size of libcore.rlib from 57834522 bytes to 71612866 bytes, or 23.8%.

@Kampfkarren
Copy link
Contributor Author

Hm. Any way around that?

@RalfJung
Copy link
Member

Yes, it was proposed here 15 days ago. That will help a lot. But as others noted above it might need some new trait system trickery.

Even better would be to make FnPtr a magic trait that is automatically implemented for all fn types.

@dtolnay dtolnay removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 6, 2022
@bors
Copy link
Contributor

bors commented Jul 19, 2022

☔ The latest upstream changes (presumably #98180) made this pull request unmergeable. Please resolve the merge conflicts.

@Kampfkarren
Copy link
Contributor Author

I don't think I have the time to make a more closer to the compiler change like an FnPtr trait right now (and it feels like I'd want to have an RFC for that anyway)

@RalfJung
Copy link
Member

FWIW a FnPtr trait could start as a library-only change: using a macro to implement FnPtr for a lot of fn types. That would still lead to a smaller rmeta file than the current approach, since we only have 1 instance per fn type, not N (one for each relevant trait).

@CAD97
Copy link
Contributor

CAD97 commented Jul 29, 2022

Related:

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 21, 2022
…pls, r=thomcc

Add default trait implementations for "c-unwind" ABI function pointers

Following up on rust-lang#92964, only add default trait implementations for the `c-unwind` family of function pointers. The previous attempt in rust-lang#92964 added trait implementations for many more ABIs and ran into concerns regarding the increase in size of the libcore rlib.

An attempt to abstract away function pointer types behind a unified trait to reduce the duplication of trait impls is being discussed in rust-lang#99531 but this change looks to be blocked on a lang MCP.

Following `@RalfJung's` suggestion in rust-lang#99531 (comment), this commit is another cut at rust-lang#92964 but it _only_ adds the impls for `extern "C-unwind" fn` and `unsafe extern "C-unwind" fn`.

I am interested in landing this patch to unblock the stabilization of the `c_unwind` feature.

RFC: rust-lang/rfcs#2945
Tracking Issue: rust-lang#74990
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.