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

Detect cycle errors hidden by opaques during monomorphization #115801

Merged
merged 1 commit into from Sep 19, 2023

Conversation

compiler-errors
Copy link
Member

Opaque types may reveal to projections, which themselves normalize to opaques. We don't currently normalize when checking that opaques are cyclical, and we may also not know that the opaque is cyclical until monomorphization (see tests/ui/type-alias-impl-trait/mututally-recursive-overflow.rs).

Detect cycle errors in normalize_projection_ty and report a fatal overflow (in the old solver). Luckily, this is already detected as a fatal overflow in the new solver.

Fixes #112047

@rustbot
Copy link
Collaborator

rustbot commented Sep 12, 2023

r? @davidtwco

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

@rustbot rustbot added 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. labels Sep 12, 2023
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member Author

🤦

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 12, 2023
@compiler-errors
Copy link
Member Author

@rustbot ready

This rustdoc-only hack makes me sad, but it does fix a strange TAIT/RPITIT-only post-mono error in a more graceful way than ICEing. Maybe @oli-obk can leave some thoughts too.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 18, 2023
Comment on lines +53 to +56
// Rustdoc may attempt to normalize type alias types which are not
// well-formed. Rustdoc also normalizes types that are just not
// well-formed, since we don't do as much HIR analysis (checking
// that impl vars are constrained by the signature, for example).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it do that for code that passes rustc? We are allowed to cause new errors in rustdoc if the code also wouldn't compile in rustc.

Copy link
Contributor

@oli-obk oli-obk Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a more graceful way than ICEing

ah, that's the issue. hmm.. does rustdoc unwrap something instead of reporting an error? or where is the ICE coming from?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are allowed to cause new errors in rustdoc if the code also wouldn't compile in rustc

Well, the problem is that we need to do HIR analysis to make sure we don't have things like malformed and overlapping impl blocks too. These are probably not what we want to "fix" in rustdoc, since we compile rustdoc with all features enabled and stuff?

We also would need to check all type aliases as well-formed too, which we don't even do in rustc yet.

does rustdoc unwrap something instead of reporting an error? or where is the ICE coming from?

The ICE is coming from a failed projection in the QueryNormalizer.

What happens is that we hit a cycle here:

Err(ProjectionCacheEntry::Recur) => {
debug!("recur cache");
return Err(InProgress);
}

Which means we treat the projection as ambiguous and emit a projection goal with an infer var on the RHS. That causes us to hit this assertion with debug assertions enabled:

debug_assert!(!erased.has_infer(), "{erased:?}");

Or just errors out with NoSolution, which causes normalize_erasing_regions to ICE.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... yea that makes sense sadly.

Tho Rustdoc is moving towards doing wf checks in the future, so this will get cleaned up

@oli-obk
Copy link
Contributor

oli-obk commented Sep 19, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 19, 2023

📌 Commit 8fbd78c has been approved by oli-obk

It is now in the queue for this repository.

@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 Sep 19, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2023
…llaumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#113383 (style-guide: Add section on bugs, and resolving bugs)
 - rust-lang#115499 (rustc_target/riscv: Fix passing of transparent unions with only one non-ZST member)
 - rust-lang#115801 (Detect cycle errors hidden by opaques during monomorphization)
 - rust-lang#115947 (Custom code classes in docs warning)
 - rust-lang#115957 (fix mismatched symbols)
 - rust-lang#115958 (explain mysterious addition in float minimum/maximum)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0060db7 into rust-lang:master Sep 19, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 19, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2023
Rollup merge of rust-lang#115801 - compiler-errors:async-cycle-mono, r=oli-obk

Detect cycle errors hidden by opaques during monomorphization

Opaque types may reveal to projections, which themselves normalize to opaques. We don't currently normalize when checking that opaques are cyclical, and we may also not know that the opaque is cyclical until monomorphization (see `tests/ui/type-alias-impl-trait/mututally-recursive-overflow.rs`).

Detect cycle errors in `normalize_projection_ty` and report a fatal overflow (in the old solver). Luckily, this is already detected as a fatal overflow in the new solver.

Fixes rust-lang#112047
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to normalize async_fn_in_trait ICE for indirect recursion of async trait method calls
6 participants