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 doesn't handle impl section of type definitions #32077

Closed
rtbo opened this issue Mar 6, 2016 · 24 comments · Fixed by #112429 or #115201
Closed

Rustdoc doesn't handle impl section of type definitions #32077

rtbo opened this issue Mar 6, 2016 · 24 comments · Fixed by #112429 or #115201
Labels
C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@rtbo
Copy link

rtbo commented Mar 6, 2016

In rust-xcb I have the following code:

/// a key was pressed/released
pub type KeyPressEvent = base::Event<xcb_key_press_event_t>;

impl KeyPressEvent {
    /// The keycode (a number representing a physical key on the keyboard) of the key
    /// which was pressed.
    pub fn detail(&self) -> Keycode { ... }

    /// Time when the event was generated (in milliseconds).
    pub fn time(&self) -> Timestamp { ... }

    ...
}

However as one can see here, the impl section is ignored.

@steveklabnik
Copy link
Member

Yes, they're currently ignored. I believe @alexcrichton was the one who made this change?

@steveklabnik steveklabnik added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Mar 6, 2016
@alexcrichton
Copy link
Member

This was never really "added" or "changed", it's just a bug in the implementation.

@rtbo
Copy link
Author

rtbo commented Mar 7, 2016

@srinivasreddy No I'm not, but I'd be happy to see this fixed

@srinivasreddy
Copy link
Contributor

@rtbo Thanks for confirmation. I am on it.

@mitaa
Copy link
Contributor

mitaa commented Mar 8, 2016

What is the expectation here?

To only show direct impls on the typedef (impl KeyPressEvent ...) or to show all impls on the type the typedef is referencing? (impl base::Event<xcb_key_press_event_t> ...)

@mitaa
Copy link
Contributor

mitaa commented Mar 8, 2016

related #19381

@rtbo
Copy link
Author

rtbo commented Mar 8, 2016

@mitaa my expectation would be to have on the rustdoc page of the type alias the methods documentation from the same alias.

@gyscos
Copy link

gyscos commented Jul 14, 2016

I'd say methods from the original type that apply should be shown here, just like methods from Derefs would.

@aldanor
Copy link

aldanor commented Mar 17, 2017

Wonder whether it's still considered a known/ignored rustdoc bug? It's a bit of a PITA for crates that defined most types as specialisations, which is a quite common part in FFI-heavy code (this and the fact that typedefs are eagerly expanded everywhere even when it doesn't make much sense).

@raphlinus
Copy link
Contributor

+1 on this, https://docs.rs/xi-rope/0.2.0/xi_rope/rope/type.Rope.html is missing all the impl Rope methods, which is where so much of the good stuff is.

@mjkillough
Copy link
Contributor

I am looking into this, as it makes rust-xcb quite difficult to use. It's my first time poking around in the rustdoc code, so it might take me a little while.

My initial prototype renders this code:

pub trait Trait1 {
    /// From Trait1
    fn trait_func1();
}

pub trait Trait2 {
    /// From Trait2
    fn trait_func2();
}

/// This is a MyStruct.
pub struct MyStruct;

impl MyStruct {
    /// do_stuff() with MyStruct
    pub fn do_stuff(&self) {}
}

impl Trait1 for MyStruct {
    fn trait_func1() {}
}

/// This is a MyAlias
pub type MyAlias = MyStruct;

impl MyAlias {
    /// do_more_stuff() with MyAlias
    pub fn do_more_stuff(&self) {}
}

impl Trait2 for MyAlias {
    fn trait_func2() {}
}

... like this:

image

I haven't played around with Deref to see how it interacts, but I'll do that next.

@mjkillough
Copy link
Contributor

mjkillough commented May 15, 2017

I'd missed that there was a PR (#25892) referenced from related issue #19381, which took a similar approach to the one I was preparing. That PR was closed as @alexcrichton pointed out:

I'm also not sure if we want to take this route just yet due to #14072. For example on the documentation page for io::Result the parameter E is generic instead of being wired up to Error.

You can see this in the following example:

/// A generic struct.
pub struct GenericStruct<T> {
    i: T
}

impl<T> GenericStruct<T> {
    /// A method on TypedefStruct.
    pub fn generic_impl_method(arg: T) {}
}

/// A typedef for GenericStruct, without any generics.
pub type TypedefStruct = GenericStruct<u8>;

impl TypedefStruct {
    /// A method on TypedefStruct.
    pub fn typedef_impl_method() {}
}

... which renders as:

image

In the impl for GenericStruct, it isn't obvious that the type variable T is u8.

I had a look at coming up with a solution for #14072, but it's very involved and I think it's beyond my abilities. I'll continue to take a look, but I am not optimistic!

From the closed PR #25892:

I suppose it's also somewhat unclear to me what the motivation for this is because finding the documentation of the typedef itself should just be following a link.

It is worth pointing out, that in the case of rust-xcb, it has impl blocks on type aliases, which aren't visible anywhere in the docs due to this issue. It's especially hard to discover the API, as the impl blocks are automatically generated bindings.

Would it be an improvement if the page for TypedefStruct would show only the default impl and trait impls made directly on TypedefStruct, but nothing for GenericStruct? The user can still click on the GenericStruct to get to the rest of the documentation, as they do today. (This solution isn't ideal, but it avoids making #14072 worse).

@Ralith
Copy link
Contributor

Ralith commented May 15, 2017

This is also badly needed in nalgebra, which suffers both from otherwise-invisible impl blocks and from its type aliases ultimately resolving to complicated generic types which have a very large number of methods, most of which are not relevant to any particular alias the user might be coming from. In this case, displaying the otherwise-invisible impl blocks would be an improvement, though it wouldn't be a complete solution.

@mjkillough
Copy link
Contributor

This is also badly needed in nalgebra, which suffers both from otherwise-invisible impl blocks and from its type aliases ultimately resolving to complicated generic types which have a very large number of methods, most of which are not relevant to any particular alias the user might be coming from.

Ah, I hadn't even considered the need to recursively follow type aliases, which would be necessary for nalgebra:

type Matrix2<N> = MatrixN<N, U2>;
type MatrixN<N, D> = MatrixNM<N, D, D>;
type MatrixNM<N, R, C> = Matrix<N, R, C, MatrixArray<N, R, C>>;
pub struct Matrix<N: Scalar, R: Dim, C: Dim, S> { .. }

// Ideally this would be visible in the documentation for Matrix2<N>,
// with the correct types substituted throughout the impl:
impl<N, R: Dim, C: Dim, S> Matrix<N, R, C, S> 
where N: Scalar + ClosedNeg,
        S: StorageMut<N, R, C> { .. }

In this case, displaying the otherwise-invisible impl blocks would be an improvement, though it wouldn't be a complete solution.

I've created a pull request (#42027) for this simple improvement.

@aldanor
Copy link

aldanor commented May 16, 2017

@mjkillough Just to add, in my case it looks like this:

trait ID {}
struct Object<T: ID> {}
impl<T: ID> Object<T: ID> { /* shared methods */ }

struct FileID {}
impl ID for FileID {}
pub type File = Object<FileID>;
impl File { /* file-specific methods */ }

struct GroupID {}
impl ID for GroupID {}
pub type Group = Object<GroupID>;
impl Group { /* group-specific methods */ }

trait ContainerID : ID {}
impl ContainerID for FileID {}
impl ContainerID for GroupID {}
impl<T: ContainerID> Object<T> { /* container methods */ }

This way, help for File would ideally display (1) base methods from Object, (2) methods from File, (3) shared Container methods. Currently, it displays none, of course...

@steveklabnik steveklabnik added T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. and removed T-tools labels May 18, 2017
bors added a commit that referenced this issue Jun 9, 2017
Document direct implementations on type aliases.

This improves #32077, but is not a complete fix.

For a type alias `type NewType = AliasedType`, it will include any `impl NewType` and `impl
Trait for NewType` blocks in the documentation for `NewType`.

A complete fix would include the implementations from the aliased type in the type alias' documentation, so that users have a complete picture of methods that are available on the alias. However, to do this properly would require a fix for #14072, as the alias may affect the type parameters of the type alias, making the documentation difficult to understand. (That is, for `type Result = std::result::Result<(), ()>` we would ideally show documentation for `impl Result<(), ()>`, rather than generic documentation for `impl<T, E> Result<T, E>`).

I think this improvement is worthwhile, as it exposes implementations which are not currently documented by rustdoc. The documentation for the implementations on the aliased type are still accessible by clicking through to the docs for that type. (Although perhaps it's now less obvious to the user that they should click-through to get there).
@aldanor
Copy link

aldanor commented Jul 9, 2017

(e.g. this example and this one, may also involve #14072 to some extent...)

@jyn514
Copy link
Member

jyn514 commented Jan 3, 2021

MCVE:

pub struct Event {}
pub type KeyPressEvent = Event;

impl Event {
    pub fn detail(&self) {}
}

As of 1.48 stable, detail() will appear in the documentation for Event, but not in the documentation for KeyPressEvent.

@jyn514
Copy link
Member

jyn514 commented Jan 3, 2021

Note that a workaround is to use KeyPressEvent instead of Event in the impl block, which shows the methods in both places as expected.

@roblabla
Copy link
Contributor

roblabla commented May 12, 2021

This would be extremely useful for all svd2rust-generated crates (e.g. the whole embedded ecosystem). Currently, svd2rust generates a bunch of typedefs for each registers, like this: https://docs.rs/nrf51-hal/0.12.1/nrf51_hal/pac/uart0/pseltxd/type.W.html

The problem is, it can be very hard to know what function is available on this typedef, as the generic W struct will have methods for every register available on the device: https://docs.rs/nrf51-hal/0.12.1/nrf51_hal/pac/generic/struct.W.html

This makes life quite miserable.

My expectation is that typedefs would list all impls that apply to them, filtering out all impls that don't apply to that instantiation of the type (e.g. if we have type A = GenericType<B>, it shouldn't list impls of GenericType<C> unless C and B overlap).

@ehuss ehuss removed the T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. label Jan 18, 2022
@sebcrozet
Copy link
Contributor

@GuillaumeGomez One of the comments above sums up the type alias issues I mentioned: #32077 (comment) Ideally, the documentation of the alias should include whatever method is applicable to that type (taking into account potential trait bounds).

One example in nalgebra is that the Vector3 doc doesn’t mention any method (other than some conversion trait implementations), where it should ideally show methods (implemented for the aliased Matrix type) like dot, cross, etc., but should not show methods that are applicable to the generic Matrix but not Vector3 itself due to the selection of dimensions (for example the cholesky method).

@GuillaumeGomez
Copy link
Member

Noted. I'll try to fix this issue in the next weeks. Thanks for the ping!

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jun 9, 2023
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jun 9, 2023
…otriddle,lcnr

[rustdoc] List matching impls on type aliases

Fixes rust-lang#32077.

Thanks a lot to `@lcnr` who helped me a lot with this fix!

cc `@notriddle`
r? `@lcnr`
@bors bors closed this as completed in f83c8e4 Jun 9, 2023
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jun 12, 2023
@GuillaumeGomez
Copy link
Member

Sadly, we had to revert #112429 because it introduced this bug: #112515. Once this bug is fixed, we can put back the fix.

@bors bors closed this as completed in 1fb672c Sep 8, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 8, 2023
Rollup merge of rust-lang#115201 - notriddle:notriddle/type-alias-impl-list, r=GuillaumeGomez

rustdoc: list matching impls on type aliases

Fixes rust-lang#32077

Fixes rust-lang#99952

Remake of rust-lang#112429

Partially reverts rust-lang#112543, but keeps the test case.

This version of the PR avoids the infinite loop by structurally matching types instead of using full unification. This version does not support type alias trait bounds, but the compiler does not enforce those anyway (rust-lang#21903).

r? `@GuillaumeGomez`

CC `@lcnr`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet