[rustdoc] Do not take doc(cfg()) into account when filtering doctests

[GuillaumeGomez]

View all comments

Part of #147033.

Because it was using the extract_cfg_from_attrs common function, it was taking into account the doc(cfg()) attributes the same as if they were a cfg.

I didn't mark this PR as "fix" because I didn't handle the case of the doctest not being marked as ignored because I'm not sure if we should revisit the fact that we ignore these doctests or if we should just mark them as ignored (because of target_feature(enable = "...")).

Setting @fmease as reviewer as they are likely the only one with context about this issue. 😆

r? @fmease

[GuillaumeGomez]

And now it fails because we (doc) inlined some functions. Fixing that then. :)

[GuillaumeGomez]

Fixed CI \o/

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[GuillaumeGomez]

Reverted changes I made to extract_cfg_from_attrs and instead only look for the target_feature attributes.

[GuillaumeGomez]

Ah, new stuff gets added.

[rustbot]

stdarch is developed in its own repository. If possible, consider making this change to rust-lang/stdarch instead.

cc @Amanieu, @folkertdev, @sayantn

[GuillaumeGomez]

Seems like CI is happy now.

[GuillaumeGomez]

Setting a new reviewer as fmease seems busy.

r? @Urgau

[Urgau]

Can't this test be made a regular UI test with two revisions for the editions?

//@ revisions: e2015 e2024
//@[e0215] edition: 2015
//@[e0215] edition: 2024

View changes since the review

[GuillaumeGomez]

Yes but I wanted to enforce the output, which cannot be done with ui-tests sadly. I want to be absolutely sure that we have 2 passed.

[Urgau]

I think that can be done with //@ regex-error-pattern: 2 passed.

[Urgau]

Did you look at using regex-error-pattern? The UI test would have the advantage of checking the whole output, even if we are not asserting something.

If not, I'm fine approving it without it.

[GuillaumeGomez]

error in revision `e2015`: error pattern 'running 2 tests' not found!
error in revision `e2024`: error pattern 'running 2 tests' not found!

It doesn't check stdout as far as I can see.

[GuillaumeGomez]

For reference, tested with:

//@ revisions: e2015 e2024
//@[e0215] edition: 2015
//@[e0215] edition: 2024
//@ compile-flags: --test
//@ regex-error-pattern: running 2 tests
//@ regex-error-pattern: test .+/doc-cfg-filter.rs - f (line 3) \.\.\. ok
//@ regex-error-pattern: test .+/doc-cfg-filter.rs - dummy::f2 \(line 11\) \.\.\. ok
//@ regex-error-pattern: test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out;
//@ check-pass

#![feature(doc_cfg)]

/// ```
/// assert!(true);
/// ```
#[doc(cfg(spec))]
fn f() {}

#[doc(cfg(false))]
mod dummy {
    /// ```
    /// assert!(true);
    /// ```
    fn f2() {}
}

[Urgau]

It doesn't check stdout as far as I can see.

Ah, nevermind then.

[GuillaumeGomez]

Thanks for the review, updated!

[fmease]

Sorry, I wasn't really available for the last 3 weeks. I haven't looked at your latest implementation, I'll let Urgau do the reviewing, I'm just chiming in for some comments.

IMO, we (probably I) should definitely tackle #[target_feature(enable = …)] again next since I don't want the two drifting apart behavior-wise (thereby fully fixing #147033). Of course, that'd involve crater & an FCP.

View changes since this review

[fmease]

All these changes to libcore etc. make me question whether this change is actually desired / my bug report #147033 is valid ^^' Please instill certainty that it is :'D

Clearly, the user code is worse: We have to give up on the doc comments, check the cfg condition again despite being in a module that only gets compiled on macOS or doc.

Since this looks like a common code pattern I wish there was an official way to express this, don't y'all think? Unfortunately, ignore-* code block attributes won't help here. We'd need

/// ```no_run,only-macos

or arbitrary cfg exprs

/// ```no_run,ignore2-not(macos)

unfortunately, #[doc(test(attr(cfg(…))))] won't help either since it just butchers the generated test harness code. If that had worked that would've been beyond awesome :'(

[Urgau]

unfortunately, #[doc(test(attr(cfg(…))))] won't help either since it just butchers the generated test harness code. If that had worked that would've been beyond awesome :'(

What's the problem exactly with that approach?

[fmease]

#![doc(test(attr(cfg(false))))]
//! ```
//! assert!(true);
//! ```

running 1 test
test q.rs - (line 2) ... FAILED

failures:

---- q.rs - (line 2) stdout ----
error[E0601]: `main` function not found in crate `rust_out`
 --> q.rs:4:29
  |
4 | } _doctest_main_q_rs_2_0() }
  |                             ^ consider adding a `main` function to `q.rs`
<12 lines omitted>

---

#![doc(test(attr(cfg(false))))]
//! ```
//! fn main() {
//!     assert!(true);
//! }
//! ```

running 1 test
test q.rs - (line 2) ... FAILED

failures:

---- q.rs - (line 2) stdout ----
error[E0601]: `main` function not found in crate `rust_out`
 --> q.rs:5:2
  |
5 | }
  |  ^ consider adding a `main` function to `q.rs`
<12 lines omitted>

[GuillaumeGomez]

I think it's up to the users to ensure their doctest is run under the right conditions. The way core/std are making all items available might not be the best one. ^^'

[Urgau]

error[E0601]: `main` function not found in crate `rust_out`
 --> q.rs:5:2
  |
5 | }
  |  ^ consider adding a `main` function to `q.rs`
<12 lines omitted>

Hum, seems like we are not taking into account cfgs and are blanket applying to the whole doctest crate. I wonder if we should do something about it.

[GuillaumeGomez]

We are, otherwise we would see all items, whatever their cfg.

[GuillaumeGomez]

  warning: unexpected `cfg` condition name: `macos`
    --> library/core/src/os/darwin/objc.rs:97:12
     |
  97 | #[cfg_attr(macos, doc = "```no_run")]
     |            ^^^^^ help: found config with similar value: `target_os = "macos"`

So I'll remove my last commit.

[GuillaumeGomez]

Restarted flaky CI and now it passed.

@rustbot ready

[Urgau]

Thanks.

View changes since this review

[Urgau]

@bors r+

[rust-bors]

📌 Commit 329ac5f has been approved by Urgau

It is now in the queue for this repository.

[jhpratt]

@bors r- #160168 (comment)

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#160168), which was unapproved.

View changes since this unapproval