Allow #![doc(test(attr(..)))] at module level

[Urgau]

This PR adds the ability to specify #![doc(test(attr(..)))] at module level in addition to allowing it at crate-root.

This is motivated by a recent PR #140323 (by @tgross35) where we have to duplicate 2 attributes to every single f16 and f128 doctests, by allowing #![doc(test(attr(..)))] at module level we can omit them entirely and just have (in both module):

#![doc(test(attr(feature(cfg_target_has_reliable_f16_f128))))]
#![doc(test(attr(expect(internal_features))))]

Those new attributes are appended to the one found at crate-root or at a previous module. Those "global" attributes are compatible with merged doctests (they already were before).

Given the small addition that this is, I'm proposing to insta-stabilize it, but I can feature-gate it if preferred.

Best reviewed commit by commit.

r? @GuillaumeGomez

[rustbot]

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

[rustbot]

This PR modifies run-make tests.

cc @jieyouxu

[GuillaumeGomez]

Some small changes needed but otherwise it's pretty good as is. Once done, I'll start the FCP.

[GuillaumeGomez]

Just one last nit, I'll start the FCP afterward. Please note that I will add a concern about whether it should overload existing doc(test(attr(...))) (from the parent module) or if the current approach (appending to parent module's) is the one we want.

[Urgau]

I think all crate attributes support being overridden, so if a module doesn't want one from the parent (or crate-root) they can just revert to the old/default value, it's how #![allow], #![deny], ... works and what makes the most sense to me.

If we were to overload the current list with the one from the current module alone, we would need to duplicate (for the core crate) the one currently at the crate-root, which seems prone to be out-of-sync and is IMO redundant.

rust/library/core/src/lib.rs

Line 51 in f97b3c6

test(attr(allow(dead_code, deprecated, unused_variables, unused_mut)))

[GuillaumeGomez]

Not always. Some attributes like cfg_attr or deprecated cannot be overloaded for example. Not sure how important or if it is important though.

[GuillaumeGomez]

Thanks! Let's start the FCP. I'll add my concern as well.

@rfcbot fcp merge
@rfcbot concern non-overloadable attributes

[rfcbot]

Team member @GuillaumeGomez has proposed to merge this. The next step is review by the rest of the tagged team members:

• @GuillaumeGomez
• @Manishearth
• @Nemo157
• @aDotInTheVoid
• @camelid
• @fmease
• @jsha
• @notriddle

Concerns:

• non-overloadable attributes (Allow #![doc(test(attr(..)))] at module level #140560 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[Urgau]

Not always. Some attributes like cfg_attr or deprecated cannot be overloaded for example. Not sure how important or if it is important though.

Hum, indeed, didn't think of those.

Currently both would be applied to every doctests in the crate, so to me adding a #![doc(test(attr(..)))] at module level shouldn't overload/reset the list of attributes, at least not implicitly.

I could see rustdoc having an explicit way, like #![doc(test(attr(reset())))] (to be bikeshed) that would explicitly overload the list of attributes to add to the doctests and therefore not take the one from the parents.

This would have the advantage of supporting both strategy: appending and overloading.

[Nemo157]

Why is this limited to the module level? If it's being extended from crate level I would expect it to just work anywhere like most attributes.

[Urgau]

Allowing it at item level, that is for struct, enum, function, const, union, trait, impl, ... would be equivalent to put in directly inside the doctest. Doing,

#[doc(test(attr(feature(my_feature))))]
/// ```
/// ```
pub struct A;

would be identical to putting the attribute directly inside the doctest (which IMO is clearer),

/// ```
/// #![feature(my_feature)]
/// ```
pub struct A;

Then there is the case fields and variants, putting it directly on them would be the same as above; but putting it at the struct or enum level might be useful; but I don't have a usecase nor motivation for it.

#[doc(test(attr(feature(my_feature))))]
pub struct A {
    /// ```
    /// # this doctest would receive #[feature(my_feature)] from the parent
    /// # not sure how useful this would be
    /// ```
    pub field1: u32,
    /// ```
    /// # same here   
    /// ```
    pub field2: u32,
}

[Nemo157]

You can have multiple doctests on one item. The more relevant location I was thinking of was impls, that could apply to dozens of methods.

I do feel like modules would cover all usecases, with a little code rearranging for weird edgecases, it just seems like an artificial restriction to me. Are there other inherited attributes that have a similar restriction? All I'm aware of can be applied either at crate-level or anywhere.

[GuillaumeGomez]

It's a good point indeed.