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: Allow private modules be included in docs #18822

Merged
merged 1 commit into from
Nov 21, 2014

Conversation

scialex
Copy link
Contributor

@scialex scialex commented Nov 10, 2014

Changed rustdoc so that if we do not have the strip-private pass
enabled private modules will be included in the generated documentation

I added this because it is useful to be able to read the documentation in the very nice rustdoc web interface when doing internal work on a project. In this case we want the rustdoc to include modules that are hidden from consumers. Since this is not currently possible I added it here.

@huonw
Copy link
Member

huonw commented Nov 10, 2014

I was under the impression that disabling strip-private should show all private items, is this not the case?

@scialex
Copy link
Contributor Author

scialex commented Nov 10, 2014

@huonw Sort of. It will not traverse down into modules not defined as pub. This makes it do that.

@scialex
Copy link
Contributor Author

scialex commented Nov 10, 2014

If you want I can change this so that without strip-private we will include private modules, but I am not sure this will not change the output of rustdoc when the pass is included.

@alexcrichton
Copy link
Member

I agree with @huonw in this was this purpose of the strip-private pass. If you're not specifying that pass (you'll have to disable the default ones) and still not seeing private modules, then I think that's the bug that needs fixing.

@scialex
Copy link
Contributor Author

scialex commented Nov 10, 2014

@alexcrichton KK. I will update this pull request with this implemented that way.

@alexcrichton
Copy link
Member

Thanks @scialex!

@scialex
Copy link
Contributor Author

scialex commented Nov 10, 2014

@alexcrichton This is a change that should make no strip-private work correctly.

I've been having unrelated problems building and needed to start from scratch for a full test. I think this should work, but haven't been able to really test it. If anyone has a mostly built rust and wants to test it for me I'd be very appreciative.

@scialex
Copy link
Contributor Author

scialex commented Nov 10, 2014

@alexcrichton, @huonw asking you guys this since you are marked as 'owner'

this PR ( scialex@46306c6 ) seems to cause some bug in rustdoc. make install exits with the error

...
rustdoc: doc/rustc/index.html
/contrib/projects/rust/rust/src/librustc/middle/trans/glue.rs:531:31: 531:35 warning: deprecated syntax, use `for` keyword now
/contrib/projects/rust/rust/src/librustc/middle/trans/glue.rs:531                      helper: <'blk, 'tcx> |Block<'blk, 'tcx>, ValueRef, ty::t|
                                                                                                ^~~~
/contrib/projects/rust/rust/src/librustc/middle/trans/base.rs:1802:39: 1802:43 warning: deprecated syntax, use `for` keyword now
/contrib/projects/rust/rust/src/librustc/middle/trans/base.rs:1802                      maybe_load_env: <'blk, 'tcx> |Block<'blk, 'tcx>, ScopeId|
                                                                                                         ^~~~
/contrib/projects/rust/rust/src/librustc/metadata/decoder.rs:633:35: 633:39 warning: deprecated syntax, use `for` keyword now
/contrib/projects/rust/rust/src/librustc/metadata/decoder.rs:633 pub type DecodeInlinedItem<'a> = <'tcx> |cdata: Cmd,
                                                                                                   ^~~~
<stdin>:2:8: 2:9 error: unknown start of token: \\
<stdin>:2        \        \    /  /
                 ^

task '<main>' panicked at 'Box<Any>', /contrib/projects/rust/rust/src/libsyntax/diagnostic.rs:86

when I try to build it.

GDB Backtrace is:

$ LD_LIBRARY_PATH=/gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib:$LD_LIBRARY_PATH gdb --args /contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/bin/rustdoc --cfg dox --cfg stage2 /contrib/projects/rust/rust/src/librustc/lib.rs
...
#0  0x00007ffff73633f0 in rust_panic ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustrt-4e7c5e5c.so
#1  0x00007ffff7363ab6 in unwind::begin_unwind_inner::h86041e0f95d75c4dE9c ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustrt-4e7c5e5c.so
#2  0x00007ffff2e6ee29 in unwind::begin_unwind::h15144528825213807501 ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/libsyntax-4e7c5e5c.so
#3  0x00007ffff2e43187 in diagnostic::SpanHandler::span_fatal::h6faeeffc14c809dfzUF ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/libsyntax-4e7c5e5c.so
#4  0x00007ffff2e87b9b in parse::lexer::StringReader$LT$$x27a$GT$::fatal_span::h55bd0652042d52b7UUJ ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/libsyntax-4e7c5e5c.so
#5  0x00007ffff2e89e1c in parse::lexer::StringReader$LT$$x27a$GT$::fatal_span_char::h2e2befc6346c6898sWJ ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/libsyntax-4e7c5e5c.so
#6  0x00007ffff2e85545 in parse::lexer::StringReader$LT$$x27a$GT$::advance_token::h7180c916e6f0de2a6YJ
    ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/libsyntax-4e7c5e5c.so
#7  0x00007ffff2e81b85 in parse::lexer::StringReader$LT$$x27a$GT$.Reader::next_token::hba722744aa307ad5yOJ ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/libsyntax-4e7c5e5c.so
#8  0x00007ffff78ef2ec in html::highlight::highlight::h439fcb4bfb15efc1x6i ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#9  0x00007ffff7903ac8 in html::markdown::render::block::h8d36905b36a23b7cIZl ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#10 0x00007ffff7a1fdb2 in parse_block.part.18 ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#11 0x00007ffff7a22e77 in hoedown_document_render ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#12 0x00007ffff790201d in html::markdown::render::hbffb91308ecf1d0doZl ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#13 0x00007ffff790acf1 in html::markdown::Markdown$LT$$x27a$GT$.fmt..Show::fmt::h30c1a6206e162523bym
    ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#14 0x00007ffff73b3512 in fmt::write::h4c40aa9c07b41a24hHy ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustrt-4e7c5e5c.so
#15 0x00007ffff73a85c5 in fmt::Formatter$LT$$x27a$GT$::write_fmt::hd44200941aedb3ddaWy ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustrt-4e7c5e5c.so
#16 0x00007ffff794000b in html::render::document::hc8d99d037786b41dhLo ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#17 0x00007ffff793b73c in html::render::Item$LT$$x27a$GT$.fmt..Show::fmt::h6df1474bfc3e0461boo ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#18 0x00007ffff73b348f in fmt::write::h4c40aa9c07b41a24hHy ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustrt-4e7c5e5c.so
#19 0x00007ffff7927e68 in io::Writer::write_fmt::h2764658639759448054 ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#20 0x00007ffff7939fce in html::render::Context::item::render::h33470e8ebb2d63e9C5n ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#21 0x00007ffff793793a in html::render::Context::item::closure.26211 ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#22 0x00007ffff7934b0d in html::render::Context::recurse::h1197179027284130113 ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#23 0x00007ffff7913eb0 in html::render::run::hfdc7187ac9c09a7c9Fm ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#24 0x00007ffff797f837 in main_args::h0a752a5313b01ae15Ct ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#25 0x00007ffff797b86a in main::h8dadf866764c934fRyt ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustdoc-4e7c5e5c.so
#26 0x00007ffff766231d in start::closure.2818 ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/libnative-4e7c5e5c.so
#27 0x00007ffff73b8cfc in rust_try_inner ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustrt-4e7c5e5c.so
#28 0x00007ffff73b8ce6 in rust_try ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustrt-4e7c5e5c.so
#29 0x00007ffff7361313 in unwind::try::hd3616a2e4fe236admYc ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustrt-4e7c5e5c.so
#30 0x00007ffff73611dc in task::Task::run::h35d5a8207977fa8du4b ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/librustrt-4e7c5e5c.so
#31 0x00007ffff766213f in start::hbeb6d12d99d267112ma ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/libnative-4e7c5e5c.so
#32 0x00007ffff7661f76 in lang_start::h48ecbf3d8d8483a4lma ()
   from /gpfs/main/sys/shared/psfu/contrib/projects/rust/rust/x86_64-unknown-linux-gnu/stage2/lib/libnative-4e7c5e5c.so
#33 0x00007ffff6f87ead in __libc_start_main (main=<optimized out>, argc=<optimized out>, 
    ubp_av=<optimized out>, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, 
    stack_end=0x7fffffffe468) at libc-start.c:244

(note the thread that prints out the 3 warnings has already ended successfully by the time this panic happens)

Also most of rustc's documentation is generated fine. The only obviously missing problems is no back directory and no fn.main.html page.

This was not happening with my original change (which you can still get here: scialex@a5f5ee7 )

Any idea why this is happening... I couldn't make heads or tails of it.

@huonw
Copy link
Member

huonw commented Nov 10, 2014

What a confusing error message!

It looks like this is caused by rustc::middle::typeck::infer::region_inference::doc: that is a private module and so the documentation was not being rendered at all. Once the documentation starts being rendered, the code blocks (incl. indented ones) are parsed to give syntax highlighting, causing problems with those that aren't valid Rust.

The correct fix is deindenting them and instead wrapping them in a code-fence that indicates that they are not rust code like so:

```notrust
code here
```

However, the fact that this is now popping up would imply that the new strip-private is not removing private items? (Or is not removing them before rendering the markdown.)

@scialex
Copy link
Contributor Author

scialex commented Nov 10, 2014

However, the fact that this is now popping up would imply that the new strip-private is not removing private items? (Or is not removing them before rendering the markdown.)

@huonw Yeah it doesn't seem to be removing them.

I'll fix that and then update.

@scialex
Copy link
Contributor Author

scialex commented Nov 11, 2014

OK Now we actually have a strip-private that removes private modules as well, everything is working again. Can I get a pull?

@@ -158,9 +158,6 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
}

pub fn visit_view_item(&mut self, item: &ast::ViewItem, om: &mut Module) {
if item.vis != ast::Public {
return om.view_items.push(item.clone());
}
Copy link
Member

Choose a reason for hiding this comment

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

I think that this check will actually want to stay, this is specially handling reexports, not public modules I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I misinterpreted what that was doing.

@scialex
Copy link
Contributor Author

scialex commented Nov 12, 2014

OK. I've tested it and my original CL, with the new --all-modules/-a flag works and passes all the tests.

The version that was approved is at tag approved-b-rustdoc. I'm moving this back to a CL based on the original change (I removed the change to visit_ast.rs, that was unnecessary). Would you be willing to merge it?

I also looked through the code and I'm not sure it's possible to do this only in the strip-privates based on how it constructs the docs. I think to fix this we would need to do a major rewrite of the tool. (alternatively we could cheat and simply let the render check to see if the strip-private pass is run and if not don't do the checks to get rid of the modules).

@alexcrichton
Copy link
Member

Sorry I don't think I was following this closely enough. Did the tests fail when bors tried to merge this? If so, do you have the logs for the test failure? I'm curious to see what failed...

@scialex
Copy link
Contributor Author

scialex commented Nov 13, 2014

Yes Bors failed during merge.

I admit I probably should have done a full check-build, it was so long and this was modifying the doc tool so I though I could avoid it.

The current CL is more similar to my original one, with a new --all-modules flag. It does pass make check on x86_64-unknown-linux-gnu.

Failed build: http://buildbot.rust-lang.org/builders/auto-mac-64-nopt-t/builds/2219/steps/test/logs/stdio

Relevant lines:

maketest: rustdoc-search-index
----- /Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/src/test/run-make/rustdoc-search-index/ --------------------
------ stdout ---------------------------------------------
DYLD_LIBRARY_PATH="/Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/test/run-make/rustdoc-search-index:/Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/stage2/lib:" /Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/stage2/bin/rustdoc -w html -o /Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/test/run-make/rustdoc-search-index/doc index.rs
cp index.rs /Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/test/run-make/rustdoc-search-index
cp verify.sh /Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/test/run-make/rustdoc-search-index
DYLD_LIBRARY_PATH="/Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/test/run-make/rustdoc-search-index:/Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/stage1/lib/rustlib/x86_64-apple-darwin/lib:" /Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/test/run-make/rustdoc-search-index/verify.sh /Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/obj/x86_64-apple-darwin/test/run-make/rustdoc-search-index
'test_method' was erroneously excluded from search index.

I am still looking at solving this without the flag but frankly the whole system for filtering out private modules seems very ad-hoc, spread all around render.rs in addition to strip-privates, extracting all of it looks to be very difficult. It might be good to simply add this flag, which is useful, and then later possibly remake the passes and doc tool to make this cleaner.

@alexcrichton
Copy link
Member

@scialex we need to also consider the long-term stability of the rustdoc tool, and this flag seems like its a direct duplicate of --passes and not necessarily something that we would like to support for quite a long time. I don't think we want to go with a new flag, and I would much rather pursue the --passes-based solution. Would you mind changing the PR back to that so I could take another look over it?

@scialex
Copy link
Contributor Author

scialex commented Nov 17, 2014

@alexcrichton Done. the pull is now for one that fails and uses only passes

@alexcrichton
Copy link
Member

@scialex ok so after thinking about this for awhile, I'd like to first apologize for all the confusion in rustdoc :(. The rendering portion was initially designed with passes in mind, but over time they've been de-emphasized greatly and functionality has moved around.

Specifically here, the problem is that private modules cannot all be stripped from the pass to strip private items because this would strip public facing components like implementations of public traits. For this reason the stripping of private modules must happen after the actual passes (during rendering). I'm starting to reconsider the notion of "passes" for rustdoc as they haven't ever really been truly realized. Additionally this reminds me that we need to scrutinize rustdoc's command line interface, which may end up removing these flags.

Sadly now may not be the time to implement a change such as this. I think the best way to go about this change as-is is to give the list of passes to the HTML rendering so it knows whether strip-private was a pass or not (so it knows whether to strip private modules after-the-fact or not). I suspect that if we remove passes entirely then we'll want your original -a flag, but I would like to take some more time to think about the flags of rustdoc before committing to a change like that.

Sorry for the delay and confusing, but does that help clear some things up?

@scialex
Copy link
Contributor Author

scialex commented Nov 20, 2014

@alexcrichton Cool. I'll make a version that just passes that information down. which shouldn't be too hard.

Made it so that what passes are used is passed onto the renderer so it
can intelligently deal with private modules.
@scialex
Copy link
Contributor Author

scialex commented Nov 20, 2014

@alexcrichton this code now passes the tests and seems to work for me.

@alexcrichton
Copy link
Member

Thanks @scialex! Just to be 100% positive (sadly we don't have many regression tests with rustdoc), have you browsed through the output for a bit to make sure it's stayed the same? If you could host it publicly as well so I could poke around that would also be great.

Other than that, looks good to me.

@scialex
Copy link
Contributor Author

scialex commented Nov 21, 2014

@alexcrichton here: http://scialex.github.io/rustdoc-new

This was generated with my patch by the build system. It looks the same to me.

Diff reports numerous differences but most seem to be related to things like different commits, paths, id-numbers and such.

I have yet to find any real change in the rendered page.

@scialex
Copy link
Contributor Author

scialex commented Nov 21, 2014

sorry I just pushed to this accidently. Let me get it back to what it was.

Ok its back to what I meant it to be at.

@alexcrichton
Copy link
Member

Looks great, thanks @scialex!

bors added a commit that referenced this pull request Nov 21, 2014
Changed `rustdoc` so that if we do not have the `strip-private` pass
enabled private modules will be included in the generated documentation

I added this because it is useful to be able to read the documentation in the very nice `rustdoc` web interface when doing internal work on a project. In this case we want the `rustdoc` to include modules that are hidden from consumers. Since this is not currently possible I added it here.
@bors bors closed this Nov 21, 2014
@bors bors merged commit 26107f6 into rust-lang:master Nov 21, 2014
lnicola pushed a commit to lnicola/rust that referenced this pull request Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants