Reject outer attributes on cfg_select branches rust-lang/rust#155734
tests/ui/lint/unused/unused-doc-comments-for-macros.rs · outdated
| 20 | /// line1 |
|
| 21 | //~^ ERROR: unused doc comment |
|
| 22 | debug_assertions => (), |
can you make this have multiple lines of doc comments, and also test that the lint fires correctly on the _ => arm?
Some changes occurred in compiler/rustc_attr_parsing
rustbot has assigned @JonathanBrouwer.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Why was this reviewer chosen?
The reviewer was selected based on:
- Owners of files modified in this PR:
compiler compilerexpanded to 73 candidates- Random selection from 17 candidates
compiler/rustc_attr_parsing/src/attributes/cfg_select.rs · outdated
| 84 | 86 | |
| 85 | 87 | let tts = p.parse_delimited_token_tree().map_err(|e| e.emit())?; |
| 86 | 88 | let span = underscore.span.to(p.token.span); |
| 89 | lint_unused_doc_comment(p, doc_comment, lint_node_id); |
Why is the lint_unused_doc_comment here rather than immediately after the eat_outer_doc_comments call?
I assumed linting only successfully parsed branches made sense to avoid noise from parsing failures
Right, that makes sense
Reminder, once the PR becomes ready for a review, use @rustbot ready.
@bors r+ rollup
This PR allows more code to compile, so it's a language change and technically needs a T-lang FCP.
I'm sorry
@bors r-
This pull request was unapproved.
This PR was contained in a rollup (#155773), which was unapproved.
The FCP is very technically here, given this was just stabilized and I think not erroring on a doc comment in that position is very much in the spirit of the feature stabilization.
Nevertheless, let's at least let T-lang decide what they want to do here.
tests/ui/lint/unused/unused-doc-comments-for-macros.rs · outdated
| 18 | // Regression test for https://github.com/rust-lang/rust/issues/155701. |
|
| 19 | cfg_select! { |
|
| 20 | /// line1 //~ ERROR: unused doc comment |
|
| 21 | /// line2 |
|
| 22 | /// line3 |
|
| 23 | debug_assertions => (), |
|
| 24 | /// line1 //~ ERROR: unused doc comment |
|
| 25 | /// line2 |
|
| 26 | /// line3 |
|
| 27 | _ => (), |
|
| 28 | } |
What happens if you mix normal comments and doc comments here? E.g.
// Regression test for https://github.com/rust-lang/rust/issues/155701.
cfg_select! {
/// line1 //~ ERROR: unused doc comment
// line2
/// line3
debug_assertions => (),
/// line1 //~ ERROR: unused doc comment
// line2
/// line3
_ => (),
}This PR allows more code to compile, so it's a language change and technically needs a T-lang FCP.
Right I missed that, I'm so sorry
The FCP is very technically here, given this was just stabilized and I think not erroring on a doc comment in that position is very much in the spirit of the feature stabilization.
The fact that this was stabilized recently doesn't imply that the feature is malleable at will. The grammar of cfg_select is literally written down in the Reference, so any extensions are new and separate feature requests.
I would say it's very much not in the spirit to assume that certain extensions are 'natural' and implicitly follow from a past FCP that has since completed since that would defeat the whole purpose of (exhaustive) stabilization reports and it would circumnavigate the entire stabilization process.
More concretely, regarding the change at hand, I would state that is doesn't make any sense to allow outer doc comments (a special kind of outer attribute) without also allowing regular outer attributes, that's unprecedented. Now, allowing regular outer attributes comes with various non-trivial questions like which attributes should be allowed (cfg, lint control, ...; all of that would need to be implemented).
Indeed, the language design question is not trivial here.
I think the easy way out for now is to make this PR emit an error, rather than a lint.
compiler/rustc_attr_parsing/src/attributes/cfg_select.rs · outdated
| 143 | 147 | Ok(branches) |
| 144 | 148 | } |
| 145 | 149 | |
| 150 | fn eat_outer_doc_comments(p: &mut Parser<'_>) -> Option<(Span, CommentKind)> { |
We have the same confusing diagnostic + incorrect suggestion for regular attributes:
cfg_select! {
#[cfg(false)]
debug_assertions => println!("debug"), //~ ERROR expected a literal
_ => println!("not debug"),
};So this shouldn't manually parse outer doc comments (proof that this is unprecedented), it should just parse outer attributes with parse_outer_attributes and hard reject them afterwards for a good diagnostic.
This option also wouldn't need a lang-nomination.
Hmm yea, I assumed that we'd be doing something similar for macro_rules! but they don't accept attributes in that position either:
macro_rules! foo {
/// foo
($ty:ty) => {
$ty
}
}emits
error: invalid macro matcher; matchers must be contained in balanced delimiters
--> src/main.rs:5:5
|
5 | /// foo
| ^^^^^^^
error: expected `=>`, found `(`
--> src/main.rs:6:5
|
5 | /// foo
| - expected `=>`
6 | ($ty:ty) => {
| ^ unexpected token
And similar for #[...] attributes.
This PR allows more code to compile, so it's a language change and technically needs a T-lang FCP... The grammar of
cfg_selectis literally written down in the Reference, so any extensions are new and separate feature requests.
Thanks @fmease for catching this and navigating it.
The parser was modified, potentially altering the grammar of (stable) Rust
which would be a breaking change.
cc @fmease
tests/ui/macros/cfg_select.stderr · outdated
| 61 | LL | /// doc comment |
|
| 62 | | ^^^^^^^^^^^^^^^ |
|
| 63 | |
|
| 64 | error: outer attributes are not allowed on `cfg_select` branches |
I think it would be nice to special-case the error for doc comments, since it's a bit weird to call a doc comment an "outer attribute".
Not sure how that interacts with your nice MultiSpan trick tho, an alternative might be wording the error as "attributes and doc comments are not allowed on cfg_select branches"
tests/ui/macros/cfg_select.rs · resolved
| 214 | } |
|
| 215 | |
|
| 216 | cfg_select! { |
|
| 217 | #[cfg(false)] |
Could you also add a test with an inner attribute #![..] and with inner doc comments //!
tests/ui/macros/cfg_select.stderr · resolved
| 76 | error: an inner attribute is not permitted in this context |
|
| 77 | --> $DIR/cfg_select.rs:224:5 |
|
| 78 | | |
|
| 79 | LL | #![cfg(false)] |
The error here says an inner attribute is not permitted in this context but it is pointing to an outer attribute (#![..]). Can we just make the error "an attribute is not permitted in this context"?
i think it's accurate because #![...] is an inner attribute
compiler/rustc_parse/src/parser/cfg_select.rs · outdated
| 49 | &mut self, |
|
| 50 | ) -> PResult<'a, Option<CfgSelectBranchAttrSpans>> { |
|
| 51 | let inner_doc_comment_span = |
|
| 52 | if let token::DocComment(_, AttrStyle::Inner, _) = self.token.kind { |
Hmmm this logic doesn't work if the inner doc comment is not the first one, right?
Good catch, I'll add a test for it
compiler/rustc_parse/src/parser/cfg_select.rs · outdated
| 52 | // `parse_outer_attributes` recovers inner doc comments as outer ones, so collect their |
|
| 53 | // spans first and suppress the follow-up `cfg_select` branch diagnostic for them. |
|
| 54 | let mut inner_doc_comment_spans = Vec::new(); |
|
| 55 | let mut snapshot = self.create_snapshot_for_diagnostic(); |
IINM this is creating a parser snapshot in the happy path, right? this is an expensive operation and should only happen in error paths.
Why is this using a snapshot in the first place? Can't you just use parse_inner_attributes and reject inner attrs afterwards? That function is smart enough not to bail out when it encounters outer attributes so you don't need to worry about that.
calling parse_inner_attributes first for /// outer followed by //! inner would return empty at the /// and would never notice the later //!
The test case:
cfg_select! {
/// outer doc comment
//! inner doc comment
debug_assertions => {}
_ => {}
}IINM this is creating a parser snapshot in the happy path, right? this is an expensive operation and should only happen in error paths.
That’s a good point. I’m thinking of classifying recovered doc comments directly from their source snippet in the error path, so we can avoid the snapshot cost
@bors squash
View on GitHub
🚧 Squashing... this can take a few minutes.
🔨 6 commits were squashed into d42a92b.
@bors r+ rollup
I think all concerns are resolved now, since it's an error
View all comments
Fixes #155701.