Modify doctest's auto-`fn main()` to allow `Result`s #56470

llogiq · 2018-12-03T17:25:17Z

This lets the default fn main() ~~return impl Termination~~ unwrap Results, which allows the use of ? in most tests without adding it manually. This fixes #56260

~~Blocked on std::process::Termination stabilization.~~

Using Termination would have been cleaner, but this should work OK.

rust-highfive · 2018-12-03T17:25:28Z

r? @GuillaumeGomez

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

cramertj · 2018-12-03T20:28:03Z

src/test/rustdoc/process-termination.rs

+/// Papercut: Turbofish to fully specify type
+///
+/// ```rust
+/// Result::Ok::<(), &'static str>(())


cc #56225 @alexreg @petrochenkov @eddyb I use Ok::<...> for this reason all the time (at the end of a closure, async closure, try block, etc. something that needs to an inference hint for the error type). In some of these cases it's possible to replace w/ let x: Result<_, ErrorTy> = ... but not all-- this PR is one example of where it's not possible (it is, of course, still possible to write Result::<..., ErrorTy>::Ok but that's a good bit more verbose).

I should say that I wanted to leave this comment on this issue rather than a separate issue or chat because I wanted to make sure that we avoid recommending that users use a syntax that we're not planning on supporting long-term. I personally use this syntax pretty often already and so would be upset by its removal.

Variant::<...> is not going anywhere. Enum::Variant::<...> is the mistake we made, we should've moved to Enum::<...>::Variant, and ideally base it on <Enum<...>>::Variant.
But when there's a single path component, it makes sense to stuck all the parameters on there, IMO.

Yeah, I agree, it makes sense... but only in the case of single-component paths. In any case, we're not breaking anything for now; it will all be accepted, though the plan is to lint against Enum::Variant::<...> in the future.

cramertj · 2018-12-03T20:28:24Z

src/test/rustdoc/process-termination.rs

+/// It's all good as long as both cases exist in the code:
+///
+/// ```rust,should_panic
+/// Result::Err("This is returned from `main`, leading to panic")?;


Nit: Err is in the prelude

cramertj · 2018-12-03T20:28:44Z

src/test/rustdoc/process-termination.rs

+/// Papercut: Turbofish to fully specify type
+///
+/// ```rust
+/// Result::Ok::<(), &'static str>(())


nit: Ok is in the prelude

llogiq · 2018-12-03T20:47:37Z

@cramertj Thank you for spotting this. Fixed the Result.:*s in the test.

llogiq · 2018-12-03T20:55:00Z

One problem I'm seeing here is that this doesn't have a proper feature gate: Rust tells me it's gated on termination_trait_lib, yet libsyntax/feature_gate.rs has no such definition. An error?

ollie27 · 2018-12-03T20:59:24Z

@llogiq library features aren't listed in libsyntax/feature_gate.rs, they're taken from the attributes on items:

rust/src/libstd/process.rs

Line 1586 in 0c999ed

#[unstable(feature = "termination_trait_lib", issue = "43301")]

ollie27 · 2018-12-03T21:30:39Z

src/test/rustdoc/process-termination.rs

+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+


Suggested change

// compile-flags:--test

will be needed to actually run the examples as doctests.

Cool! Thank you!

QuietMisdreavus · 2018-12-04T01:21:34Z

src/librustdoc/test.rs

@@ -481,7 +481,7 @@ pub fn make_test(s: &str,
    if dont_insert_main || already_has_main {
        prog.push_str(everything_else);
    } else {
-        prog.push_str("fn main() {\n");
+        prog.push_str("fn main() -> impl std::process::Termination {\n");


We should probably also check for #![no_std] being in the doctest, and switch to core::process as appropriate.

If the trait is in core you can just always use core, I think we want to make both of them available? cc @petrochenkov @joshtriplett @aturon

It's now using core::process::Termination.

Which unfortunately isn't there yet.

llogiq · 2018-12-05T20:15:39Z

There is one problem with nostd code: we cannot move the Termnation trait to libcore because that would pull all reporting, formatting, etc. into scope, and we want to avoid that. On the other hand, I'm not too happy with the idea of not having this on nostd code. Perhaps we can use the system panic handler (which would abort on Err) instead?

In any event, this means we'll have to distinguish nostd crates. @QuietMisdreavus any idea how to do this?

eddyb · 2018-12-07T10:31:54Z

I assumed #56470 (comment) meant core::process::Termination already existed, what gives?

QuietMisdreavus · 2018-12-07T16:12:32Z

@eddyb That was a wild assumption on my part; i didn't realize that the Termination trait was only in std.

QuietMisdreavus · 2018-12-07T16:17:48Z

@llogiq Any top-level crate attributes in the doctest will already be set in the prog string right before your change in test.rs, so you may be able to read through that to look for a #![no_std] attribute.

llogiq · 2018-12-07T20:35:48Z

The problem with a nostd Termination trait is that the current implementation formats any Err value it encounters, thus pulling in formatting logic, which we want to avoid on nostd. So we'd need an easier implementation in libcore.

llogiq · 2018-12-10T08:27:16Z

@QuietMisdreavus do you mean prog.contains("#[no_std]")? Does that also work with #[cfg_attr(..)]?

QuietMisdreavus · 2018-12-10T14:59:44Z

@llogiq Sadly, it won't catch that, so if we want to get fancier we'll need to run a similar parsing loop over the attributes.

(On the other hand, having a #![cfg_attr] in a doctest isn't that useful, since --cfg flags aren't given to doctests, but if someone's showing it off, it's probably worth accommodating.)

llogiq · 2018-12-10T21:16:33Z

Ok, so for starters, prog.contains("#[no_std]") || prog.contains("#![no_std]") should suffice. We can go back and add some syntactic parsing later.

… r=GuillaumeGomez Modify doctest's auto-`fn main()` to allow `Result`s This lets the default `fn main()` ~~return `impl Termination`~~ unwrap Results, which allows the use of `?` in most tests without adding it manually. This fixes rust-lang#56260 ~~Blocked on `std::process::Termination` stabilization.~~ Using `Termination` would have been cleaner, but this should work OK.

scottmcm · 2019-02-18T10:01:29Z

src/test/rustdoc/process-termination.rs

+/// You can err with anything that implements `Debug`:
+///
+/// ```rust,should_panic
+/// Err("This is returned from `main`, leading to panic")?;


I find this use of should_panic confusing, since that's not how #[test]s work: #49909 #48854 (comment) #49911

I think that Err paths should look for Errs directly, and that should_panic tests should only pass if the code block raised a panic.

Ok, but how should I test this particular function then? I could replace the should_panic with a no_run, but that would fail to catch a change that swallows the panic.

… r=GuillaumeGomez Modify doctest's auto-`fn main()` to allow `Result`s This lets the default `fn main()` ~~return `impl Termination`~~ unwrap Results, which allows the use of `?` in most tests without adding it manually. This fixes rust-lang#56260 ~~Blocked on `std::process::Termination` stabilization.~~ Using `Termination` would have been cleaner, but this should work OK.

bors · 2019-02-19T06:44:24Z

⌛ Testing commit dad211e with merge e5e874dd68f1e3651902f4ff5660e7e9c10e5e25...

bors · 2019-02-19T07:20:10Z

💔 Test failed - checks-travis

rust-highfive · 2019-02-19T07:20:11Z

The job test-various 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.

[00:01:43]    Compiling getopts v0.2.17
[00:01:43]    Compiling lazy_static v0.2.11
[00:01:43]    Compiling build_helper v0.1.0 (/checkout/src/build_helper)
[00:01:45]    Compiling petgraph v0.4.13
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

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)

kennytm · 2019-02-19T16:52:45Z

@bors retry

… r=GuillaumeGomez Modify doctest's auto-`fn main()` to allow `Result`s This lets the default `fn main()` ~~return `impl Termination`~~ unwrap Results, which allows the use of `?` in most tests without adding it manually. This fixes rust-lang#56260 ~~Blocked on `std::process::Termination` stabilization.~~ Using `Termination` would have been cleaner, but this should work OK.

Rollup of 24 pull requests Successful merges: - #56470 (Modify doctest's auto-`fn main()` to allow `Result`s) - #58044 (Make overflowing and wrapping negation const) - #58303 (Improve stability tags display) - #58336 (Fix search results interactions) - #58384 (Fix tables display) - #58392 (Use less explicit shifting in std::net::ip) - #58409 (rustdoc: respect alternate flag when formatting impl trait) - #58456 (Remove no longer accurate diagnostic code about NLL) - #58528 (Don't use an allocation for ItemId in StmtKind) - #58530 (Monomorphize less code in fs::{read|write}) - #58534 (Mention capping forbid lints) - #58536 (Remove UB in pointer tests) - #58538 (Add missing fmt structs examples) - #58539 (Add alias methods to PathBuf for underlying OsString (#58234)) - #58544 (Fix doc for rustc "-g" flag) - #58545 (Add regression test for a specialization-related ICE (#39448)) - #58546 (librustc_codegen_llvm => 2018) - #58551 (Explain a panic in test case net::tcp::tests::double_bind) - #58553 (Use more impl header lifetime elision) - #58562 (Fix style nits) - #58565 (Fix typo in std::future::Future docs) - #58568 (Fix a transposition in driver.rs.) - #58569 (Reduce Some Code Repetitions like `(n << amt) >> amt`) - #58576 (Stabilize iter::successors and iter::from_fn)

llogiq · 2019-02-20T13:11:32Z

🎉

ghost · 2019-02-20T16:11:21Z

src/doc/rustdoc/src/documentation-tests.md

+/// use std::io;
+/// let mut input = String::new();
+/// io::stdin().read_line(&mut input)?;
+/// # Ok::<(), io:Error>(())


Typo: io:Error should be io::Error :)

Good catch. WIll fix in a follow-up.

Pkgsrc changes: * Bump required rust version to build to 1.33.0. * Adapt patches to changed file locations. * (I worry about 32-bit ports, now that Atomic64 apparently is First-Class; this has been built on NetBSD/amd64 so far) Upstream changes: Version 1.34.0 (2019-04-11) ========================== Language -------- - [You can now use `#[deprecated = "reason"]`][58166] as a shorthand for `#[deprecated(note = "reason")]`. This was previously allowed by mistake but had no effect. - [You can now accept token streams in `#[attr()]`,`#[attr[]]`, and `#[attr{}]` procedural macros.][57367] - [You can now write `extern crate self as foo;`][57407] to import your crate's root into the extern prelude. Compiler -------- - [You can now target `riscv64imac-unknown-none-elf` and `riscv64gc-unknown-none-elf`.][58406] - [You can now enable linker plugin LTO optimisations with `-C linker-plugin-lto`.][58057] This allows rustc to compile your Rust code into LLVM bitcode allowing LLVM to perform LTO optimisations across C/C++ FFI boundaries. - [You can now target `powerpc64-unknown-freebsd`.][57809] Libraries --------- - [The trait bounds have been removed on some of `HashMap<K, V, S>`'s and `HashSet<T, S>`'s basic methods.][58370] Most notably you no longer require the `Hash` trait to create an iterator. - [The `Ord` trait bounds have been removed on some of `BinaryHeap<T>`'s basic methods.][58421] Most notably you no longer require the `Ord` trait to create an iterator. - [The methods `overflowing_neg` and `wrapping_neg` are now `const` functions for all numeric types.][58044] - [Indexing a `str` is now generic over all types that implement `SliceIndex<str>`.][57604] - [`str::trim`, `str::trim_matches`, `str::trim_{start, end}`, and `str::trim_{start, end}_matches` are now `#[must_use]`][57106] and will produce a warning if their returning type is unused. - [The methods `checked_pow`, `saturating_pow`, `wrapping_pow`, and `overflowing_pow` are now available for all numeric types.][57873] These are equivalvent to methods such as `wrapping_add` for the `pow` operation. Stabilized APIs --------------- #### std & core * [`Any::type_id`] * [`Error::type_id`] * [`atomic::AtomicI16`] * [`atomic::AtomicI32`] * [`atomic::AtomicI64`] * [`atomic::AtomicI8`] * [`atomic::AtomicU16`] * [`atomic::AtomicU32`] * [`atomic::AtomicU64`] * [`atomic::AtomicU8`] * [`convert::Infallible`] * [`convert::TryFrom`] * [`convert::TryInto`] * [`iter::from_fn`] * [`iter::successors`] * [`num::NonZeroI128`] * [`num::NonZeroI16`] * [`num::NonZeroI32`] * [`num::NonZeroI64`] * [`num::NonZeroI8`] * [`num::NonZeroIsize`] * [`slice::sort_by_cached_key`] * [`str::escape_debug`] * [`str::escape_default`] * [`str::escape_unicode`] * [`str::split_ascii_whitespace`] #### std * [`Instant::checked_add`] * [`Instant::checked_sub`] * [`SystemTime::checked_add`] * [`SystemTime::checked_sub`] Cargo ----- - [You can now use alternative registries to crates.io.][cargo/6654] Misc ---- - [You can now use the `?` operator in your documentation tests without manually adding `fn main() -> Result<(), _> {}`.][56470] Compatibility Notes ------------------- - [`Command::before_exec` is now deprecated in favor of the unsafe method `Command::pre_exec`.][58059] - [Use of `ATOMIC_{BOOL, ISIZE, USIZE}_INIT` is now deprecated.][57425] As you can now use `const` functions in `static` variables. [58370]: rust-lang/rust#58370 [58406]: rust-lang/rust#58406 [58421]: rust-lang/rust#58421 [58166]: rust-lang/rust#58166 [58044]: rust-lang/rust#58044 [58057]: rust-lang/rust#58057 [58059]: rust-lang/rust#58059 [57809]: rust-lang/rust#57809 [57873]: rust-lang/rust#57873 [57604]: rust-lang/rust#57604 [57367]: rust-lang/rust#57367 [57407]: rust-lang/rust#57407 [57425]: rust-lang/rust#57425 [57106]: rust-lang/rust#57106 [56470]: rust-lang/rust#56470 [cargo/6654]: rust-lang/cargo#6654 [`Any::type_id`]: https://doc.rust-lang.org/std/any/trait.Any.html#tymethod.type_id [`Error::type_id`]: https://doc.rust-lang.org/std/error/trait.Error.html#tymethod.type_id [`atomic::AtomicI16`]: https://doc.rust-lang.org/std/atomic/struct.AtomicI16.html [`atomic::AtomicI32`]: https://doc.rust-lang.org/std/atomic/struct.AtomicI32.html [`atomic::AtomicI64`]: https://doc.rust-lang.org/std/atomic/struct.AtomicI64.html [`atomic::AtomicI8`]: https://doc.rust-lang.org/std/atomic/struct.AtomicI8.html [`atomic::AtomicU16`]: https://doc.rust-lang.org/std/atomic/struct.AtomicU16.html [`atomic::AtomicU32`]: https://doc.rust-lang.org/std/atomic/struct.AtomicU32.html [`atomic::AtomicU64`]: https://doc.rust-lang.org/std/atomic/struct.AtomicU64.html [`atomic::AtomicU8`]: https://doc.rust-lang.org/std/atomic/struct.AtomicU8.html [`convert::Infallible`]: https://doc.rust-lang.org/std/convert/enum.Infallible.html [`convert::TryFrom`]: https://doc.rust-lang.org/std/convert/trait.TryFrom.html [`convert::TryInto`]: https://doc.rust-lang.org/std/convert/trait.TryInto.html [`iter::from_fn`]: https://doc.rust-lang.org/std/iter/fn.from_fn.html [`iter::successors`]: https://doc.rust-lang.org/std/iter/fn.successors.html [`num::NonZeroI128`]: https://doc.rust-lang.org/std/num/struct.NonZeroI128.html [`num::NonZeroI16`]: https://doc.rust-lang.org/std/num/struct.NonZeroI16.html [`num::NonZeroI32`]: https://doc.rust-lang.org/std/num/struct.NonZeroI32.html [`num::NonZeroI64`]: https://doc.rust-lang.org/std/num/struct.NonZeroI64.html [`num::NonZeroI8`]: https://doc.rust-lang.org/std/num/struct.NonZeroI8.html [`num::NonZeroIsize`]: https://doc.rust-lang.org/std/num/struct.NonZeroIsize.html [`slice::sort_by_cached_key`]: https://doc.rust-lang.org/std/slice/fn.sort_by_cached_key [`str::escape_debug`]: https://doc.rust-lang.org/std/primitive.str.html#method.escape_debug [`str::escape_default`]: https://doc.rust-lang.org/std/primitive.str.html#method.escape_default [`str::escape_unicode`]: https://doc.rust-lang.org/std/primitive.str.html#method.escape_unicode [`str::split_ascii_whitespace`]: https://doc.rust-lang.org/std/primitive.str.html#method.split_ascii_whitespace [`Instant::checked_add`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.checked_add [`Instant::checked_sub`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.checked_sub [`SystemTime::checked_add`]: https://doc.rust-lang.org/std/time/struct.SystemTime.html#method.checked_add [`SystemTime::checked_sub`]: https://doc.rust-lang.org/std/time/struct.SystemTime.html#method.checked_sub

rust-highfive assigned GuillaumeGomez Dec 3, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 3, 2018

This comment has been minimized.

Sign in to view

cramertj reviewed Dec 3, 2018

View reviewed changes

llogiq force-pushed the process-termination-doctest branch from 7486014 to 07b4f34 Compare December 3, 2018 20:46

ollie27 reviewed Dec 3, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

llogiq force-pushed the process-termination-doctest branch from 07b4f34 to 6a95bc2 Compare December 3, 2018 22:37

This comment has been minimized.

Sign in to view

QuietMisdreavus mentioned this pull request Dec 4, 2018

Add implicit result return for rustdoc main function #56310

Closed

QuietMisdreavus reviewed Dec 4, 2018

View reviewed changes

llogiq force-pushed the process-termination-doctest branch from 6a95bc2 to d291327 Compare December 4, 2018 11:15

This comment has been minimized.

Sign in to view

llogiq force-pushed the process-termination-doctest branch from d291327 to af624c3 Compare December 10, 2018 08:44

This comment has been minimized.

Sign in to view

llogiq mentioned this pull request Dec 13, 2018

Stabilize termination_trait_lib #56791

Closed

GuillaumeGomez mentioned this pull request Feb 17, 2019

Rollup of 7 pull requests #58550

Closed

Centril added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Feb 18, 2019

scottmcm reviewed Feb 18, 2019

View reviewed changes

Mark-Simulacrum mentioned this pull request Feb 18, 2019

Rollup of 16 pull requests #58566

Closed

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 19, 2019

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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2019

kennytm mentioned this pull request Feb 19, 2019

Rollup of 24 pull requests #58578

Merged

bors merged commit dad211e into rust-lang:master Feb 20, 2019

llogiq deleted the process-termination-doctest branch February 20, 2019 13:10

ghost reviewed Feb 20, 2019

View reviewed changes

kennytm mentioned this pull request Feb 22, 2019

Spurious 30 minutes timeout on Travis CI #56112

Closed

Centril added this to the 1.34 milestone Apr 26, 2019

Modify doctest's auto-fn main() to allow Results #56470

Modify doctest's auto-fn main() to allow Results #56470

Conversation

llogiq commented Dec 3, 2018 • edited Loading

rust-highfive commented Dec 3, 2018

This comment has been minimized.

Choose a reason for hiding this comment

cramertj Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogiq commented Dec 3, 2018

llogiq commented Dec 3, 2018

ollie27 commented Dec 3, 2018

ollie27 Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

eddyb Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

llogiq commented Dec 5, 2018 • edited Loading

eddyb commented Dec 7, 2018

QuietMisdreavus commented Dec 7, 2018

QuietMisdreavus commented Dec 7, 2018

llogiq commented Dec 7, 2018

llogiq commented Dec 10, 2018

This comment has been minimized.

QuietMisdreavus commented Dec 10, 2018

llogiq commented Dec 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 19, 2019

bors commented Feb 19, 2019

rust-highfive commented Feb 19, 2019

kennytm commented Feb 19, 2019

llogiq commented Feb 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Modify doctest's auto-`fn main()` to allow `Result`s #56470

Modify doctest's auto-`fn main()` to allow `Result`s #56470

llogiq commented Dec 3, 2018 •

edited

Loading

cramertj Dec 3, 2018 •

edited

Loading

ollie27 Dec 3, 2018 •

edited

Loading

eddyb Dec 4, 2018 •

edited

Loading

llogiq commented Dec 5, 2018 •

edited

Loading