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: rename the type in an impl to match its containing page #56013

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@QuietMisdreavus
Member

QuietMisdreavus commented Nov 17, 2018

Currently, if you pub use Something as SomethingElse, and Something has some impl blocks on it, they won't be renamed to match SomethingElse, creating confusion if the original type name is not actually public, or expected to be the public API of that type. This leads to situations where the file name, page title, and item definition section will use the name from the re-export, and all the impls will use the original name. This PR changes how we render impls if they come from item pages, so that their name matches their containing item instead of the original declaration.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 17, 2018

r? @steveklabnik

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

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Nov 17, 2018

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 17, 2018

Thanks!

@bors: r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 17, 2018

📌 Commit 002b000 has been approved by GuillaumeGomez

@ollie27

This comment has been minimized.

Contributor

ollie27 commented Nov 17, 2018

I guess this is trying to fix #41072 and #42675. However it isn't complete as the old name still appears in methods signatures and generic parameters. The bigger issue is that the for type doesn't always match the page it's being documented on:

mod inner {
    pub struct SomeStruct;
    impl SomeStruct {
        pub fn new() -> SomeStruct { SomeStruct }
    }
    impl From<SomeStruct> for Vec<u8> {
        fn from(x: SomeStruct) -> Vec<u8> {
            Vec::new()
        }
    }
}
pub use inner::SomeStruct as MyStruct;

results in:

image

with this PR.

I think a solution is to look up the last name from cache().paths inside resolved_path for all types rather than trying to pass the renamed value through everything.

@bors r-

@Aaronepower

This comment has been minimized.

Contributor

Aaronepower commented Nov 25, 2018

Triage; @QuietMisdreavus Hello, have you been able to get back to this PR?

@TimNN

This comment has been minimized.

Contributor

TimNN commented Dec 4, 2018

Ping from triage @QuietMisdreavus: It looks like some changes have been requested to your PR.

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Dec 4, 2018

Sorry! It's been a couple weeks since i ran through my GitHub notifications - starting with American Thanksgiving and following up with a cold - and i ran through all my reviews first. I'll update this PR "Soon ™️".

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Dec 4, 2018

@ollie27 So i just tried using cache().paths, and it looks like it won't be useful if an item is public and being renamed in another location (paths has the original name), or if an item is being exported in different locations with different names (paths has the first name it saw), or both (paths has the last name it saw, oddly enough). We'll probably need a separate map of all known re-exports of a thing, so we can track which one is being documented.

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Dec 12, 2018

I think for the time being, i'll close this PR. This needs to go back to the drawing board, but i'll need to revisit it later.

@QuietMisdreavus QuietMisdreavus deleted the QuietMisdreavus:a-rose-by-any-other-name branch Dec 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment