-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Compute a better lint_node_id
during expansion
#87146
Conversation
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3d3f11e
to
1f2711b
Compare
This comment has been minimized.
This comment has been minimized.
1f2711b
to
6b9b1ae
Compare
This comment has been minimized.
This comment has been minimized.
@petrochenkov: This PR is now ready for review |
There are about 30 I think it's enough to track IDs that can be immediate parents of macro invocations (with "macro invocations" including uses of macro attributes). If we are considering attributes, then some interesting situations may arise.
Apparently it does, because (I'll check which nodes can be immediate parents of macro invocations tomorrow.) |
Looks like I was wrong, pretty much anything with a However, it's not necessary to track them all, we only need to track parent nodes (not necessarily immediate) supporting attributes like |
4ad9fe7
to
cb862d5
Compare
@petrochenkov: I've added the |
@bors r+ |
📌 Commit cb862d5cac071a21735886dac9878ffb4562ae9f has been approved by |
☔ The latest upstream changes (presumably #86676) made this pull request unmergeable. Please resolve the merge conflicts. |
When we need to emit a lint at a macro invocation, we currently use the `NodeId` of its parent definition (e.g. the enclosing function). This means that any `#[allow]` / `#[deny]` attributes placed 'closer' to the macro (e.g. on an enclosing block or statement) will have no effect. This commit computes a better `lint_node_id` in `InvocationCollector`. When we visit/flat_map an AST node, we assign it a `NodeId` (earlier than we normally would), and store than `NodeId` in current `ExpansionData`. When we collect a macro invocation, the current `lint_node_id` gets cloned along with our `ExpansionData`, allowing it to be used if we need to emit a lint later on. This improves the handling of `#[allow]` / `#[deny]` for `SEMICOLON_IN_EXPRESSIONS_FROM_MACROS` and some `asm!`-related lints. The 'legacy derive helpers' lint retains its current behavior (I've inlined the now-removed `lint_node_id` function), since there isn't an `ExpansionData` readily available.
cb862d5
to
1c1c794
Compare
@bors r=petrochenkov |
📌 Commit 1c1c794 has been approved by |
⌛ Testing commit 1c1c794 with merge 9cc5500d72f84850fbde80fdf5520e2c97c1626d... |
💥 Test timed out |
@bors retry |
☀️ Test successful - checks-actions |
When we need to emit a lint at a macro invocation, we currently use the
NodeId
of its parent definition (e.g. the enclosing function). Thismeans that any
#[allow]
/#[deny]
attributes placed 'closer' to themacro (e.g. on an enclosing block or statement) will have no effect.
This commit computes a better
lint_node_id
inInvocationCollector
.When we visit/flat_map an AST node, we assign it a
NodeId
(earlierthan we normally would), and store than
NodeId
in currentExpansionData
. When we collect a macro invocation, the currentlint_node_id
gets cloned along with ourExpansionData
, allowing itto be used if we need to emit a lint later on.
This improves the handling of
#[allow]
/#[deny]
forSEMICOLON_IN_EXPRESSIONS_FROM_MACROS
and someasm!
-related lints.The 'legacy derive helpers' lint retains its current behavior
(I've inlined the now-removed
lint_node_id
function), sincethere isn't an
ExpansionData
readily available.