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.

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

Ban custom inner attributes in expressions and statements #83488

Merged
merged 2 commits into from
Mar 26, 2021

Conversation

Aaron1011
Copy link
Member

Split out from #82608

Custom inner attributes are unstable, so this won't break any stable users.
This allows us to speed up token collection, and avoid a redundant call to collect_tokens_no_attrs when parsing an Expr that has outer attributes.

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 25, 2021
compiler/rustc_expand/src/expand.rs Show resolved Hide resolved
compiler/rustc_expand/src/expand.rs Outdated Show resolved Hide resolved
src/test/ui/proc-macro/inner-attrs.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

One more question - what happens with inner #![cfg]s and #![cfg_attr]s in expressions?
They are stable and cfg_eval and derive still need to know their token ranges to be able to process them.

@petrochenkov 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 Mar 25, 2021
@Aaron1011
Copy link
Member Author

@petrochenkov: In PR #82608, I plan to implement your earlier suggestion of re-parsing with extra collection when we expand derive/cfg_eval. By banning custom inner attributes (which need token collection in order to remove the attribute from the token stream), we can bail out early during normal parsing of expressions without outer attributes.

@Aaron1011 Aaron1011 force-pushed the ban-expr-inner-attrs branch from 0353306 to 7504b9b Compare March 25, 2021 22:06
@Aaron1011
Copy link
Member Author

@petrochenkov: I've addressed your comments.

@Aaron1011 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 Mar 26, 2021
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 26, 2021

📌 Commit 7504b9b has been approved by petrochenkov

@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 Mar 26, 2021
@petrochenkov petrochenkov added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Mar 26, 2021
@bors
Copy link
Contributor

bors commented Mar 26, 2021

⌛ Testing commit 7504b9b with merge 5e65467...

@bors
Copy link
Contributor

bors commented Mar 26, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 26, 2021
@bors bors merged commit 5e65467 into rust-lang:master Mar 26, 2021
@rustbot rustbot added this to the 1.53.0 milestone Mar 26, 2021
@klensy
Copy link
Contributor

klensy commented Mar 26, 2021

This ICE'd in perf: https://perf.rust-lang.org/status.html scroll down to see.

Perf runner don't report ICE's anywhere?
Old syn version from perf pool:

Compiling syn v0.11.11 (/tmp/.tmpnHQAja) ...

thread 'rustc' panicked at 'found unstable fingerprints for evaluate_obligation(e3352ed64d6e2ccd-c82ee1c6b3ce2c20): Ok(EvaluatedToOk)', /rustc/b8719c51e0e44483cff9b6975a830f6e51812a48/compiler/rustc_query_system/src/query/plumbing.rs:593:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.53.0-nightly (b8719c51e 2021-03-26) running on x86_64-unknown-linux-gnu

note: compiler flags: -Z self-profile=/tmp/.tmpnHQAja/self-profile-output -C opt-level=3 -C embed-bitcode=no -C incremental --crate-type lib

note: some of the compiler flags provided by cargo are hidden

query stack during panic:
#0 [evaluate_obligation] evaluating trait selection obligation `quote::Tokens: std::marker::Unpin`
#1 [is_unpin_raw] computing whether `quote::Tokens` is `Unpin`
end of query stack
warning: 49 warnings emitted

thread 'main' panicked at 'assertion failed: status.success()', collector/src/rustc-fake.rs:76:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: could not compile `syn`

@Aaron1011
Copy link
Member Author

It looks like that ICE is actually coming from the current nightly. I thought that #83220 would have fixed that, but it's apparently a different issue (there are no lifetimes involved).

@klensy
Copy link
Contributor

klensy commented Mar 26, 2021

Wait, perf runner started running this pr again.

Last perf run ICE'd on syn too, so this is not this pr root if problem.

@Aaron1011
Copy link
Member Author

Opened #83538 to track resolving the syn crash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants