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

Remove assert for unstable expectation IDs #94968

Closed

Conversation

xFrednet
Copy link
Member

The assert requires that the check_crate query was executed before HandlerInner is dropped. This will not always be the case and can cause ICEs.

Closes #94953

The ICE was introduced in #94670

The assert requires that the `check_crate` query was executed before the `HandlerInner` is dropped. This will not always be the case and can cause ICEs.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 15, 2022
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Mar 15, 2022
@Mark-Simulacrum
Copy link
Member

It is not obvious to me that removing the assert here is the right step, instead of fixing whatever is causing us to not handle the stashed IDs. I'm not very familiar with this infrastructure though so maybe r? @wesleywiser?

@wesleywiser
Copy link
Member

It is not obvious to me that removing the assert here is the right step, instead of fixing whatever is causing us to not handle the stashed IDs.

Agreed, I think the assert is important, so we don't accidentally forget about stashed diagnostics leading to them never being displayed in some case. Without the assert, it will be difficult to track down why they aren't being emitted. If this is only happening with -Zunpretty=expanded, then I don't this bug is super urgent and it would be better to do the refactorings you mentioned and then solve this in a more principled manner.

@xFrednet
Copy link
Member Author

I believe that this will remain a problem even with the planned refactorings. However, I'm okay with leaving the issue open and revisiting it later :). I'll close this PR meanwhile. :)

@xFrednet xFrednet closed this Mar 25, 2022
@xFrednet xFrednet deleted the 94953-rm-unstable-id-assert branch June 3, 2022 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
5 participants