move #[macro_export] on declarative macro check to rustc_attr_parsing - #163314
darkraider01 wants to merge 4 commits into
Conversation
|
Some changes occurred in compiler/rustc_attr_parsing cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
rustbot
added
A-attributes
|
Thanks for the pull request, and welcome! The Rust Project has assigned @oli-obk (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks. Please see the contribution instructions and our LLM policy for more information. Why was this reviewer chosen?The reviewer was selected based on:
|
|
For which parts of this PR did you use an LLM? |
oli-obk
added
S-waiting-on-author
|
i used codex sol 6 for giving it implementation steps, and how to approach, and code written part is by me since it wouldnt edit the codebase . Reviewed it via codex and asked it to make a PR description. So thats the llm usage in this pr |
|
Please read our LLM policy and edit your PR to be within the rules. |
|
sorry, updated the PR description, its the only part which it LLM touched by itself. |
This comment has been minimized.
This comment has been minimized.
88f9d76 to
b99f1e3
Compare
|
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. |
|
r? me |
JonathanBrouwer
added
S-waiting-on-review
|
|
||
| let attr_span = cx.attr_span; | ||
| if cx.target == Target::MacroDef | ||
| && let Some(item) = cx.target_item |
There was a problem hiding this comment.
This would silently not emit anything if target_item is None, which should never happen. Can you make it panic instead?
There was a problem hiding this comment.
the resolver’s early pass uses ShouldEmit::Nothing and passes no target_item and the guard skips that pass because an unconditional unwrap caused an ICE. The normal lint-emitting path still panics if target_item is unexpectedly missing.
There was a problem hiding this comment.
I think the real problem is then that finalize_checks is even being called when ShouldEmit::Nothing is active.
Could you fix that and panic if target_item is None?
@rustbot author
There was a problem hiding this comment.
that deferred checks now skip the resolver’s ShouldEmit::Nothing pass, and that the normal path still panics if target_item is unexpectedly missing.
| pub(crate) target: Target, | ||
|
|
||
| /// The AST item these attributes were applied to, when the target is an item. | ||
| pub(crate) target_item: Option<&'p rustc_ast::ast::Item>, |
There was a problem hiding this comment.
I'm not sure how I feel about moving target_item here, I think it's unneccesary. Can you move it back, and place the lint emitting code in the finalize_check function?
There was a problem hiding this comment.
target_item is back in FinalizeCheckContext, and the unecessary SharedContext changes were removed. The lint check now runs in finalize_check.
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
b99f1e3 to
7f542b6
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot ready |
rustbot
added
S-waiting-on-review
This branch has not been deployed
5 participants
Summary:
Moves the declarvative mactor
#[macro_export]check to attribute parsing inrustc_attr_parsing, which makestarget_itemavailable in theSharedContextso that the parser can check for!macro_def.macro_rules. Also removes the old check fromrustc_passes::check_attr.Tests:
./x test tests/ui/attributes/macro_export_on_decl_macro.rsPart of rust-lang/rust#153101.