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: links from items in a trait impl are inconsistent #83068

Merged
merged 8 commits into from
Apr 14, 2021

Conversation

mockersf
Copy link
Contributor

@mockersf mockersf commented Mar 13, 2021

Depending on where the struct implementing a trait is coming from, or the current page, the items in a trait impl are not linking to the same thing:

item trait page, implementors trait page, implementations on Foreign Types struct page, trait implementations
function link to current impl link to first impl in the list link to trait def
default function not present not present link to trait def
default function with custom impl link to current impl link to trait def link to trait def
constant link to current impl link to trait def link to trait def
associated type link to current impl link to trait def link to trait def
missing link to trait def function link wrong + missing link to current impl missing link to current impl
rust code with those cases
pub trait MyTrait {
    type Assoc;
    const VALUE: u32;
    fn trait_function(&self);
    fn defaulted(&self) {}
    fn defaulted_override(&self) {}
}

impl MyTrait for String {
    /// will link to trait def
    type Assoc = ();
    /// will link to trait def
    const VALUE: u32 = 5;
    /// will link to first foreign implementor
    fn trait_function(&self) {}
    /// will link to trait def
    fn defaulted_override(&self) {}
}

impl MyTrait for Vec<u8> {
    /// will link to trait def
    type Assoc = ();
    /// will link to trait def
    const VALUE: u32 = 5;
    /// will link to first foreign implementor
    fn trait_function(&self) {}
    /// will link to trait def
    fn defaulted_override(&self) {}
}

impl MyTrait for MyStruct {
    /// in trait page, will link to current impl
    ///
    /// in struct page, will link to trait def
    type Assoc = bool;
    /// in trait page, will link to current impl
    ///
    /// in struct page, will link to trait def
    const VALUE: u32 = 20;
    /// in trait page, will link to current impl
    ///
    /// in struct page, will link to trait def
    fn trait_function(&self) {}
    /// in trait page, will link to current impl
    ///
    /// in struct page, will link to trait def
    fn defaulted_override(&self) {}
}

pub struct MyStruct;

In this PR, I fixed all links to target the trait definition, and added an anchor-link to the current implementation appearing on mouse hover.

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(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 Mar 13, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

Looks good to me, thanks! Can you add a test in src/test/rustdoc to ensure that it works as expected please? You already wrote the file with all the cases so that should be quick. ;)

@mockersf
Copy link
Contributor Author

test added 👍

// @has trait_impl_items_links_and_anchors/struct.MyStruct.html '//h4[@id="method.trait_function"]//a[@class="fnname"]/@href' ../trait_impl_items_links_and_anchors/trait.MyTrait.html#tymethod.trait_function
// @has trait_impl_items_links_and_anchors/struct.MyStruct.html '//h4[@id="method.trait_function"]//a[@class="anchor"]/@href' #method.trait_function
fn trait_function(&self) {}
// @has trait_impl_items_links_and_anchors/trait.MyTrait.html '//h4[@id="method.defaulted_override-3"]//a[@class="fnname"]/@href' #method.defaulted_override
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 lost here: what's the point of testing that this exists on the trait when we're currently on a trait implementation on a type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is on the struct impl for the trait on the trait doc page. the method.defaulted_override-3 is targeting the impl, not the trait itself

Copy link
Member

Choose a reason for hiding this comment

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

Apparently not because the href you test is "#method.defaulted_override".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh right, in the h4 with id method.defaulted_override-3, there is now the function name (class fnname that links to it's trait definition, so #method.defaulted_override, and anchor (class anchor) that links to itself (the line under this one)

this pr makes it so that every item in a trait impl is that way

Copy link
Member

Choose a reason for hiding this comment

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

That part makes sense, but why are you testing that on the trait page whereas we "are" (so to speak) on the type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as the type implements the trait, it is present on both page

@bors
Copy link
Contributor

bors commented Mar 26, 2021

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

@camelid camelid added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Mar 29, 2021
@jyn514
Copy link
Member

jyn514 commented Apr 13, 2021

ping @GuillaumeGomez - this is waiting on your review.

@mockersf do you mind fixing the merge conflict in the meantime?

@GuillaumeGomez
Copy link
Member

Still in my review list. :)

@rust-log-analyzer

This comment has been minimized.

@mockersf
Copy link
Contributor Author

@mockersf do you mind fixing the merge conflict in the meantime?

fixed!

@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 14, 2021

📌 Commit e36ca09 has been approved by GuillaumeGomez

@bors 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 Apr 14, 2021
@bors
Copy link
Contributor

bors commented Apr 14, 2021

⌛ Testing commit e36ca09 with merge b203b0d...

@bors
Copy link
Contributor

bors commented Apr 14, 2021

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing b203b0d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 14, 2021
@bors bors merged commit b203b0d into rust-lang:master Apr 14, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

8 participants