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

Don't ICE when deducing future output if other errors already occurred #120057

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Don't ICE when deducing future output if other errors already occurred #120057

merged 1 commit into from
Jan 18, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jan 17, 2024 •
edited
Loading

The situation can't really happen outside of erroneous code. What was interesting is that it ICEd before emitting any other diagnostics. This was because the other errors were silenced due to cycle_delay_bug cycle errors.

r? @compiler-errors

fixes #119890

@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 Jan 17, 2024
compiler-errors
compiler-errors reviewed Jan 17, 2024
View reviewed changes
compiler/rustc_hir_typeck/src/closure.rs Outdated
.find_map(|(p, s)| get_future_output(p.as_predicate(), s))?,
ty::Alias(kind, ty::AliasTy { def_id, args, .. }) => {
match kind {
ty::Projection => assert!(self.tcx.is_impl_trait_in_trait(def_id)),
Copy link
Member

Choose a reason for hiding this comment

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

This projection should have been normalized to its corresponding opaque at this point. Why isn't that happening here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming it's one of the other errors. I'll see if I can find something out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't figured out yet how we get here, but the root issue seems to be that we have a query cycle at

#0 [object_safety_violations] determining object safety of trait `Foo`
#1 [check_is_object_safe] checking if trait `Foo` is object safe
#2 [fn_sig] computing function signature of `Foo::foo`

The fn_sig query tries to emit an error. But to get the information to emit a good diagnostic, it ends up triggering a cycle error, thus hiding the as-of-yet unreported diagnostic and instead cycle_delay_bugging out with an error fn signature.

Copy link
Member

Choose a reason for hiding this comment

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

ok thats fine i guess

could you delay this as a bug saying something like "this projection should have been projected to an opaque type"?

r=me after

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have found an entirely different solution. How do you like this one?

@oli-obk oli-obk changed the title Enable deducing future output for impl trait in trait Treat errors in signatures as an object safety violation Jan 17, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2024

Could not assign reviewer from: compiler-errors.
User(s) compiler-errors are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@oli-obk oli-obk changed the title Treat errors in signatures as an object safety violation Don't ICE when deducing future output if other errors already occurred Jan 17, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2024

Could not assign reviewer from: compiler-errors.
User(s) compiler-errors are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 17, 2024

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Jan 17, 2024

📌 Commit f1ef930 has been approved by compiler-errors

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 Jan 17, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 18, 2024
@matthiaskrgr
Rollup merge of rust-lang#120057 - oli-obk:not_sure_wtf_is_going_on, …
5f850f5 
…r=compiler-errors

Don't ICE when deducing future output if other errors already occurred

The situation can't really happen outside of erroneous code. What was interesting is that it ICEd before emitting any other diagnostics. This was because the other errors were silenced due to cycle_delay_bug cycle errors.

r? `@compiler-errors`

fixes rust-lang#119890
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 18, 2024
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 18, 2024
@matthiaskrgr
Rollup merge of rust-lang#120057 - oli-obk:not_sure_wtf_is_going_on, …
b9235f1 
…r=compiler-errors

Don't ICE when deducing future output if other errors already occurred

The situation can't really happen outside of erroneous code. What was interesting is that it ICEd before emitting any other diagnostics. This was because the other errors were silenced due to cycle_delay_bug cycle errors.

r? ``@compiler-errors``

fixes rust-lang#119890
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 18, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2024
@bors
Auto merge of rust-lang#120088 - matthiaskrgr:rollup-4eak9yf, r=matth…
6b1dad9 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#119172 (Detect `NulInCStr` error earlier.)
 - rust-lang#119833 (Make tcx optional from StableMIR run macro and extend it to accept closures)
 - rust-lang#119955 (Modify GenericArg and Term structs to use strict provenance rules)
 - rust-lang#120021 (don't store const var origins for known vars)
 - rust-lang#120038 (Don't create a separate "basename" when naming and opening a MIR dump file)
 - rust-lang#120057 (Don't ICE when deducing future output if other errors already occurred)
 - rust-lang#120073 (Remove spastorino from users_on_vacation)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 18, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2024
@bors
Auto merge of rust-lang#120089 - matthiaskrgr:rollup-xyfqrb5, r=matth…
377cbb6 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#119172 (Detect `NulInCStr` error earlier.)
 - rust-lang#119833 (Make tcx optional from StableMIR run macro and extend it to accept closures)
 - rust-lang#119967 (Add `PatKind::Err` to AST/HIR)
 - rust-lang#119978 (Move async closure parameters into the resultant closure's future eagerly)
 - rust-lang#120021 (don't store const var origins for known vars)
 - rust-lang#120038 (Don't create a separate "basename" when naming and opening a MIR dump file)
 - rust-lang#120057 (Don't ICE when deducing future output if other errors already occurred)
 - rust-lang#120073 (Remove spastorino from users_on_vacation)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2024
@bors
Auto merge of rust-lang#120089 - matthiaskrgr:rollup-xyfqrb5, r=matth…
8424f8e 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#119172 (Detect `NulInCStr` error earlier.)
 - rust-lang#119833 (Make tcx optional from StableMIR run macro and extend it to accept closures)
 - rust-lang#119967 (Add `PatKind::Err` to AST/HIR)
 - rust-lang#119978 (Move async closure parameters into the resultant closure's future eagerly)
 - rust-lang#120021 (don't store const var origins for known vars)
 - rust-lang#120038 (Don't create a separate "basename" when naming and opening a MIR dump file)
 - rust-lang#120057 (Don't ICE when deducing future output if other errors already occurred)
 - rust-lang#120073 (Remove spastorino from users_on_vacation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 34362b8 into rust-lang:master Jan 18, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 18, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2024
@rust-timer
Unrolled build for rust-lang#120057
53982cd 
Rollup merge of rust-lang#120057 - oli-obk:not_sure_wtf_is_going_on, r=compiler-errors

Don't ICE when deducing future output if other errors already occurred

The situation can't really happen outside of erroneous code. What was interesting is that it ICEd before emitting any other diagnostics. This was because the other errors were silenced due to cycle_delay_bug cycle errors.

r? ```@compiler-errors```

fixes rust-lang#119890
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors left review comments

Assignees

@compiler-errors compiler-errors

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
Milestone
1.77.0
Development

Successfully merging this pull request may close these issues.

ice: async fn coroutine return type not an inference variable
4 participants
@oli-obk @rustbot @bors @compiler-errors