fix ICE for deref coercions with type errors #120972

lukas-code · 2024-02-12T14:18:04Z

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):

rust/compiler/rustc_hir_typeck/src/coercion.rs

Lines 339 to 464 in b17491c

    
           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

compiler-errors · 2024-02-12T15:59:02Z

@bors r+

bors · 2024-02-12T15:59:04Z

📌 Commit 95c5b06 has been approved by compiler-errors

It is now in the queue for this repository.

…=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

…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

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

…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

fix ICE for deref coercions with type errors

95c5b06

rustbot assigned compiler-errors Feb 12, 2024

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 approved these changes Feb 12, 2024

View reviewed changes

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

This was referenced Feb 12, 2024

Rollup of 12 pull requests #120977

Closed

Rollup of 11 pull requests #120980

Merged

bors merged commit 8e5f722 into rust-lang:master Feb 12, 2024

rustbot added this to the 1.78.0 milestone Feb 12, 2024

lukas-code deleted the autoderef-type-error branch February 12, 2024 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix ICE for deref coercions with type errors #120972

fix ICE for deref coercions with type errors #120972

Uh oh!

lukas-code commented Feb 12, 2024

Uh oh!

compiler-errors commented Feb 12, 2024

Uh oh!

bors commented Feb 12, 2024

Uh oh!

Uh oh!

	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);
	};

fix ICE for deref coercions with type errors #120972

fix ICE for deref coercions with type errors #120972

Uh oh!

Conversation

lukas-code commented Feb 12, 2024

Uh oh!

compiler-errors commented Feb 12, 2024

Uh oh!

bors commented Feb 12, 2024

Uh oh!

Uh oh!