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

fix: inner attribute followed by outer attribute causing ICE #90267

Merged
merged 2 commits into from
Oct 27, 2021
Merged

fix: inner attribute followed by outer attribute causing ICE #90267

merged 2 commits into from
Oct 27, 2021

Conversation

EliseZeroTwo
Copy link
Contributor

Fixes #87936, #88938, and #89971.

This removes the assertion that validates that there are no outer attributes following inner attributes. Where the inner attribute is invalid you get an actual error.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2021
@Mark-Simulacrum
Copy link
Member

r? @Aaron1011

@Aaron1011
Copy link
Member

Aaron1011 commented Oct 25, 2021 •
edited

This will cause incorrectly placed outer attributes to get re-ordered in some cases (e.g. impl Foo { #![inner] #[outer] } would becime #[outer] impl Foo { #![inner] } . Since compilation is going to fail anyway, I think this is reasonable.

@EliseZeroTwo Could you change the assert to a delay_span_bug to make sure that compilation never succeeds with misplaced attributes? Also, could you add a test for one of the closed issues (I can help you with writing it if you want

EDIT: Adding in a delay_span_bug call in that location would actually be pretty difficult. It's fine to just add a test to this PR.

cc @dtolnay - this is somewhat related to #90256

@EliseZeroTwo
Copy link
Contributor Author

I had attempted to add delay_span_bug but then I ran into a cyclic dependency issue which I think is why you said it would be hard to do?

I added a test for this though, it panics on the commit before mine and runs successfully after my commit.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@EliseZeroTwo EliseZeroTwo force-pushed the elisezerotwo/fix_invalid_attrs_ice branch from 3255dc4 to d4ad543 Compare October 26, 2021 10:45
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@EliseZeroTwo EliseZeroTwo force-pushed the elisezerotwo/fix_invalid_attrs_ice branch from d4ad543 to 2a7f0cb Compare October 26, 2021 12:06
@EliseZeroTwo EliseZeroTwo force-pushed the elisezerotwo/fix_invalid_attrs_ice branch from 2a7f0cb to 7f0d43a Compare October 27, 2021 08:13
@Aaron1011
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 27, 2021

📌 Commit 7f0d43a 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-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2021
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 27, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 27, 2021
@bors
Auto merge of rust-lang#90347 - matthiaskrgr:rollup-rp2ms7j, r=matthi…
4e0d397 
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#90239 (Consistent big O notation in map.rs)
 - rust-lang#90267 (fix: inner attribute followed by outer attribute causing ICE)
 - rust-lang#90288 (Add hint for people missing `TryFrom`, `TryInto`, `FromIterator` import pre-2021)
 - rust-lang#90304 (Add regression test for rust-lang#75961)
 - rust-lang#90344 (Add tracking issue number to const_cstr_unchecked)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Oct 27, 2021

⌛ Testing commit 7f0d43a with merge 4e0d397...

@bors bors merged commit 17d1742 into rust-lang:master Oct 27, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 27, 2021
This was referenced Oct 28, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aaron1011 Aaron1011 Aaron1011 left review comments

Assignees

@Aaron1011 Aaron1011

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.58.0
Development

Successfully merging this pull request may close these issues.

ICE "Found outer attribute Attribute"
7 participants
@EliseZeroTwo @rust-highfive @Mark-Simulacrum @Aaron1011 @rust-log-analyzer @bors @rustbot