Replace hoedown with pull in rustdoc #40338

GuillaumeGomez · 2017-03-08T00:02:06Z

cc @rust-lang/docs

rust-highfive · 2017-03-08T00:02:10Z

r? @steveklabnik

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

steveklabnik

I will give this a real review tomorrow, but 🎊

steveklabnik · 2017-03-08T03:12:25Z

src/librustdoc/html/markdown.rs

@@ -179,8 +181,8 @@ extern {
    fn hoedown_document_free(md: *mut hoedown_document);

    fn hoedown_buffer_new(unit: libc::size_t) -> *mut hoedown_buffer;
-    fn hoedown_buffer_put(b: *mut hoedown_buffer, c: *const libc::c_char,
-                          n: libc::size_t);
+    /*fn hoedown_buffer_put(b: *mut hoedown_buffer, c: *const libc::c_char,


Why leave this in but commented out?

steveklabnik · 2017-03-08T03:12:47Z

src/librustdoc/html/markdown.rs

@@ -594,7 +598,7 @@ impl<'a> fmt::Display for MarkdownHtml<'a> {
 }

 pub fn plain_summary_line(md: &str) -> String {
-    extern fn link(_ob: *mut hoedown_buffer,
+    /*extern fn link(_ob: *mut hoedown_buffer,


frewsxcv · 2017-03-08T03:13:54Z

src/librustdoc/html/markdown.rs

+    let mut register_header = None;
+    'main: loop {
+        let next_event = parser.next();
+        if let Some(event) = next_event {


If you made this:

let event = match next_event { Some(event) => event, None => break, }; ....

This would reduce the indentation

At that point, couldn't this be a:

while let Some(event) = parser.next() { ... }

steveklabnik

Seems good to me, though I agree with @frewsxcv 's comments about nesting

GuillaumeGomez · 2017-03-10T17:36:13Z

This is now ready to review!

frewsxcv · 2017-03-10T17:45:47Z

src/librustdoc/html/markdown.rs

+        let mut content = String::new();
+        loop {
+            let event = parser.next();
+            if let Some(event) = event {


Could this could just be a for loop?

Agreed, this is for event in parser { match event { … } } (see main review comment)

frewsxcv · 2017-03-10T17:46:00Z

src/librustdoc/html/markdown.rs

+    fn link(parser: &mut Parser, buffer: &mut String, url: &str, mut title: String) {
+        loop {
+            let event = parser.next();
+            if let Some(event) = event {


for loop?

frewsxcv · 2017-03-10T17:46:08Z

src/librustdoc/html/markdown.rs

+        let mut content = String::new();
+        loop {
+            let event = parser.next();
+            if let Some(event) = event {


for loop?

frewsxcv · 2017-03-10T17:58:35Z

src/librustdoc/html/markdown.rs

 use std::fmt::{self, Write};
-use std::slice;
+//use std::slice;


Remove commented out imports

frewsxcv · 2017-03-10T17:59:16Z

src/librustdoc/html/markdown.rs

 /// A unit struct which has the `fmt::Display` trait implemented. When
 /// formatted, this struct will emit the HTML corresponding to the rendered
 /// version of the contained markdown string.
-pub struct Markdown<'a>(pub &'a str);
+// The second parameter is whether we need a shorter version or not.


What is a "shorter version"?

Maybe this should be a struct with named fields instead of using unnamed fields

yeah, boolean parameters are usually a code smell

I think I'd prefer an enum MarkdownOutputStyle { Compact, Fancy } (or some other fitting variant names) instead of bool

@killercup: Good idea!

frewsxcv · 2017-03-10T17:59:49Z

src/librustdoc/html/markdown.rs

@@ -26,13 +26,13 @@

 #![allow(non_camel_case_types)]

-use libc;
+//use libc;


Do we still use the libc crate in rustdoc?

frewsxcv · 2017-03-10T18:01:58Z

src/librustdoc/html/markdown.rs

+                    (b'0' <= c && c <= b'9') ||
+                    c == b'-' || c == b'_' || c == b'.' ||
+                    c == b'~' || c == b'!' || c == b'\'' ||
+                    c == b'(' || c == b')' || c == b'*'


Pull in rust-url for this?

This is not part of the change, however I can do it next up.

frewsxcv · 2017-03-10T18:04:42Z

src/librustdoc/html/markdown.rs

    }

+    fn looper<'a>(parser: &'a mut Parser, buffer: &mut String, next_event: Option<Event<'a>>,
+                  toc_builder: &mut Option<TocBuilder>, shorter: bool) -> bool {
+        if let Some(event) = next_event {


Could return early here if this is None

Doesn't change much from my point of view... I think this syntax is actually better.

steveklabnik · 2017-03-10T18:23:02Z

/cc @raphlinus ❤️

steveklabnik · 2017-03-10T19:15:30Z

So, I'm spot-checking some text to find regressions.

std::collections module page

std::convert module page

prelude:

some smooshed text

std::sync::LockResult

std::string

that's what i found just now. we may need to tweak some text, and it's probably good to go through the whole entire thing once the technical side is good and clear up these kinds of issues.

killercup

Nice to see some progress here!

One pattern I immediately saw was all the loop constructs matching the event and tag. I'd:

use Event::* and use Tag::*; to reduce the visual noise
Try to rewrite them as let events = parser.$filter_relevant_events; for event in events { match event {…} }, where $filter_relevant_events are some operations on the iterator to get the events you are interested in (e.g., to use take_while instead of adding break arms).

FYI, I wrote a macro once for this exact use case to allow events.take_while(not!(end Tag::Paragraph)) and similar. This may help converting most of these loops into nice iterators.

You can find it and example using it here. You'll probably need to extend it to support multiple end tag variants as used below.

I'd also really try to get rid of the shorter: bool stuff and replace with some semantically meaningful names :)

Switching parser is not something I expect to just work, even when both apparently conform to the CommonMark spec. Before merging this, I'd render a bunch of docs (std and some larger crates), and try to

set up a script that opens and takes screenshots of all generated html pages to visually diff them with ones generated by the current stable rustdoc,
or let humans look at the rendered pages,
or do both 😄

killercup · 2017-03-10T19:08:41Z

src/Cargo.lock

@@ -1203,6 +1203,7 @@ dependencies = [
 "build_helper 0.1.0",
 "gcc 0.3.43 (registry+https://github.com/rust-lang/crates.io-index)",
 "log 0.0.0",
+ "pulldown-cmark 0.0.8 (registry+https://github.com/rust-lang/crates.io-index)",


0.0.8… Another lib we should help get to 1.0

killercup · 2017-03-10T19:12:18Z

src/librustdoc/html/markdown.rs

            }
        }
+        buffer.push_str(&format!("<table>{}{}</table>",
+                                 content,
+                                 if shorter || rows.is_empty() {


Is this the only place where shorter is actually used? The name shorter does not imply "no tbody tag" to me…

In paragraph as well. The point is to display only the first line, so everything that could be on more than one line use it.

killercup · 2017-03-10T19:14:50Z

src/librustdoc/html/markdown.rs

 /// A unit struct which has the `fmt::Display` trait implemented. When
 /// formatted, this struct will emit the HTML corresponding to the rendered
 /// version of the contained markdown string.
-pub struct Markdown<'a>(pub &'a str);
+// The second parameter is whether we need a shorter version or not.


I think I'd prefer an enum MarkdownOutputStyle { Compact, Fancy } (or some other fitting variant names) instead of bool

killercup · 2017-03-10T19:22:14Z

src/librustdoc/html/markdown.rs

+        let mut content = String::new();
+        loop {
+            let event = parser.next();
+            if let Some(event) = event {


Agreed, this is for event in parser { match event { … } } (see main review comment)

killercup · 2017-03-10T19:35:56Z

src/librustdoc/html/markdown.rs

+                            _ => {}
+                        }
+                    } else {
+                        break 'main;


finally, a good reason not use a for loop ;)

GuillaumeGomez · 2017-03-11T00:45:20Z

Most problems should be solved now. I'll write a macro next to allow to remove all these loops.

ollie27 · 2017-03-11T00:47:24Z

Is there any reason you're not using pulldown_cmark::html::push_html to actually render the HTML?

bors · 2017-03-11T08:30:16Z

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

GuillaumeGomez · 2017-03-11T11:46:43Z

@ollie27: Unfortunately yes: the generation of urls mainly (for play.rust-lang in blockcodes).

steveklabnik · 2017-03-11T15:05:28Z

src/librustdoc/html/markdown.rs

-                            "&lt;", "&gt;", "&amp;", "&#39;", "&quot;"];
-        for sub in repl_sub {
-            id = id.replace(sub, "");
-        }


shouldn't this mean the comment above goes away too?

Absolutely!

killercup

Code looks nicer with the macro, though I'd probably make it more generic.

Also, why not use Event::* and use Tag::*?

killercup · 2017-03-11T15:39:55Z

src/librustdoc/html/markdown.rs

@@ -104,6 +104,25 @@ thread_local!(pub static PLAYGROUND: RefCell<Option<(Option<String>, String)>> =
    RefCell::new(None)
 });

+macro_rules! event_loop_break {


Nice. I'd call it collect_md_events, though, and make calling it a bit more expressive, e.g.

collect_md_events!(from parser into &mut buf (toc_builder, shorter) until End(Header(_)))

Not sure if this is super useful. Anyone else has an opinion on it?

killercup · 2017-03-11T15:42:36Z

src/librustdoc/html/markdown.rs

+              shorter: MarkdownOutputStyle) -> fmt::Result {
+    fn block(parser: &mut Parser, buffer: &mut String, lang: &str) {
+        let mut origtext = String::new();
+        while let Some(event) = parser.next() {


Could also be using the macro (e.g. when calling without toc builder and shorter)

I didn't see much interest in expanding the macro to handle this case. But yes it's possible.

GuillaumeGomez · 2017-03-12T00:09:47Z

Also, why not use Event::* and use Tag::*?

Simply because I like the code being explicit. At least with this syntax, you don't have to think about it, the information is already there.

ollie27 · 2017-03-12T03:07:39Z

@ollie27: Unfortunately yes: the generation of urls mainly (for play.rust-lang in blockcodes).

You can still use push_html and add these links by implementing an iterator adapter around Parser. It would pass most Events through but you could filter and modify the ones you're interested in like CodeBlock and Header. This way you don't have to reimplement all the HTML rendering that pulldown-cmark already provides.

GuillaumeGomez · 2017-03-28T18:33:29Z

Updated.

frewsxcv

I have a bunch of nitpicky style things, but they aren't important enough to hold up this PR any longer. Looks good to me!

GuillaumeGomez · 2017-03-29T05:47:10Z

@frewsxcv, @steveklabnik: Since both of you agreed on the changes, I'll r+ this PR. Don't hesitate to r- if you find anything!

@bors: r=frewsxcv,steveklabnik

bors · 2017-03-29T05:47:11Z

📌 Commit a7c6d3e has been approved by frewsxcv,steveklabnik

bors · 2017-03-29T07:06:18Z

⌛ Testing commit a7c6d3e with merge abf5592...

…veklabnik Replace hoedown with pull in rustdoc cc @rust-lang/docs

bors · 2017-03-29T09:48:56Z

☀️ Test successful - status-appveyor, status-travis
Approved by: frewsxcv,steveklabnik
Pushing abf5592 to master...

ollie27 · 2017-03-29T11:06:55Z

Was this actually finished? It's missing support for horizontal rules, footnotes and images. There are also several bugs like missing the <br /> for HardBreaks, the title part of links being rendered as visible text rather than as an attribute on the <a> tag and MarkdownHtml now doing the same as Markdown when it should be escaping raw HTML.

As I previously mentioned, using push_html would have been much easier than trying to reimplement all of the HTML rendering.

frewsxcv · 2017-03-29T13:11:19Z

Was this actually finished? It's missing support for horizontal rules, footnotes and images. There are also several bugs like missing the <br /> for HardBreaks, the title part of links being rendered as visible text rather than as an attribute on the <a> tag and MarkdownHtml now doing the same as Markdown when it should be escaping raw HTML.

Have you confirmed any of these? I can bring up these concerns during today's Documentation Team meeting.

ollie27 · 2017-03-29T13:43:21Z

Have you confirmed any of these?

Yes. Try rendering the following in the current rustdoc and after this change:

/// markdown test
///
/// this is a [link].
///
/// [link]: https://example.com "this is a title"
///
/// hard break:  
/// after hard break
///
/// -----------
///
/// a footnote[^footnote].
///
/// [^footnote]: Thing
///
/// ![Rust](https://www.rust-lang.org/logos/rust-logo-128x128-blk-v2.png)
#[deprecated(note = "Struct<T>")]
pub fn f() {}

Before:

After:

steveklabnik · 2017-03-29T14:08:37Z

Two things:

this landed after last night's nightly, so tonight's is the first night where it lands. I was already planning on trying to publicize "please look at this since it's a big change", so knowing this stuff is good.
I am currently building docs with the commit before and the commit after, and then i'm going to diff them to see exactly what's different.

I don't think that this missing a few things is the end of the world; now that this has landed, fixing stuff up is much easier. We've still got plenty of time before the release; four weeks until beta, and then we could backport anything catastrophic. Getting this out in the wild sooner rather than later is important IMHO.

steveklabnik · 2017-03-29T18:38:10Z

Here is an un-semantic diff steveklabnik/docdiff@5d17061

steveklabnik · 2017-03-29T20:43:01Z

A much prettier diff steveklabnik/docdiff@HEAD~1...master

steveklabnik · 2017-03-29T21:24:17Z

I've filed #40912 to track these regressions. 😄

mbrubeck · 2017-09-28T23:12:28Z

This was disabled by default in #41431, and #44229 is tracking the plan to enable it by default.

Is it really time? Have our months, no, *years* of suffering come to an end? Are we finally able to cast off the pall of Hoedown? The weight which has dragged us down for so long? ----- So, timeline for those who need to catch up: * Way back in December 2016, [we decided we wanted to switch out the markdown renderer](rust-lang#38400). However, this was put on hold because the build system at the time made it difficult to pull in dependencies from crates.io. * A few months later, in March 2017, [the first PR was done, to switch out the renderers entirely](rust-lang#40338). The PR itself was fraught with CI and build system issues, but eventually landed. * However, not all was well in the Rustdoc world. During the PR and shortly after, we noticed [some differences in the way the two parsers handled some things](rust-lang#40912), and some of these differences were major enough to break the docs for some crates. * A couple weeks afterward, [Hoedown was put back in](rust-lang#41290), at this point just to catch tests that Pulldown was "spuriously" running. This would at least provide some warning about spurious tests, rather than just breaking spontaneously. * However, the problems had created enough noise by this point that just a few days after that, [Hoedown was switched back to the default](rust-lang#41431) while we came up with a solution for properly warning about the differences. * That solution came a few weeks later, [as a series of warnings when the HTML emitted by the two parsers was semantically different](rust-lang#41991). But that came at a cost, as now rustdoc needed proc-macro support (the new crate needed some custom derives farther down its dependency tree), and the build system was not equipped to handle it at the time. It was worked on for three months as the issue stumped more and more people. * In that time, [bootstrap was completely reworked](rust-lang#43059) to change how it ordered compilation, and [the method by which it built rustdoc would change](rust-lang#43482), as well. This allowed it to only be built after stage1, when proc-macros would be available, allowing the "rendering differences" PR to finally land. * The warnings were not perfect, and revealed a few [spurious](rust-lang#44368) [differences](rust-lang#45421) between how we handled the renderers. * Once these were handled, [we flipped the switch to turn on the "rendering difference" warnings all the time](rust-lang#45324), in October 2017. This began the "warning cycle" for this change, and landed in stable in 1.23, on 2018-01-04. * Once those warnings hit stable, and after a couple weeks of seeing whether we would get any more reports than what we got from sitting on nightly/beta, [we switched the renderers](rust-lang#47398), making Pulldown the default but still offering the option to use Hoedown. And that brings us to the present. We haven't received more new issues from this in the meantime, and the "switch by default" is now on beta. Our reasoning is that, at this point, anyone who would have been affected by this has run into it already.

rust-highfive assigned steveklabnik Mar 8, 2017

steveklabnik reviewed Mar 8, 2017

View reviewed changes

frewsxcv reviewed Mar 8, 2017

View reviewed changes

GuillaumeGomez force-pushed the pulldown-switch branch from 9268ec6 to 2c33d62 Compare March 8, 2017 12:07

steveklabnik reviewed Mar 8, 2017

View reviewed changes

GuillaumeGomez force-pushed the pulldown-switch branch 4 times, most recently from 376b5dd to 2537b41 Compare March 10, 2017 17:35

GuillaumeGomez changed the title ~~[WIP] Replace hoedown with pull in rustdoc~~ Replace hoedown with pull in rustdoc Mar 10, 2017

frewsxcv reviewed Mar 10, 2017

View reviewed changes

killercup reviewed Mar 10, 2017

View reviewed changes

steveklabnik reviewed Mar 11, 2017

View reviewed changes

GuillaumeGomez force-pushed the pulldown-switch branch 2 times, most recently from c132361 to 8b61716 Compare March 11, 2017 15:41

killercup reviewed Mar 11, 2017

View reviewed changes

GuillaumeGomez force-pushed the pulldown-switch branch from 3c6a31f to 7f190fe Compare March 12, 2017 00:07

Improve function naming

a7c6d3e

GuillaumeGomez force-pushed the pulldown-switch branch from 83972b3 to a7c6d3e Compare March 28, 2017 17:54

frewsxcv approved these changes Mar 28, 2017

View reviewed changes

bors added a commit that referenced this pull request Mar 29, 2017

Auto merge of #40338 - GuillaumeGomez:pulldown-switch, r=frewsxcv,ste…

abf5592

…veklabnik Replace hoedown with pull in rustdoc cc @rust-lang/docs

bors merged commit a7c6d3e into rust-lang:master Mar 29, 2017

GuillaumeGomez deleted the pulldown-switch branch March 29, 2017 14:25

steveklabnik mentioned this pull request Mar 29, 2017

Tracking issue for hoedown -> pulldown regressions #40912

Closed

13 tasks

frewsxcv mentioned this pull request Mar 29, 2017

Migrate from hoedown to pulldown-cmark for markdown parsing #38400

Closed

SergioBenitez mentioned this pull request Mar 30, 2017

Panic caused by Markdown in latest nightly #40918

Closed

jdm mentioned this pull request Mar 31, 2017

Docs build is failing servo/servo#16213

Closed

steveklabnik added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 1, 2017

nrc mentioned this pull request Sep 28, 2017

Tracking issue (Rustdoc): hoedown -> pulldown migration #44229

Closed

10 tasks

QuietMisdreavus mentioned this pull request Feb 16, 2018

Remove hoedown from rustdoc #48274

Merged

Replace hoedown with pull in rustdoc #40338

Replace hoedown with pull in rustdoc #40338

Conversation

GuillaumeGomez commented Mar 8, 2017 • edited Loading

rust-highfive commented Mar 8, 2017

steveklabnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Mar 10, 2017

Choose a reason for hiding this comment

killercup Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik commented Mar 10, 2017

steveklabnik commented Mar 10, 2017

killercup left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Mar 11, 2017

ollie27 commented Mar 11, 2017

bors commented Mar 11, 2017

GuillaumeGomez commented Mar 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Mar 12, 2017

ollie27 commented Mar 12, 2017 • edited Loading

GuillaumeGomez commented Mar 28, 2017

frewsxcv left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Mar 29, 2017

bors commented Mar 29, 2017

bors commented Mar 29, 2017

bors commented Mar 29, 2017

ollie27 commented Mar 29, 2017

frewsxcv commented Mar 29, 2017

ollie27 commented Mar 29, 2017

steveklabnik commented Mar 29, 2017

steveklabnik commented Mar 29, 2017

steveklabnik commented Mar 29, 2017

steveklabnik commented Mar 29, 2017

mbrubeck commented Sep 28, 2017

GuillaumeGomez commented Mar 8, 2017 •

edited

Loading

killercup Mar 10, 2017 •

edited

Loading

killercup left a comment •

edited

Loading

killercup Mar 10, 2017 •

edited

Loading

GuillaumeGomez commented Mar 11, 2017 •

edited

Loading

ollie27 commented Mar 12, 2017 •

edited

Loading