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

async move breaks doc tests #59313

Closed
ghost opened this issue Mar 20, 2019 · 10 comments · Fixed by #60065
Closed

async move breaks doc tests #59313

ghost opened this issue Mar 20, 2019 · 10 comments · Fixed by #60065
Labels
A-async-await Area: Async & Await A-doctests Area: Documentation tests, run by rustdoc AsyncAwait-Polish Async-await issues that are part of the "polish" area C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@ghost
Copy link

ghost commented Mar 20, 2019

Steps to reproduce:

  1. Create a library project with the following contents in lib.rs:
//! ```
//! #![feature(async_await)]
//!
//! fn foo() {
//!     drop(async move {});
//! }
//! ```
  1. Run tests:
repro master $ cargo --version
cargo 1.35.0-nightly (0e35bd8af 2019-03-13)
repro master $ rustc --version
rustc 1.35.0-nightly (3eb4890df 2019-03-19)
repro master $ cargo test
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
     Running target/debug/deps/repro-0e401502cf52c662

running 0 tests

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

   Doc-tests repro

running 1 test
test src/lib.rs -  (line 1) ... FAILED

failures:

failures:
    src/lib.rs -  (line 1)

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

error: test failed, to rerun pass '--doc'
  1. Now delete the move inside async move {}:
//! ```
//! #![feature(async_await)]
//!
//! fn foo() {
//!     drop(async {});
//! }
//! ```
  1. Run tests again:
repro master $ cargo test
   Compiling repro v0.1.0 (/home/stjepan/work/repro)
    Finished dev [unoptimized + debuginfo] target(s) in 0.25s
     Running target/debug/deps/repro-0e401502cf52c662

running 0 tests

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

   Doc-tests repro

running 1 test
test src/lib.rs -  (line 1) ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
@jonas-schievink jonas-schievink added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. C-bug Category: This is a bug. A-async-await Area: Async & Await A-doctests Area: Documentation tests, run by rustdoc labels Mar 20, 2019
@nikomatsakis
Copy link
Contributor

This seems to work outside of rustdoc

@nikomatsakis nikomatsakis added the AsyncAwait-Polish Async-await issues that are part of the "polish" area label Apr 16, 2019
@nikomatsakis
Copy link
Contributor

Marking as blocking for async await stabilization pending some investigation.

cc @rust-lang/rustdoc -- can you all give any tips to try and gain insight into what error is occurring etc?

@GuillaumeGomez
Copy link
Member

Can you give a try to:

//! #![feature(async_await)]
//!
//! # fn main() {
//! fn foo() {
//!     drop(async move {});
//! }
//! # foo();
//! # }

But beyond that, I'm wondering if it isn't just a missing flag or something along the line?

Maybe you have other ideas @QuietMisdreavus ?

@QuietMisdreavus
Copy link
Member

I spent some time yesterday poking at this, and it seems like it's failing somewhere in the pre-parsing of the doctest. Some time around the part where it parses the move keyword, it just... dies. I'm having trouble pinning down exactly where (and how) it fails, since it's not panicking, and setting -Z treat-err-as-bug (which i have to modify the code to do since we usually don't pass that into doctests) makes it fail at an earlier spot.

@nikomatsakis
Copy link
Contributor

I've sometimes seen problems around FatalError.raise()

@QuietMisdreavus
Copy link
Member

Funny you should say that... It looks like one of those is being hit when processing this sample:

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

It doesn't seem to hit this when compiling regularly, though.

@QuietMisdreavus
Copy link
Member

I think i've figured it out. The "pre-parse" step of doctest execution runs the parser separately from the later execution of it. In this run, the "default edition" is never set, so libsyntax assumes everything is being run under Rust 2015. This causes it to fail when parsing the async move block. What's confusing is how this has never been a problem until now... 🤔

@QuietMisdreavus
Copy link
Member

I've got a PR posted that should fix this: #60065

@Nemo157
Copy link
Member

Nemo157 commented Apr 18, 2019

Hmm, I was going to suggest that might be the issue, but I tried a test containing async { let foo = 5; } (which fails to compile on edition 2015) and that works fine (unsurprisingly since futures use a lot of doc-tests containing something similar).

@QuietMisdreavus
Copy link
Member

Plain async blocks probably pass in the current nightly because they fail after the initial parse, or perhaps they bubble up a single error from parse_item instead of emitting it directly, which causes rustdoc to stop parsing and continue creating the doctest. The difference with async move blocks is that they use a keyword that exists in 2015, which causes it to attempt parsing a closure (instead of a struct expression), so it totally fails to parse instead of just creating a real expression that would fail a later check.

bors added a commit that referenced this issue May 19, 2019
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-doctests Area: Documentation tests, run by rustdoc AsyncAwait-Polish Async-await issues that are part of the "polish" area C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants