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: overhaul code block lexing errors #56884

Merged
merged 2 commits into from Jan 20, 2019

Conversation

Projects
None yet
7 participants
@euclio
Copy link
Contributor

euclio commented Dec 16, 2018

Fixes #53919.

This PR moves the reporting of code block lexing errors from rendering time to an early pass, so we can use the compiler's error reporting mechanisms. This dramatically improves the diagnostics in this situation: we now de-emphasize the lexing errors as a note under a warning that has a span and suggestion instead of just emitting errors at the top level.

Additionally, this PR generalizes the markdown -> source span calculation function, which should allow other rustdoc warnings to use better spans in the future.

Last, the PR makes sure that the code block is always emitted in the docs, even if it fails to highlight correctly.

Of note:

  • The new pass unfortunately adds another pass over the docs to gather the doc blocks for syntax-checking. I wonder if this could be combined with the pass that looks for testable blocks? I'm not familiar with that code, so I don't know how feasible that is.
  • pulldown_cmark doesn't make it easy to find the spans of the code blocks, so the code that calculates the spans is a little nasty. It works for all the test cases I threw at it, but I wouldn't be surprised if an edge case would break it. Should have a thorough review.
  • This PR worsens the state of #56885, since those certain fatal lexing errors are now emitted before docs get generated at all.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 16, 2018

r? @QuietMisdreavus

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

/// \_
/// ```
pub fn ok() {}

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Dec 16, 2018

Member

nit: Extra empty line.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 16, 2018

The new pass unfortunately adds another pass over the docs to gather the doc blocks for syntax-checking. I wonder if this could be combined with the pass that looks for testable blocks? I'm not familiar with that code, so I don't know how feasible that is.

We could do everything in just one pass, not sure we'd like that. For instance, we check intra links and if private items have doc tests in one pass. But maybe that'd be a nice thing to only have one pass and iterate over passes for each item. To be determined...

This PR worsens the state of #56885, since those certain fatal lexing errors are now emitted before docs get generated at all.

But at least you get the information, which is the most important in my opinion.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Dec 17, 2018

Reading through now. This PR will probably take some time to give a real review for, though...

The new pass unfortunately adds another pass over the docs to gather the doc blocks for syntax-checking. I wonder if this could be combined with the pass that looks for testable blocks? I'm not familiar with that code, so I don't know how feasible that is.

I wonder if we could make a kind of "item pass" so we could combine all the ones that need to check things, but not add new items, into one actual "pass" that runs multiple functions over each item as it finds it? That way we reduce the number of crate scans that need to happen, but still make the checks somewhat modular.

@euclio

This comment has been minimized.

Copy link
Contributor

euclio commented Jan 2, 2019

@QuietMisdreavus I'm unclear on the scope of the changes that you'd like me to make here. Would it suffice to just attempt to combine the passes for gathering doctests and checking their syntax?

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jan 14, 2019

Sorry, i was speaking hypothetically when i mentioned "item passes". This PR can stay using a separate pass to perform syntax-checking; the refactoring can come later. I'm reading over the PR now...

@QuietMisdreavus
Copy link
Member

QuietMisdreavus left a comment

One last nit, then this is ready to go!

Show resolved Hide resolved src/librustdoc/passes/check_code_block_syntax.rs Outdated

euclio added some commits Dec 15, 2018

@euclio euclio force-pushed the euclio:codeblock-diagnostics branch from 431f58b to 8c93798 Jan 15, 2019

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jan 15, 2019

Thanks so much! This is ready to go!

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 15, 2019

📌 Commit 8c93798 has been approved by QuietMisdreavus

Centril added a commit to Centril/rust that referenced this pull request Jan 15, 2019

Rollup merge of rust-lang#56884 - euclio:codeblock-diagnostics, r=Qui…
…etMisdreavus

rustdoc: overhaul code block lexing errors

Fixes rust-lang#53919.

This PR moves the reporting of code block lexing errors from rendering time to an early pass, so we can use the compiler's error reporting mechanisms. This dramatically improves the diagnostics in this situation: we now de-emphasize the lexing errors as a note under a warning that has a span and suggestion instead of just emitting errors at the top level.

Additionally, this PR generalizes the markdown -> source span calculation function, which should allow other rustdoc warnings to use better spans in the future.

Last, the PR makes sure that the code block is always emitted in the docs, even if it fails to highlight correctly.

Of note:
- The new pass unfortunately adds another pass over the docs to gather the doc blocks for syntax-checking. I wonder if this could be combined with the pass that looks for testable blocks? I'm not familiar with that code, so I don't know how feasible that is.
- `pulldown_cmark` doesn't make it easy to find the spans of the code blocks, so the code that calculates the spans is a little nasty. It works for all the test cases I threw at it, but I wouldn't be surprised if an edge case would break it. Should have a thorough review.
- This PR worsens the state of rust-lang#56885, since those certain fatal lexing errors are now emitted before docs get generated at all.

bors added a commit that referenced this pull request Jan 16, 2019

Auto merge of #57637 - Centril:rollup, r=Centril
Rollup of 6 pull requests

Successful merges:

 - #56884 (rustdoc: overhaul code block lexing errors)
 - #57065 (Optimize try_mark_green and eliminate the lock on dep node colors)
 - #57107 (Add a regression test for mutating a non-mut #[thread_local])
 - #57268 (Add a target option "merge-functions", and a corresponding -Z flag (works around #57356))
 - #57551 (resolve: Add a test for issue #57539)
 - #57598 (Add missing unpretty option help message)

Failed merges:

r? @ghost

bors added a commit to rust-lang/cargo that referenced this pull request Jan 17, 2019

Auto merge of #6559 - euclio:relax-rustdoc-output, r=ehuss
relax rustdoc output assertion

The output of this rustdoc error is changing in rust-lang/rust#56884. This PR relaxes an assertion on the output so that the test will still pass once the rustdoc PR is merged.

bors added a commit to rust-lang/cargo that referenced this pull request Jan 17, 2019

Auto merge of #6559 - euclio:relax-rustdoc-output, r=ehuss
relax rustdoc output assertion

The output of this rustdoc error is changing in rust-lang/rust#56884. This PR relaxes an assertion on the output so that the test will still pass once the rustdoc PR is merged.

@ehuss ehuss referenced this pull request Jan 18, 2019

Merged

Update cargo #57721

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 18, 2019

⌛️ Testing commit 8c93798 with merge 93780ea...

bors added a commit that referenced this pull request Jan 18, 2019

Auto merge of #56884 - euclio:codeblock-diagnostics, r=QuietMisdreavus
rustdoc: overhaul code block lexing errors

Fixes #53919.

This PR moves the reporting of code block lexing errors from rendering time to an early pass, so we can use the compiler's error reporting mechanisms. This dramatically improves the diagnostics in this situation: we now de-emphasize the lexing errors as a note under a warning that has a span and suggestion instead of just emitting errors at the top level.

Additionally, this PR generalizes the markdown -> source span calculation function, which should allow other rustdoc warnings to use better spans in the future.

Last, the PR makes sure that the code block is always emitted in the docs, even if it fails to highlight correctly.

Of note:
- The new pass unfortunately adds another pass over the docs to gather the doc blocks for syntax-checking. I wonder if this could be combined with the pass that looks for testable blocks? I'm not familiar with that code, so I don't know how feasible that is.
- `pulldown_cmark` doesn't make it easy to find the spans of the code blocks, so the code that calculates the spans is a little nasty. It works for all the test cases I threw at it, but I wouldn't be surprised if an edge case would break it. Should have a thorough review.
- This PR worsens the state of #56885, since those certain fatal lexing errors are now emitted before docs get generated at all.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 18, 2019

💔 Test failed - checks-travis

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 18, 2019

The job x86_64-gnu-aux of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[01:55:30] test workspaces::ws_warn_unused ... ok
[01:55:30] 
[01:55:30] failures:
[01:55:30] 
[01:55:30] ---- doc::output_not_captured stdout ----
[01:55:30] running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/cargo doc`
[01:55:30] thread 'doc::output_not_captured' panicked at '
[01:55:30]     but: expected to find:
[01:55:30] 1 | ☃
[01:55:30] 
[01:55:30] did not find in output:
[01:55:30] did not find in output:
[01:55:30]     Checking a v0.0.1 (/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t534/foo/a)
[01:55:30]  Documenting a v0.0.1 (/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t534/foo/a)
[01:55:30] warning: could not parse code block as Rust code
[01:55:30]  --> a/src/lib.rs:2:17
[01:55:30] 2 |               /// ```
[01:55:30]   |  _________________^
[01:55:30] 3 | |             /// ☃
[01:55:30] 4 | |             /// ```
[01:55:30] 4 | |             /// ```
[01:55:30]   | |___________________^
[01:55:30]   |
[01:55:30]   = note: error from rustc: unknown start of token: \u{2603}
[01:55:30] help: mark blocks that do not contain Rust code as text
[01:55:30] 2 |             /// ```text
[01:55:30]   |                 ^^^^^^^
[01:55:30] 
[01:55:30] 
[01:55:30]  Documenting foo v0.0.1 (/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t534/foo)
[01:55:30] ', src/tools/cargo/tests/testsuite/support/mod.rs:773:13
[01:55:30] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:55:30] 
[01:55:30] 
---
[01:55:30] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/tools/cargo/Cargo.toml" "--features" "rustc-workspace-hack/all-static"
[01:55:30] expected success, got: exit code: 101
[01:55:30] 
[01:55:30] 
[01:55:30] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/test/pretty src/test/run-pass/pretty src/test/run-fail/pretty src/test/run-pass-valgrind/pretty src/test/run-pass-fulldeps/pretty src/test/run-fail-fulldeps/pretty src/tools/cargo src/tools/cargotest
[01:55:30] Build completed unsuccessfully in 0:33:24
[01:55:30] Makefile:50: recipe for target 'check-aux' failed
[01:55:30] make: *** [check-aux] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:010b80d8
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Fri Jan 18 07:35:07 UTC 2019
---
travis_time:end:18da0358:start=1547796909999471540,finish=1547796910007939736,duration=8468196
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:06e8c442
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:start:crashlog
obj/cores/core.3537.!checkout!obj!build!x86_64-unknown-linux-gnu!stage2!bin!rustc
[New LWP 3539]
[New LWP 3537]
warning: Could not load shared library symbols for 8 libraries, e.g. /lib/x86_64-linux-gnu/libc.so.6.
Use the "info sharedlibrary" command to see the complete listing.
Do you need "set solib-search-path" or "set sysroot"?
Core was generated by `rustc --crate-name foo src/lib.rs --color never --crate-type lib --emit=dep-inf'.
Program terminated with signal SIGABRT, Aborted.
#0  0x00007f95aef29428 in ?? ()
[Current thread is 1 (LWP 3539)]
#0  0x00007f95aef29428 in ?? ()
#1  0x00007f95aef2b02a in ?? ()
#2  0x0000000000000020 in ?? ()
#3  0x0000000000000000 in ?? ()
travis_time:end:06e8c442:start=1547796910029709220,finish=1547796911991933712,duration=1962224492
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:05419626
travis_time:start:05419626
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:049524ec
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jan 18, 2019

[01:55:30] ---- doc::output_not_captured stdout ----
[01:55:30] running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/cargo doc`
[01:55:30] thread 'doc::output_not_captured' panicked at '
[01:55:30] Expected: execs
[01:55:30]     but: expected to find:
[01:55:30] 1 | ☃
[01:55:30] 
[01:55:30] did not find in output:
[01:55:30]     Checking a v0.0.1 (/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t534/foo/a)
[01:55:30]  Documenting a v0.0.1 (/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t534/foo/a)
[01:55:30] warning: could not parse code block as Rust code
[01:55:30]  --> a/src/lib.rs:2:17
[01:55:30]   |
[01:55:30] 2 |               /// ```
[01:55:30]   |  _________________^
[01:55:30] 3 | |             /// ☃
[01:55:30] 4 | |             /// ```
[01:55:30]   | |___________________^
[01:55:30]   |
[01:55:30]   = note: error from rustc: unknown start of token: \u{2603}
[01:55:30] help: mark blocks that do not contain Rust code as text
[01:55:30]   |
[01:55:30] 2 |             /// ```text
[01:55:30]   |                 ^^^^^^^
[01:55:30] 
[01:55:30]  Documenting foo v0.0.1 (/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t534/foo)
[01:55:30]     Finished dev [unoptimized + debuginfo] target(s) in 4.58s
[01:55:30] ', src/tools/cargo/tests/testsuite/support/mod.rs:773:13
[01:55:30] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:55:30] 
[01:55:30] 
[01:55:30] failures:
[01:55:30]     doc::output_not_captured

This looks like one of Cargo's tests, where it's scanning the output of rustdoc to make sure Cargo hasn't swallowed it. However, i'm not totally sure what the error is saying here.

@rust-lang/cargo, this PR changes the output format of doctest failures, and it seems to have tripped up one of Cargo's tests. How should we change the Cargo test to make it work? (And how should we coordinate the submodule update with this PR? >_>)

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Jan 18, 2019

@QuietMisdreavus already fixed in rust-lang/cargo#6559. You'll have to rebase on top of #57721 to unblock this.

@euclio

This comment has been minimized.

Copy link
Contributor

euclio commented Jan 18, 2019

Yeah, I noticed that failure yesterday. My fix is already in cargo master, we just have to wait for #57721 to land.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jan 18, 2019

Oh! Well, i'm glad it's almost already taken care of! 😅

bors added a commit that referenced this pull request Jan 19, 2019

Auto merge of #57721 - ehuss:update-cargo, r=alexcrichton
Update cargo

Unblocks #56884  cc @euclio

6 commits in 2b4a5f1f0bb6e13759e88ea9512527b0beba154f..ffe65875fd05018599ad07e7389e99050c7915be
2019-01-12 04:13:12 +0000 to 2019-01-17 23:57:50 +0000
- Better error message for bad manifest with `cargo install`. (rust-lang/cargo#6560)
- relax rustdoc output assertion (rust-lang/cargo#6559)
- touch some files when we use them (rust-lang/cargo#6477)
- Add documentation for new package/publish feature flags. (rust-lang/cargo#6553)
- Update chat link to Discord. (rust-lang/cargo#6554)
- Fix typo (rust-lang/cargo#6552)

r? @alexcrichton
@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 19, 2019

@euclio #57721 has landed.

@euclio

This comment has been minimized.

Copy link
Contributor

euclio commented Jan 20, 2019

This just needs a retry, then!

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jan 20, 2019

On the assumption that this will Just Work...

@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 20, 2019

⌛️ Testing commit 8c93798 with merge 846ea58...

bors added a commit that referenced this pull request Jan 20, 2019

Auto merge of #56884 - euclio:codeblock-diagnostics, r=QuietMisdreavus
rustdoc: overhaul code block lexing errors

Fixes #53919.

This PR moves the reporting of code block lexing errors from rendering time to an early pass, so we can use the compiler's error reporting mechanisms. This dramatically improves the diagnostics in this situation: we now de-emphasize the lexing errors as a note under a warning that has a span and suggestion instead of just emitting errors at the top level.

Additionally, this PR generalizes the markdown -> source span calculation function, which should allow other rustdoc warnings to use better spans in the future.

Last, the PR makes sure that the code block is always emitted in the docs, even if it fails to highlight correctly.

Of note:
- The new pass unfortunately adds another pass over the docs to gather the doc blocks for syntax-checking. I wonder if this could be combined with the pass that looks for testable blocks? I'm not familiar with that code, so I don't know how feasible that is.
- `pulldown_cmark` doesn't make it easy to find the spans of the code blocks, so the code that calculates the spans is a little nasty. It works for all the test cases I threw at it, but I wouldn't be surprised if an edge case would break it. Should have a thorough review.
- This PR worsens the state of #56885, since those certain fatal lexing errors are now emitted before docs get generated at all.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 20, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: QuietMisdreavus
Pushing 846ea58 to master...

@bors bors merged commit 8c93798 into rust-lang:master Jan 20, 2019

1 check passed

homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment