-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Fix rare ICE during typeck in rustdoc scrape_examples #90349
Conversation
match types.node_type_opt(f.hir_id) { | ||
Some(ty) => (ty, ex.span), | ||
None => { | ||
return; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this can be related to overriding the typeck
query, even the overridden version still calls DEFAULT_QUERY_PROVIDERS.typeck
. Can you post the error you were seeing before?
@jyn514 the specific error I encountered was programs with const expressions in type declarations: const fn f() -> usize { 4 }
pub struct Foo([usize; foobar::f()]); Then the visitor would find the expression Specifically, this line assumes a body exists: https://github.com/rust-lang/rust/blob/master/src/librustdoc/core.rs#L290 Note that the |
I don't understand though; why isn't the type of the body available? Is this something specific to constants? If so I think the comment about overriding |
It's not that the type of the body isn't available, it's that there is no body. A type definition has a DefId, but it doesn't have a body, only functions do (AFAIK). |
Ok. That's a pre-existing bug then - can you fix that instead of special-casing constant items only for scrape-examples? Otherwise I'm worried that rustdoc will ICE elsewhere and we just haven't found the code that triggers it yet. |
Upon further investigation, it seems like So I think it's correct that the caller of typeck needs to check that the body exists, it shouldn't be handled by the typeck provider. |
@jyn514 does that resolve your concern? |
@willcrichton I think so - I'm still slightly confused how this can happen when visitor passes OnlyBodies, but not enough to block on it. Can you update the comment to say "the caller needs to ensure this is a function body" rather having a red herring about overriding typeck? |
436f6ed
to
b8ecc9f
Compare
Done! |
@bors r+ |
📌 Commit b8ecc9f has been approved by |
…n514 Fix rare ICE during typeck in rustdoc scrape_examples While testing the `--scrape-examples` extension on the [wasmtime](https://github.com/bytecodealliance/wasmtime) repository, I found some additional edge cases. Specifically, when asking to typecheck a body containing a function call, I would sometimes get an ICE if: * The body doesn't exist * The function's HIR node didn't have a type This adds checks for both of those conditions. (Also this updates a test to check that the sources of a reverse-dependency are correctly generated and linked.) r? `@jyn514`
…askrgr Rollup of 8 pull requests Successful merges: - rust-lang#89826 (Feature gate + make must_not_suspend allow-by-default) - rust-lang#89929 (Handling submodule update failures more gracefully from x.py) - rust-lang#90333 (rustdoc: remove flicker during page load) - rust-lang#90349 (Fix rare ICE during typeck in rustdoc scrape_examples) - rust-lang#90398 (Document `doc(keyword)` unstable attribute) - rust-lang#90441 (Test that promotion follows references when looking for drop) - rust-lang#90450 (Remove `rustc_hir::hir_id::HirIdVec`) - rust-lang#90452 (Remove unnecessary `Option` from `promote_candidate` return type) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
While testing the
--scrape-examples
extension on the wasmtime repository, I found some additional edge cases. Specifically, when asking to typecheck a body containing a function call, I would sometimes get an ICE if:This adds checks for both of those conditions.
(Also this updates a test to check that the sources of a reverse-dependency are correctly generated and linked.)
r? @jyn514