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

rustc: Filter out bogus extern crate warnings #46405

Merged
merged 1 commit into from Dec 2, 2017

Conversation

Projects
None yet
6 participants
@alexcrichton
Member

alexcrichton commented Nov 30, 2017

Rustdoc has for some time now used the "everybody loops" pass in the compiler to
avoid typechecking and otherwise avoid looking at implementation details.
In #46115 the placement of this pass was pushed back in the compiler to after
macro expansion to ensure that it works with macro-expanded code as well. This
in turn caused the regression in #46271.

The bug here was that the resolver was producing def_id instances for
"possibly unused extern crates" which would then later get processed during
typeck to actually issue lint warnings. The problem was that after resolution
these def_id nodes were actually removed from the AST by the "everybody loops"
pass. This later, when we tried to take a look at def_id, caused the compiler
to panic.

The fix applied here is a bit of a heavy hammer which is to just, in this one
case, ignore the extern crate lints if the def_id looks "bogus" in any way
(basically if it looks like the node was removed after resolution). The real
underlying bug here is probably that the "everybody loops" AST pass is being
stressed to much beyond what it was originally intended to do, but this should
at least fix the ICE for now...

Closes #46271

rustc: Filter out bogus extern crate warnings
Rustdoc has for some time now used the "everybody loops" pass in the compiler to
avoid typechecking and otherwise avoid looking at implementation details.
In #46115 the placement of this pass was pushed back in the compiler to after
macro expansion to ensure that it works with macro-expanded code as well. This
in turn caused the regression in #46271.

The bug here was that the resolver was producing `def_id` instances for
"possibly unused extern crates" which would then later get processed during
typeck to actually issue lint warnings. The problem was that *after* resolution
these `def_id` nodes were actually removed from the AST by the "everybody loops"
pass. This later, when we tried to take a look at `def_id`, caused the compiler
to panic.

The fix applied here is a bit of a heavy hammer which is to just, in this one
case, ignore the `extern crate` lints if the `def_id` looks "bogus" in any way
(basically if it looks like the node was removed after resolution). The real
underlying bug here is probably that the "everybody loops" AST pass is being
stressed to much beyond what it was originally intended to do, but this should
at least fix the ICE for now...

Closes #46271
@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Nov 30, 2017

Collaborator

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Nov 30, 2017

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Nov 30, 2017

Contributor

@bors r+ rollup

Contributor

estebank commented Nov 30, 2017

@bors r+ rollup

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 30, 2017

Contributor

📌 Commit 4e74eb5 has been approved by estebank

Contributor

bors commented Nov 30, 2017

📌 Commit 4e74eb5 has been approved by estebank

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Dec 1, 2017

Member

Since it's a critical bug, I give it a high priority.

@bors: p=10

Member

GuillaumeGomez commented Dec 1, 2017

Since it's a critical bug, I give it a high priority.

@bors: p=10

kennytm added a commit to kennytm/rust that referenced this pull request Dec 1, 2017

Rollup merge of rust-lang#46405 - alexcrichton:fix-rustdoc, r=estebank
rustc: Filter out bogus extern crate warnings

Rustdoc has for some time now used the "everybody loops" pass in the compiler to
avoid typechecking and otherwise avoid looking at implementation details.
In rust-lang#46115 the placement of this pass was pushed back in the compiler to after
macro expansion to ensure that it works with macro-expanded code as well. This
in turn caused the regression in rust-lang#46271.

The bug here was that the resolver was producing `def_id` instances for
"possibly unused extern crates" which would then later get processed during
typeck to actually issue lint warnings. The problem was that *after* resolution
these `def_id` nodes were actually removed from the AST by the "everybody loops"
pass. This later, when we tried to take a look at `def_id`, caused the compiler
to panic.

The fix applied here is a bit of a heavy hammer which is to just, in this one
case, ignore the `extern crate` lints if the `def_id` looks "bogus" in any way
(basically if it looks like the node was removed after resolution). The real
underlying bug here is probably that the "everybody loops" AST pass is being
stressed to much beyond what it was originally intended to do, but this should
at least fix the ICE for now...

Closes rust-lang#46271
@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Dec 1, 2017

Member

@GuillaumeGomez The p=10 has no effect when rollup is still active 😕.

Member

kennytm commented Dec 1, 2017

@GuillaumeGomez The p=10 has no effect when rollup is still active 😕.

bors added a commit that referenced this pull request Dec 1, 2017

Auto merge of #46430 - kennytm:rollup, r=kennytm
Rollup of 13 pull requests

- Successful merges: #45880, #46280, #46373, #46376, #46385, #46386, #46387, #46392, #46400, #46401, #46405, #46412, #46421
- Failed merges:

@bors bors merged commit 4e74eb5 into rust-lang:master Dec 2, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Dec 2, 2017

Contributor

@kennytm sorry about that, didn't realize that was a side effect :-/

Contributor

estebank commented Dec 2, 2017

@kennytm sorry about that, didn't realize that was a side effect :-/

@alexcrichton alexcrichton deleted the alexcrichton:fix-rustdoc branch Jan 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment