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 ICE with buffered lint referring to AST node deleted by everybody_loops #89500

Merged
merged 1 commit into from
Oct 4, 2021
Merged

Fix ICE with buffered lint referring to AST node deleted by everybody_loops #89500

merged 1 commit into from
Oct 4, 2021

Conversation

FabianWolff
Copy link
Contributor

Fixes #87308. Note the following comment:

rust/compiler/rustc_lint/src/early.rs

Lines 415 to 417 in 08759c6

// All of the buffered lints should have been emitted at this point.
// If not, that means that we somehow buffered a lint for a node id
// that was not lint-checked (perhaps it doesn't exist?). This is a bug.

As it turns out, this is not always a bug, because -Zunpretty=everybody_loops causes a lot of AST nodes to be deleted, and thus some buffered lints will refer to non-existent node ids. To fix this, my changes simply ignore buffered lints if -Zunpretty=everybody_loops is enabled, which, from my understanding, shouldn't be a big issue because it only affects pretty-printing. Of course, a more elegant solution would only ignore buffered lints that actually point at deleted node ids, but I haven't figured out an easy way of achieving this.

For the concrete example in #87308, the buffered lint is created here with the lint_node_id from here, i.e. it points at the macro expansion, which then gets deleted by ReplaceBodyWithLoop here.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 3, 2021
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 4, 2021

📌 Commit a28a78f 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 Oct 4, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 4, 2021
@Manishearth
Rollup merge of rust-lang#89500 - FabianWolff:issue-87308, r=petroche…
ecd5bb8 
…nkov

Fix ICE with buffered lint referring to AST node deleted by everybody_loops

Fixes rust-lang#87308. Note the following comment:
https://github.com/rust-lang/rust/blob/08759c691e2e9799a3c6780ffdf910240ebd4a6b/compiler/rustc_lint/src/early.rs#L415-L417

As it turns out, this is not _always_ a bug, because `-Zunpretty=everybody_loops` causes a lot of AST nodes to be deleted, and thus some buffered lints will refer to non-existent node ids. To fix this, my changes simply ignore buffered lints if `-Zunpretty=everybody_loops` is enabled, which, from my understanding, shouldn't be a big issue because it only affects pretty-printing. Of course, a more elegant solution would only ignore buffered lints that actually point at deleted node ids, but I haven't figured out an easy way of achieving this.

For the concrete example in rust-lang#87308, the buffered lint is created [here](https://github.com/rust-lang/rust/blob/08759c691e2e9799a3c6780ffdf910240ebd4a6b/compiler/rustc_expand/src/mbe/macro_rules.rs#L145-L151) with the `lint_node_id` from [here](https://github.com/rust-lang/rust/blob/08759c691e2e9799a3c6780ffdf910240ebd4a6b/compiler/rustc_expand/src/mbe/macro_rules.rs#L319), i.e. it points at the macro _expansion_, which then gets deleted by `ReplaceBodyWithLoop` [here](https://github.com/rust-lang/rust/blob/08759c691e2e9799a3c6780ffdf910240ebd4a6b/compiler/rustc_interface/src/passes.rs#L377).
@Manishearth Manishearth mentioned this pull request Oct 4, 2021
Closed
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 4, 2021
@Manishearth
Rollup merge of rust-lang#89500 - FabianWolff:issue-87308, r=petroche…
357f09c 
…nkov

Fix ICE with buffered lint referring to AST node deleted by everybody_loops

Fixes rust-lang#87308. Note the following comment:
https://github.com/rust-lang/rust/blob/08759c691e2e9799a3c6780ffdf910240ebd4a6b/compiler/rustc_lint/src/early.rs#L415-L417

As it turns out, this is not _always_ a bug, because `-Zunpretty=everybody_loops` causes a lot of AST nodes to be deleted, and thus some buffered lints will refer to non-existent node ids. To fix this, my changes simply ignore buffered lints if `-Zunpretty=everybody_loops` is enabled, which, from my understanding, shouldn't be a big issue because it only affects pretty-printing. Of course, a more elegant solution would only ignore buffered lints that actually point at deleted node ids, but I haven't figured out an easy way of achieving this.

For the concrete example in rust-lang#87308, the buffered lint is created [here](https://github.com/rust-lang/rust/blob/08759c691e2e9799a3c6780ffdf910240ebd4a6b/compiler/rustc_expand/src/mbe/macro_rules.rs#L145-L151) with the `lint_node_id` from [here](https://github.com/rust-lang/rust/blob/08759c691e2e9799a3c6780ffdf910240ebd4a6b/compiler/rustc_expand/src/mbe/macro_rules.rs#L319), i.e. it points at the macro _expansion_, which then gets deleted by `ReplaceBodyWithLoop` [here](https://github.com/rust-lang/rust/blob/08759c691e2e9799a3c6780ffdf910240ebd4a6b/compiler/rustc_interface/src/passes.rs#L377).
@Manishearth Manishearth mentioned this pull request Oct 4, 2021
Closed
@workingjubilee workingjubilee mentioned this pull request Oct 4, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2021
@bors
Auto merge of rust-lang#89530 - workingjubilee:rollup-ua14iq6, r=work…
003d8d3 
…ingjubilee

Rollup of 13 pull requests

Successful merges:

 - rust-lang#83655 ([aarch64] add target feature outline-atomics)
 - rust-lang#87091 (implement advance_(back_)_by on more iterators)
 - rust-lang#88451 (Fix an ICE caused by type mismatch errors being ignored)
 - rust-lang#88452 (VecDeque: improve performance for From<[T; N]>)
 - rust-lang#89400 (Improve wording of `map_or_else` docs)
 - rust-lang#89407 (Recommend running `cargo clean` in E0514 output)
 - rust-lang#89443 (Include the length in BTree hashes)
 - rust-lang#89444 (rustdoc: use slice::contains instead of open-coding it)
 - rust-lang#89447 (Improve error message for missing angle brackets in `[_]::method`)
 - rust-lang#89453 (Consistently use 'supertrait'.)
 - rust-lang#89483 (Practice diagnostic message convention)
 - rust-lang#89500 (Fix ICE with buffered lint referring to AST node deleted by everybody_loops)
 - rust-lang#89508 (Stabilize `const_panic`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3d4467d into rust-lang:master Oct 4, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@petrochenkov petrochenkov

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.57.0
Development

Successfully merging this pull request may close these issues.

ICE: hygienic-label-1.rs -Zunpretty=everybody_loops : failed to process buffered lint here (dummy = false)
5 participants
@FabianWolff @rust-highfive @petrochenkov @bors @rustbot