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

Doctests in markdown should have a way to run in edition 2018 #52623

Closed
scottmcm opened this issue Jul 22, 2018 · 11 comments
Closed

Doctests in markdown should have a way to run in edition 2018 #52623

scottmcm opened this issue Jul 22, 2018 · 11 comments
Labels
A-rust-2018-preview Area: The 2018 edition preview E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@scottmcm
Copy link
Member

I was working on try{}, which is 2018-only, so needed to disable the unstable book doctest for it since I couldn't find a way to run it in 2018: https://github.com/rust-lang/rust/pull/52602/files#diff-61f80654219faf8a22cb14554f6a9ae0R12

This would presumably also be useful for the edition guide, which is currently ignoreing most of its doctests, which has let typos slip in (like rust-lang/edition-guide#30).

@Nemo157
Copy link
Member

Nemo157 commented Jul 24, 2018

Maybe related depending on how it’s implemented, some crates may want to provide examples in a different edition to what they use, so being able to override the edition on an example by example basis could be useful.

@QuietMisdreavus
Copy link
Member

I had some ideas about this, then proceeded to open a new issue for it before realizing i left this link in my notes. 🤦‍♀️ Here's what i wrote up in that other issue:


Right now, doctests will compile in the edition that the rest of the crate is set for. (Presumably, you can pass an edition for standalone markdown files, but i dunno whether mdbook will let you do that easily.) However, if you specifically want to show off how your crate interacts in various editions, there's no way to do that right now.

It would be cool if we had a way to set an edition2015/edition2018/etc flag on doctests so that we could specifically compile a doctest in a certain version. It would be even cooler if we then connected this to a little banner on the code sample, like we currently do with ignore and compile_fail.

cc @rust-lang/rustdoc

(when i get a moment, i'll set up some mentoring instructions...)

@QuietMisdreavus QuietMisdreavus added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rust-2018-preview Area: The 2018 edition preview labels Sep 5, 2018
@QuietMisdreavus
Copy link
Member

(One Month Later...)

Rustdoc parses out code block commands in src/librustdoc/html/markdown.rs, using the LangString struct. It processes the rust/ignore/no_run/etc flags in this function here. We would probably want to add in a new field to LangString called edition that uses the existing syntax::edition::Edition enum. Fortunately, there's already come code in there that breaks apart a doctest command and parses the rest: the error code command, used with the compile_fail command in the error code index. In fact, the code to parse out the edition will probably look the same, since Edition already implements FromStr.

(Since the --edition flag on rustdoc is still unstable, we may want to have it be nightly only? You can use the allow_error_code_check parameter to tell whether you're on nightly or not - true if on nightly, false if not.)

Once you have the edition loaded, that LangString gets passed down to the add_test function in src/librustdoc/test.rs. Currently, that let edition = self.edition; line just pulls the edition that the crate is currently being compiled under, but now that you have that new field, you can use that instead. (We'll probably want to fall back to the active edition in case the doctest doesn't set one.) That variable is then passed into the compiler config when building the test, so the existing code will take it from there.


And that's it! We'll probably want to add a test or two that exercises this, too. Rustdoc tests live in a few places, but we'll probably want to add one in src/test/rustdoc. To make sure it runs doctests instead of building docs, we'll want to add a test command comment compile-flags:--test, like in the edition-flag.rs test. There will probably need to be two tests: one that sets edition:2015 and uses a doctest with 2018-specific syntax, and one that sets edition:2018 and uses a 2018-specific keyword to name a variable. (At least, i think there's a new keyword we can use? Probably worth asking around to see if we can force it one way or another.)

For more information on the layout of rustdoc's code and how to build/run/test it, check out the rustdoc chapter of the Guide to Rustc Development.

@QuietMisdreavus QuietMisdreavus added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Sep 6, 2018
@Mark-Simulacrum
Copy link
Member

(Since the --edition flag on rustdoc is still unstable, we may want to have it be nightly only? You can use the allow_error_code_check parameter to tell whether you're on nightly or not - true if on nightly, false if not.)

We'll want to stabilize this shortly so we can probably not bother too much with this.

@QuietMisdreavus
Copy link
Member

We'll want to stabilize this shortly so we can probably not bother too much with this.

Aha, that makes this easier, then. It's not too much bother to add it here - the nightly check is already available - but yeah, it'll need to be stabilized soon enough.


Bonus points: I would personally like to see the edition used in our tooltips, like the ones we have for compile_fail or ignore doctests. Those are generated in src/librustdoc/html/markdown.rs, in the Iterator impl for the CodeBlocks wrapper. The compile_fail and ignore variables are already used to hold those flags, so we could easily add a new one to hold edition information too. Those variables are used to construct the tooltip farther down in the function, and just below that some CSS classes are set which colors the tooltip flag itself. There's sort of a design question of what we want to say if both compile_fail and an edition are set, though. You'll also want to make sure you add some appropriate CSS classes in the theme CSS files to match what we have for ignore and compile-fail. I would suggest coordinating with @GuillaumeGomez if you want to do this, since he did a lot of the initial work for these tooltips.

@Munksgaard
Copy link
Contributor

I'd like to take a stab at this tomorrow or the day after :)

@GuillaumeGomez
Copy link
Member

I can add the tooltip if needed.

@QuietMisdreavus
Copy link
Member

@Munksgaard Still want to work on this? Since it's an edition feature i'd like to get it in soon-ish so we can get some eyes on it.

@Munksgaard
Copy link
Contributor

@Munksgaard Still want to work on this? Since it's an edition feature i'd like to get it in soon-ish so we can get some eyes on it.

Yes, sorry I've been silent. I've got the basic functionality working, but I've still to write some tests and get the banner up and running. I plan on getting the tests finished today, and then I hope to tackle the banner tomorrow.

Should I create a PR, even though I'm not done?

You can see my first commit here: Munksgaard@b94d38b

@QuietMisdreavus
Copy link
Member

@Munksgaard No worries. If you're working on it now, then i won't push it more.

If you have trouble getting the banner together, open the PR without it and @GuillaumeGomez can put it in afterward. In fact, if you want us to take a look and provide more tips with your code, you can open the PR and put "WIP" in the title if you want us to use github's review tools.

@GuillaumeGomez
Copy link
Member

Opening a PR would actually make the review easier and allow us to remain more or less up-to-date. ;)

pietroalbini added a commit to pietroalbini/rust that referenced this issue Sep 22, 2018
…est, r=steveklabnik

Support specifying edition in doc test

Fixes rust-lang#52623

r? @QuietMisdreavus
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust-2018-preview Area: The 2018 edition preview E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. 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

6 participants