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

add no_compile doctest attribute #96573

Closed
wants to merge 3 commits into from

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Apr 30, 2022

This makes it possible to write a doctest

```no_compile
my_fancy_macro!(12);
```

which is detected as and highlighted as rust code, but is not compiled. ignore kind of serves this purpose, but is still compiled and run when --include-ignored or --ignored is used.

Maybe should be feature gated / limited to working on the nightly channel (but I personally think this is simple and obviously correct enough that it could potentially just ride the release train).

Fixes #63193, Fixes (kinda) #87586

Big Picture View

https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/Cleaning.20up.20docblock.20attributes

We currently have

doctest attribute test attribute compile run
ignore yes with --ignored
no_run yes no
ignore with --ignored with --ignored
compile_fail fail
cfg(any()) no

I want to clean up this to be

doctest attribute test attribute compile run
ignore ignore yes with --ignored
no_run yes no
compile_fail fail
no_compile cfg(any()) no

This does the simple part of adding no_compile. After no_compile is available, it becomes possible to tell current uses of ignore for does-not-compile rust code to use no_compile instead, and move towards ignore always compiling the code in the next edition.

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Apr 30, 2022
@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 30, 2022
@CAD97 CAD97 changed the title add no_compile doctest attribute add no_compile doctest attribute Apr 30, 2022
@jyn514
Copy link
Member

jyn514 commented Apr 30, 2022

The test for this would live in src/test/rustdoc-ui.

@CAD97
Copy link
Contributor Author

CAD97 commented Apr 30, 2022

UI test has been added.

@rust-log-analyzer

This comment has been minimized.

@CAD97
Copy link
Contributor Author

CAD97 commented Apr 30, 2022

Okay tidy, I'm trying to unblock stopping using ignore for no-compile examples (so maybe the lint will have to cover no_compile as well)

@CAD97
Copy link
Contributor Author

CAD97 commented Apr 30, 2022

Some interesting stats on ```ignore (reason):

  • 13× only-for-syntax-highlighting or syntax-highlighting-only (obvious no_compile candidate)
  • 10× pseudo-rust (obvious no_compile candidate)
  • 3× illustrative (obvious no_compile candidate)
  • 5× incomplete (obvious no_compile candidate)
  • 6× hypothetical (obvious no_compile candidate)
  • 2× exposition (obvious no_compile candidate... or just actually compile)
  • 1× pacify the merciless tidy (should be no_compile)
  • 2× not-usage-example (maybe should be ignore, maybe should just be tested)
  • 1× ignore future compatibility warning
  • 9× needs multiple files/crates
  • 8× no longer emitted error code
  • 2× feature got removed
  • 5× platform dependent
  • 1× requries external plugin
  • 2× depends on release channel
  • 1× can't specify compiler flags
  • 1× needs dependency
  • 12× cannot test private items
  • 6× diagnostic (should be text imho)
  • 10× extern declaration (should maybe be no_run?)
  • 14× ```ignore examples/tests

(I definitely missed a some, I got to clippy in the github.dev search and bailed)

@GuillaumeGomez
Copy link
Member

I'm not sure to understand the use case here. It means you'd want to have a rust code example that can never be checked. It's a strange use case. Why do you need this?

Also, just like for ignore, it should display the i alongside the code block in the generated documentation.

@CAD97
Copy link
Contributor Author

CAD97 commented May 2, 2022

I'm not sure to understand the use case here. [...]

So, fundamentally, I believe the current usage of ```ignore is wrong.

#[ignore] — Indicates that the test function will be compiled, but not run by default [emphasis mine]. See the --ignored and --include-ignored options to run these tests. [rustc book §6 Tests, Test attributes]

#[ignore], and thus by extension ```ignore, should be used for tests which should always be compiled but only ran when opted into with --ignored or --include-ignored; flaky, long-running, or test otherwise undesirable to always be running, but which should be able to be run.

This is demonstrably not how ```ignore is used in practice. In practice, ```ignore is used because there is no other way to get Rust syntax highlighting in rustdoc without trying to compile the example.

Examples in this repo

Some of these should maybe be made to compile, but most want no_compile semantics, where they aren't tested at all.

/// ```ignore (only-for-syntax-highlight)
/// <Vec<T> as a::b::Trait>::AssociatedItem
/// ^~~~~ ~~~~~~~~~~~~~~^
/// ty position = 3
///
/// <Vec<T>>::AssociatedItem
/// ^~~~~ ^
/// ty position = 0
/// ```

/// ```ignore (only-for-syntax-highlight)
/// trait Foo {
/// type Assoc<'a, 'b> where Self: 'a, Self: 'b;
/// }
/// impl Foo for () {
/// type Assoc<'a, 'b> where Self: 'a = () where Self: 'b;
/// // ^^^^^^^^^^^^^^ first where clause
/// // ^^^^^^^^^^^^^^ second where clause
/// }
/// ```

/// ```ignore (only-for-syntax-highlight)
/// struct Struct<'a, ..., 'z, A, B: DeclaredTrait, C, ..., Z> where C: WhereTrait {
/// a: A,
/// b: B::Item,
/// b1: <B as DeclaredTrait>::Item,
/// c1: <C as WhereTrait>::Item,
/// c2: Option<<C as WhereTrait>::Item>,
/// ...
/// }
/// ```

/// ```ignore (only-for-syntax-highlight)
/// impl<'a, ..., 'z, A, B: DeclaredTrait, C, ... Z> where
/// C: WhereTrait,
/// A: DerivedTrait + B1 + ... + BN,
/// B: DerivedTrait + B1 + ... + BN,
/// C: DerivedTrait + B1 + ... + BN,
/// B::Item: DerivedTrait + B1 + ... + BN,
/// <C as WhereTrait>::Item: DerivedTrait + B1 + ... + BN,
/// ...
/// {
/// ...
/// }
/// ```

/// ```ignore (pseudo-Rust)
/// async fn foo(<pattern> @ x: Type) {
/// async move {
/// let <pattern> = x;
/// }
/// }
/// ```

/// ```ignore (illustrative)
/// ctor
/// .ctor_hir_id()
/// .and_then(|ctor_id| tcx.hir().find(tcx.hir().get_parent_node(ctor_id)))
/// .and_then(|parent| parent.ident())
/// ```

/// ```ignore (pseudo-rust)
/// # extern crate rustc_errors;
/// # use rustc_errors::Applicability;
/// # extern crate rustc_span;
/// # use rustc_span::{symbol::Ident, Span};
/// # extern crate rust_middle;
/// # use rustc_middle::ty::Ty;
/// #[derive(SessionDiagnostic)]
/// #[error(code = "E0505", slug = "borrowck-move-out-of-borrow")]
/// pub struct MoveOutOfBorrowError<'tcx> {
/// pub name: Ident,
/// pub ty: Ty<'tcx>,
/// #[primary_span]
/// #[label]
/// pub span: Span,
/// #[label = "first-borrow-label"]
/// pub first_borrow_span: Span,
/// #[suggestion(code = "{name}.clone()")]
/// pub clone_sugg: Option<(Span, Applicability)>
/// }
/// ```

/// ```ignore (pseudo-rust)
/// sess.emit_err(MoveOutOfBorrowError {
/// expected,
/// actual,
/// span,
/// first_borrow_span,
/// clone_sugg: Some(suggestion, Applicability::MachineApplicable),
/// });
/// ```

/// ```ignore (pseudo-rust)
/// #[derive(SessionSubdiagnostic)]
/// pub enum ExpectedIdentifierLabel<'tcx> {
/// #[label(slug = "parser-expected-identifier")]
/// WithoutFound {
/// #[primary_span]
/// span: Span,
/// }
/// #[label(slug = "parser-expected-identifier-found")]
/// WithFound {
/// #[primary_span]
/// span: Span,
/// found: String,
/// }
/// }
///
/// #[derive(SessionSubdiagnostic)]
/// #[suggestion_verbose(slug = "parser-raw-identifier")]
/// pub struct RawIdentifierSuggestion<'tcx> {
/// #[primary_span]
/// span: Span,
/// #[applicability]
/// applicability: Applicability,
/// ident: Ident,
/// }
/// ```

/// ```ignore (pseudo-rust)
/// diag.subdiagnostic(ExpectedIdentifierLabel::WithoutFound { span });
///
/// diag.subdiagnostic(RawIdentifierSuggestion { span, applicability, ident });
/// ```

/// ```ignore (not-usage-example)
/// /// Suggest `==` when users wrote `===`.
/// #[suggestion(slug = "parser-not-javascript-eq", code = "{lhs} == {rhs}")]
/// struct NotJavaScriptEq {
/// #[primary_span]
/// span: Span,
/// lhs: Ident,
/// rhs: Ident,
/// }
/// ```

/// ```ignore (not-usage-example)
/// format!("{lhs} == {rhs}", lhs = self.lhs, rhs = self.rhs)
/// ```

/// ```ignore (incomplete)
/// let a = foo::<7>();
/// // ^ Calling `opt_const_param_of` for this argument,
///
/// fn foo<const N: usize>()
/// // ^ returns this `DefId`.
///
/// fn bar() {
/// // ^ While calling `opt_const_param_of` for other bodies returns `None`.
/// }
/// ```

/// ```ignore (incomplete)
/// type X: Bound + 'lt
/// // ^^^^^^^^^^^
/// impl Debug + Display
/// // ^^^^^^^^^^^^^^^
/// ```

/// ```ignore (pseudo-Rust)
/// async move {
/// let x: T = expr;
/// foo.await
/// ...
/// }
/// ```

/// ```ignore (incomplete snippet)
/// type Foo = impl Baz;
/// fn bar() -> Foo {
/// // ^^^ This is the span we are looking for!
/// }
/// ```

... you get the point, just do a code search for ```ignore.

The use of ```ignore for this purpose breaks --[include-]ignored, as now the flag includes testing of documentation blocks which aren't intended to be compiled/tested. Ultimately, I would like ```ignore in edition 2024+ to behave more like #[ignore] — always checked to compile, tested with the --[include-]ignored flag. Doing so requires providing a mechanism to get the behavior people are using ```ignore for today, though — applying Rust syntax highlighting in the rendered documentation, but never tying to compile/test the code block. That's what no_compile is.

The behavior I would like to see is that in edition 2024 ```ignore is by default checked to compile, and if an ```ignore documentation block fails to compile, a "use compile_fail if the snippet is an example of code which fails to compile, or no_compile if the snippet should not be tested" hint is provided.

Flag for stop interpreting code blocks as doctests #63193, No way to run no_run doctests #82715, and Ignored doctests are run with cargo test -- --ignored. #87586 contain more discussion of motivation.

Also, just like for ignore, it should display the i alongside the code block in the generated documentation.

Agreed, I'm going to look into fixing that this week.

@GuillaumeGomez
Copy link
Member

Ok, now I understand the problem. It seems super weird that ignore tests are compiled. Let's take this example:

/// ```ignore
/// kawakunga
/// ```
pub use ::std as x;

If I run rustdoc, nothing is compiled as far as I can see and if I run rustdoc --test, I get:

running 1 test
test foo.rs - x (line 1) ... ignored

test result: ok. 0 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in 0.00s

So maybe the documentation itself is wrong? Needs to be checked more in detail.

@ChrisDenton
Copy link
Member

The docs aren't super clear but for rustdoc I believe you need something like this to run ignored tests:

rustdoc library.rs --test --test-args=--ignored

@GuillaumeGomez
Copy link
Member

Some clarification would be appreciated... So in any case, with this in mind, I don't see why no_compile would be needed.

@CAD97
Copy link
Contributor Author

CAD97 commented May 2, 2022

@ChrisDenton is correct. With cargo, the command is cargo test -- --ignored.

The current status of things is explained by the following table:

doctest attribute test attribute compile run
ignore yes with --ignored
no_run yes no
ignore with --ignored with --ignored
compile_fail fail
cfg(any()) no

The point of #[ignore] is that the tests are ignored by default, but can be run with an opt in with --ignored. With current ```ignore, the tests are treated as #[test] #[ignore] fn rustdoctest() { compile(test).run() }, meaning that the documentation block is compiled and run when --ignored is provided.

no_compile's purpose is to fill in that bottom left cell:

doctest attribute test attribute compile run
ignore yes with --ignored
no_run yes no
ignore with --ignored with --ignored
compile_fail fail
no_compile cfg(any()) no

giving an option for documentation blocks which should be highlighted as Rust code in the documentation but should not be considered tests — they should not be tested with rustdoc --test --test-args=--ignored, and they should not show up in the listing of tests at all when running rustdoc --test.

Looking further forward

In edition 2024, I would like to change the table to

doctest attribute test attribute compile run
ignore ignore yes with --ignored
no_run yes no
compile_fail fail
no_compile cfg(any()) no

to improve the experience of passing --ignored to the test suite. This is a long-term goal and a much harder sell than no_compile (basically requiring touching every ```ignore when upgrading the edition), but I believe this to be a much cleaner state of things than where ```ignore tests are only checked to compile when explicitly asking to run ignored tests.

A different table that's fully backwards compatible would be

doctest attribute test attribute compile run
bikeshed ignore yes with --ignored
no_run yes no
compile_fail fail
ignore cfg(any()) no

instead making ```ignore never compile/run the tests nor show up in the test listing, and adding a new attribute to get the behavior of #[ignore] in normal tests. I personally think that making ```ignore and #[ignore] behave differently is a footgun that can and should be avoided if possible, but accept that this is also a viable state to land in.

@CAD97
Copy link
Contributor Author

CAD97 commented May 2, 2022

See also some discussion in zulip

@GuillaumeGomez
Copy link
Member

Ok, now I see the problem. I'm not used at using cargo with rustdoc enough... It's strange that they pass this option by default. I mean: the whole point of ignore is that you have rust highlighting without running the compiler on it unless you pass the option.

Anyway, what do you think about this @rust-lang/rustdoc ?

@CAD97
Copy link
Contributor Author

CAD97 commented May 2, 2022

It's strange that they pass this option by default.

To be clear, the default is to ignore the tests:

cargo test calls rustdoc --test
cargo test -- --ignored calls rustdoc --test --test-args=--ignored

@GuillaumeGomez
Copy link
Member

Then again I don't see the need for no_compile. If you want to test that some ignore blocks are compiling, why not checking it all the test with no_run? If you don't want that some code examples to be compiled, why running --ignored?

@ChrisDenton
Copy link
Member

Because you do want to run ignored tests in general. They may all be too slow or too janky to run by default but you do want some way to run them as a group. They're opt-in when you know what you're doing.

But there are also some code in doc comments which you never ever ever want to run. Because it's not really rust code (but you want the same syntax highlighting) or it's just for illustrative purposes or whatever.

The first group you definitely do want to run at some point. The second group you don't want to be compiled ever at all. These are both currently using the same ignore attribute, which can be run.

@CAD97
Copy link
Contributor Author

CAD97 commented May 2, 2022

The point is that running --ignored is useful, e.g. #82715:

It follows from ["examples such as 'Here's how to retrieve a web page,' which you would want to ensure compiles, but might be run in a test environment that has no network access"] that in some cases you do want to run doctests marked as no_run, such as if the test environment does have network access. This is possible with regular tests marked #[ignore] by passing -- --ignored to cargo test, but the same does not work with [no_run] documentation tests.

Similarly, there's desire to suppress the ignored message for non-test Rust documentation blocks, e.g. #63193:

Code blocks inside documentation are interpreted as doctests by default. Sometimes the code is not for testing at all (for example, writing pseudo-code for an algorithm), and we can add ignore at the beginning of the code block.

However, even if ignore is added, cargo test [equiv. rustdoc --test] still interprets the code block as doctest, and prints "... ignored" on each occurrence of ignore-tagged code blocks. In fact, those code blocks should not be interpreted as doctest, and cargo test [equiv. rustdoc --test] should silently ignore those code blocks.

It's my position that such uses should use no_compile.

It's pretty frustrating to have to choose between syntax highlighting and "this code is never to be compiled or tested regardless of flags" -- would be really nice to have a simple way to handle this seemingly common case [https://github.com//issues/63193#issuecomment-1007624653]

I'll also quote @thomcc's motivation from #87586:

If you use rust,ignore to ignore doctests which don't compile (often because you want to include example code that's entirely expository in your documentation — something that doesn't compile at all, and should merely syntax highlight as Rust), they still are attempted to compile and run when you pass --ignored to the test runner.

This is unfortunate, as this is what you use to run #[ignore]d tests, which are good (intended?) for tests that are too slow to complete in normal execution, often ones which are exhaustive. Having doctests that fail to compile be mixed in here means this use of #[ignore] is effectively broken [emphasis mine].

@jonhoo @jyn514 it'd be helpful if you could explain your position here as well that you alluded to in #82715. cc also @thomcc from #87586

@jyn514
Copy link
Member

jyn514 commented May 2, 2022

I don't want to be involved in this, sorry.

@Manishearth
Copy link
Member

So I'm in favor of this in general, but before we go forward with this can we have a doc or something talking about the different options that currently exist (there's compile_fail, ignore, text, no_run, and now no_compile) and what they each mean and how they relate to each other.

The reason I say this is that this space is kinda complicated already, and I would rather not have people confused about which option to pick. If we can pick a name that's clear and document it well, I'd be in favor of this.

I am a little skeptical about --ignore being used at all in doctests. I do see the use case presented, and I kinda wonder if it's better served by a cfg or something. I'm not sure, I find the concept of an ignorable-ignore to be kinda iffy. But we already have that, so I can't complain too much.

A pretty major use case for non-compiling code is writing snippets, rustdoc does let you "fill in" the snippets with # but this can quickly make a small doc section giant, and is a pain to keep in sync. Sometimes crates are complicated and need a lot of scaffolding for something to actually compile.

Fundamentally the distinction being drawn here seems to be "this will not compile, this will never compile, never try" (no_compile) vs "this could compile based on the environment, don't try by default" (ignore) vs "this will not compile, and the fact that it doesn't compile is relevant" (compile_fail)

@notriddle
Copy link
Contributor

So I'm in favor of this in general, but before we go forward with this can we have a doc or something talking about the different options that currently exist

I'm pretty sure that belongs here, in the rustdoc book.

@CAD97
Copy link
Contributor Author

CAD97 commented May 4, 2022

Here's a draft of a replacement for the Attributes section of the documentation tests part of the rustdoc book, both one for clarity on the current situation and one for my "ideal future":

Today

Code blocks can be annotated with attributes that help rustdoc do the right thing when testing your code.

compile_fail tells rustdoc that the example should fail to compile. If compilation succeeds, then the test will fail. However, please note that code which fails to compile with the current Rust release may be made to work in a future release, as new features are added.

/// ```compile_fail
/// let x = 5;
/// x += 2; // shouldn't compile!
/// ```

no_run tells rustdoc that the example should compile, but to not run the code. This is useful for examples which cannot be made to run under the test fixture, would fail to terminate, or which would be Undefined Behavior if run.

/// ```no_run
/// loop {
///     println!("Hello, world");
/// }
/// ```

should_panic tells rustdoc that the example should compile, but panic during execution. If the code fails to panic, the test will fail.

/// ```should_panic
/// assert!(false);
/// ```

ignore tells the test runner to ignore the doctest by default, unless specifically opting in to running ignored tests with the --ignored or --include-ignored flags (used as cargo test -- --ignored or rustdoc --test-args=--ignored). This is useful for examples such as "here's how to retrieve a web page" which use external resources such as the network, as the test environment may lack such resources.

If the test is ignored by the test runner, rustdoc does not attempt to compile the example. As such, a predominant current use of ignore is for code which should be highlighted as Rust code, but which shouldn't be interpreted as a documentation test. This is rarely what you actually want, as the code is tested when the correct flags are used, and it's better to mark examples as text if they are not Rust code or no_compile if they deliberately fail to compile.

/// ```ignore
/// fn foo() {
/// ```

edition2015, edition2018, and edition2021 tell rustdoc that the code sample should be compiled using the specified edition of Rust. When running rustdoc through cargo, the package's edition is used by default.

/// Only compiles in the 2015 edition.
///
/// ```edition2015
/// try!(Result::<(), std::io::Error>::Ok(()));
/// Ok::<(), std::io::Error>(())
/// ```
Future

(replacing the previous, starting at ignore)

ignore tells rustdoc that this example should be interpreted as a test, but ignored by default, as if you had marked the test with #[ignore]. This is useful for examples like "here's how to retrieve a web page" which use external resources such as the network, but the test environment may lack. Even when not run, the test is still checked to compile.

/// This test requires user interaction.
///
/// ```ignore
/// assert!(show_yes_no_dialog("Should this test succeed?"));
/// ```

no_compile tells rustdoc that this example is not a test and should not be compiled. If the example isn't Rust code, it should be marked as text instead. However, no_compile is useful when the example should still be highlighted as Rust code, such as expository pseudo-Rust which can be syntax highlighted but shouldn't be treated as a test.

/// The syntax of a function declaration is
///
/// ```no_compile
/// $vis $(unsafe)? $(extern $($literal)?)? fn $ident( $( $pat : $ty ),* $(,)? ) $(-> $ty)? {
///     $($stmt)*
///     $($expr)?
/// }
/// ```

(return to the previous, starting at edition2015)

@bors
Copy link
Contributor

bors commented May 21, 2022

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

@camelid
Copy link
Member

camelid commented Sep 14, 2022

@rfcbot reviewed

This addition seems good to me. I hadn't realized that ignore was so strange in rustdoc, so it seems good to have an option with clearer semantics.

@Manishearth it looks like rfcbot didn't recognize resolved as a command.

@notriddle
Copy link
Contributor

@rfcbot resolved stablization-report

@Manishearth
Copy link
Member

@rfcbot resolved feature-gate

@camelid
Copy link
Member

camelid commented Sep 14, 2022

Weird... the feature-gate concern is still there, though the other one's resolution worked.

@notriddle
Copy link
Contributor

@rfcbot resolved feature-gate

@camelid
Copy link
Member

camelid commented Sep 14, 2022

I think this might be an rfcbot glitch. Asked on Zulip.

@notriddle
Copy link
Contributor

@rfcbot resolved feature-gate We should have a feature gate for this attribute

@xxchan
Copy link
Contributor

xxchan commented Sep 27, 2022

only the original author can mark their concern as resolved? cc @Manishearth

@Manishearth
Copy link
Member

@rfcbot resolved feature-gate We should have a feature gate for this attribute

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Sep 27, 2022
@rfcbot
Copy link

rfcbot commented Sep 27, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@riking
Copy link

riking commented Oct 6, 2022

I have a comment about the "shiny future" presented -- I don't like ignore as the preferred spelling for this behavior.
However, the only direct equivalent in other test frameworks I can think of is that Bazel calls it tags = ["manual"] (test only runs when named on the command line).

For the instant issue, no complaints!

@CAD97
Copy link
Contributor Author

CAD97 commented Oct 7, 2022

less important details

FWIW: the reason for ignore behaving this way is that it's already the documented and expected behavior of #[ignore] to allow running the tests with --include-ignored (run ignored and not ignored tests) and --ignored (run only ignored tests). Where this becomes very surprising is that the rustdoc test runner only compiles doctests once the libtest test runner has decided to run it; effectively #[test] #[ignore] fn doctest() { compile(include_str!())?.run()? }.

I don't have any idea the best way to handle this -- it might be just to disable -[-include]-ignored when rustdoc is the test runner. Fundamentally, there's a conflict of interest here aiui -- rustdoc wants to list the ```ignore as a present-but-ignored example-test, the developer has written something that's likely not going to compile, and the libtest runner wants you to be able to opt-in to running #[test] #[ignore] tests.

The important thing imho is that cargo test -- --ignored should work to run #[test] #[ignored] tests. On 1.64.0 and 1.65.0-beta.2, though, for basically any project using doctests this will fail due to ```ignore "doctests" which don't compile (#87586).

HOWEVER, something apparently has changed between 1.65.0-beta.2 and 1.66.0-nightly-2022-10-03: nightly cargo test -- --ignored now no longer runs doctests:

> cargo +beta -V 
cargo 1.65.0-beta.2 (082503982 2022-09-13)

> cargo +beta test -- --ignored > $null
    Finished test [unoptimized + debuginfo] target(s) in 0.01s
     Running unittests src\lib.rs (D:\.rust\target\debug\deps\dynty-5fe814ab794244b1.exe)
     Running unittests src\lib.rs (D:\.rust\target\debug\deps\dynty_macros-1aaf157df7fac3ba.exe)
     Running unittests src\lib.rs (D:\.rust\target\debug\deps\dynty_macros_impl-6f94d6f3dc68c979.exe)
   Doc-tests dynty
   Doc-tests dynty-macros
   Doc-tests dynty-macros-impl

> cargo +nightly -V
cargo 1.66.0-nightly (0b84a35c2 2022-10-03)

> cargo +nightly test -- --ignored > $null
   Compiling dynty v0.1.0 (D:\git\cad97\dynty\lib)
    Finished test [unoptimized + debuginfo] target(s) in 0.33s
     Running unittests src\lib.rs (D:\.rust\target\debug\deps\dynty-e50eb211e71fe749.exe)
     Running unittests src\lib.rs (D:\.rust\target\debug\deps\dynty_macros-ce1215d5d07a1208.exe)
     Running unittests src\lib.rs (D:\.rust\target\debug\deps\dynty_macros_impl-d6d1d0e03e3d048f.exe)

Investigating a bit, it looks like this is probably a bugfix, as cargo test filter has always suppressed doctests, but cargo test -- filter hasn't. If this change is kept, then that fixes #87586 and makes this change imho no longer necessary.

#63193 is asking for ```no_compile (or to make the change to ```ignore); to completely skip the code block rather than including it in the cargo test output as #[ignore]d. I honestly do not have any opinion here; my motive is just to make cargo test -- --ignored not include deliberately noncompiling documentation "tests".

(That said, what makes ```ignore special that it should be listed in cargo test output but e.g. ```text isn't listed? In practice, people use ```ignore to mark a block as not a test, so #63193 has a point in saying that rustdoc shouldn't be reporting them as tests.)

@rfcbot concern does-the-cargo-behavior-change-make-this-unnecessary

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 7, 2022
@rfcbot
Copy link

rfcbot commented Oct 7, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Oct 18, 2022
@GuillaumeGomez
Copy link
Member

Any information about #96573 (comment) @CAD97 ?

@pitaj
Copy link
Contributor

pitaj commented May 5, 2023

@CAD97 @GuillaumeGomez ping from triage. Have you come to a conclusion on whether this is necessary?

@Dylan-DPC Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2023
@RaulTrombin
Copy link

Can I maintain the formatting using rust-analyser while using no_compile?

Is it possible to include conditional triggers in no_compile? For example: /// #[cfg(target_arch = "armv7")]

I encountered some issues on CI where the documentation examples were running with cargo test and causing annoying errors. I discovered that I can skip the example using /// #[cfg(target_arch = "armv7")] { example }, but it looks good in the documentation...

This tool is very helpful, and I would like to know if it's possible to make it conditional while still performing format checks.

@GuillaumeGomez
Copy link
Member

You can hide lines in code examples by starting them with # .

@bors
Copy link
Contributor

bors commented Sep 16, 2023

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

@CAD97
Copy link
Contributor Author

CAD97 commented Sep 17, 2023

Any information about #96573 (comment)

(Sorry for the delay; life happened)

I've come to the eventual conclusion that ```ignore should be sufficient. It's unfortunate that different "kinds" of ignored tests exist, but they do. It's odd that cargo test --all-targets excludes cargo test --doc, but it does. At this point I think ```no_compile would only really serve to muddy things further.

The normal test harness has the advantage that tests have names, so it's possible (even if not necessarily nice) to run a subset of ignored tests that actually work in the current environment. Doctests don't really have that affordance.

If the test runner ever gets more interesting (e.g. ignore_if), then this might be worth revisiting. Until then, though, I no longer think this change to be particularly beneficial. The test runner needs to recognize a difference between "ignored by default but may be meaningful in some environments" and "ignored and will never pass" before this addition actually means anything.

As such, closing this. If someone else thinks this is a valuable addition, feel free to take the code and make your argument for it.

@CAD97 CAD97 closed this Sep 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flag for stop interpreting code blocks as doctests