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

skip rpit constraint checker if borrowck return type error #117884

Merged
merged 1 commit into from
Dec 17, 2023

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Nov 13, 2023

Fixes #117794
Fixes #117886
Fixes #119025

Prior to change #117418, the value of concrete_opaque_types for mir_borrock(T::a::opaque) was None. However, due to modifications in body.local_decls, the return value had been changed.

The changed of body.local_decls has let to the addition of ty:Error to infcx.opaque_type_storage.opaque_types during TypeChecker::equate_inputs_and_outputs. This is due to it utilizing the output of a function signature that was appended during construct_error(which previously only appended a ty::Error) and then execute TypeChecker::Related_types.

Therefore, in this PR, I've implemented a condition to bypass the rpit check when an error is encountered.

r? @compiler-errors

@compiler-errors
Copy link
Member

compiler-errors commented Nov 13, 2023

I'm still not sure why the ICE in #117794 happens, and this seems like a strange way to be avoiding an ICE in mir-building...

Can you explain more specifically why the root cause occurs, and how this avoids it?

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Nov 14, 2023

I think the root cause of the issue can be traced back to the modifications made in body.local_decls:

  1. Prior to change Build a better MIR body when errors are encountered #117418, it only contained ty::Error, but now it includes the output of the function signature.
  2. This has led to changes in mir_opaque_ty, which previously held None, but now contains ty: ty::Error. Specifically, this amendment took place in checker.equate_inputs_and_outputs.
  3. This means that we have executed additional RpitConstraintChecker, and ice: bug!() in mir building #117794 is to encounter a panic during the visit.

Therefore, in this PR, I've implemented a condition to bypass the checker when mir_opaque_ty includes a ty::Error.

Another potential solution to this issue could involve altering the handling for ty:: Closure as introduced in #117418.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 21, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Nov 21, 2023

I have made a minor modification: if a type error is found, then the borrow check is skipped during the RpitConstraintChecker visit. This should maintain the same effect as before.

@compiler-errors
Copy link
Member

I'm still not certain this is the right way to fix this bug. I would be much more satisfied if we understood what is wrong with mir building that leads to this issue -- this seems to be fixing a symptom of the problem, rather than the actual problem itself, in my opinion.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Dec 7, 2023

if we understood what is wrong with mir building that leads to this issue

Thanks for this tip, and I'll delve deeper

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Dec 13, 2023

I have recently investigated this issue:

However, I'm still not certain about the best solution and might need to investigate further.

@compiler-errors
Copy link
Member

Hi @bvanjoi:

I investigated this more. I think this is the correct approach, but with a minor tweak. Please add the check for references_error() to:

if let Some(mir_opaque_ty) = mir_opaque_ty {

And you can remove the other checks for references_error().

Sorry for the back and forth -- this was a very difficult issue.

@compiler-errors
Copy link
Member

This also should fix #119025.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 17, 2023

📌 Commit 64e311a has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 17, 2023
@bors
Copy link
Contributor

bors commented Dec 17, 2023

⌛ Testing commit 64e311a with merge d14e52b...

@bors
Copy link
Contributor

bors commented Dec 17, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing d14e52b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 17, 2023
@bors bors merged commit d14e52b into rust-lang:master Dec 17, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 17, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d14e52b): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.1% [3.1%, 3.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.166s -> 672.948s (0.12%)
Artifact size: 312.45 MiB -> 312.38 MiB (-0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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
5 participants