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

rustdoc: Clone's implementations on CString is considered foreign #97610

Closed
jsha opened this issue Jun 1, 2022 · 2 comments
Closed

rustdoc: Clone's implementations on CString is considered foreign #97610

jsha opened this issue Jun 1, 2022 · 2 comments
Labels
A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@jsha
Copy link
Contributor

jsha commented Jun 1, 2022

On https://doc.rust-lang.org/nightly/std/clone/trait.Clone.html#foreign-impls, CString, FromBytesUntilNulError, FromBytesWithNulError, FromVecWithNulError, IntoStringError, and NulError are considered "Implementations on Foreign Types", even though each of those types exists in std.

On stable (https://doc.rust-lang.org/stable/std/clone/trait.Clone.html#foreign-impls), those types are not considered "Implementations on Foreign Types."

I believe this is caused indirectly by 7f3cc2f (/cc @petrochenkov). That commit changed the export style for CString etc from pub use to pub type CString = alloc::ffi::CString.

However, this could also be considered a rustdoc bug. MCVE:

yarble.rs

pub struct Yarb;
pub struct Proust;
pub struct Dsug;

foo.rs

extern crate yarble;

pub trait Bar {}

impl Bar for yarble::Yarb{}
impl Bar for yarble::Proust{}

pub use yarble::Yarb;

pub type Proust = yarble::Proust;
pub type Dsug = yarble::Dsug;

impl Bar for Dsug{}

run:

rustc --crate-type lib yarble.rs && rm -r doc ; rustdoc -L . foo.rs && open doc/foo/trait.Bar.html

Expected result:

All three of Yarble, Dsug, and Proust are considered plain "Implementations".

Actual result:

Proust is considered an "Implementation on Foreign Type" (screenshot below).

image

@jsha jsha added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. C-bug Category: This is a bug. A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate labels Jun 1, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 1, 2022
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 1, 2022
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 1, 2022
…e, r=GuillaumeGomez

rustdoc: Improve calculation of "Impls on Foreign Types"

The existing code to calculate whether an implementation was on a "Foreign Type" was duplicated across the sidebar generation and the page generation. It also came to the wrong conclusion for some cases where both the trait and the "for" type were re-exports.

This PR extracts the logic into a method of `Impl`, breaks it into a multi-line method so it can be commented, and adds a case for when the trait and the "for" type came from the same crate. This fixes some cases - like the platform-specific integer types (`__m256`, `__m128`, etc). But it doesn't fix all cases. See the screenshots below.

[Before](https://doc.rust-lang.org/nightly/std/clone/trait.Clone.html#foreign-impls):

<img src="https://user-images.githubusercontent.com/220205/171338226-59ce6daf-3d76-4bad-bc8d-72a8259a8f43.png" width=200>

[After](https://rustdoc.crud.net/jsha/implementation-is-on-local-type/std/clone/trait.Clone.html):

<img src="https://user-images.githubusercontent.com/220205/171338147-28308a65-1597-4223-be47-9550062404dd.png" width=200>

The remaining types (`CString`, `NulError`, etc) are all from the `alloc` crate, while the `Clone` trait is from the `core` crate. Since `CString` and `Clone` are both re-exported by `std`, they are logically local to each other, but I couldn't figure out a good way to detect that in this code. I figure this is still a good step forward.

Related: rust-lang#97610

r? `@camelid`
@jsha
Copy link
Contributor Author

jsha commented Jun 2, 2022

With #97613 merged, the situation is better for the __m256 types, but still is not solved for CString, NulError, etc.

It occurred to me that std should actually have no cases of "Implementations on Foreign Types," so we can get a broader view of the issue by searching for that string in std docs:

$ rg "on Foreign Types" build/x86_64-unknown-linux-gnu/doc/ -l | sed 's,.*/doc/,https://doc.rust-lang.org/nightly/,'

https://doc.rust-lang.org/nightly/std/clone/trait.Clone.html
https://doc.rust-lang.org/nightly/std/fmt/trait.Display.html
https://doc.rust-lang.org/nightly/std/fmt/trait.Debug.html
https://doc.rust-lang.org/nightly/std/cmp/trait.Eq.html
https://doc.rust-lang.org/nightly/std/cmp/trait.PartialOrd.html
https://doc.rust-lang.org/nightly/std/cmp/trait.PartialEq.html
https://doc.rust-lang.org/nightly/std/cmp/trait.Ord.html
https://doc.rust-lang.org/nightly/std/borrow/trait.Borrow.html
https://doc.rust-lang.org/nightly/std/borrow/trait.ToOwned.html
https://doc.rust-lang.org/nightly/std/convert/trait.AsRef.html
https://doc.rust-lang.org/nightly/std/convert/trait.From.html
https://doc.rust-lang.org/nightly/std/default/trait.Default.html
https://doc.rust-lang.org/nightly/std/marker/trait.StructuralEq.html
https://doc.rust-lang.org/nightly/std/marker/trait.StructuralPartialEq.html
https://doc.rust-lang.org/nightly/std/hash/trait.Hash.html
https://doc.rust-lang.org/nightly/std/error/trait.Error.html
https://doc.rust-lang.org/nightly/std/ops/trait.Index.html
https://doc.rust-lang.org/nightly/std/ops/trait.Deref.html
https://doc.rust-lang.org/nightly/std/ops/trait.Drop.html
https://doc.rust-lang.org/nightly/alloc/string/trait.ToString.html
https://doc.rust-lang.org/nightly/alloc/slice/trait.Join.html
https://doc.rust-lang.org/nightly/alloc/slice/trait.Concat.html
https://doc.rust-lang.org/nightly/alloc/borrow/trait.ToOwned.html

Some interesting ones in there:

alloc::string::ToString considers the impls on char, u8, i8, and str to be foreign, while std:::string::ToString does not.

alloc::slice:::Concat considers both of its impls to be foreign, while std::slice::Concat does not.

alloc::borrow::ToOwned considers its impls on [T: Clone], str, and CStr to be foreign, while std::borrow::ToOwned only considers CStr to be foreign.

The remaining traits consider the alloc::ffi structs to be foreign.

@jsha
Copy link
Contributor Author

jsha commented Jun 19, 2022

This is fixed (in a hacky but okay way) by #97735.

@jsha jsha closed this as completed Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants