rustdoc: set the default edition when pre-parsing a doctest #60065

QuietMisdreavus · 2019-04-18T01:43:51Z

Fixes #59313 (possibly more? i think we've had issues with parsing edition-specific syntax in doctests at some point)

When handling a doctest, rustdoc needs to parse it beforehand, so that it can see whether it declares a fn main or extern crate my_crate explicitly. However, while doing this, rustdoc doesn't set the "default edition" used by the parser like the regular compilation runs do. This caused a problem when parsing a doctest with an async move block in it, since it was expecting the move keyword to start a closure, not a block.

This PR changes the rustdoc::test::make_test function to set the parser's default edition while looking for a main function and extern crate statement. However, to do this, make_test needs to know what edition to set. Since this is also used during the HTML rendering process (to make playground URLs), now the HTML renderer needs to know about the default edition. Upshot: rendering standalone markdown files can now accept a "default edition" for their doctests with the --edition flag! (I'm pretty sure i waffled around how to set that a long time ago when we first added the --edition flag... >_>)

I'm posting this before i stop for the night so that i can write this description while it's still in my head, but before this merges i want to make sure that (1) the rustdoc-ui/failed-doctest-output test still works (i expect it doesn't), and (2) i add a test with the sample from the linked issue.

QuietMisdreavus · 2019-04-18T14:19:38Z

Turns out, i guessed the wrong failing test. The failed-doctest-output test is fine, but the playground URL tests failed because i made it start emitting the edition flag all the time. I've also pushed up the test from the linked issue, so this should be ready to go now.

r? @rust-lang/rustdoc

GuillaumeGomez · 2019-04-18T20:31:31Z

src/librustdoc/config.rs

@@ -383,27 +383,28 @@ impl Options {
            }
        }

+        let edition = matches.opt_str("edition").unwrap_or("2015".to_string());


We tend to prefer .unwrap_or_else. ;)

This was existing code that i moved for this PR, but i can fix it.

GuillaumeGomez · 2019-04-18T20:33:23Z

Just a small nit but everything else looks good. r=me once nit fixed and CI passed.

ollie27 · 2019-04-18T20:46:41Z

I don't think that this completely fixes #59313 because rustdoc will still fail while looking for fn main if the edition is set to 2015. rustdoc should never fail at that stage and it means you don't get a useful error message:

running 1 test
test issue-59313.rs -  (line 1) ... FAILED

failures:

failures:
    issue-59313.rs -  (line 1)

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

It should output:

running 1 test
test issue-59313.rs -  (line 1) ... FAILED

failures:

---- issue-59313.rs -  (line 1) stdout ----
error: expected one of `!`, `)`, `,`, `.`, `::`, `?`, `{`, or an operator, found `move`
 --> issue-59313.rs:5:16
  |
6 |     drop(async move {});
  |                ^^^^ expected one of 8 possible tokens here

error: aborting due to previous error

thread 'issue-59313.rs -  (line 1)' panicked at 'couldn't compile the test', src\librustdoc\test.rs:310:13
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.


failures:
    issue-59313.rs -  (line 1)

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

QuietMisdreavus · 2019-04-19T14:29:59Z

@ollie27 That's a valid point. To solve that, i think we'd have to change this line in Parser::expect_one_of:

rust/src/libsyntax/parse/parser.rs

Lines 791 to 792 in 22fa4bb

    
           } else if self.last_unexpected_token_span == Some(self.span) { 
        
               FatalError.raise();

That FatalError.raise is what's causing the parsing thread to abort without an error message. I'm not super sure what i should replace it with, though, so i need to call in @rust-lang/compiler to see if someone who knows their way around the parser can give some guidance.

ollie27 · 2019-04-19T16:33:52Z

How about wrapping the code looking for fn main in a panic::catch_unwind?

Zoxc · 2019-04-19T16:57:56Z

This PR seems to have quite a lot of overlap with #59742, which may fix the same issue as a side effect.

QuietMisdreavus · 2019-04-19T19:34:40Z

@Zoxc Oh yeah, you do seem to make a lot of the same changes! I gave your branch a try, and it does make the doctest pass under 2018, but it doesn't solve the issue that @ollie27 mentioned - there is still no output for the failure under 2015. I'd like to fold my work into your PR somehow and have someone check out the FatalError thing separately, if possible.

(On a side note, it strikes me as odd that we have a DEFAULT_EDITION constant but not an implementation of the Default trait... 🤔)

QuietMisdreavus · 2019-05-02T14:54:46Z

Looks like the issue @ollie27 mentioned will get fixed by #60220, and @Zoxc's PR does almost the same thing this one does. (This one goes farther in spreading the edition-passing through rustdoc's code itself, so it's not the same, but fixes the same issue in the end.)

@Zoxc Should we land this PR so you can use the other rustdoc changes in your PR?

Zoxc · 2019-05-02T22:37:56Z

@QuietMisdreavus Don't block on my PR

estebank · 2019-05-02T23:25:01Z

@QuietMisdreavus that hard failure is there because we want to avoid spamming the user with errors caused by error recovery, in other words, spurious errors. It should be possible to make this configurable on the Parser and have rustdoc configure it to be less dogmatic and continue returning an Err and maybe generating a few extra errors while keeping the output clean-ish for rustc users.

bors · 2019-05-03T18:09:03Z

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

QuietMisdreavus · 2019-05-06T20:49:40Z

I've rebased the PR to include #60220. IMO we should get this in so that it can be used in #59742.

GuillaumeGomez · 2019-05-07T08:13:48Z

Looks good to me. Just waiting for @ollie27 confirmation.

Zoxc · 2019-05-19T14:44:49Z

What is this PR waiting on now?

ollie27 · 2019-05-19T15:19:56Z

My bad, lets get this in.

@bors r+

bors · 2019-05-19T15:19:57Z

📌 Commit 76b4900 has been approved by ollie27

bors · 2019-05-19T16:48:20Z

⌛ Testing commit 76b4900 with merge 6afcb56...

rustdoc: set the default edition when pre-parsing a doctest Fixes #59313 (possibly more? i think we've had issues with parsing edition-specific syntax in doctests at some point) When handling a doctest, rustdoc needs to parse it beforehand, so that it can see whether it declares a `fn main` or `extern crate my_crate` explicitly. However, while doing this, rustdoc doesn't set the "default edition" used by the parser like the regular compilation runs do. This caused a problem when parsing a doctest with an `async move` block in it, since it was expecting the `move` keyword to start a closure, not a block. This PR changes the `rustdoc::test::make_test` function to set the parser's default edition while looking for a main function and `extern crate` statement. However, to do this, `make_test` needs to know what edition to set. Since this is also used during the HTML rendering process (to make playground URLs), now the HTML renderer needs to know about the default edition. Upshot: rendering standalone markdown files can now accept a "default edition" for their doctests with the `--edition` flag! (I'm pretty sure i waffled around how to set that a long time ago when we first added the `--edition` flag... `>_>`) I'm posting this before i stop for the night so that i can write this description while it's still in my head, but before this merges i want to make sure that (1) the `rustdoc-ui/failed-doctest-output` test still works (i expect it doesn't), and (2) i add a test with the sample from the linked issue.

bors · 2019-05-19T19:54:27Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: ollie27
Pushing 6afcb56 to master...

QuietMisdreavus mentioned this pull request Apr 18, 2019

async move breaks doc tests #59313

Closed

This comment has been minimized.

Sign in to view

QuietMisdreavus changed the title ~~[wip] rustdoc: set the default edition when pre-parsing a doctest~~ rustdoc: set the default edition when pre-parsing a doctest Apr 18, 2019

This comment has been minimized.

Sign in to view

QuietMisdreavus force-pushed the async-move-doctests branch from 8961f20 to 186d063 Compare April 18, 2019 16:29

This comment has been minimized.

Sign in to view

GuillaumeGomez reviewed Apr 18, 2019

View reviewed changes

QuietMisdreavus mentioned this pull request Apr 24, 2019

doc tests fail without any information when missing back quotes. #59557

Closed

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 3, 2019

QuietMisdreavus added 6 commits May 6, 2019 15:20

set the default edition when pre-parsing a doctest

20a5aa3

update playground url tests with new edition behavior

e66e1c7

add test for async move in a doctest

a6d26eb

update rustdoc unit tests

5b167bf

update rustdoc doc test

e61ff77

don't pre-allocate the default edition string

76b4900

QuietMisdreavus force-pushed the async-move-doctests branch from f4bf9af to 76b4900 Compare May 6, 2019 20:45

Zoxc mentioned this pull request May 18, 2019

Move edition outside the hygiene lock and avoid accessing it #59742

Merged

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 19, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label May 19, 2019

bors merged commit 76b4900 into rust-lang:master May 19, 2019

QuietMisdreavus deleted the async-move-doctests branch May 20, 2019 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: set the default edition when pre-parsing a doctest #60065

rustdoc: set the default edition when pre-parsing a doctest #60065

QuietMisdreavus commented Apr 18, 2019

This comment has been minimized.

QuietMisdreavus commented Apr 18, 2019

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez Apr 18, 2019

QuietMisdreavus Apr 18, 2019

GuillaumeGomez commented Apr 18, 2019

ollie27 commented Apr 18, 2019

QuietMisdreavus commented Apr 19, 2019

ollie27 commented Apr 19, 2019

Zoxc commented Apr 19, 2019

QuietMisdreavus commented Apr 19, 2019

QuietMisdreavus commented May 2, 2019

Zoxc commented May 2, 2019

estebank commented May 2, 2019

bors commented May 3, 2019

QuietMisdreavus commented May 6, 2019

GuillaumeGomez commented May 7, 2019

Zoxc commented May 19, 2019

ollie27 commented May 19, 2019

bors commented May 19, 2019

bors commented May 19, 2019

bors commented May 19, 2019

rustdoc: set the default edition when pre-parsing a doctest #60065

rustdoc: set the default edition when pre-parsing a doctest #60065

Conversation

QuietMisdreavus commented Apr 18, 2019

This comment has been minimized.

QuietMisdreavus commented Apr 18, 2019

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez Apr 18, 2019

Choose a reason for hiding this comment

QuietMisdreavus Apr 18, 2019

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 18, 2019

ollie27 commented Apr 18, 2019

QuietMisdreavus commented Apr 19, 2019

ollie27 commented Apr 19, 2019

Zoxc commented Apr 19, 2019

QuietMisdreavus commented Apr 19, 2019

QuietMisdreavus commented May 2, 2019

Zoxc commented May 2, 2019

estebank commented May 2, 2019

bors commented May 3, 2019

QuietMisdreavus commented May 6, 2019

GuillaumeGomez commented May 7, 2019

Zoxc commented May 19, 2019

ollie27 commented May 19, 2019

bors commented May 19, 2019

bors commented May 19, 2019

bors commented May 19, 2019