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

fix ICE for deref coercions with type errors #120972

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

lukas-code
Copy link
Member

Follow-up to #120895, where I made types with errors go through the full coercion code, which is necessary if we want to build MIR for bodies with errors (#120550).

The code for coercing &T to &U currently assumes that autoderef for &T will succeed for at least two steps (&T and T):

let mut first_error = None;
let mut r_borrow_var = None;
let mut autoderef = self.autoderef(span, a);
let mut found = None;
for (referent_ty, autoderefs) in autoderef.by_ref() {
if autoderefs == 0 {
// Don't let this pass, otherwise it would cause
// &T to autoref to &&T.
continue;
}
// At this point, we have deref'd `a` to `referent_ty`. So
// imagine we are coercing from `&'a mut Vec<T>` to `&'b mut [T]`.
// In the autoderef loop for `&'a mut Vec<T>`, we would get
// three callbacks:
//
// - `&'a mut Vec<T>` -- 0 derefs, just ignore it
// - `Vec<T>` -- 1 deref
// - `[T]` -- 2 deref
//
// At each point after the first callback, we want to
// check to see whether this would match out target type
// (`&'b mut [T]`) if we autoref'd it. We can't just
// compare the referent types, though, because we still
// have to consider the mutability. E.g., in the case
// we've been considering, we have an `&mut` reference, so
// the `T` in `[T]` needs to be unified with equality.
//
// Therefore, we construct reference types reflecting what
// the types will be after we do the final auto-ref and
// compare those. Note that this means we use the target
// mutability [1], since it may be that we are coercing
// from `&mut T` to `&U`.
//
// One fine point concerns the region that we use. We
// choose the region such that the region of the final
// type that results from `unify` will be the region we
// want for the autoref:
//
// - if in sub mode, that means we want to use `'b` (the
// region from the target reference) for both
// pointers [2]. This is because sub mode (somewhat
// arbitrarily) returns the subtype region. In the case
// where we are coercing to a target type, we know we
// want to use that target type region (`'b`) because --
// for the program to type-check -- it must be the
// smaller of the two.
// - One fine point. It may be surprising that we can
// use `'b` without relating `'a` and `'b`. The reason
// that this is ok is that what we produce is
// effectively a `&'b *x` expression (if you could
// annotate the region of a borrow), and regionck has
// code that adds edges from the region of a borrow
// (`'b`, here) into the regions in the borrowed
// expression (`*x`, here). (Search for "link".)
// - if in lub mode, things can get fairly complicated. The
// easiest thing is just to make a fresh
// region variable [4], which effectively means we defer
// the decision to region inference (and regionck, which will add
// some more edges to this variable). However, this can wind up
// creating a crippling number of variables in some cases --
// e.g., #32278 -- so we optimize one particular case [3].
// Let me try to explain with some examples:
// - The "running example" above represents the simple case,
// where we have one `&` reference at the outer level and
// ownership all the rest of the way down. In this case,
// we want `LUB('a, 'b)` as the resulting region.
// - However, if there are nested borrows, that region is
// too strong. Consider a coercion from `&'a &'x Rc<T>` to
// `&'b T`. In this case, `'a` is actually irrelevant.
// The pointer we want is `LUB('x, 'b`). If we choose `LUB('a,'b)`
// we get spurious errors (`ui/regions-lub-ref-ref-rc.rs`).
// (The errors actually show up in borrowck, typically, because
// this extra edge causes the region `'a` to be inferred to something
// too big, which then results in borrowck errors.)
// - We could track the innermost shared reference, but there is already
// code in regionck that has the job of creating links between
// the region of a borrow and the regions in the thing being
// borrowed (here, `'a` and `'x`), and it knows how to handle
// all the various cases. So instead we just make a region variable
// and let regionck figure it out.
let r = if !self.use_lub {
r_b // [2] above
} else if autoderefs == 1 {
r_a // [3] above
} else {
if r_borrow_var.is_none() {
// create var lazily, at most once
let coercion = Coercion(span);
let r = self.next_region_var(coercion);
r_borrow_var = Some(r); // [4] above
}
r_borrow_var.unwrap()
};
let derefd_ty_a = Ty::new_ref(
self.tcx,
r,
TypeAndMut {
ty: referent_ty,
mutbl: mutbl_b, // [1] above
},
);
match self.unify(derefd_ty_a, b) {
Ok(ok) => {
found = Some(ok);
break;
}
Err(err) => {
if first_error.is_none() {
first_error = Some(err);
}
}
}
}
// Extract type or return an error. We return the first error
// we got, which should be from relating the "base" type
// (e.g., in example above, the failure from relating `Vec<T>`
// to the target type), since that should be the least
// confusing.
let Some(InferOk { value: ty, mut obligations }) = found else {
let err = first_error.expect("coerce_borrowed_pointer had no error");
debug!("coerce_borrowed_pointer: failed with err = {:?}", err);
return Err(err);
};

But for types with errors, we previously only returned the no-op autoderef step (&{type error} -> &{type error}) and then stopped early. This PR changes autoderef for types with errors to still go through the built-in derefs (e.g. &&{type error} -> &{type error} -> {type error}) and only stop early when it would have to go looking for Deref trait impls.

fixes #120945

r? @compiler-errors or compiler

@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 Feb 12, 2024
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 12, 2024

📌 Commit 95c5b06 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 Feb 12, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 12, 2024
…=compiler-errors

fix ICE for deref coercions with type errors

Follow-up to rust-lang#120895, where I made types with errors go through the full coercion code, which is necessary if we want to build MIR for bodies with errors (rust-lang#120550).

The code for coercing `&T` to `&U` currently assumes that autoderef for `&T` will succeed for at least two steps (`&T` and `T`):

https://github.com/rust-lang/rust/blob/b17491c8f6d555386104dfd82004c01bfef09c95/compiler/rustc_hir_typeck/src/coercion.rs#L339-L464

But for types with errors, we previously only returned the no-op autoderef step (`&{type error}` -> `&{type error}`) and then stopped early. This PR changes autoderef for types with errors to still go through the built-in derefs (e.g. `&&{type error}` -> `&{type error}` -> `{type error}`) and only stop early when it would have to go looking for `Deref` trait impls.

fixes rust-lang#120945

r? `@compiler-errors` or compiler
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 12, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#120765 (Reorder diagnostics API)
 - rust-lang#120833 (More internal emit diagnostics cleanups)
 - rust-lang#120899 (Gracefully handle non-WF alias in `assemble_alias_bound_candidates_recur`)
 - rust-lang#120917 (Remove a bunch of dead parameters in functions)
 - rust-lang#120928 (Add test for recently fixed issue)
 - rust-lang#120933 (check_consts: fix duplicate errors, make importance consistent)
 - rust-lang#120936 (improve `btree_cursors` functions documentation)
 - rust-lang#120944 (Check that the ABI of the instance we are inlining is correct)
 - rust-lang#120956 (Clean inlined type alias with correct param-env)
 - rust-lang#120962 (Add myself to library/std review)
 - rust-lang#120972 (fix ICE for deref coercions with type errors)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8e5f722 into rust-lang:master Feb 12, 2024
11 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 12, 2024
Rollup merge of rust-lang#120972 - lukas-code:autoderef-type-error, r=compiler-errors

fix ICE for deref coercions with type errors

Follow-up to rust-lang#120895, where I made types with errors go through the full coercion code, which is necessary if we want to build MIR for bodies with errors (rust-lang#120550).

The code for coercing `&T` to `&U` currently assumes that autoderef for `&T` will succeed for at least two steps (`&T` and `T`):

https://github.com/rust-lang/rust/blob/b17491c8f6d555386104dfd82004c01bfef09c95/compiler/rustc_hir_typeck/src/coercion.rs#L339-L464

But for types with errors, we previously only returned the no-op autoderef step (`&{type error}` -> `&{type error}`) and then stopped early. This PR changes autoderef for types with errors to still go through the built-in derefs (e.g. `&&{type error}` -> `&{type error}` -> `{type error}`) and only stop early when it would have to go looking for `Deref` trait impls.

fixes rust-lang#120945

r? ``@compiler-errors`` or compiler
@rustbot rustbot added this to the 1.78.0 milestone Feb 12, 2024
@lukas-code lukas-code deleted the autoderef-type-error branch February 12, 2024 20:29
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 26, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#120765 (Reorder diagnostics API)
 - rust-lang#120833 (More internal emit diagnostics cleanups)
 - rust-lang#120899 (Gracefully handle non-WF alias in `assemble_alias_bound_candidates_recur`)
 - rust-lang#120917 (Remove a bunch of dead parameters in functions)
 - rust-lang#120928 (Add test for recently fixed issue)
 - rust-lang#120933 (check_consts: fix duplicate errors, make importance consistent)
 - rust-lang#120936 (improve `btree_cursors` functions documentation)
 - rust-lang#120944 (Check that the ABI of the instance we are inlining is correct)
 - rust-lang#120956 (Clean inlined type alias with correct param-env)
 - rust-lang#120962 (Add myself to library/std review)
 - rust-lang#120972 (fix ICE for deref coercions with type errors)

r? `@ghost`
`@rustbot` modify labels: rollup
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.

ICE: coerce_borrowed_pointer had no error
4 participants