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

Do not ICE when failing to fulfill known obligations due to normalization #92943

Closed
wants to merge 1 commit into from

Conversation

cjgillot
Copy link
Contributor

Part of #91743

r? @jackh726

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 15, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 15, 2022
@Aaron1011
Copy link
Member

Isn't this potentially unsound, if the errors produced were legitimate?

@cjgillot
Copy link
Contributor Author

Overzealous normalization may "lose" some bounds. In the test, mk_cycle is instanciated with try_execute_query's C::V, which implements Debug because of the bound in Cache. When inlined, mk_cycle is instanciated with get_query's Q::V (identical to <Q::C as Cache>::V, but the trait solver may have forgot this type Debug.

Normally, all the bounds would have been proven by trait inference, hence this is a delay_span_bug.

@Aaron1011, I agree. This PR can be reframed into the following question: how to separate legitimate from illegitimate errors, to make all this sound?

@jackh726
Copy link
Member

This doesn't seem correct - especially not in isolation. The test passes on current nightly; is this expected?

If there are in fact errors that pop up in #91743 for this test, then that's curious. I'd like to know exactly what the errors are.

@cjgillot
Copy link
Contributor Author

The test only ICEs with MIR inlining. See godbolt for the error:

error: internal compiler error: Encountered errors `[FulfillmentError(Obligation(predicate=Binder(TraitPredicate(<<Q as Query>::V as std::fmt::Debug>, polarity:Positive), []), depth=0),Unimplemented)]` resolving bounds after type-checking
  |
  = note: delayed at compiler/rustc_trait_selection/src/traits/codegen.rs:125:24

thread 'rustc' panicked at 'no errors encountered even though `delay_span_bug` issued', compiler/rustc_errors/src/lib.rs:1188:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@Aaron1011
Copy link
Member

That looks like a bug in fulfillment (maybe something with normalization?) - maybe inlining isnt updating the ParamEnv correctly?

@jackh726
Copy link
Member

I...think...this could be a variation of #91985.

We have type V: Debug, then type C: Cache<V = Self::V> without an explicit V: Debug bound here. Which means later we'll have to prove that. but we don't have that bound in the ParamEnv. I have a POC branch linked in that issue that might solve this, maybe take a look? (It's not 100% mergable right now, but all but like 3 tests pass).

@bors
Copy link
Collaborator

bors commented Jan 31, 2022

☔ The latest upstream changes (presumably #93498) made this pull request unmergeable. Please resolve the merge conflicts.

@jackh726
Copy link
Member

I'm going to go ahead an close this. This isn't really the right approach for this. I have a branch that might fix this, but has some other things wrong that needs to be thought about. Might be worth checking out. jackh726@18042d9

@jackh726 jackh726 closed this Feb 24, 2022
@cjgillot cjgillot deleted the bad-normalize branch February 24, 2022 15:47
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
Development

Successfully merging this pull request may close these issues.

6 participants