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

Added traits implemented by FnPtr to fn docs with example function #112106

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mj10021
Copy link
Contributor

@mj10021 mj10021 commented May 30, 2023

As noted in #111182, the fn-ptr docs were not updated to include the FnPtr trait, so I added the implemented traits with an example fn

@rustbot
Copy link
Collaborator

rustbot commented May 30, 2023

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 30, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@@ -1564,6 +1564,36 @@ mod prim_ref {}
///
/// In addition, all *safe* function pointers implement [`Fn`], [`FnMut`], and [`FnOnce`], because
/// these traits are specially known to the compiler.
///
/// All function pointers implement the trait [`FnPtr`], which implements the following traits:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should mention FnPtr as it's currently unstable.

///
/// All function pointers implement the trait [`FnPtr`], which implements the following traits:
///
/// * [`ConstParamTy`]
Copy link
Member

Choose a reason for hiding this comment

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

We should probably leave the unstable traits out of this list, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the only stable trait that is added is Sized, should I just add that to the list of traits on line 1552 and not mention anything about FnPtr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and then should I delete the dummy impl for FnPtr and add one for Sized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @thomcc, just following up on this

@thomcc
Copy link
Member

thomcc commented Jun 1, 2023

@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 Jun 1, 2023
@Dylan-DPC
Copy link
Member

@mj10021 any updates on this?

@mj10021
Copy link
Contributor Author

mj10021 commented Aug 19, 2023

@mj10021 any updates on this?

Hi! Sorry it took me a bit to respond. I just removed the references to unstable features and pushed the update

@mj10021 mj10021 force-pushed the issue-111182-fix branch 2 times, most recently from f788cf6 to fb52b28 Compare August 19, 2023 15:55
@mj10021
Copy link
Contributor Author

mj10021 commented Aug 19, 2023

@rustbot review

@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 Aug 19, 2023
Copy link
Contributor

@darklyspaced darklyspaced left a comment

Choose a reason for hiding this comment

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

other than the phrasing of the docs, it looks great! thanks!

reason = "internal trait for implementing various traits for all function pointers"
)]
#[doc(fake_variadic)]
/// This trait is implemented on function pointers with any number of arguments.
Copy link
Contributor

Choose a reason for hiding this comment

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

function pointers with any number of arguments

is probably better phrased as "all function pointers" for consistency and simplicity's sake

reason = "internal trait for implementing various traits for all function pointers"
)]
#[doc(fake_variadic)]
/// This trait is implemented on function pointers with any number of arguments.
Copy link
Contributor

Choose a reason for hiding this comment

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

same applies here

@darklyspaced
Copy link
Contributor

@rustbot label -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 2, 2023
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2023
@bors
Copy link
Contributor

bors commented Sep 18, 2023

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

@thomcc
Copy link
Member

thomcc commented Feb 1, 2024

I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks.

r? libs

@rustbot rustbot assigned cuviper and unassigned thomcc Feb 1, 2024
@bors
Copy link
Contributor

bors commented Feb 8, 2024

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

@cuviper
Copy link
Member

cuviper commented Feb 11, 2024

Sorry to bounce it back yet again, but can you resolve the conflicts?

@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 Feb 11, 2024
@mj10021
Copy link
Contributor Author

mj10021 commented Feb 11, 2024

Sorry to bounce it back yet again, but can you resolve the conflicts?

@rustbot author

No problem, I think that should be fine now

@rustbot review

@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 Feb 11, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-16 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
#12 writing image sha256:ba46c54faead369cb2d3269fa0f8d1e149e41b446ca0f91db6a9bc3daa587501 done
#12 naming to docker.io/library/rust-ci done
#12 DONE 10.0s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-16]
##[group]Clock drift check
  local time: Sun Feb 11 19:06:57 UTC 2024
  network time: Sun, 11 Feb 2024 19:06:57 GMT
  network time: Sun, 11 Feb 2024 19:06:57 GMT
##[endgroup]
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-16', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'build.optimized-compiler-builtins', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-16/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
##[endgroup]
Testing GCC stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
   Compiling y v0.1.0 (/checkout/compiler/rustc_codegen_gcc/build_system)
    Finished release [optimized] target(s) in 1.39s
     Running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-codegen/x86_64-unknown-linux-gnu/release/y test --use-system-gcc --use-backend gcc --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc --release --no-default-features --mini-tests --std-tests`
Using system GCC
Using system GCC
[BUILD] example
[AOT] mini_core_hello_world
/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc/mini_core_hello_world
abc
---
   Compiling compiler_builtins v0.1.105
error[E0206]: the trait `Copy` cannot be implemented for this type
    --> library/core/src/primitive_docs.rs:1695:23
     |
1695 | impl<Ret, T> Copy for fn(T) -> Ret {


error[E0322]: explicit impls for the `FnPtr` trait are not permitted
     |
     |
1708 | impl<Ret, T> FnPtr for fn(T) -> Ret {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ impl of `FnPtr` not allowed
Some errors have detailed explanations: E0206, E0322.
For more information about an error, try `rustc --explain E0206`.
error: could not document `core`


Caused by:
  process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustdoc --edition=2021 --crate-type lib --crate-name core library/core/src/lib.rs --target x86_64-unknown-linux-gnu -o /checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/x86_64-unknown-linux-gnu/doc/x86_64-unknown-linux-gnu/doc -Zunstable-options --check-cfg 'cfg(feature, values("debug_refcell", "panic_immediate_abort"))' --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat -C metadata=b8766bc33b7c8bac -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/x86_64-unknown-linux-gnu/doc/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/x86_64-unknown-linux-gnu/doc/release/deps -Csymbol-mangling-version=v0 '--check-cfg=cfg(feature,values(any()))' -Zunstable-options '--check-cfg=cfg(bootstrap)' '--check-cfg=cfg(stdarch_intel_sde)' '--check-cfg=cfg(no_fp_fmt_parse)' '--check-cfg=cfg(no_global_oom_handling)' '--check-cfg=cfg(no_rc)' '--check-cfg=cfg(no_sync)' '--check-cfg=cfg(backtrace_in_libstd)' '--check-cfg=cfg(target_env,values("libnx"))' '--check-cfg=cfg(target_arch,values("spirv","nvptx","xtensa"))' -Dwarnings '-Wrustdoc::invalid_codeblock_attributes' --crate-version '1.78.0-nightly (63e43870a 2024-02-11)' '-Zcrate-attr=doc(html_root_url="https://doc.rust-lang.org/nightly/")' '-Zcrate-attr=warn(rust_2018_idioms)' -Z unstable-options --resource-suffix 1.78.0 --markdown-css rust.css --markdown-no-toc --index-page /checkout/src/doc/index.md` (exit status: 1)
Build completed unsuccessfully in 0:46:16
  local time: Sun Feb 11 19:53:47 UTC 2024
  network time: Sun, 11 Feb 2024 19:53:47 GMT
##[error]Process completed with exit code 1.

@mj10021
Copy link
Contributor Author

mj10021 commented Feb 13, 2024

I'm not exactly sure how to work around that error, should the FnPtr be implemented differently? iirc this is a new error

@cuviper
Copy link
Member

cuviper commented Feb 14, 2024

Sorry, I don't know either!

@Amanieu
Copy link
Member

Amanieu commented Feb 21, 2024

@GuillaumeGomez This PR tries to make trait impls appear in rustdoc for function pointer types. Do you have a better idea on how we can make this work? Or some suggestions?

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Feb 21, 2024

It seems to be a limitation coming from how rustdoc uses the compiler. We might need to get help from the compiler team here to figure how to solve this issue.

Maybe you might have an idea @fmease ?

@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Feb 21, 2024
@cuviper
Copy link
Member

cuviper commented Mar 7, 2024

@rustbot blocked

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 7, 2024
@bors
Copy link
Contributor

bors commented Mar 26, 2024

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

@fmease
Copy link
Member

fmease commented Apr 19, 2024

Sorry for not getting back to you. I've discussed this with Guillaume and a T-compiler member a while ago. This error is intentional to uphold some soundness invariants of the trait solver and caused by the #[rustc_deny_explicit_impl] on trait FnPtr.

While we could theoretically turn it into a #[cfg_attr(not(doc), rustc_deny_explicit_impl)] to make your PR compile, I guess I would advice against this since it would make it possible to bypass rustc_deny_explicit_impl in the (arguably unlikely) scenario of a user compiling core themselves with doc enabled manually (e.g. with -Zbuild-std + --cfg doc).

An alternative to that would be patching the trait solver(s) to skip the rustc_deny_explicit_impl check if actually_rustdoc.

I'm not super happy about any of these options (there are more sophisticated ones like introducing more internal attributes etc.).

@lcnr
Copy link
Contributor

lcnr commented Aug 2, 2024

An alternative to that would be patching the trait solver(s) to skip the rustc_deny_explicit_impl check if actually_rustdoc.

That seems easiest to me 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues about the rust-lang/rust repository. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.