Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Do not collect tokens for doc comments #78782

Merged
merged 1 commit into from
Nov 12, 2020
Merged

Do not collect tokens for doc comments #78782

merged 1 commit into from
Nov 12, 2020

Conversation

petrochenkov
Copy link
Contributor

Doc comment is a single token and AST has all the information to re-create it precisely.
Doc comments are also responsible for majority of calls to collect_tokens (with num_calls == 1 and num_calls == 0, cc #78736).

(I also moved token collection into fn parse_attribute to deduplicate code a bit.)

r? @Aaron1011

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2020
@petrochenkov petrochenkov mentioned this pull request Nov 5, 2020
Merged
@petrochenkov
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 5, 2020

⌛ Trying commit 209c152a7c8a212c1edaa562db7270e7829da760 with merge 7dcbd145ebb5dcf64e4f6b51c333208da42c56d7...

@petrochenkov
Copy link
Contributor Author

See also #60930 and issues/PRs linked to it, that's how we made doc comments almost as cheap as plain comments. Recent token collection changes turned out to be a regression from that state.

@bors
Copy link
Contributor

bors commented Nov 5, 2020

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

@rust-timer
Copy link
Collaborator

Queued 7dcbd145ebb5dcf64e4f6b51c333208da42c56d7 with parent b1d9f31, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (7dcbd145ebb5dcf64e4f6b51c333208da42c56d7): 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 modify labels: +S-waiting-on-review -S-waiting-on-perf

@jyn514 jyn514 added A-parser Area: The parsing of Rust source code to an AST. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-compiletime Issue: Problems and improvements with respect to compile times. labels Nov 6, 2020
@Aaron1011
Copy link
Member

r=me once #78712 lands, and this is updated to call visit_lazy_tts for attribute tokens.

@Aaron1011 Aaron1011 added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2020
@petrochenkov
Copy link
Contributor Author

@bors r=Aaron1011

@bors
Copy link
Contributor

bors commented Nov 8, 2020

📌 Commit 12de1e8 has been approved by Aaron1011

@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-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Nov 8, 2020
@bors
Copy link
Contributor

bors commented Nov 9, 2020

⌛ Testing commit 12de1e8 with merge 2228bc15ce0de4da8260d0b0a63f0eb1f85fe1a9...

@bors
Copy link
Contributor

bors commented Nov 9, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 9, 2020
@petrochenkov petrochenkov mentioned this pull request Nov 9, 2020
Closed
@petrochenkov
Copy link
Contributor Author

#78665 - spurious, but known to fail often.
Let's @bors retry anyway.

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 9, 2020
@Aaron1011
Copy link
Member

@bors retry

@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 Nov 9, 2020
@bors
Copy link
Contributor

bors commented Nov 10, 2020

⌛ Testing commit 12de1e8 with merge fffe6ad56a1d04e9c54a1085a47f2f730e01f6b6...

@bors
Copy link
Contributor

bors commented Nov 11, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 11, 2020
@petrochenkov
Copy link
Contributor Author

#78665 again.
@bors retry

@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 Nov 11, 2020
@bors
Copy link
Contributor

bors commented Nov 11, 2020

⌛ Testing commit 12de1e8 with merge 6fbdeeb3268e0e28019c13d56861a5ed43c990be...

@bors
Copy link
Contributor

bors commented Nov 11, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 11, 2020
@petrochenkov
Copy link
Contributor Author

@bors retry

@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 Nov 11, 2020
@bors
Copy link
Contributor

bors commented Nov 12, 2020

⌛ Testing commit 12de1e8 with merge 5a6a41e...

@bors
Copy link
Contributor

bors commented Nov 12, 2020

☀️ Test successful - checks-actions
Approved by: Aaron1011
Pushing 5a6a41e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 12, 2020
@bors bors merged commit 5a6a41e into rust-lang:master Nov 12, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 12, 2020
flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 20, 2020
@bors
Auto merge of rust-lang#78782 - petrochenkov:nodoctok, r=Aaron1011
92ece84 
Do not collect tokens for doc comments

Doc comment is a single token and AST has all the information to re-create it precisely.
Doc comments are also responsible for majority of calls to `collect_tokens` (with `num_calls == 1` and `num_calls == 0`, cc rust-lang#78736).

(I also moved token collection into `fn parse_attribute` to deduplicate code a bit.)

r? `@Aaron1011`
calebcartwright
calebcartwright reviewed Nov 25, 2020
View reviewed changes
compiler/rustc_parse/src/parser/attr.rs
Comment on lines -91 to -96
pub fn parse_attribute(&mut self, permit_inner: bool) -> PResult<'a, ast::Attribute> {
debug!("parse_attribute: permit_inner={:?} self.token={:?}", permit_inner, self.token);
let inner_parse_policy =
if permit_inner { InnerAttrPolicy::Permitted } else { DEFAULT_INNER_ATTR_FORBIDDEN };
self.parse_attribute_with_inner_parse_policy(inner_parse_policy)
}
Copy link
Member

Choose a reason for hiding this comment

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

@petrochenkov - we were utilizing this function on the rustfmt side because we do some special case parsing with cfg_if macros to ensure we can properly format out of line mods imported within cfg_if blocks. We've got a broken toolstate (#79407) that will require we bump the rustc-ap crates within rustfmt, and doing so will pull in this change that is breaking for us.

At least as a tactical workaround, would it be alright with you if I opened a PR that added back a public helper/wrapper function we could consume, or alternatively one that made the new parse_attribute function public along with InnerAttrPolicy?

Definitely open to any alternatives as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@calebcartwright
Yes, feel free to make parse_attribute and InnerAttrPolicy public if rustfmt needs it.

@calebcartwright calebcartwright mentioned this pull request Nov 26, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 27, 2020
@bors
Auto merge of rust-lang#79433 - calebcartwright:parse-attr-vis, r=pet…
361543d 
…rochenkov

rustc_parse: restore public visibility on parse_attribute

Make `parse_attribute` public as rustfmt is a downstream consumer. Refs rust-lang#78782 (comment)

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

@calebcartwright calebcartwright calebcartwright left review comments

Assignees

@Aaron1011 Aaron1011

Labels
A-parser Area: The parsing of Rust source code to an AST. I-compiletime Issue: Problems and improvements with respect to compile times. merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@petrochenkov @rust-timer @bors @Aaron1011 @calebcartwright @rust-highfive @jyn514 @rustbot