Don't list escaping bound regions in nested for<...> binders of E0308 notes - #159232
Conversation
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @fee1-dead (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
|
Binders make me a bit dizzy, so... @rustbot reroll |
|
r? types |
| let region = match r.kind() { | ||
| ty::ReBound(ty::BoundVarIndexKind::Bound(db), br) if db >= self.current_index => { | ||
| *self.region_map.entry(br).or_insert_with(|| name(Some(db), self.current_index, br)) | ||
| let binder_offset = db.as_u32() - self.current_index.as_u32(); |
There was a problem hiding this comment.
So, this only makes sense until we exit the current binder; if we reenter the binder, then we have an issue.
There was a problem hiding this comment.
Agreed. The >= arm was also actively wrong for escaping regions: the lt_idx > binder_level_idx special case returned an INNERMOST region which fold_region then rebound to self.current_index, shifting the escaping region's De Bruijn index down. Dropped the offset keying and moved to ==, which also makes that special case (and the two DebruijnIndex parameters of the naming closure) dead code, so they are removed too. Full tests/ui passes with no other .stderr changes.
| let region_map = region_map | ||
| .into_items() | ||
| .filter_map(|((binder_offset, br), region)| { | ||
| (binder_offset == 0).then_some((br, region)) |
There was a problem hiding this comment.
If you always filter by 0, then why are you inserting any others into the map at all?
There was a problem hiding this comment.
Good point, there was no reason to. My original thinking was that escaping regions needed a map entry so repeated occurrences get the same name, but by the time the inner binder is folded they already carry a NamedForPrinting kind from the enclosing binder (cmp_fn_sig recurses on the folded value), so the entry never bought anything. With == nothing but the current binder's regions (and placeholders) is inserted, and the offset keying and the filter are gone.
| @@ -2732,7 +2738,11 @@ impl<'a, 'tcx> ty::TypeFolder<TyCtxt<'tcx>> for RegionFolder<'a, 'tcx> { | |||
| let name = &mut self.name; | |||
| let region = match r.kind() { | |||
| ty::ReBound(ty::BoundVarIndexKind::Bound(db), br) if db >= self.current_index => { | |||
There was a problem hiding this comment.
It's possibly true that the right fix is if db == self.current_index instead of >=.
There was a problem hiding this comment.
You are right, that is the simpler and more correct fix: with == the escaping regions are never touched, so they keep their index and the name the enclosing binder already gave them, and they never reach region_map in the first place. Switched to db == self.current_index in the latest push.
…08 notes The `for<...>` prefixes in `cmp_fn_sig` are built from the region map returned by `name_all_regions`. That map also contained regions that are bound by an enclosing binder and merely escape through the binder being named, so nested binders in expected/found notes listed lifetimes they don't bind, printing invalid types such as `&mut for<'a> fn(for<'a> fn(&'a ()))` for `&mut for<'a> fn(fn(&'a ()))`. Key the folder's map by the region's binder offset and only return the regions actually bound by the binder being named. The offset in the key also fixes a latent collision between a bound and an escaping region sharing the same bound variable index. The printed text is unaffected: the `name` closure already skips escaping regions when writing to the printer, which is why diagnostic labels were already correct.
a8395d4 to
2686c8c
Compare
|
|
Simplified the fix as suggested: @rustbot review |
|
@bors r+ rollup |
…nting, r=jackh726
Don't list escaping bound regions in nested `for<...>` binders of E0308 notes
The expected/found notes of "one type is more general than the other" errors could print types that are not even valid syntax, repeating lifetimes from an outer binder inside nested `for<...>` lists:
```
= note: expected mutable reference `&mut for<'a> fn(for<'a> fn(&'a ()))`
found mutable reference `&mut fn(fn(&()))`
```
The expected type here is actually `&mut for<'a> fn(fn(&'a ()))`, the inner fn pointer binds nothing. In the example from rust-lang#111365 the note printed `for<'o> fn(for<'a, 'o> fn(&'a (), &'o ()))` even though the inner binder only binds `'a`.
The cause is in how `cmp_fn_sig` builds its `for<...>` prefixes. It calls `name_all_regions` and joins every region of the returned map into the list. But `RegionFolder` folded every region at or above the depth of the binder being printed (`db >= self.current_index`), so regions that are bound by an enclosing binder and merely escape through the current one also ended up in the map. The pretty printer's own text output already skipped those regions via a special case in the naming closure (which is why the labels of these diagnostics were correct, see rust-lang#102392), but the returned map kept them, and `cmp_fn_sig` recurses through nested fn pointers one binder at a time, so every nested `for<...>` list picked up all the outer lifetimes. That special case also rebound the escaping region to `self.current_index`, i.e. shifted its De Bruijn index down so that it looked like it was bound by the current binder.
This PR makes `RegionFolder` only fold regions bound by the binder being named (`db == self.current_index`). Escaping regions are left untouched: they keep their index, and they already carry a name because the enclosing binder named them when it was folded (`cmp_fn_sig` recurses on the folded value). This makes the rust-lang#102392 special case in the naming closure dead, so it is removed along with the two `DebruijnIndex` parameters it needed.
With this change, the two examples above print:
```
= note: expected mutable reference `&mut for<'a> fn(fn(&'a ()))`
found mutable reference `&mut fn(fn(&()))`
```
```
= note: expected fn pointer `for<'o> fn(for<'a> fn(&'a (), &'o ()))`
found fn pointer `fn(for<'a> fn(&'a (), &'a ()))`
```
The `found` line of `tests/ui/nll/relate_tys/placeholder-outlives-existential.rs` also loses its leaked binders and now matches the type written in the test's source.
Fixes rust-lang#134410
This also fixes the leaked outer lifetimes reported in rust-lang#111365, but I left that issue open since it additionally asks for renaming of shadowed lifetimes (`for<'a> fn(for<'a> ...)` for two distinct lifetimes both named `'a` in the source), which is a separate problem.
…nting, r=jackh726
Don't list escaping bound regions in nested `for<...>` binders of E0308 notes
The expected/found notes of "one type is more general than the other" errors could print types that are not even valid syntax, repeating lifetimes from an outer binder inside nested `for<...>` lists:
```
= note: expected mutable reference `&mut for<'a> fn(for<'a> fn(&'a ()))`
found mutable reference `&mut fn(fn(&()))`
```
The expected type here is actually `&mut for<'a> fn(fn(&'a ()))`, the inner fn pointer binds nothing. In the example from rust-lang#111365 the note printed `for<'o> fn(for<'a, 'o> fn(&'a (), &'o ()))` even though the inner binder only binds `'a`.
The cause is in how `cmp_fn_sig` builds its `for<...>` prefixes. It calls `name_all_regions` and joins every region of the returned map into the list. But `RegionFolder` folded every region at or above the depth of the binder being printed (`db >= self.current_index`), so regions that are bound by an enclosing binder and merely escape through the current one also ended up in the map. The pretty printer's own text output already skipped those regions via a special case in the naming closure (which is why the labels of these diagnostics were correct, see rust-lang#102392), but the returned map kept them, and `cmp_fn_sig` recurses through nested fn pointers one binder at a time, so every nested `for<...>` list picked up all the outer lifetimes. That special case also rebound the escaping region to `self.current_index`, i.e. shifted its De Bruijn index down so that it looked like it was bound by the current binder.
This PR makes `RegionFolder` only fold regions bound by the binder being named (`db == self.current_index`). Escaping regions are left untouched: they keep their index, and they already carry a name because the enclosing binder named them when it was folded (`cmp_fn_sig` recurses on the folded value). This makes the rust-lang#102392 special case in the naming closure dead, so it is removed along with the two `DebruijnIndex` parameters it needed.
With this change, the two examples above print:
```
= note: expected mutable reference `&mut for<'a> fn(fn(&'a ()))`
found mutable reference `&mut fn(fn(&()))`
```
```
= note: expected fn pointer `for<'o> fn(for<'a> fn(&'a (), &'o ()))`
found fn pointer `fn(for<'a> fn(&'a (), &'a ()))`
```
The `found` line of `tests/ui/nll/relate_tys/placeholder-outlives-existential.rs` also loses its leaked binders and now matches the type written in the test's source.
Fixes rust-lang#134410
This also fixes the leaked outer lifetimes reported in rust-lang#111365, but I left that issue open since it additionally asks for renaming of shadowed lifetimes (`for<'a> fn(for<'a> ...)` for two distinct lifetimes both named `'a` in the source), which is a separate problem.
…uwer Rollup of 5 pull requests Successful merges: - #161328 (Enforce even more library clippy lints in CI) - #161684 (Replace `Allocator + Clone` with `AllocatorClone` in btree) - #159232 (Don't list escaping bound regions in nested `for<...>` binders of E0308 notes) - #161464 (various cleanups of `rustc_builtin_macros`) - #161774 (Add codegen test for disjunction fed to unreachable_unchecked)
Rollup merge of #159232 - Rani367:fix-nested-hrtb-binder-printing, r=jackh726 Don't list escaping bound regions in nested `for<...>` binders of E0308 notes The expected/found notes of "one type is more general than the other" errors could print types that are not even valid syntax, repeating lifetimes from an outer binder inside nested `for<...>` lists: ``` = note: expected mutable reference `&mut for<'a> fn(for<'a> fn(&'a ()))` found mutable reference `&mut fn(fn(&()))` ``` The expected type here is actually `&mut for<'a> fn(fn(&'a ()))`, the inner fn pointer binds nothing. In the example from #111365 the note printed `for<'o> fn(for<'a, 'o> fn(&'a (), &'o ()))` even though the inner binder only binds `'a`. The cause is in how `cmp_fn_sig` builds its `for<...>` prefixes. It calls `name_all_regions` and joins every region of the returned map into the list. But `RegionFolder` folded every region at or above the depth of the binder being printed (`db >= self.current_index`), so regions that are bound by an enclosing binder and merely escape through the current one also ended up in the map. The pretty printer's own text output already skipped those regions via a special case in the naming closure (which is why the labels of these diagnostics were correct, see #102392), but the returned map kept them, and `cmp_fn_sig` recurses through nested fn pointers one binder at a time, so every nested `for<...>` list picked up all the outer lifetimes. That special case also rebound the escaping region to `self.current_index`, i.e. shifted its De Bruijn index down so that it looked like it was bound by the current binder. This PR makes `RegionFolder` only fold regions bound by the binder being named (`db == self.current_index`). Escaping regions are left untouched: they keep their index, and they already carry a name because the enclosing binder named them when it was folded (`cmp_fn_sig` recurses on the folded value). This makes the #102392 special case in the naming closure dead, so it is removed along with the two `DebruijnIndex` parameters it needed. With this change, the two examples above print: ``` = note: expected mutable reference `&mut for<'a> fn(fn(&'a ()))` found mutable reference `&mut fn(fn(&()))` ``` ``` = note: expected fn pointer `for<'o> fn(for<'a> fn(&'a (), &'o ()))` found fn pointer `fn(for<'a> fn(&'a (), &'a ()))` ``` The `found` line of `tests/ui/nll/relate_tys/placeholder-outlives-existential.rs` also loses its leaked binders and now matches the type written in the test's source. Fixes #134410 This also fixes the leaked outer lifetimes reported in #111365, but I left that issue open since it additionally asks for renaming of shadowed lifetimes (`for<'a> fn(for<'a> ...)` for two distinct lifetimes both named `'a` in the source), which is a separate problem.
The expected/found notes of "one type is more general than the other" errors could print types that are not even valid syntax, repeating lifetimes from an outer binder inside nested
for<...>lists:The expected type here is actually
&mut for<'a> fn(fn(&'a ())), the inner fn pointer binds nothing. In the example from #111365 the note printedfor<'o> fn(for<'a, 'o> fn(&'a (), &'o ()))even though the inner binder only binds'a.The cause is in how
cmp_fn_sigbuilds itsfor<...>prefixes. It callsname_all_regionsand joins every region of the returned map into the list. ButRegionFolderfolded every region at or above the depth of the binder being printed (db >= self.current_index), so regions that are bound by an enclosing binder and merely escape through the current one also ended up in the map. The pretty printer's own text output already skipped those regions via a special case in the naming closure (which is why the labels of these diagnostics were correct, see #102392), but the returned map kept them, andcmp_fn_sigrecurses through nested fn pointers one binder at a time, so every nestedfor<...>list picked up all the outer lifetimes. That special case also rebound the escaping region toself.current_index, i.e. shifted its De Bruijn index down so that it looked like it was bound by the current binder.This PR makes
RegionFolderonly fold regions bound by the binder being named (db == self.current_index). Escaping regions are left untouched: they keep their index, and they already carry a name because the enclosing binder named them when it was folded (cmp_fn_sigrecurses on the folded value). This makes the #102392 special case in the naming closure dead, so it is removed along with the twoDebruijnIndexparameters it needed.With this change, the two examples above print:
The
foundline oftests/ui/nll/relate_tys/placeholder-outlives-existential.rsalso loses its leaked binders and now matches the type written in the test's source.Fixes #134410
This also fixes the leaked outer lifetimes reported in #111365, but I left that issue open since it additionally asks for renaming of shadowed lifetimes (
for<'a> fn(for<'a> ...)for two distinct lifetimes both named'ain the source), which is a separate problem.