Skip to content

Reject outer attributes on cfg_select branches#155734

Open
qaijuang wants to merge 3 commits intorust-lang:mainfrom
qaijuang:cfg-select-doc-comment
Open

Reject outer attributes on cfg_select branches#155734
qaijuang wants to merge 3 commits intorust-lang:mainfrom
qaijuang:cfg-select-doc-comment

Conversation

@qaijuang
Copy link
Copy Markdown
Contributor

@qaijuang qaijuang commented Apr 24, 2026 •
edited by rustbot
Loading

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 24, 2026
@qaijuang qaijuang force-pushed the cfg-select-doc-comment branch from fb8cf2e to 9c49112 Compare April 24, 2026 14:12
folkertdev
folkertdev reviewed Apr 24, 2026
View reviewed changes
Comment thread tests/ui/lint/unused/unused-doc-comments-for-macros.rs Outdated
@qaijuang
test with multi-line doc comment
4bb2b07 
Co-authored-by: Copilot <copilot@github.com>
@qaijuang qaijuang marked this pull request as ready for review April 24, 2026 14:42
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 24, 2026

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 24, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 24, 2026

r? @JonathanBrouwer

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
  • compiler expanded to 73 candidates
  • Random selection from 17 candidates

@qaijuang qaijuang requested a review from folkertdev April 24, 2026 16:59
JonathanBrouwer
JonathanBrouwer reviewed Apr 25, 2026
View reviewed changes
Comment thread compiler/rustc_attr_parsing/src/attributes/cfg_select.rs Outdated
@rustbot rustbot 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 Apr 25, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 25, 2026

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

@JonathanBrouwer
Copy link
Copy Markdown
Contributor

JonathanBrouwer commented Apr 25, 2026

@bors r+ rollup

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented Apr 25, 2026

📌 Commit 4bb2b07 has been approved by JonathanBrouwer

It is now in the queue for this repository.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 25, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Apr 25, 2026
@JonathanBrouwer
Rollup merge of rust-lang#155734 - qaijuang:cfg-select-doc-comment, r…
054e0eb 
…=JonathanBrouwer

Lint doc comments in cfg_select branches

Fixes rust-lang#155701.
@JonathanBrouwer JonathanBrouwer mentioned this pull request Apr 25, 2026
Closed
rust-bors Bot pushed a commit that referenced this pull request Apr 25, 2026
@bors
Auto merge of #155773 - JonathanBrouwer:rollup-As77GJX, r=JonathanBro…
2174489 
…uwer

Rollup of 6 pull requests

Successful merges:

 - #154803 (Fix ICE from cfg_attr_trace )
 - #155485 (Add an edge-case test for `--remap-path-prefix` for `rustc` & `rustdoc`)
 - #155659 (cleanup, restructure and merge `tests/ui/deriving` into `tests/ui/derives`)
 - #155696 (Add a higher-level API for parsing attributes)
 - #155734 (Lint doc comments in cfg_select branches)
 - #155769 (triagebot.toml: Ping Enselic when tests/debuginfo/basic-stepping.rs changes)
@fmease
Copy link
Copy Markdown
Member

fmease commented Apr 25, 2026 •
edited
Loading

This PR allows more code to compile, so it's a language change and technically needs a T-lang FCP.

cc @rust-lang/lang

@fmease
Copy link
Copy Markdown
Member

fmease commented Apr 25, 2026

I'm sorry

@bors r-

@rust-bors rust-bors Bot 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 25, 2026
@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented Apr 25, 2026

This pull request was unapproved.

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

View changes since this unapproval

folkertdev
folkertdev reviewed Apr 25, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@folkertdev folkertdev left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

View changes since this review

Comment thread tests/ui/lint/unused/unused-doc-comments-for-macros.rs Outdated
@folkertdev folkertdev added I-lang-nominated Nominated for discussion during a lang team meeting. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination S-waiting-on-t-lang Status: Awaiting decision from T-lang and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 25, 2026
@JonathanBrouwer
Copy link
Copy Markdown
Contributor

JonathanBrouwer commented Apr 25, 2026

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

@fmease
Copy link
Copy Markdown
Member

fmease commented Apr 25, 2026 •
edited
Loading

@folkertdev

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

@JonathanBrouwer
Copy link
Copy Markdown
Contributor

JonathanBrouwer commented Apr 25, 2026

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.

fmease
fmease reviewed Apr 25, 2026
View reviewed changes
Comment thread compiler/rustc_attr_parsing/src/attributes/cfg_select.rs Outdated
@folkertdev folkertdev added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed I-lang-nominated Nominated for discussion during a lang team meeting. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination S-waiting-on-t-lang Status: Awaiting decision from T-lang labels Apr 25, 2026
@traviscross
Copy link
Copy Markdown
Contributor

traviscross commented Apr 25, 2026

This PR allows more code to compile, so it's a language change and technically needs a T-lang FCP... The grammar of cfg_select is literally written down in the Reference, so any extensions are new and separate feature requests.

Thanks @fmease for catching this and navigating it.

@qaijuang qaijuang changed the title Lint doc comments in cfg_select branches Reject outer attributes on cfg_select branches Apr 25, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 25, 2026

The parser was modified, potentially altering the grammar of (stable) Rust
which would be a breaking change.

cc @fmease

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 25, 2026
@qaijuang qaijuang requested a review from folkertdev April 25, 2026 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JonathanBrouwer JonathanBrouwer Awaiting requested review from JonathanBrouwer

@fmease fmease Awaiting requested review from fmease

@folkertdev folkertdev Awaiting requested review from folkertdev

Requested changes must be addressed to merge this pull request.

Assignees

@JonathanBrouwer JonathanBrouwer

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cfg_select! + doc comment gives cryptic compiler error

6 participants

@qaijuang @rustbot @JonathanBrouwer @fmease @traviscross @folkertdev