Synthesize tokens for attributes - #160133
Draft
nnethercote wants to merge 7 commits into
Draft
Conversation
Prior to rust-lang#159633, the compiler made an invalid suggestion: ``` help: must be of the form | 1 - #[cfg_attr(true, cfg_attr(true, 3))] 1 + #[cfg_attr(true, #[cfg_attr(predicate, attr1, attr2, ...)])] ``` Now it is correct: ``` help: must be of the form | 1 - #[cfg_attr(true, cfg_attr(true, 3))] 1 + #[cfg_attr(true, cfg_attr(predicate, attr1, attr2, ...))] ``` But there was no test for it. This commit adds one.
This commit adds a new test for some uncovered cases. The thing to note here is the varying correctness of the `help:` lines. This is correct: ``` LL | #[inline] | ^^^^^^^^^ help: remove this attribute ``` The following two are half correct. They give the right idea but if applied literally they will produce invalid syntax. ``` LL | #[cfg_attr(true, inline)] | ^^^^^^ help: remove this attribute LL | #[cfg_attr(true, inline, deprecated)] | ^^^^^^ help: remove this attribute ``` For this reason, this commit also marks these help message as not machine-applicable.
Currently it covers the entire span except in the case of `cfg_attr`-expanded attributes. This commit eliminates that exception. There is one place (`fake_token_stream_for_file_mod`) where the code needs to know if an attr is from a `cfg_attr` expansion. So the commit adds a new boolean `from_cfg_attr` field to `AttrItem` for this purpose. This is simpler and more reliable than consulting spans to determine `cfg_attr`-ness. One test is affected, with spans covering an entire attribute in help suggestions. Two of the suggestions become better (removing the entire attribute is more correct), two of them become worse. Not a big deal either way.
Instead of collecting them. In the vast majority of cases we can synthesize tokens that are indistinguishable from collected tokens. This is feasible because (a) attributes have a rigid micro-grammar and (b) we record tokens for for the internal pieces in the interesting cases (e.g. in `AttrArgs` within `AttrItem::args`). This requires adding a flag `use_precise_delim_token_spans` to distinguish the common case (perfect fidelity) from the uncommon case (token spans are degraded). This is a small perf win. It also has some other benefits. - `NormalAttr` shrinks slightly because the `tokens` field is removed. This means `NormalAttr` is now a trivial wrapper around `AttrItem` (and will be removed in a subsequent commit). - `Attribute` no longer needs a (no-op) impl of `HasAttrs`. - Builtin attributes had grown synthesized token streams (e.g. rust-lang#159277). These are no longer needed and `mk_attr_tokens` is removable. - `expand_cfg_attr_item` no longer has to do its token surgery. The commit modifies the attr-complex-fn.rs test to demonstrate how `#`/`!`/`[`/`]` token spans degrade in the uncommon case. (This case is so uncommon that no existing tests triggered it.)
rustbot
added
A-attributes
Contributor
Author
|
@petrochenkov: this may be controversial, but I wanted to try it out, and to see what you think. It was partly in response to the hand-written token streams added for internal attributes in #159277, which I don't like. The two "Synthesize" commits are the most interesting, if you want to start there. |
Contributor
Author
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
rust-bors Bot
pushed a commit
that referenced
this pull request
Jul 29, 2026
Synthesize tokens for attributes
This comment has been minimized.
This comment has been minimized.
The previous commit did all the hard work. Various places are slightly simpler, removing a `collect_tokens` usage, a `WithTokens` usage, `WithTokens::map`, a `ForceCollect` argument, and several `item.tokens.is_none()` assertions. The output of `tests/ui/proc-macro/capture-macro-rules-invoke.rs` also changed slightly for the better. The old output had `r#unsafe` even though the test's code only has `unsafe`.
It's now just a zero-value wrapper around `AttrItem`.
nnethercote
force-pushed
the
synth-attr-tokens
branch
from
July 29, 2026 10:01
df6585a to
54485e0
Compare
rustbot
added
T-clippy
petrochenkov
added
S-waiting-on-review
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Contributor
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Finished benchmarking commit (2559685): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.5%, secondary -1.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.4%, secondary -3.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 490.002s -> 488.859s (-0.23%) |
rustbot
removed
the
S-waiting-on-perf
Contributor
It is the "almost" part that bothers me most.
And in response to that we say "yeah, we technically can preserve everything correctly, but we'll use an approximation anyway". |
| /// For internally constructed spans (`mk_attr_*`) the exact meaning may differ. | ||
| pub span: Span, | ||
| /// Was this created by expanding a `#[cfg_attr(pred, foo)]` attribute? | ||
| pub from_cfg_attr: bool, |
Contributor
There was a problem hiding this comment.
Note, that this is also best effort.
E.g. in
#[identity_macro]
#[cfg_attr(true, inline)]
fn foo() {}the from_cfg_attr flag will be lost, because identity_macro will receive #[inline] fn foo() {} as tokens, and will re-emit it as tokens too, to be parsed again.
(This is very similar to cfg and cfg_attr traces.)
petrochenkov
added
S-waiting-on-author
Contributor
|
☔ The latest upstream changes (presumably #160238) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instead of collecting them. This is simpler and faster and almost always gives identical results. Details in individual commits.