Require passing an `AttrWrapper` to `collect_tokens_trailing_token` #81286

Aaron1011 · 2021-01-22T21:13:12Z

This is a pure refactoring split out from #80689.
It represents the most invasive part of that PR, requiring changes in
every caller of parse_outer_attributes

In order to eagerly expand #[cfg] attributes while preserving the
original TokenStream, we need to know the range of tokens that
corresponds to every attribute target. This is accomplished by making
parse_outer_attributes return an opaque AttrWrapper struct. An
AttrWrapper must be converted to a plain AttrVec by passing it to
collect_tokens_trailing_token. This makes it difficult to accidentally
construct an AST node with attributes without calling collect_tokens_trailing_token,
since AST nodes store an AttrVec, not an AttrWrapper.

As a result, we now call collect_tokens_trailing_token for attribute
targets which only support inert attributes, such as generic arguments
and struct fields. Currently, the constructed LazyTokenStream is
simply discarded. Future PRs will record the token range corresponding
to the attribute target, allowing those tokens to be removed from an
enclosing collect_tokens_trailing_token call if necessary.

rust-highfive · 2021-01-22T21:13:17Z

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

Aaron1011 · 2021-01-22T21:13:26Z

@bors try @rust-timer queue

rust-timer · 2021-01-22T21:13:28Z

Awaiting bors try build completion.

bors · 2021-01-22T21:13:38Z

⌛ Trying commit 31748687020643dfc694f3dc9c70a6021c6b0b54 with merge 7db61ae90bf5c11a0be4801fa0c76f791029c5b4...

lcnr · 2021-01-22T21:19:47Z

r? @petrochenkov

bors · 2021-01-22T22:14:02Z

☀️ Try build successful - checks-actions
Build commit: 7db61ae90bf5c11a0be4801fa0c76f791029c5b4 (7db61ae90bf5c11a0be4801fa0c76f791029c5b4)

rust-timer · 2021-01-22T22:14:04Z

Queued 7db61ae90bf5c11a0be4801fa0c76f791029c5b4 with parent b814b63, future comparison URL.

@rustbot label: +S-waiting-on-perf

rust-timer · 2021-01-23T01:07:01Z

Finished benchmarking try commit (7db61ae90bf5c11a0be4801fa0c76f791029c5b4): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

petrochenkov · 2021-01-28T20:54:22Z

I'm unlikely to get to reviewing this until the next weekend (Feb 7).

compiler/rustc_parse/src/parser/mod.rs

compiler/rustc_parse/src/parser/expr.rs

compiler/rustc_parse/src/parser/stmt.rs

petrochenkov · 2021-02-13T13:15:27Z

compiler/rustc_parse/src/parser/stmt.rs

+                            mem::swap(stmt_attrs, &mut attrs);
+                            stmt_attrs.extend(attrs);
+                        });
+                        // Make sure we capture the token::Interpolated


Is this necessary?
The interpolated token is going to always be replaced by its underlying tokens.

Same applies to parse_item_common.

Currently, I don't think this is strictly necessary, since we'll end up with the same attributes and tokens even if we don't call collect_tokens_trailing_token.

However, this will become necessary when we add PreexpTokenStream - calling collect_tokens_trailing_token will ensure that we properly insert any new attributes into the PreexpTokenStream. That is, we need #[cfg(FALSE)] $item to get removed from the PreexpTokenStream.

Alternatively, we could remove this logic, and directly manipulate the PreexpTokenStream (once it gets added). However, I think it's cleaner to have all of the token-related attribute handling go through collect_tokens_trailing_token.

petrochenkov · 2021-02-13T13:23:23Z

compiler/rustc_parse/src/parser/pat.rs

+                    }?;
+                    ate_comma = this.eat(&token::Comma);
+                    // We just ate a comma, so there's no need to use
+                    // `TrailingToken::Comma`


I think we should migrate to this scheme everywhere and eliminate TrailingToken entirely (not in this PR).

The main obstacle to doing this will be parse_seq_to_before_token - it relies on being able to eat the separating tokens, and performs some non-trivial recovery logic. However, I have no objection to removing TrailingToken.

compiler/rustc_parse/src/parser/nonterminal.rs

This is a pure refactoring split out from rust-lang#80689. It represents the most invasive part of that PR, requiring changes in every caller of `parse_outer_attributes` In order to eagerly expand `#[cfg]` attributes while preserving the original `TokenStream`, we need to know the range of tokens that corresponds to every attribute target. This is accomplished by making `parse_outer_attributes` return an opaque `AttrWrapper` struct. An `AttrWrapper` must be converted to a plain `AttrVec` by passing it to `collect_tokens_trailing_token`. This makes it difficult to accidentally construct an AST node with attributes without calling `collect_tokens_trailing_token`, since AST nodes store an `AttrVec`, not an `AttrWrapper`. As a result, we now call `collect_tokens_trailing_token` for attribute targets which only support inert attributes, such as generic arguments and struct fields. Currently, the constructed `LazyTokenStream` is simply discarded. Future PRs will record the token range corresponding to the attribute target, allowing those tokens to be removed from an enclosing `collect_tokens_trailing_token` call if necessary.

Aaron1011 · 2021-02-13T17:50:59Z

@petrochenkov: I've addressed your comments. I've also moved AttrWrapper and some related functions to a new module, so that we actually enforce the privacy of AttrWrapper.attrs

petrochenkov · 2021-02-14T12:22:38Z

@bors r+

bors · 2021-02-14T12:22:39Z

📌 Commit 3321d70 has been approved by petrochenkov

bors · 2021-02-14T15:00:56Z

⌛ Testing commit 3321d70 with merge 5fa22fe...

bors · 2021-02-14T18:07:20Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 5fa22fe to master...

rust-highfive assigned lcnr Jan 22, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2021

rust-highfive assigned petrochenkov and unassigned lcnr Jan 22, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 22, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 23, 2021

Aaron1011 force-pushed the collect-tokens-attrs branch from 3174868 to 6922177 Compare January 23, 2021 05:17

Aaron1011 mentioned this pull request Jan 31, 2021

ICE (Missing tokens for nt) on nightly with stmt_expr_attributes and proc_macro_hygiene #81555

Closed

petrochenkov reviewed Feb 13, 2021

View reviewed changes

petrochenkov 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 Feb 13, 2021

Aaron1011 force-pushed the collect-tokens-attrs branch from 6922177 to 474d1a6 Compare February 13, 2021 17:49

This comment has been minimized.

Sign in to view

Address review comments

3321d70

Aaron1011 force-pushed the collect-tokens-attrs branch from 474d1a6 to 3321d70 Compare February 13, 2021 18:05

Aaron1011 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 Feb 13, 2021

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 Feb 14, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 14, 2021

bors merged commit 5fa22fe into rust-lang:master Feb 14, 2021

rustbot added this to the 1.52.0 milestone Feb 14, 2021

bors mentioned this pull request Feb 14, 2021

Simplify pattern grammar, improve or-pattern diagnostics #81869

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require passing an `AttrWrapper` to `collect_tokens_trailing_token` #81286

Require passing an `AttrWrapper` to `collect_tokens_trailing_token` #81286

Aaron1011 commented Jan 22, 2021 •

edited

Loading

rust-highfive commented Jan 22, 2021

Aaron1011 commented Jan 22, 2021

rust-timer commented Jan 22, 2021

bors commented Jan 22, 2021

lcnr commented Jan 22, 2021

bors commented Jan 22, 2021

rust-timer commented Jan 22, 2021

rust-timer commented Jan 23, 2021

petrochenkov commented Jan 28, 2021

petrochenkov Feb 13, 2021

petrochenkov Feb 13, 2021

Aaron1011 Feb 13, 2021

petrochenkov Feb 13, 2021

Aaron1011 Feb 13, 2021

Aaron1011 commented Feb 13, 2021

This comment has been minimized.

petrochenkov commented Feb 14, 2021

bors commented Feb 14, 2021

bors commented Feb 14, 2021

bors commented Feb 14, 2021

Require passing an AttrWrapper to collect_tokens_trailing_token #81286

Require passing an AttrWrapper to collect_tokens_trailing_token #81286

Conversation

Aaron1011 commented Jan 22, 2021 • edited Loading

rust-highfive commented Jan 22, 2021

Aaron1011 commented Jan 22, 2021

rust-timer commented Jan 22, 2021

bors commented Jan 22, 2021

lcnr commented Jan 22, 2021

bors commented Jan 22, 2021

rust-timer commented Jan 22, 2021

rust-timer commented Jan 23, 2021

petrochenkov commented Jan 28, 2021

petrochenkov Feb 13, 2021

Choose a reason for hiding this comment

petrochenkov Feb 13, 2021

Choose a reason for hiding this comment

Aaron1011 Feb 13, 2021

Choose a reason for hiding this comment

petrochenkov Feb 13, 2021

Choose a reason for hiding this comment

Aaron1011 Feb 13, 2021

Choose a reason for hiding this comment

Aaron1011 commented Feb 13, 2021

This comment has been minimized.

petrochenkov commented Feb 14, 2021

bors commented Feb 14, 2021

bors commented Feb 14, 2021

bors commented Feb 14, 2021

Require passing an `AttrWrapper` to `collect_tokens_trailing_token` #81286

Require passing an `AttrWrapper` to `collect_tokens_trailing_token` #81286

Aaron1011 commented Jan 22, 2021 •

edited

Loading