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

Tracking issue for hoedown -> pulldown regressions #40912

Closed
13 tasks done
steveklabnik opened this issue Mar 29, 2017 · 31 comments
Closed
13 tasks done

Tracking issue for hoedown -> pulldown regressions #40912

steveklabnik opened this issue Mar 29, 2017 · 31 comments
Assignees
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@steveklabnik
Copy link
Member

steveklabnik commented Mar 29, 2017

In #40338, we landed pulldown-cmark. 🎊

But, given that it's a different renderer, there are bound to be differences. This comment pointed out some obvious problems. These bugs are going to be much easier to clean up than the initial PR was to land, though 👍

To help with this work, I generated docs for both the commit before and the commit of, ran all the HTML files through tidy-html5, and put it up here: steveklabnik/docdiff@ddda1fe the ' -> " changes are expected, but that tool doesn't have the ability to re-write those, so I left them in for now, which, frankly, makes the diff kinda large. working on it.

Here's the stuff we've found so far:

This change will land in tonight (3/29)'s nightly, so we can also poke at them then. I plan on making a users post tomorrow to advertise this bug.

Tagging as a regression so we make sure to take care of it. Marking as P-high and assigning @GuillaumeGomez and @frewsxcv who are both already working at knocking some of this out.

@steveklabnik steveklabnik added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Mar 29, 2017
@ScottAbbey
Copy link
Contributor

In core/intrinsics/fn.overflowing_sub.html.txt (and elsewhere?), hoedown was rendering 2^N as 2<sup>N</sup>, the new version just leaves it as 2^N.

This isn't recognized in the CommonMark spec, probably need to change the source to 2<sup>N</sup> ?

@steveklabnik
Copy link
Member Author

@ScottAbbey agree 100%; we'll have to send in a PR to fix. I wonder what the easiest way of finding all of them is.....

@ScottAbbey
Copy link
Contributor

ScottAbbey commented Mar 29, 2017

It looks like markdown.rs is squishing things together when a list item spans multiple lines. In pulldown-cmark's html.rs, this would have been a SoftBreak that got turned into a \n, it appears this rendering is just throwing those away. Maybe not all of them? But some?

@steveklabnik
Copy link
Member Author

steveklabnik commented Mar 29, 2017

For the ^ issue; rg -g !*.cpp -g !*.h \^ | rg "///" is giving me pretty reasonable results. Still lots of false positives, but manageable.

@ScottAbbey
Copy link
Contributor

In src/libstd/collections/hash/map.rs under "Relevant papers/articles:", it appears the ordered list has been rendered as an unordered list instead.

@SergioBenitez
Copy link
Contributor

SergioBenitez commented Mar 30, 2017

I'm getting an unexpected panic from rustc that appears to be caused by the transition:

---- src/request/form/from_form_value.rs - request::form::from_form_value::FromFormValue (line 59) stdout ----
	error: unknown start of token: `
 --> <anon>:2:42
  |
2 | A value is validated successfully if the `from_str` method for the given
  |                                          ^

thread 'rustc' panicked at 'Box<Any>', /checkout/src/libsyntax/parse/lexer/mod.rs:81
note: Run with `RUST_BACKTRACE=1` for a backtrace.
thread 'rustc' panicked at 'couldn't compile the test', /checkout/src/librustdoc/test.rs:270

The markdown being referenced in the error looks like this:

/// Rocket implements `FromFormValue` for many standard library types. Their
/// behavior is documented here.
///
///   * **f32, f64, isize, i8, i16, i32, i64, usize, u8, u16, u32, u64**
///
///   **IpAddr, Ipv4Addr, Ipv6Addr, SocketAddrV4, SocketAddrV6, SocketAddr**
///
///     A value is validated successfully if the `from_str` method for the given
///     type returns successfully. Otherwise, the raw form value is returned as
///     the `Err` value.

Edit: I'll open a separate issue for this.

@bluss
Copy link
Member

bluss commented Mar 31, 2017

Here's one I found. (The diff is the fix needed, the extra line, to have the link render as a link).

diff --git a/src/impl_views.rs b/src/impl_views.rs
index c26a4ef1..ac2f787c 100644
--- a/src/impl_views.rs
+++ b/src/impl_views.rs
@@ -19,6 +19,7 @@ use StrideShape;
 /// Methods for read-only array views `ArrayView<'a, A, D>`
 ///
 /// Note that array views implement traits like [`From`][f] and `IntoIterator` too.
+///
 /// [f]: #method.from
 impl<'a, A, D> ArrayBase<ViewRepr<&'a A>, D>
     where D: Dimension,

@GuillaumeGomez
Copy link
Member

@bluss: A commonmark thing once again. Get a look here. When you add a newline, it works as expected.

@ollie27
Copy link
Member

ollie27 commented Mar 31, 2017

Another bit of expected breakage is that autolinks now require <> around them. This can be seen in the error index: E0033.

If anyone is interested, I've started implementing what I suggested here: #40338 (comment), using push_html to render the HTML and iterator adapters to modify to our needs. This will simplify rustdoc and fix most of the issues with the current implementation. Unfortunately it will involve removing most of the rendering code that's there at the moment.

@GuillaumeGomez
Copy link
Member

@ollie27: Fine by me. But please wait for #40919 to get merged before please so that we'll avoid regressions.

@bluss
Copy link
Member

bluss commented Mar 31, 2017

Linebreak issue in bullet points:

Input:

//! - Still iterating on and evolving the crate this is a longer sentence that
//!   is going to wrap

Output includes the two words pulled together across the linebreak: “thatis”

“• Still iterating on and evolving the crate this is a longer sentence thatis going to wrap”

@steveklabnik
Copy link
Member Author

Output includes the two words pulled together across the linebreak: “thatis”

I thought I made a bullet for this, but did not. However, I think this is related to the hard/soft break stuff. Making a bullet now though.

@GuillaumeGomez
Copy link
Member

It is. I'll handle when the current fix PR is merged.

@Marwes
Copy link
Contributor

Marwes commented Apr 1, 2017

#28712 seems to have been fixed causing this code block to be treated as a doc test https://travis-ci.org/gluon-lang/gluon/jobs/217399595#L668.

Fixed in gluon-lang/gluon@b772329

bors added a commit that referenced this issue Apr 2, 2017
…veklabnik

Add support for image, rules and footnotes

Part of #40912.

r? @rust-lang/docs

PS: the footnotes are waiting for pulldown-cmark/pulldown-cmark#21 to be merged to be fully working.
@GuillaumeGomez
Copy link
Member

@bluss: HardBreak issue has been fixed.

@bluss
Copy link
Member

bluss commented Apr 3, 2017

Great!

@steveklabnik
Copy link
Member Author

steveklabnik commented Apr 5, 2017

I just reviewed this again. oustanding things:

footnotes (waiting for pulldown update)

raph is busy, and this is a tiny thing. We should still see if we can get it landed. I think it can be worked around with an extra newline? This is still an under-spec'd thing.

the title part of links being rendered as visible text rather than as an attribute on the tag

@ollie27 you had said this here, but I can't see what you're referring to. Any pointers?

MarkdownHtml now doing the same as Markdown when it should be escaping raw HTML.

@ollie27 same here, is this still a thing or not?

frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 5, 2017
…, r=frewsxcv

Handle ordered lists as well

Part of rust-lang#40912.

r? @rust-lang/docs
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 5, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 5, 2017
…, r=frewsxcv

Handle ordered lists as well

Part of rust-lang#40912.

r? @rust-lang/docs
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 5, 2017
@GuillaumeGomez
Copy link
Member

MarkdownHtml now doing the same as Markdown when it should be escaping raw HTML.

I didn't fix this at all. I actually don't even know where this is generated.

@bluss
Copy link
Member

bluss commented Apr 6, 2017

rustc 1.18.0-nightly (91ae22a 2017-04-05) still shows a hard break issue. Not in regular paragraphs anymore, but in bullets.

Repro:

//! * Test line
//!   break

Expected Output: • Test line break

Actual output: • Test linebreak

@steveklabnik
Copy link
Member Author

@bluss specifically what happens and what do you expect?

@bluss
Copy link
Member

bluss commented Apr 6, 2017

Updated the comment with that. It pulls together the word across the line break. Still #40912 (comment) in fact (it's also a bullet)

@GuillaumeGomez
Copy link
Member

Ah nice! However, this isn't a "hard" break issue but a soft one.

@ollie27
Copy link
Member

ollie27 commented Apr 6, 2017

the title part of links being rendered as visible text rather than as an attribute on the tag

@ollie27 you had said this here, but I can't see what you're referring to. Any pointers?

This was fixed for HTML by #40919 but is still broken for plain_summary_line. I've fixed it in #41112.

MarkdownHtml now doing the same as Markdown when it should be escaping raw HTML.

@ollie27 same here, is this still a thing or not?

MarkdownHtml is supposed to treat raw HTML as plain text so for example Struct<br> in Markdown would result in Struct&lt;br&gt; HTML. I've fixed this in #41112.

I've gone through a diff of the std docs and found two issues caused by differences between hoedown and pulldown-cmark not listed in the above checklist:

  1. pulldown-cmark now needs a blank line before reference link URLs. I've fixed all example I could find of this in Fix Markdown issues in the docs #41111.
  2. autolinks now require <> around them. Affected pages:
error-index.html
std/process/struct.Command.html
*/hash/struct.SipHasher*.html
std/net/struct.UdpSocket.html

frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 6, 2017
…umeGomez

Fix Markdown issues in the docs

* Since the switch to pulldown-cmark reference links need a blank line
before the URLs. (rust-lang#40912)
* Reference link references are not case sensitive.
* Doc comments need to be indented uniformly otherwise rustdoc gets
confused.
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 6, 2017
…umeGomez

Fix Markdown issues in the docs

* Since the switch to pulldown-cmark reference links need a blank line
before the URLs. (rust-lang#40912)
* Reference link references are not case sensitive.
* Doc comments need to be indented uniformly otherwise rustdoc gets
confused.
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 6, 2017
rustdoc: Use pulldown-cmark for Markdown HTML rendering

Instead of rendering all of the HTML in rustdoc this relies on
pulldown-cmark's `push_html` to do most of the work. A few iterator
adapters are used to make rustdoc specific modifications to the output.

This also fixes MarkdownHtml and link titles in plain_summary_line.

https://ollie27.github.io/rust_doc_test/ is the docs built with this change and rust-lang#41111.

Part of rust-lang#40912.

cc @GuillaumeGomez

r? @steveklabnik
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 6, 2017
…umeGomez

Fix Markdown issues in the docs

* Since the switch to pulldown-cmark reference links need a blank line
before the URLs. (rust-lang#40912)
* Reference link references are not case sensitive.
* Doc comments need to be indented uniformly otherwise rustdoc gets
confused.
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 6, 2017
rustdoc: Use pulldown-cmark for Markdown HTML rendering

Instead of rendering all of the HTML in rustdoc this relies on
pulldown-cmark's `push_html` to do most of the work. A few iterator
adapters are used to make rustdoc specific modifications to the output.

This also fixes MarkdownHtml and link titles in plain_summary_line.

https://ollie27.github.io/rust_doc_test/ is the docs built with this change and rust-lang#41111.

Part of rust-lang#40912.

cc @GuillaumeGomez

r? @steveklabnik
@steveklabnik
Copy link
Member Author

@ollie27 thanks so much!

@steveklabnik
Copy link
Member Author

It looks like this is complete, modulo some small possible changes upstream with regards to footnotes.

Great work everyone! Thank you so much ❤️

@bluss
Copy link
Member

bluss commented Apr 8, 2017

Thanks ❤️

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Feb 16, 2018
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.
djrenren pushed a commit to djrenren/libtest that referenced this issue Jan 22, 2019
part of rust-lang/rust#40912

[]\n() is not actually a link.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants