Restore trait impl docs by pierzchalski · Pull Request #32935 · rust-lang/rust

pierzchalski · 2016-04-13T14:09:36Z

Currently, documentation on methods in trait implementations doesn't get rendered. This changes that; trait implementations have all documentation associated with impl items displayed (documentation from the trait definition is ignored).

Fixes#24838
Fixes#26871

rust-highfive · 2016-04-13T14:09:44Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

steveklabnik · 2016-04-13T14:23:47Z

Love this change conceptually, not great with rustdoc's codebase yet. r? @alexcrichton

mitaa · 2016-04-13T14:36:10Z

manual trait implementations have their documentation displayed, however derived traits don't.

I think it would make sense to instead only document(..) such impl-items if they actually have doc-comments (item.doc_value().is_some())

Then you could remove the render_docs: bool parameter and instead change the behaviour based on link: AssocItemLink.

mitaa · 2016-04-13T14:40:55Z

Could you also add some tests in src/test/rustdoc/?
(you can run these tests with, for example,make check-stage1-rustdocck)

GuillaumeGomez · 2016-04-13T15:08:32Z

What the hell is happening in commits messages in here?! XD

mitaa · 2016-04-13T15:13:17Z

and instead change the behaviour based on link: AssocItemLink.

Actually, I think the current match on AssocItemLink in doctraititem could be just removed (and the if guard converted to a normal if..) since html::render::document already checks for item.doc_value().is_some() (it would render stability attributes too, but I think that's not really a problem).

So the only necessary change would be to make here.

alexcrichton · 2016-04-13T16:32:56Z

I'm on board with everything @mitaa has mentioned :)

Add test to check this resolvesrust-lang#24838 and rust-lang#26871.

pierzchalski · 2016-04-14T06:17:31Z

Applied both of @mitaa's suggestions. Currently this means derived implementations also print any associated docs. Is that an issue?

@GuillaumeGomez I don't know what you're talking about :D
For future reference, I force-pushed a clean branch because @mitaa's suggestions were basically a re-write of what I'd done (and also because I prefer to rebase off master than merging it in). Is that an acceptable practice for Rust PRs, or would a bunch of reversions/merges be preferable?

mitaa · 2016-04-14T06:31:40Z

Currently this means derived implementations also print any associated docs. Is that an issue?

I would assume that they don't have any 😄
(also from looking at the source just now it doesn't look that way)

For future reference, I force-pushed a clean branch because @mitaa's suggestions were basically a re-write of what I'd done (and also because I prefer to rebase off master than merging it in). Is that an acceptable practice for Rust PRs, or would a bunch of reversions/merges be preferable?

Yes, that's completely fine.

In `test/rustdoc/manual_impl.rs` there are now three structs: * S1 implements and documents required method `a_method`. * S2 implements and documents `a_method` as well as provided method `b_method`. * S3 implements `a_method` and `b_method`, but only documents `b_method`. For a struct, we want the rendered trait impls to include documentation if and only if it appears on the trait implementation itself (since users can just go to the trait definition for anything not covered in the impl docs). This means we expect: * S1, S2, and S3 to all include top-level trait impl docs. * S1, S2, and S3 to exclude all trait definition docs. * S1 to show impl docs for `a_method`. * S2 to show impl docs for `a_method` and `b_method`. * S3 to show impl docs for `b_method`. These tests cover those cases.

pierzchalski · 2016-04-15T04:07:41Z

@mitaa I'm looking at the docs (well, what make docs outputs), and I'm seeing some weird behaviour. For instance on the derived Clone impls for Option and Result, the required method clone has no docblock but the provided method clone_from does have one, even though they both have docs in the source for Clone. Turns out this also happens on manual trait impls.

I've updated the test to demonstrate the intended vs. actual behaviour, and now I'm hunting through html/render.rs and clean/mod.rs looking for why provided methods get the "doc" attribute of their definition but manual methods don't (which is what I'm guessing is causing the issue).

mitaa · 2016-04-15T04:21:58Z

Nice catch!

For non-overridden default methods we actually render the method of the trait itself (which obviously has the doc attribute).
https://github.com/pierzchalski/rust/blob/restore_trait_impl_docs/src/librustdoc/html/render.rs#L2555-L2581

doctraititem probably needs a ~is_default_item: bool argument after all.

mitaa · 2016-04-15T04:27:17Z

Well, that coupled with checking that link == Anchor::GotoSource(..). (we still want to show the docs on the trait itself)

We don't want to render default item docs but previously `doctraititem` naively delegated to the trait definition in those cases. Updated tests to also check that this doesn't strip default item docs from the trait definition.

pierzchalski · 2016-04-15T04:43:44Z

Done.

Also, it looks like the AssocItemLink::Anchor test was unnecessary since doctraititem is only called to render trait impls, and doesn't affect the rendering of trait definitions including default items.

alexcrichton · 2016-04-15T16:01:52Z

@bors: r+ ec5e0f8

Thanks @pierzchalski!

…s, r=alexcrichton Restore trait impl docs Currently, documentation on methods in trait implementations doesn't get rendered. This changes that; trait implementations have all documentation associated with impl items displayed (documentation from the trait definition is ignored). Fixesrust-lang#24838Fixesrust-lang#26871

Rollup of 11 pull requests - Successful merges: #32923, #32926, #32929, #32931, #32935, #32945, #32946, #32964, #32970, #32973, #32997 - Failed merges:

rust-highfive assigned steveklabnik Apr 13, 2016

rust-highfive assigned alexcrichton and unassigned steveklabnik Apr 13, 2016

Delegate whether to print docblocks to 'document'
d8d8669
Add test to check this resolvesrust-lang#24838 and rust-lang#26871.

pierzchalski force-pushed the restore_trait_impl_docs branch from da0ef9d to d8d8669Compare April 14, 2016 06:13

Add flag for whether an item is default or not.
ec5e0f8
We don't want to render default item docs but previously `doctraititem` naively delegated to the trait definition in those cases. Updated tests to also check that this doesn't strip default item docs from the trait definition.

Manishearth mentioned this pull request Apr 15, 2016
Rollup of 11 pull requests #33005
Merged

bors added a commit that referenced this pull request Apr 15, 2016
Auto merge of#33005- Manishearth:rollup, r=Manishearth
576229f
Rollup of 11 pull requests - Successful merges: #32923, #32926, #32929, #32931, #32935, #32945, #32946, #32964, #32970, #32973, #32997 - Failed merges:

bors merged commit ec5e0f8 into rust-lang:masterApr 16, 2016

pierzchalski deleted the restore_trait_impl_docs branch April 16, 2016 23:06

Uh oh!

Uh oh!

Conversation

pierzchalski commented Apr 13, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Apr 13, 2016

Uh oh!

steveklabnik commented Apr 13, 2016

Uh oh!

mitaa commented Apr 13, 2016

Uh oh!

mitaa commented Apr 13, 2016

Uh oh!

GuillaumeGomez commented Apr 13, 2016

Uh oh!

mitaa commented Apr 13, 2016

Uh oh!

alexcrichton commented Apr 13, 2016

Uh oh!

pierzchalski commented Apr 14, 2016

Uh oh!

mitaa commented Apr 14, 2016

Uh oh!

pierzchalski commented Apr 15, 2016

Uh oh!

mitaa commented Apr 15, 2016

Uh oh!

mitaa commented Apr 15, 2016

Uh oh!

pierzchalski commented Apr 15, 2016

Uh oh!

alexcrichton commented Apr 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pierzchalski commented Apr 13, 2016•
edited
Loading