rustdoc: include external files in documentation (RFC 1990) #44781

QuietMisdreavus · Sep 23, 2017

Part of rust-lang/rfcs#1990 (needs work on the error reporting, which i'm deferring to after this initial PR)

cc #44732

Also fixes #42760, because the prep work for the error reporting made it easy to fix that at the same time.

rust-highfive · Sep 23, 2017

r? @GuillaumeGomez

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

QuietMisdreavus · Sep 23, 2017

Travis failure is because my test for 42760 failed -_- Back to the drawing board...

QuietMisdreavus · Sep 23, 2017

update: apparently is_sugared_doc is false for doc comments when you get them in with_desugared_doc. The issue was not fixed, because there's no way for it to see the difference between a doc comment and a raw doc attribute where it's extracting them. >:(

QuietMisdreavus · Sep 23, 2017

So i wound up modifying syntax::ast::Attribute::with_desugared_doc to set is_sugared_doc on the attribute when it calls the given closure. I'm not sure if this is entirely valid w.r.t. the original intent of that method, but rustdoc was the only in-tree user of that method that i found, so it may work out anyway. It fixed the issue in the test rendering i had, so i'll let travis see if i messed anything else up.

QuietMisdreavus · Sep 23, 2017

Travis was green, so i'm declaring this Ready To Be Looked At. I updated one test to make sure combining included docs and inline docs worked and squashed the commits. The libsyntax change is still in its own commit, though, since it was only included to make the fix for #42760 work properly, and i'm still not sure whether it's the totally right thing to do.

GuillaumeGomez · Sep 23, 2017

GuillaumeGomez reviewed Sep 23, 2017

View changes

GuillaumeGomez · Sep 23, 2017

src/librustdoc/clean/mod.rs

+                            match Attributes::load_include(include_path, &filename) {
+                                Ok(frag) => {
+                                    let line = doc_line;
+                                    doc_line += frag.len();


Oh so you decided to put in the "real" line number instead of the one from the original file?

Yeah. My thought was that if you're trying to trace a line number from the total markdown blob, then you could use that to get the doc fragment, since each attribute is joined with a line break anyway.

GuillaumeGomez · Sep 23, 2017

It'd be nice to have some outputs (or even tests?) for the case of failing doc codes.

QuietMisdreavus · Sep 23, 2017

It'd be nice to have some outputs (or even tests?) for the case of failing doc codes.

That's what i was forgetting! Give me a moment, i can make a quick run.

QuietMisdreavus · Sep 23, 2017

With the following files:

lib.rs:

#![feature(external_doc)]

/// hey yo check out these docs, i can splice them in from another file
///
/// ```rust
/// panic!("oh no");
/// ```
///
#[doc(include = "some-file.md")]
pub struct SomeStruct;

/// Item docs.
///
#[doc="Hello there!"]
///
/// # Example
///
/// ```rust
/// // some code here
/// panic!("oh no");
/// ```
pub struct OtherStruct;

#[doc(include = "bad-docs.md")]
#[doc(include = "no-docs.md")]
pub struct AllTheStruct;

some-file.md:

hey check out this included file

this is so sick, i can just paste this anywhere

bad-docs.md:

# some bad docs

```rust
panic!("oh no");
```

(no-docs.md deliberately missing)

Running cargo test gives the following output:

This is with the latest force push. (I neglected to ask rustdoctest to combine doc fragments, so that's fixed now.)

EDIT: The current line numbering refers to the line within the final markdown, not the line in the source file. So i guess to properly translate between this line number and the source line, you need to store both the "markdown line" and the "source line". This could get hairy to properly account for multiple #[doc] attributes written on the same line (rocket has a macro that does this), but i guess if you check the spans while loading, it may work out.

GuillaumeGomez · Sep 24, 2017

And a column as well for the "real" line number (so a Span?). If you want, I can work on that part (the test one).

QuietMisdreavus · Sep 28, 2017

@GuillaumeGomez I added a Span to each DocFragment and made sure that they get properly combined in collapse-docs, does this seem all right to you?

GuillaumeGomez · Sep 28, 2017

Please post a test failure output. :)

QuietMisdreavus · Sep 28, 2017

I haven't touched the failure reporting code, so it'll look the same as in #44781 (comment). Do you want me to make that part of this PR, or would you like to merge this as-is and take it up yourself afterward?

GuillaumeGomez · Sep 29, 2017

I don't mind merging as is. Just tell me what you prefer to do.

QuietMisdreavus · Sep 29, 2017

If i remember right, we discussed fixing up the error reporting in a separate PR, so that's what i was assuming we would do here (and my preference, just so we can get this in >_>). If you want to wrap it all into one PR, just tell me what code is likely to need the span information and i can take a shot at it.

ollie27 · Sep 29, 2017

This doesn't work cross-crate. If you re-export an item that uses #[doc(include)] then it will look for the file relative to the current crate not the crate that actually contains the #[doc(include)].

QuietMisdreavus · Oct 4, 2017

@ollie27 Oh yikes, that's a good point. On the other hand, i don't really see a good way to reliably do it cross-crate without asking rustc to load the file in. In the RFC discussion we talked about making it an alias for #[doc=include_str!("file.md")] which would do exactly that, but would require work within the compiler to make sure certain macros could be used within attributes and expand correctly. (That's also outside my expertise. >_>)

SimonSapin · Oct 5, 2017

Having a special case for doc include in rustc might be easier than supporting arbitrary macros in arbitrary attributes. (Or maybe not, I don’t know.)

bors · Oct 5, 2017

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

aidanhs · Oct 12, 2017

@QuietMisdreavus I've set this back as with author for the merge conflicts. After that, I'm also not clear after that if you're going to do more work or it's going back to review by @GuillaumeGomez?

QuietMisdreavus · Oct 12, 2017

@aidanhs I'm actually not sure either. I'm concerned it may need to be scrapped entirely in favor of the #[doc=include_str!()] method alluded to above. I'm not entirely sure how to go about that, though - it would require work on the compiler frontend to extend #34981 to include (at least) some builtin macros as an allowed expression in attributes, then also adding #[doc(include="file.md")] as sugar for that. Should we pull in compiler-team to take a look?

GuillaumeGomez · Oct 13, 2017

Won't kill to add them in the loop. :)

cc @rust-lang/compiler

GuillaumeGomez · Nov 21, 2017

GuillaumeGomez reviewed Nov 21, 2017

View changes

GuillaumeGomez · Nov 21, 2017

src/librustdoc/clean/mod.rs

+        mi.meta_item_list().and_then(|list| {
+            for meta in list {
+                if meta.check_name("include") {
+                    //the actual compiled `#[doc(include="filename")]` gets expanded to


Please put a whitespace after //.

GuillaumeGomez · Nov 21, 2017

GuillaumeGomez reviewed Nov 21, 2017

View changes

GuillaumeGomez · Nov 21, 2017

src/librustdoc/html/render.rs

-    if let Some(s) = item.doc_value() {
-        render_markdown(w, s, item.source.clone(), cx.render_type, prefix, &cx.shared)?;
+    if let Some(s) = cx.shared.maybe_collapsed_doc_value(item) {
+        trace!("Doc block: =====\n{}\n=====", s);


Shouldn't debug! be used instead? (Don't really know the good macro we're supposed to use in such cases...)

Probably? I'm not that solid on what counts as what either, but i'll go ahead and switch it over.

GuillaumeGomez · Nov 21, 2017

GuillaumeGomez reviewed Nov 21, 2017

View changes

GuillaumeGomez · Nov 21, 2017

src/librustdoc/passes/collapse_docs.rs

+                match curr_frag {
+                    DocFragment::SugaredDoc(_, _, ref mut doc_string)
+                        | DocFragment::RawDoc(_, _, ref mut doc_string) => {
+                            //add a newline for extra padding between segments


Whitespace after // please.

GuillaumeGomez · Nov 21, 2017

GuillaumeGomez reviewed Nov 21, 2017

View changes

src/libsyntax/ext/expand.rs

GuillaumeGomez · Nov 21, 2017

GuillaumeGomez reviewed Nov 21, 2017

View changes

GuillaumeGomez · Nov 21, 2017

src/libsyntax/ext/expand.rs

+                        let err_count = self.cx.parse_sess.span_diagnostic.err_count();
+                        self.check_attribute(&at);
+                        if self.cx.parse_sess.span_diagnostic.err_count() > err_count {
+                            //avoid loading the file if they haven't enabled the feature


GuillaumeGomez · Nov 21, 2017

All good for me from code reading. Just a few nits and it's good to go imo.

QuietMisdreavus · Nov 21, 2017

I've force-pushed to address the review comments.

GuillaumeGomez · Nov 21, 2017

Then it's all good for me. r=me once CI is green.

QuietMisdreavus · Nov 21, 2017

@bors r=GuillaumeGomez

bors · Nov 21, 2017

📌 Commit 52ee203 has been approved by GuillaumeGomez

bors · Nov 22, 2017

⌛️ Testing commit 52ee203 with merge 0893ecc...

bors · Nov 22, 2017

💔 Test failed - status-travis

QuietMisdreavus · Nov 22, 2017

Failure... doesn't look legit? I'm not caught up on the known spurious failures.

Installing deploy dependencies
ERROR:  Could not find a valid gem 'aws-sdk-resources' (= 2.10.88) in any repository
ERROR:  Possible alternatives: aws-sdk-resources
/home/travis/.rvm/rubies/ruby-2.2.7/lib/ruby/site_ruby/2.2.0/rubygems/core_ext/kernel_require.rb:59:in `require': cannot load such file -- aws-sdk (LoadError)
	from /home/travis/.rvm/rubies/ruby-2.2.7/lib/ruby/site_ruby/2.2.0/rubygems/core_ext/kernel_require.rb:59:in `require'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider.rb:85:in `requires'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider/s3.rb:6:in `<class:S3>'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider/s3.rb:5:in `<class:Provider>'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider/s3.rb:4:in `<module:DPL>'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider/s3.rb:3:in `<top (required)>'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider.rb:59:in `const_get'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider.rb:59:in `block in new'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/cli.rb:41:in `fold'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider.rb:56:in `new'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/cli.rb:31:in `run'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/cli.rb:7:in `run'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/bin/dpl:5:in `<top (required)>'
	from /home/travis/.rvm/gems/ruby-2.2.7/bin/dpl:23:in `load'
	from /home/travis/.rvm/gems/ruby-2.2.7/bin/dpl:23:in `<main>'

kennytm · Nov 22, 2017

@bors retry #44159

Looks like that previous attempt to fix it doesn't work.

bors · Nov 22, 2017

⌛️ Testing commit 52ee203 with merge 3755fe9...

bors · Nov 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: GuillaumeGomez
Pushing 3755fe9 to master...

rust-highfive assigned GuillaumeGomez Sep 23, 2017

estebank added the S-waiting-on-review label Sep 23, 2017

QuietMisdreavus force-pushed the QuietMisdreavus:doc-include branch from 6f188ec to b25e8bd Sep 23, 2017

QuietMisdreavus changed the title ~~[WIP] rustdoc: include external files in documentation (RFC 1990)~~ rustdoc: include external files in documentation (RFC 1990) Sep 23, 2017

QuietMisdreavus force-pushed the QuietMisdreavus:doc-include branch from b25e8bd to 7e482de Sep 23, 2017

mgattozzi referenced this pull request Oct 3, 2017
Open
Better documentation for github-rs #63

aidanhs added S-waiting-on-author and removed S-waiting-on-review labels Oct 12, 2017

nrc referenced this pull request Nov 21, 2017
Open
Tracking issue: rfc 1990 - add external doc attribute to rustc #44732
2 of 3 tasks complete

kennytm added S-waiting-on-author and removed S-waiting-on-review labels Nov 21, 2017

QuietMisdreavus added some commits Sep 22, 2017

allow loading external files in documentation
Partial implementation of rust-lang/rfcs#1990 (needs error reporting work) cc #44732

f9f3611

make with_unsugared_doc preserve is_sugared_doc

Loading status checks…

52ee203

QuietMisdreavus force-pushed the QuietMisdreavus:doc-include branch from abc9aca to 52ee203 Nov 21, 2017

kennytm added S-waiting-on-bors and removed S-waiting-on-author labels Nov 22, 2017

bors merged commit 52ee203 into rust-lang:master Nov 22, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

Eh2406 referenced this pull request Nov 30, 2017
Closed
What needs to be done? #17

QuietMisdreavus deleted the QuietMisdreavus:doc-include branch Dec 6, 2017

ozkriff referenced this pull request May 19, 2018
Closed
Are badges in ggez's top level doc comment really required? #379

rust-lang/rust

Join GitHub today

rustdoc: include external files in documentation (RFC 1990) #44781

Conversation

QuietMisdreavus commented Sep 23, 2017

rust-highfive assigned GuillaumeGomez Sep 23, 2017

This comment has been minimized.

rust-highfive commented Sep 23, 2017

estebank added the S-waiting-on-review label Sep 23, 2017

This comment has been minimized.

QuietMisdreavus commented Sep 23, 2017

This comment has been minimized.

QuietMisdreavus commented Sep 23, 2017

This comment has been minimized.

QuietMisdreavus commented Sep 23, 2017

QuietMisdreavus force-pushed the QuietMisdreavus:doc-include branch from 6f188ec to b25e8bd Sep 23, 2017

QuietMisdreavus changed the title [WIP] rustdoc: include external files in documentation (RFC 1990) rustdoc: include external files in documentation (RFC 1990) Sep 23, 2017

This comment has been minimized.

QuietMisdreavus commented Sep 23, 2017

GuillaumeGomez reviewed Sep 23, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez commented Sep 23, 2017

This comment has been minimized.

QuietMisdreavus commented Sep 23, 2017

QuietMisdreavus force-pushed the QuietMisdreavus:doc-include branch from b25e8bd to 7e482de Sep 23, 2017

This comment has been minimized.

QuietMisdreavus commented Sep 23, 2017 • edited

This comment has been minimized.

GuillaumeGomez commented Sep 24, 2017

This comment has been minimized.

QuietMisdreavus commented Sep 28, 2017

This comment has been minimized.

GuillaumeGomez commented Sep 28, 2017

This comment has been minimized.

QuietMisdreavus commented Sep 28, 2017

This comment has been minimized.

GuillaumeGomez commented Sep 29, 2017

This comment has been minimized.

QuietMisdreavus commented Sep 29, 2017

This comment has been minimized.

ollie27 commented Sep 29, 2017

mgattozzi referenced this pull request Oct 3, 2017

This comment has been minimized.

QuietMisdreavus commented Oct 4, 2017

This comment has been minimized.

SimonSapin commented Oct 5, 2017

This comment has been minimized.

bors commented Oct 5, 2017

aidanhs added S-waiting-on-author and removed S-waiting-on-review labels Oct 12, 2017

This comment has been minimized.

aidanhs commented Oct 12, 2017

This comment has been minimized.

QuietMisdreavus commented Oct 12, 2017

This comment has been minimized.

GuillaumeGomez commented Oct 13, 2017

nrc referenced this pull request Nov 21, 2017

GuillaumeGomez reviewed Nov 21, 2017 View changes

This comment has been minimized.

GuillaumeGomez reviewed Nov 21, 2017 View changes

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez reviewed Nov 21, 2017 View changes

This comment has been minimized.

GuillaumeGomez reviewed Nov 21, 2017 View changes

GuillaumeGomez reviewed Nov 21, 2017 View changes

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez commented Nov 21, 2017

kennytm added S-waiting-on-author and removed S-waiting-on-review labels Nov 21, 2017

QuietMisdreavus added some commits Sep 22, 2017

QuietMisdreavus force-pushed the QuietMisdreavus:doc-include branch from abc9aca to 52ee203 Nov 21, 2017

This comment has been minimized.

QuietMisdreavus commented Nov 21, 2017

This comment has been minimized.

GuillaumeGomez commented Nov 21, 2017

This comment has been minimized.

QuietMisdreavus commented Nov 21, 2017

This comment has been minimized.

QuietMisdreavus force-pushed the QuietMisdreavus:doc-include branch from `6f188ec` to `b25e8bd` Sep 23, 2017

GuillaumeGomez reviewed Sep 23, 2017

View changes

QuietMisdreavus force-pushed the QuietMisdreavus:doc-include branch from `b25e8bd` to `7e482de` Sep 23, 2017

QuietMisdreavus commented Sep 23, 2017 •

edited

GuillaumeGomez reviewed Nov 21, 2017

View changes

GuillaumeGomez reviewed Nov 21, 2017

View changes

GuillaumeGomez reviewed Nov 21, 2017

View changes

GuillaumeGomez reviewed Nov 21, 2017

View changes

GuillaumeGomez reviewed Nov 21, 2017

View changes

QuietMisdreavus force-pushed the QuietMisdreavus:doc-include branch from `abc9aca` to `52ee203` Nov 21, 2017

QuietMisdreavus commented Nov 22, 2017 •

edited

bors merged commit `52ee203` into rust-lang:master Nov 22, 2017
2 checks passed