[evavh]

View all comments

A small change as part of the target checking refactor for #[doc] attribute parsing.

r? @JonathanBrouwer

[rustbot]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[evavh]

I tried to add tests for the crate-level attribute checks, but I'm running into an interesting problem: there seems to be no way for the target check to fail. The possible ways for a crate-level attribute to be applied to a non-crate target (that I can think of) are:

1. A #[doc(CRATE_LEVEL_ONLY)] attribute applied to an item: this outputs the error crate-level attribute should be an inner attribute: add an exclamation mark
2. A #![doc(CRATE_LEVEL_ONLY)] attribute applied to an item (ie not at the top of the file): the error is an inner attribute is not permitted in this context

In both cases the compiler throws an error before we even reach the target checking code in doc.rs. The same applies to doc attribute arguments that shouldn't be used crate-level, like fake_variadic. It seems the distinction between inner (#!) and outer (#) attributes is enough to guard against wrong crate/non-crate targets.

The test I wrote covers all doc attribute arguments that check whether they are applied at crate level, and they all throw errors in earlier parsing steps. What do we do with the target checking code that does nothing?

[JonathanBrouwer]

@evavh I took a random crate-level doc attribute, and applied it to a function:

#[doc(html_no_source)]
fn test1() { }
fn test2() {
    #![doc(html_no_source)]
}

In both cases this produces "this attribute can only be applied at the crate level"
This diagnostic is produced in target checking, so if you hit it you are doing target checking somewhere:

rust/compiler/rustc_attr_parsing/src/target_checking.rs

Line 215 in ff619e2

pub(crate) fn check_crate_level(cx: &mut AcceptContext<'_, 'sess>, warn: bool) {

The diagnostic "an inner attribute is not permitted in this context" is produced when you apply inner attributes to an item that doesn't support inner attributes, such as a struct, and is indeed produced in the parser

struct Test {
    #![doc(html_no_source)]
}

@rustbot author

[mejrs]

These are breaking changes, so they will need a crater run. Alternatively, you could refactor this in a way that doesn't change behavior.

[evavh]

Turns out I was very confused by the "new" diagnostic emitted by check_crate_level called by AttributeParser::check_target. The target checking is still happening correctly with this refactor, though I do think the diagnostic has become worse.

I tried changing the diagnostic in check_crate_level, but that broke all kinds of unrelated tests.

As for breaking changes, using a crate-level doc attribute on the wrong target now emits an ERROR instead of a WARN.

@rustbot ready

[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.

[JonathanBrouwer]

@bors r+ rollup
Thanks a lot!

[rust-bors]

📋 This PR cannot be approved because it currently has the following labels: needs-fcp, needs-crater.

[JonathanBrouwer]

@bors r+ rollup

[rust-bors]

📌 Commit 873c9c0 has been tentatively approved by JonathanBrouwer

It will be put into the queue for this repository once PR CI succeeds.

[mejrs]

This is incorrect, this needs a primary span, otherwise if used as an error it won't have a span to point at and will be displayed like this:

error: this attribute can only be applied at the crate level
   |
   = note: read <https://doc.rust-lang.org/nightly/rustdoc/the-doc-attribute.html#at-the-crate-level> for more information

(Fixed in #164582)

[JonathanBrouwer]

Right, thanks!