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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement RFC 1946 - intra-rustdoc links #47046

Merged
merged 46 commits into from Jan 23, 2018

Conversation

@Manishearth
Member

Manishearth commented Dec 28, 2017

rust-lang/rfcs#1946 #43466

Note for reviewers: The plain line counts are a little inflated because of how the markdown link parsing was done. Read the file diff with "whitespace only" changes removed to get a better view of what actually changed there.

This pulls the name/path resolution mechanisms out of the compiler and runs it on the markdown in a crate's docs, so that links can be made to SomeStruct directly rather than finding the folder path to struct.SomeStruct.html. Check the src/test/rustdoc/intra-paths.rs test in this PR for a demo. The change was... a little invasive, but unlocks a really powerful mechanism for writing documentation that doesn't care about where an item was written to on the hard disk.

Items included:

  • Make work with the hoedown renderer
  • Handle relative paths
  • Parse out the "path ambiguities" qualifiers ([crate foo], [struct Foo], [foo()], [static FOO], [foo!], etc)
  • Resolve foreign macros
  • Resolve local macros
  • Handle the use of inner/outer attributes giving different resolution scopes (handling for non-modules pushed to different PR)

Items not included:

  • Make sure cross-crate inlining works (blocked on refactor described in #47046 (comment))
  • Implied Shortcut Reference Links (where just doing [::std::iter::Iterator][] without a reference anchor will resolve using the reference name rather than the link target) (requires modifying the markdown parser - blocked on Hoedown/Pulldown switch and raphlinus/pulldown-cmark#121)
  • Handle enum variants and UFCS methods (Enum variants link to the enum page, associated methods don't link at all)
  • Emit more warnings/errors when things fail to resolve (linking to a value-namespaced item without a qualifier will emit an error, otherwise the link is just treated as a url, not a rust path)
  • Give better spans for resolution errors (currently the span for the first doc comment is used)
  • Check for inner doc comments on things that aren't modules

I'm making the PR, but it should be noted that most of the work was done by Misdreavus 馃槃

(Editor's note: This has become a lie, check that commit log, Manish did a ton of work after this PR was opened >_>)

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Dec 28, 2017

r? @GuillaumeGomez

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

@@ -39,21 +39,21 @@ use doctree::*;
// also, is there some reason that this doesn't use the 'visit'
// framework from syntax?
pub struct RustdocVisitor<'a, 'tcx: 'a> {
cstore: &'tcx CrateStore,
pub struct RustdocVisitor<'a, 'b: 'a, 'tcx: 'b, 'rcx: 'b> {

This comment has been minimized.

@eddyb

eddyb Dec 28, 2017

Member

Investigate whether 'b is actually needed.

@@ -22,8 +22,8 @@ use clean::{AttributesExt, NestedAttributesExt};
/// Similar to `librustc_privacy::EmbargoVisitor`, but also takes
/// specific rustdoc annotations into account (i.e. `doc(hidden)`)
pub struct LibEmbargoVisitor<'a, 'b: 'a, 'tcx: 'b> {
cx: &'a ::core::DocContext<'b, 'tcx>,
pub struct LibEmbargoVisitor<'a, 'b: 'a, 'tcx: 'b, 'rcx: 'b> {

This comment has been minimized.

@eddyb

eddyb Dec 28, 2017

Member

Would be neat if we can remove 'b here.

@@ -12,7 +12,7 @@
html_favicon_url = "https://doc.rust-lang.org/favicon.ico",
html_root_url = "https://doc.rust-lang.org/nightly/",
html_playground_url = "https://play.rust-lang.org/")]
#![deny(warnings)]

This comment has been minimized.

@eddyb

eddyb Dec 28, 2017

Member

Need to put this back.

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Dec 28, 2017

I got @eddyb's remarks, a couple nits of my own, and the tidy errors. I also updated it to work with Hoedown as well. I'd like to add a few tests before i'm ready to call it good.

@QuietMisdreavus QuietMisdreavus referenced this pull request Dec 28, 2017

Open

Tracking issue for RFC 1946 - intra-rustdoc links #43466

12 of 17 tasks complete
@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Dec 28, 2017

This current travis failure: Somehow, one of the markdown changes has affected how item summaries are rendered. It looks like whatever happened to the Pulldown rendering made it strip out the "Read more" link on things like trait method descriptions. Investigating...

@Manishearth

This comment has been minimized.

Member

Manishearth commented Dec 29, 2017

Implied Shortcut Reference Links (where just doing [::std::iter::Iterator][] without a reference anchor will resolve using the reference name rather than the link target)

Do we need to implement this this way? Bear in mind that [::std::iter::Iterator] desugars to [::std::iter::Iterator][::std::iter::Iterator] already so what we do need to do is handle that case. The empty-reference trick seems unnecessary.

edit: reread the spec, this is for both cases

@Manishearth

This comment has been minimized.

Member

Manishearth commented Dec 29, 2017

My hunch was correct; fixed the "read more" bug.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Dec 29, 2017

Thinking about the shorthand more: Neither hoedown or pulldown let us intercept [foo] and [foo][bar] style markdown links with missing references; they turn them into plaintext too early. So we're in a bit of a pickle there. We could reparse; but ... ick.

(It is not hard to make this work if we're allowed to edit hoedown and pulldown. I'd rather not.)

@Manishearth

This comment has been minimized.

Member

Manishearth commented Dec 29, 2017

Handled error recovery

@Manishearth

This comment has been minimized.

Member

Manishearth commented Dec 29, 2017

One thing that's missing is that if you try resolving an enum variant it will link to the enum, and if you try resolving a method it won't work at all. Haven't looked into that.

href() doesn't support fragments, I think, so we'll have to make that work. Added to the TODO as an optional thing.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Dec 29, 2017

Btw, foreign items do not work. Also, because we generate the module page before its items, the cache isn't populated when we attempt to generate the markdown for the module page, and href fails. So the short summary stuff doesn't work either.

We probably need to hook directly into the stuff populating the cache for this to work. I'm not sure how easy that is.

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Dec 29, 2017

@Manishearth What situation are you seeing where the cache isn't there? The test i added works. Is it when you're linking to stuff in submodules?

Unless i'm mistaken, the stuff in the cache that it's looking through should be populated in the previous stage of documentation. If it's not, it shouldn't be difficult to move it there.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Dec 29, 2017

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Dec 29, 2017

Is it the same issue as #38386? Summary lines on trait methods haven't had links ever, but i don't remember offhand if module item summaries do the same thing.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Dec 30, 2017

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jan 1, 2018

Gah, it was a silly bug, we were pulling stuff off the wrong item. My bad.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jan 1, 2018

@QuietMisdreavus happy new year Manishearth@7effbeb 馃槃 馃帀

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jan 1, 2018

I don't have a satisfactory solution for cross-crate inlining. What we could do is shove a .links map somewhere in the target dir and reuse that, i guess.

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Jan 2, 2018

The way we solved cross-crate inlining for #[doc(include="file.md")] was to do the processing in libsyntax and stuff the results back into another attribute so it would get saved in the metadata for the crate. However, to apply that procedure here we would need to integrate the markdown parser(s) into libsyntax, to properly extract all the links. I'm not exactly comfortable with making the Rust syntax parser also interpret Markdown as well, though. >_>

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jan 2, 2018

Yeah, that won't work well here. We could design a good way of persisting such info cross crate in the output docdir somewhere.

This does mean that such links will always be broken if you use the rustdoc tool directly.

@eddyb do you have any suggestions?

@eddyb

This comment has been minimized.

Member

eddyb commented Jan 2, 2018

What you actually want to persist cross-crate is the scope information that lets you resolve names in a certain (module) scope, after the fact. It'd also allow not doing all the the resolver hacks.

And there's a very good reason to want it in the compiler, at least same-crate: we could (finally) tell if a type is in scope somewhere so we can actually print Vec<_> instead of std::vec::Vec<_> etc.

So I think what you'll trying to do here is pretty cool, but maybe a bit ambitious at the moment?
And adding more hacks is delaying a really nice solution.

cc @jseyfried @petrochenkov

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jan 3, 2018

In that case perhaps we can land this without attempting to handle foreign items, and wait for a refactoring of resolution to do that.

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Jan 5, 2018

I'm wary of landing this without cross-crate inlining, but if we can expedite the "really nice solution" that @eddyb mentioned, then it may be okay. There's enough here that's good, and it's not like the folder-path links are going away. (It just feels like an impotent feature without it, especially for something like std where it would help out the most. >_>)

So! Before we rush to get this reviewed, let's take stock of what we wanted to have here and what we want to push to later implementation. (Specifically that checklist in the OP that we keep editing.) I'm going to give macro resolution a try. I gave it an initial shot last week, but my initial implementation wasn't working, so it may need to be postponed as well.

@Manishearth What were you meaning with that one checklist item for "enums and UFCS methods"? Are enum variants and trait methods not available in the Resolver using ::?

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Jan 5, 2018

I just ran the test manually to debug the macro resolution and noticed a bunch of "rendering difference" errors. When i ran it with Pulldown, i noticed something off.

Apparently, links with spaces in them don't work in commonmark at all.

In this crate we would like to link to:

* [`ThisType`](struct ::ThisType)
* [`ThisEnum`](enum ::ThisEnum)
* [`ThisTrait`](trait ::ThisTrait)
* [`ThisAlias`](type ::ThisAlias)
* [`ThisUnion`](union ::ThisUnion)
* [`this_function`](::this_function())
* [`THIS_CONST`](const ::THIS_CONST)
* [`THIS_STATIC`](static ::THIS_STATIC)
<p>In this crate we would like to link to:</p>
<ul>
<li>[<code>ThisType</code>](struct ::ThisType)</li>
<li>[<code>ThisEnum</code>](enum ::ThisEnum)</li>
<li>[<code>ThisTrait</code>](trait ::ThisTrait)</li>
<li>[<code>ThisAlias</code>](type ::ThisAlias)</li>
<li>[<code>ThisUnion</code>](union ::ThisUnion)</li>
<li><a href="::this_function()"><code>this_function</code></a></li>
<li>[<code>THIS_CONST</code>](const ::THIS_CONST)</li>
<li>[<code>THIS_STATIC</code>](static ::THIS_STATIC)</li>
</ul>

This completely renders the "path ambiguity" qualifiers useless because right now they'll generate a bunch of rendering warnings, and once we take out Hoedown they'll take out the links entirely. Our options are (1) taking them out, or (2) using something other than the space to separate the qualifier from the path. Something like # maybe?

@eddyb

This comment has been minimized.

Member

eddyb commented Jan 5, 2018

I've suggested unified pages before, such that you'd have doc.rust-lang.org/std/vec/Vec#struct.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jan 22, 2018

let link_out = format!("<a href=\"{link}\"{title}>{content}</a>",
link = link_buf,
title = title.map_or(String::new(),
|t| format!(" title=\"{}\"", t)),

This comment has been minimized.

@ollie27

ollie27 Jan 22, 2018

Contributor

The title also needs to be html escaped: https://github.com/hoedown/hoedown/blob/980b9c549b4348d50b683ecee6abee470b98acda/src/html.c#L243.

format!(" title=\"{}\"", Escape(&t)) should do it.

This comment has been minimized.

@ollie27

ollie27 Jan 29, 2018

Contributor

I've submitted a fix for this: #47855

@bors

This comment has been minimized.

Contributor

bors commented Jan 22, 2018

鈱涳笍 Testing commit cfc9f5a with merge 2386e4c...

bors added a commit that referenced this pull request Jan 22, 2018

Auto merge of #47046 - Manishearth:intra-doc-links, r=eddyb,Guillaume鈥
鈥omez,QuietMisdreavus,Manishearth,killercup

Implement RFC 1946 - intra-rustdoc links

rust-lang/rfcs#1946 #43466

Note for reviewers: The plain line counts are a little inflated because of how the markdown link parsing was done. [Read the file diff with "whitespace only" changes removed](https://github.com/rust-lang/rust/pull/47046/files?w=1) to get a better view of what actually changed there.

This pulls the name/path resolution mechanisms out of the compiler and runs it on the markdown in a crate's docs, so that links can be made to `SomeStruct` directly rather than finding the folder path to `struct.SomeStruct.html`. Check the `src/test/rustdoc/intra-paths.rs` test in this PR for a demo. The change was... a little invasive, but unlocks a really powerful mechanism for writing documentation that doesn't care about where an item was written to on the hard disk.

Items included:

 - [x] Make work with the hoedown renderer
 - [x] Handle relative paths
 - [x] Parse out the "path ambiguities" qualifiers (`[crate foo]`, `[struct Foo]`, `[foo()]`, `[static FOO]`, `[foo!]`, etc)
 - [x] Resolve foreign macros
 - [x] Resolve local macros
 - [x] Handle the use of inner/outer attributes giving different resolution scopes (handling for non-modules pushed to different PR)

Items not included:

 - [ ] Make sure cross-crate inlining works (blocked on refactor described in #47046 (comment))
 - [ ] Implied Shortcut Reference Links (where just doing `[::std::iter::Iterator][]` without a reference anchor will resolve using the reference name rather than the link target) (requires modifying the markdown parser - blocked on Hoedown/Pulldown switch and raphlinus/pulldown-cmark#121)
 - [ ] Handle enum variants and UFCS methods (Enum variants link to the enum page, associated methods don't link at all)
 - [ ] Emit more warnings/errors when things fail to resolve (linking to a value-namespaced item without a qualifier will emit an error, otherwise the link is just treated as a url, not a rust path)
 - [ ] Give better spans for resolution errors (currently the span for the first doc comment is used)
 - [ ] Check for inner doc comments on things that aren't modules

I'm making the PR, but it should be noted that most of the work was done by Misdreavus 馃槃

(Editor's note: This has become a lie, check that commit log, Manish did a ton of work after this PR was opened `>_>`)
@bors

This comment has been minimized.

Contributor

bors commented Jan 22, 2018

馃挃 Test failed - status-travis

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Jan 22, 2018

[01:25:34] 锟絒m锟絒m锟絒32m锟絒1m Documenting锟絒m rustc v0.0.0 (file:///checkout/src/librustc)
[01:26:04] error: unknown start of token: `
[01:26:04]  --> <stdin>:1:1
[01:26:04]   |
[01:26:04] 1 | `foo` `f` Call#1 `y` Yield `bar` `g` Call#3 Call#2 Call#0
[01:26:04]   | ^
[01:26:04]   |
[01:26:04] help: unicode character '`' (Grave Accent) looks like ''' (Single Quote), but it's not
[01:26:04]  --> <stdin>:1:1
[01:26:04]   |
[01:26:04] 1 | `foo` `f` Call#1 `y` Yield `bar` `g` Call#3 Call#2 Call#0
[01:26:04]   | ^
[01:26:04] 
[01:26:05] 锟絒m锟絒m锟絒31m锟絒1merror:锟絒m Could not document `rustc`.

I... have no idea where this even is. There's no file listed here. >_>

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Jan 22, 2018

Found the culprit, but again, i'm not sure how this got here.

/// Obviously, the result of `f()` was created before the yield
/// (and therefore needs to be kept valid over the yield) while
/// the result of `g()` occurs after the yield (and therefore
/// doesn't). If we want to infer that, we can look at the
/// postorder traversal:
/// ```
/// `foo` `f` Call#1 `y` Yield `bar` `g` Call#3 Call#2 Call#0
/// ```

I feel like this should have been caught in the main #46278 PR? o_O

@ollie27

This comment has been minimized.

Contributor

ollie27 commented Jan 22, 2018

Those docs are on a private field so prior to this PR they weren't rendered at all. Now they are "rendered" to look for links. The markdown_links function shouldn't be using hoedown_block or CodeBlocks anyway as they won't affect finding links.

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Jan 22, 2018

Good catch! I just pushed a commit to bypass processing code blocks in markdown_links.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jan 23, 2018

@bors r=eddyb,GuillaumeGomez,QuietMisdreavus,Manishearth

New commit lgtm

@bors

This comment has been minimized.

Contributor

bors commented Jan 23, 2018

馃搶 Commit 63811b6 has been approved by eddyb,GuillaumeGomez,QuietMisdreavus,Manishearth

@bors

This comment has been minimized.

Contributor

bors commented Jan 23, 2018

鈱涳笍 Testing commit 63811b6 with merge 48a7ea9...

bors added a commit that referenced this pull request Jan 23, 2018

Auto merge of #47046 - Manishearth:intra-doc-links, r=eddyb,Guillaume鈥
鈥omez,QuietMisdreavus,Manishearth

Implement RFC 1946 - intra-rustdoc links

rust-lang/rfcs#1946 #43466

Note for reviewers: The plain line counts are a little inflated because of how the markdown link parsing was done. [Read the file diff with "whitespace only" changes removed](https://github.com/rust-lang/rust/pull/47046/files?w=1) to get a better view of what actually changed there.

This pulls the name/path resolution mechanisms out of the compiler and runs it on the markdown in a crate's docs, so that links can be made to `SomeStruct` directly rather than finding the folder path to `struct.SomeStruct.html`. Check the `src/test/rustdoc/intra-paths.rs` test in this PR for a demo. The change was... a little invasive, but unlocks a really powerful mechanism for writing documentation that doesn't care about where an item was written to on the hard disk.

Items included:

 - [x] Make work with the hoedown renderer
 - [x] Handle relative paths
 - [x] Parse out the "path ambiguities" qualifiers (`[crate foo]`, `[struct Foo]`, `[foo()]`, `[static FOO]`, `[foo!]`, etc)
 - [x] Resolve foreign macros
 - [x] Resolve local macros
 - [x] Handle the use of inner/outer attributes giving different resolution scopes (handling for non-modules pushed to different PR)

Items not included:

 - [ ] Make sure cross-crate inlining works (blocked on refactor described in #47046 (comment))
 - [ ] Implied Shortcut Reference Links (where just doing `[::std::iter::Iterator][]` without a reference anchor will resolve using the reference name rather than the link target) (requires modifying the markdown parser - blocked on Hoedown/Pulldown switch and raphlinus/pulldown-cmark#121)
 - [ ] Handle enum variants and UFCS methods (Enum variants link to the enum page, associated methods don't link at all)
 - [ ] Emit more warnings/errors when things fail to resolve (linking to a value-namespaced item without a qualifier will emit an error, otherwise the link is just treated as a url, not a rust path)
 - [ ] Give better spans for resolution errors (currently the span for the first doc comment is used)
 - [ ] Check for inner doc comments on things that aren't modules

I'm making the PR, but it should be noted that most of the work was done by Misdreavus 馃槃

(Editor's note: This has become a lie, check that commit log, Manish did a ton of work after this PR was opened `>_>`)
@bors

This comment has been minimized.

Contributor

bors commented Jan 23, 2018

鈽锔 Test successful - status-appveyor, status-travis
Approved by: eddyb,GuillaumeGomez,QuietMisdreavus,Manishearth
Pushing 48a7ea9 to master...

@bors bors merged commit 63811b6 into rust-lang:master Jan 23, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@Manishearth

This comment has been minimized.

Member

Manishearth commented Jan 23, 2018

WOOOOOOOOOO

@Manishearth Manishearth deleted the Manishearth:intra-doc-links branch Jan 23, 2018

@sgrif

This comment has been minimized.

Contributor

sgrif commented Jan 23, 2018

So will this "just work" if I compile master and try it out? Is it feature gated somehow?

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jan 23, 2018

Yep, it should just work. It's nightly-gated; so if you try this with a stable compiler in 12 weeks stuff won't look great.

@sgrif

This comment has been minimized.

Contributor

sgrif commented Jan 23, 2018

馃憤 time to compile from master then. My office was getting a tad cold anyway.

@sgrif

This comment has been minimized.

Contributor

sgrif commented Jan 23, 2018

Hm is there additional magic I have to do for this? I tried changing a line that was

[`ToSql`]: ../../../serialize/trait.ToSql.html

to

[`ToSql`]: serialize::ToSql

but it just leaves serialize::ToSql as the href

@KillTheMule

This comment has been minimized.

Contributor

KillTheMule commented Jan 23, 2018

Can't check, but shouldn't there be brackets somewhere? Like [ToSql](serialize::ToSql)?

(e) No idea how to get the backticks correct, but I guess everyone can guess what I mean...

@sgrif

This comment has been minimized.

Contributor

sgrif commented Jan 23, 2018

Yes, the actual link in the text appears as [``ToSql``] (with only one backtick of course)

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Jan 23, 2018

Is the serialize module in-scope at that definition site? It should only resolve the link if that path would resolve where that comment would be written.

@sgrif

This comment has been minimized.

Contributor

sgrif commented Jan 23, 2018

Gotcha. Changing it to ::serialize works as expected. I suppose it makes sense that it resolves as dispatch would and not use

kennytm added a commit to kennytm/rust that referenced this pull request Jan 30, 2018

Rollup merge of rust-lang#47855 - ollie27:rustdoc_hoedown_link_title,鈥
鈥 r=QuietMisdreavus

rustdoc: Fix link title rendering with hoedown

The link title needs to be HTML escaped.

It was broken by rust-lang#47046.

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