-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
[NLL] yield-subtype.rs test ICEs #47587
Comments
#47586 is probably the same issue. |
So I think the problem here is that generators are not completely integrated into the universal region setup. First problem: The MIR visitor doesn't visit the Lines 91 to 92 in 5965b79
but the MIR visitor doesn't seem to visit it: rust/src/librustc/mir/visit.rs Lines 278 to 305 in 5965b79
I would expect something like if let Some(yield_ty) = &$($mutability)* mir.yield_ty {
self.visit_ty(yield_ty, TyContext::YieldTy(SourceInfo {
span: mir.span,
scope: ARGUMENT_VISIBILITY_SCOPE,
}));
} |
As a result of that, the MIR renumbering code will not replace the regions in the yield type with inference variables. You can see that if you look at the mir-dump, at least you can if you do it on my branch from #47353. This is contents of
Note that the region in the yield type is a |
However, it's not enough for the renumberer to visit the Let me try to explain that (in follow-up comments to come). |
Basically, the idea here is that all the types and regions in the The The idea is that for each MIR we have a "defining type": rust/src/librustc_mir/borrow_check/nll/universal_regions.rs Lines 68 to 71 in 5965b79
The defining type kind of captures everything we need to compute the types of the "external things", such as the function signature. For a generator, this includes the "yield type" (and "return type"). In the case of the parameters, we have some other fields in the rust/src/librustc_mir/borrow_check/nll/universal_regions.rs Lines 73 to 85 in 5965b79
(Those fields are "unnormalized", which means that associated types are not normalized. This is not especially relevant to yield types.) I think we will want to add a Anyway, the code that relates the types in rust/src/librustc_mir/borrow_check/nll/type_check/input_output.rs Lines 55 to 61 in 5965b79
We want to add some code after this loop like so: assert!(
mir.yield_ty.is_some() && universal_regions.yield_ty.is_some() ||
mir.yield_ty.is_nome() && universal_regions.yield_ty.is_nome()
);
if let Some(mir_yield_ty) = mir.yield_ty {
let ur_yield_ty = universal_regions.yield_ty.unwrap();
self.equate_normalized_input_or_output(start_position, ur_yield_ty, mir_yield_ty);
} To initialize the rust/src/librustc_mir/borrow_check/nll/universal_regions.rs Lines 114 to 117 in 5965b79
we can extract the yield_ty by invoking Lines 325 to 328 in 5965b79
|
Oh, and I think that this test case is the same bug (it comes from a generator PR): #![feature(nll, generators, generator_trait)]
use std::ops::Generator;
fn main() {
let a = 1;
let mut generator = unsafe { || {
let b = &a;
yield(b); // CRASH the compiler
// adding .clone() to b or removing feature(nll) makes the compiler happy
}};
println!("{:?}", generator.resume());
} |
…akis renumber regions in generators This fixes #47189, but I think we still have to double check various things around how to treat generators in MIR type check + borrow check (e.g., what borrows should be invalidated by a `Suspend`? What consistency properties should type check be enforcing anyway around the "interior" type?) Also fixes #47587 thanks to @spastorino's commit. r? @pnkfelix
The text was updated successfully, but these errors were encountered: