Skip to content

Commit

Permalink
Rollup merge of #100508 - BoxyUwU:make_less_things_late_bound, r=niko…
Browse files Browse the repository at this point in the history
…matsakis

avoid making substs of type aliases late bound when used as fn args

fixes #47511
fixes #85533
(although I did not know theses issues existed when i was working on this 🙃)

currently `Alias<...>` is treated the same as `Struct<...>` when deciding if generics should be late bound or early bound but this is not correct as `Alias` might normalize to a projection which does not constrain the generics.

I think this needs more tests before merging
more explanation of PR [here](https://hackmd.io/v44a-QVjTIqqhK9uretyQg?view)

Hackmd inline for future readers:
---

This assumes reader is familiar with the concept of early/late bound lifetimes. There's a section on rustc-dev-guide if not (although i think some details are a bit out of date)

## problem & background

Not all lifetimes on a fn can be late bound:
```rust
fn foo<'a>() -> &'a ();
impl<'a> Fn<()> for FooFnDef {
    type Output = &'a (); // uh oh unconstrained lifetime
}
```
so we make make them early bound
```rust
fn foo<'a>() -> &'a ();
impl<'a> Fn<()> for FooFnDef<'a> {// wow look at all that lifetimey
     type Output = &'a ();
}
```
(Closures have the same constraint however it is not enforced leading to soundness bugs, [#84385](#84385) implements this "downgrading late bound to early bound" for closures)

lifetimes on fn items are only late bound when they are "constrained" by the fn args:
```rust
fn foo<'a>(_: &'a ()) -> &'a ();
//               late bound, not present on `FooFnItem`
//               vv
impl<'a> Trait<(&'a (),)> for FooFnItem {
    type Output = &'a ();
}

// projections do not constrain inputs
fn bar<'a, T: Trait>(_: <T as Trait<'a>>::Assoc) -> &'a (); //  early bound
                                                            //  vv
impl<'a, T: Trait> Fn<(<T as Trait<'a>>::Assoc,)> for BarFnItem<'a, T> {
    type Output = &'a ();
}
```

current logic for determining if inputs "constrain" a lifetime works off of HIR so does not normalize aliases. It also assumes that any path with no self type constrains all its substs (i.e. `Foo<'a, u32>` has no self type but `T::Assoc` does). This falls apart for top level type aliases (see linked issues):

```rust
type Alias<'a, T> = <T as Trait<'a>>::Assoc;
//                      wow look its a path with no self type uwu
//                      i bet that constrains `'a` so it should be latebound
//                      vvvvvvvvvvv
fn foo<'a, T: Trait>(_: Alias<'a, T>) -> &'a ();
//                     `Alias` normalized to make things clearer
//                     vvvvvvvvvvvvvvvvvvvvvvv
impl<'a, T: Trait> Fn<(<T as Trait<'a>>::Assoc,)> for FooFnDef<T> {
    type Output = &'a ();
    // oh no `'a` isnt constrained wah wah waaaah *trumbone noises*
    // i think, idk what musical instrument that is
}
```

## solution

The PR solves this by having the hir visitor that checks for lifetimes in constraining uses check if the path is a `DefKind::Alias`. If it is we ""normalize"" it by calling `type_of` and walking the returned type. This is a bit hacky as it requires a mapping between the substs on the path in hir, and the generics of the `type Alias<...>` which is on the ty layer.

Alternative solutions may involve calculating the "late boundness" of lifetimes after/during astconv rather than relying on hir at all. We already have code to determine whether a lifetime SHOULD be late bound or not as this is currently how the error for `fn foo<'a, T: Trait>(_: Alias<'a, T>) -> &'a ();` gets emitted.

It is probably not possible to do this right now, late boundness is used by `generics_of` and `gather_explicit_predicates_of` as we currently do not put late bound lifetimes in `Generics`. Although this seems sus to me as the long term goal is to make all generics late bound which would result in `generics_of(function)` being empty? [#103448](#103448) places all lifetimes in `Generics` regardless of late boundness so that may be a good step towards making this possible.
  • Loading branch information
Manishearth authored Nov 9, 2022
2 parents 8d36948 + 983a90d commit f162e3a
Show file tree
Hide file tree
Showing 9 changed files with 184 additions and 29 deletions.
104 changes: 99 additions & 5 deletions compiler/rustc_hir_analysis/src/collect/lifetimes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use rustc_middle::bug;
use rustc_middle::hir::map::Map;
use rustc_middle::hir::nested_filter;
use rustc_middle::middle::resolve_lifetime::*;
use rustc_middle::ty::{self, DefIdTree, TyCtxt};
use rustc_middle::ty::{self, DefIdTree, TyCtxt, TypeSuperVisitable, TypeVisitor};
use rustc_span::def_id::DefId;
use rustc_span::symbol::{sym, Ident};
use rustc_span::Span;
Expand Down Expand Up @@ -1781,7 +1781,7 @@ fn is_late_bound_map(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Option<&FxIndexSet<

let mut late_bound = FxIndexSet::default();

let mut constrained_by_input = ConstrainedCollector::default();
let mut constrained_by_input = ConstrainedCollector { regions: Default::default(), tcx };
for arg_ty in decl.inputs {
constrained_by_input.visit_ty(arg_ty);
}
Expand Down Expand Up @@ -1834,12 +1834,65 @@ fn is_late_bound_map(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Option<&FxIndexSet<
debug!(?late_bound);
return Some(tcx.arena.alloc(late_bound));

#[derive(Default)]
struct ConstrainedCollector {
/// Visits a `ty::Ty` collecting information about what generic parameters are constrained.
///
/// The visitor does not operate on `hir::Ty` so that it can be called on the rhs of a `type Alias<...> = ...;`
/// which may live in a separate crate so there would not be any hir available. Instead we use the `type_of`
/// query to obtain a `ty::Ty` which will be present even in cross crate scenarios. It also naturally
/// handles cycle detection as we go through the query system.
///
/// This is necessary in the first place for the following case:
/// ```
/// type Alias<'a, T> = <T as Trait<'a>>::Assoc;
/// fn foo<'a>(_: Alias<'a, ()>) -> Alias<'a, ()> { ... }
/// ```
///
/// If we conservatively considered `'a` unconstrained then we could break users who had written code before
/// we started correctly handling aliases. If we considered `'a` constrained then it would become late bound
/// causing an error during astconv as the `'a` is not constrained by the input type `<() as Trait<'a>>::Assoc`
/// but appears in the output type `<() as Trait<'a>>::Assoc`.
///
/// We must therefore "look into" the `Alias` to see whether we should consider `'a` constrained or not.
///
/// See #100508 #85533 #47511 for additional context
struct ConstrainedCollectorPostAstConv {
arg_is_constrained: Box<[bool]>,
}

use std::ops::ControlFlow;
use ty::Ty;
impl<'tcx> TypeVisitor<'tcx> for ConstrainedCollectorPostAstConv {
fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<!> {
match t.kind() {
ty::Param(param_ty) => {
self.arg_is_constrained[param_ty.index as usize] = true;
}
ty::Projection(_) => return ControlFlow::Continue(()),
_ => (),
}
t.super_visit_with(self)
}

fn visit_const(&mut self, _: ty::Const<'tcx>) -> ControlFlow<!> {
ControlFlow::Continue(())
}

fn visit_region(&mut self, r: ty::Region<'tcx>) -> ControlFlow<!> {
debug!("r={:?}", r.kind());
if let ty::RegionKind::ReEarlyBound(region) = r.kind() {
self.arg_is_constrained[region.index as usize] = true;
}

ControlFlow::Continue(())
}
}

struct ConstrainedCollector<'tcx> {
tcx: TyCtxt<'tcx>,
regions: FxHashSet<LocalDefId>,
}

impl<'v> Visitor<'v> for ConstrainedCollector {
impl<'v> Visitor<'v> for ConstrainedCollector<'_> {
fn visit_ty(&mut self, ty: &'v hir::Ty<'v>) {
match ty.kind {
hir::TyKind::Path(
Expand All @@ -1850,6 +1903,47 @@ fn is_late_bound_map(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Option<&FxIndexSet<
// (defined above)
}

hir::TyKind::Path(hir::QPath::Resolved(
None,
hir::Path { res: Res::Def(DefKind::TyAlias, alias_def), segments, span },
)) => {
// See comments on `ConstrainedCollectorPostAstConv` for why this arm does not just consider
// substs to be unconstrained.
let generics = self.tcx.generics_of(alias_def);
let mut walker = ConstrainedCollectorPostAstConv {
arg_is_constrained: vec![false; generics.params.len()].into_boxed_slice(),
};
walker.visit_ty(self.tcx.type_of(alias_def));

match segments.last() {
Some(hir::PathSegment { args: Some(args), .. }) => {
let tcx = self.tcx;
for constrained_arg in
args.args.iter().enumerate().flat_map(|(n, arg)| {
match walker.arg_is_constrained.get(n) {
Some(true) => Some(arg),
Some(false) => None,
None => {
tcx.sess.delay_span_bug(
*span,
format!(
"Incorrect generic arg count for alias {:?}",
alias_def
),
);
None
}
}
})
{
self.visit_generic_arg(constrained_arg);
}
}
Some(_) => (),
None => bug!("Path with no segments or self type"),
}
}

hir::TyKind::Path(hir::QPath::Resolved(None, ref path)) => {
// consider only the lifetimes on the final
// segment; I am not sure it's even currently
Expand Down
18 changes: 0 additions & 18 deletions src/test/ui/issues/issue-47511.stderr

This file was deleted.

5 changes: 5 additions & 0 deletions src/test/ui/late-bound-lifetimes/auxiliary/upstream_alias.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pub trait Trait<'a> {
type Assoc;
}

pub type Alias<'a, T> = <T as Trait<'a>>::Assoc;
10 changes: 10 additions & 0 deletions src/test/ui/late-bound-lifetimes/cross_crate_alias.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// aux-build:upstream_alias.rs
// check-pass

extern crate upstream_alias;

fn foo<'a, T: for<'b> upstream_alias::Trait<'b>>(_: upstream_alias::Alias<'a, T>) -> &'a () {
todo!()
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// check-pass

trait Gats<'a> {
type Assoc;
type Assoc2;
}

trait Trait: for<'a> Gats<'a> {
fn foo<'a>(_: &mut <Self as Gats<'a>>::Assoc) -> <Self as Gats<'a>>::Assoc2;
}

impl<'a> Gats<'a> for () {
type Assoc = &'a u32;
type Assoc2 = ();
}

type GatsAssoc<'a, T> = <T as Gats<'a>>::Assoc;
type GatsAssoc2<'a, T> = <T as Gats<'a>>::Assoc2;

impl Trait for () {
fn foo<'a>(_: &mut GatsAssoc<'a, Self>) -> GatsAssoc2<'a, Self> {}
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -1,9 +1,4 @@
// check-fail
// known-bug: #47511

// Regression test for #47511: anonymous lifetimes can appear
// unconstrained in a return type, but only if they appear just once
// in the input, as the input to a projection.
// check-pass

fn f(_: X) -> X {
unimplemented!()
Expand Down
16 changes: 16 additions & 0 deletions src/test/ui/late-bound-lifetimes/late_bound_through_alias.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// check-pass

fn f(_: X) -> X {
unimplemented!()
}

fn g<'a>(_: X<'a>) -> X<'a> {
unimplemented!()
}

type X<'a> = &'a ();

fn main() {
let _: for<'a> fn(X<'a>) -> X<'a> = g;
let _: for<'a> fn(X<'a>) -> X<'a> = f;
}
12 changes: 12 additions & 0 deletions src/test/ui/late-bound-lifetimes/mismatched_arg_count.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// ensures that we don't ICE when there are too many args supplied to the alias.

trait Trait<'a> {
type Assoc;
}

type Alias<'a, T> = <T as Trait<'a>>::Assoc;

fn bar<'a, T: Trait<'a>>(_: Alias<'a, 'a, T>) {}
//~^ error: this type alias takes 1 lifetime argument but 2 lifetime arguments were supplied

fn main() {}
17 changes: 17 additions & 0 deletions src/test/ui/late-bound-lifetimes/mismatched_arg_count.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
error[E0107]: this type alias takes 1 lifetime argument but 2 lifetime arguments were supplied
--> $DIR/mismatched_arg_count.rs:9:29
|
LL | fn bar<'a, T: Trait<'a>>(_: Alias<'a, 'a, T>) {}
| ^^^^^ -- help: remove this lifetime argument
| |
| expected 1 lifetime argument
|
note: type alias defined here, with 1 lifetime parameter: `'a`
--> $DIR/mismatched_arg_count.rs:7:6
|
LL | type Alias<'a, T> = <T as Trait<'a>>::Assoc;
| ^^^^^ --

error: aborting due to previous error

For more information about this error, try `rustc --explain E0107`.

0 comments on commit f162e3a

Please sign in to comment.