Skip to content

Conversation

@JonathanBrouwer
Copy link
Contributor

@JonathanBrouwer JonathanBrouwer commented Dec 4, 2025

This PR changes two things:

  • This decouples the AttributeLintKind from the Lint it is emitted in. cx.emit_lint now takes both as an argument, rather than inferring the Lint from the AttributeLintKind. This is nice because:
    • It allows us to remove AttributeLintKind::InvalidMacroExportArguments
    • It allows us to move the choice between USELESS_DEPRECATED and UNUSED_ATTRIBUTES out of the lint emitting code
    • It allows the next change:
  • This moves AttributeLintKind to rustc_lint_defs, and the decorating code to rustc_lint. This is nice because:

This PR does not change observable output of the compiler, as can be seen by no uitests being affected.

r? @jdonszelmann

@rustbot
Copy link
Collaborator

rustbot commented Dec 4, 2025

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rustbot rustbot added the A-attributes Area: Attributes (`#[…]`, `#![…]`) label Dec 4, 2025
@rustbot rustbot added 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. labels Dec 4, 2025
@bors
Copy link
Collaborator

bors commented Dec 5, 2025

☔ The latest upstream changes (presumably #149646) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 5, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 5, 2025
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2025
span,
ast::CRATE_NODE_ID,
BuiltinLintDiag::IllFormedAttributeInput {
BuiltinLintDiag::AttributeLint(AttributeLintKind::IllFormedAttributeInput {
Copy link
Contributor

Choose a reason for hiding this comment

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

is the reason we can't use the diagnostics infra like in https://github.com/rust-lang/rust/pull/147634/files#diff-2b4127dee93eb2b198d202fad306da78f9b475e75affb3e146dab4a579a041a2R236 still that we need to hash the lints for lowering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the specific place this comment is on (this one is a bit special since it's not called from an attribute parser), we could theoretically convert to using dyn lint diagnostics now, but that would make the code uglier since we then need to inline the decorating code.

In general for all other attribute lints, the problem is that in late attribute parsing Attribute Lints are placed on a HirId, while the buffer_lint function expects a NodeId. I am planning on seeing if we can make buffer_lint also take a HirId, but not sure how hard that will be and that's definitely not for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

wait but buffer_lint doesn't do anything during late parsing those buffered lints are already emitted at this point

Copy link
Contributor

Choose a reason for hiding this comment

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

did I miss something about how you implemented this??

Copy link
Contributor

Choose a reason for hiding this comment

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

right we never use buffer_lint with a hirid. during later parsing those lints are sent to the hir owner info of the owner node that's being lowered, to be hashed with that node, and emitted once hir is built

Copy link
Contributor Author

@JonathanBrouwer JonathanBrouwer Dec 5, 2025

Choose a reason for hiding this comment

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

Everything stays the same during late parsing, it's still put in the OwnerInfo of the hir.
The only difference for Late is here, which is different because the decorating code moved https://github.com/rust-lang/rust/pull/149662/files#diff-0f333b55aca600b036a38495f514656022cea3e3e0d41a3d4b3ce484c7279d34R161

Copy link
Contributor

Choose a reason for hiding this comment

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

right, but I guess what I was asking first is that it's probably still hard to convert these lints to be opaque diagnostics since we have to hash them into owner nodes. We'll always have to keep an enum around like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, yeah that will be annoying :c

Copy link
Contributor

@jdonszelmann jdonszelmann left a comment

Choose a reason for hiding this comment

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

Some questions, but looks good overall. Indeed, especially the part where those lints can access the tcx. this seems very important to support in some way going forward

View changes since this review

@jdonszelmann
Copy link
Contributor

@rustbot author

@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 Dec 5, 2025
@rustbot
Copy link
Collaborator

rustbot commented Dec 5, 2025

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

@rustbot

This comment has been minimized.

@JonathanBrouwer
Copy link
Contributor Author

JonathanBrouwer commented Dec 5, 2025

Oops I accidentally undid my rebase :p (Yes I should start using jj)
one second

@rustbot
Copy link
Collaborator

rustbot commented Dec 5, 2025

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
Copy link
Contributor Author

JonathanBrouwer commented Dec 5, 2025

@rustbot ready

@jdonszelmann Here is one link with the two range diffs above combined, so you don't have to review me undoing and redoing my rebase :P
https://triagebot.infra.rust-lang.org/gh-range-diff/rust-lang/rust/864339abf952f07098dd82610256338520167d4a..942e4b2c7ad79e8ab0dbc424c7d384a5299cfa22/864339abf952f07098dd82610256338520167d4a..8f59eb017753e4e72f0d2b0a736ba9172002e321

Thanks to the new method warn_ill_formed_attribute_input and removing the LintEmitter this PR now also has a significantly negative diff :D

@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 Dec 5, 2025
@jdonszelmann
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Dec 5, 2025

📌 Commit 8f59eb0 has been approved by jdonszelmann

It is now in the queue for this repository.

@bors bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2025
bors added a commit that referenced this pull request Dec 6, 2025
Rollup of 4 pull requests

Successful merges:

 - #149563 (f*::min/max: fix comparing with libm and IEEE operations)
 - #149592 (`is_const_default_method` is completely handled by the `constness` query)
 - #149662 (Move attribute lints to `rustc_lint`)
 - #149684 (rustc-dev-guide subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 28435f8 into rust-lang:main Dec 6, 2025
11 checks passed
@rustbot rustbot added this to the 1.94.0 milestone Dec 6, 2025
rust-timer added a commit that referenced this pull request Dec 6, 2025
Rollup merge of #149662 - JonathanBrouwer:lint-rework, r=jdonszelmann

Move attribute lints to `rustc_lint`

This PR changes two things:
- This decouples the `AttributeLintKind` from the `Lint` it is emitted in. `cx.emit_lint` now takes both as an argument, rather than inferring the `Lint` from the `AttributeLintKind`. This is nice because:
  - It allows us to remove `AttributeLintKind::InvalidMacroExportArguments`
  - It allows us to move the choice between `USELESS_DEPRECATED` and `UNUSED_ATTRIBUTES` out of the lint emitting code
  - It allows the next change:
- This moves `AttributeLintKind` to `rustc_lint_defs`, and the decorating code to `rustc_lint`. This is nice because:
  - It allows attribute lint decorating code to access the TypeCtxt, which unblocks #149215
  - It might allow most early buffered attribute lints to become dyn lint diagnostics in the future, as in #147634
  - It deduplicates `IllFormedAttributeInput`

This PR does not change observable output of the compiler, as can be seen by no uitests being affected.

r? `@jdonszelmann`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants