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

Treat weak alias types more like ADTs when computing implied bounds #122340

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fmease
Copy link
Member

@fmease fmease commented Mar 11, 2024

For context, we want to treat lazy type aliases / weak alias types just like ADTs when computing implied bounds (#118479, T-lang design meeting 2023-11-09) but the way I've implemented it in #119350, there are some deviations in behavior which this PR eliminates.

I found this oversight while trying to build alloc with LTAs enabled by default: struct ExtractIfInner<'a, K, V> would no longer build requiring the explicit bounds K: 'a and V: 'a.

With this patch, we imply K: 'a, V: 'a instead of Root<K, V>: 'a in the following code which is consistent with ADTs:

#![feature(lazy_type_alias)]
struct ExtractIfInner<'a, K, V>(&'a mut Root<K, V>);
type Root<K, V> = (K, V);

Instead of treating weak alias types as abstract when computing the outlives-components like we do for the other kinds of alias types, we shallowly walk their generic args like we do with ADTs.

This is yet another case in a series (#121344, #120780) where weak alias types deviate in behavior from the other kinds of alias types, well... ^^'.

r? @oli-obk

@fmease fmease added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-types Relevant to the types team, which will review and decide on the PR/issue. A-implied-bounds Area: Implied bounds / inferred outlives-bounds F-lazy_type_alias `#![feature(lazy_type_alias)]` labels Mar 11, 2024
@@ -144,7 +144,7 @@ pub(crate) fn insert_outlives_predicate<'tcx>(
}
}

fn is_free_region(region: Region<'_>) -> bool {
fn is_early_bound_region(region: Region<'_>) -> bool {
Copy link
Member Author

@fmease fmease Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a drive-by renaming, cc #117908. I don't know why this used the term free (ReLateParam). It should've always been named is_early_bound_region or sth. in that vein.

@rust-log-analyzer

This comment has been minimized.

@fmease fmease force-pushed the lta-implied-bounds-recurse-shallow branch from 118c6c1 to c5165ba Compare March 11, 2024 18:21
@@ -5,7 +5,6 @@ mod test_lifetime_param {
fn defining(a: &str) -> Ty<'_> { a }
fn assert_static<'a: 'static>() {}
fn test<'a>() where Ty<'a>: 'static { assert_static::<'a>() }
//~^ ERROR: lifetime may not live long enough
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Member Author

@fmease fmease Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WeakAlias<'co>: 'static now implies 'co: 'static similar to ADTs. Is that fine / sound?

Copy link
Contributor

@oli-obk oli-obk Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is, but we need tests that show that we actually enforce it, too. I don't know if we have any weak alias type tests for that, even if we have old-style type alias tests for it

@@ -27,7 +26,6 @@ mod test_type_param {
fn defining<A>(s: A) -> Ty<A> { s }
fn assert_static<A: 'static>() {}
fn test<A>() where Ty<A>: 'static { assert_static::<A>() }
//~^ ERROR: parameter type `A` may not live long enough
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Member Author

@fmease fmease Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, WeakAlias<Co>: 'static now implies Co: 'static similar to ADTs. Is that acceptable?

@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 23, 2024
@bors

This comment was marked as resolved.

@JohnCSimon

This comment was marked as resolved.

@alex-semenyuk alex-semenyuk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 28, 2024
@fmease fmease force-pushed the lta-implied-bounds-recurse-shallow branch from c5165ba to 0e7952c Compare December 7, 2024 14:53
@fmease fmease marked this pull request as draft December 7, 2024 14:54
@fmease
Copy link
Member Author

fmease commented Dec 7, 2024

I'm having second thoughts about this. Treating weak aliases like ADTs during infer_outlives, i.e., super-visiting them is meh (visiting its generic arguments), esp. for TAIT. Weak aliases aren't really nominal after all.

Let's take a look at ADTs: I can see why it's convenient to imply the bound T: 'a in struct Outer<'a, T>(&'a Inner<T>); (where struct Inner<T>(T);) even though this super-visiting approach is relatively "unrefined"/simple:

For example, for struct Outer<'a, T: Trait>(&'a T::P); (where trait Trait { type P; }) we "conservatively" infer &'a <T as Trait>::P (which is good!) instead of let's say T: 'a. However, once we hide the projection behind another ADT, we're back to the "component-wise" outlives inference.
Like, we imply the bound T: 'a in struct Outer<'a, T: Trait>(&'a Inner<T>) where struct Inner<T: Trait>(T::P); :)

So for weak aliases, this ADT-behavior would be even "worse". I don't think we want to imply the bound T: 'a in the following scenario:

#![feature(lazy_type_alias)]
type Ty<'a, T: Trait> = &'a Aux<T>;
type Aux<T: Trait> = T::P;
trait Trait { type P; }

On master, we imply the bound Aux<T>: 'a for Ty contrary to this branch which currently implies the bound T: 'a as it visits T inside &'a Aux<T>.


Yet again, I'm wondering if we should instead employ "type_of"/expand_weak_alias_tys here, even though I cringe at the thought already (well, regarding expand_weak_alias_tys that'd probably be problematic since it recursively drills through weak aliases without touching any potential predicates on the corresponding lazy type aliases, which seems pretty bad but I haven't spent much thought on that yet).

Ignoring the details for now, the "type_of approach" would allow us to keep imply the bound Opaque<T>: 'a in the following code I'm pretty sure:

type Opaque<T> = impl Sized;
fn define<T>() -> Opaque<T> {}
struct Ty<'a, T>(&'a Opaque<T>);

On this branch, we currently imply the bound T: 'a instead ... which is yuck?!


@oli-obk, thoughts? ^^'

@fmease
Copy link
Member Author

fmease commented Dec 7, 2024

In summary, the options might be:

  1. Do nothing / keep as is. I.e., treat Alias(Weak) like any other Alias. This would mean we imply the bound Weak<T>: 'a when we're presented with e.g. <'a, T> &'a Weak<T>. As mentioned in the PR description, this would mean ExtractIfInner would no longer compile without modifications (i.e., adding more bounds).
  2. Treat Alias(Weak) like ADTs (this PR). We would imply T: 'a when faced with <'a, T> &'a Weak<T>. "Issues" are explained in the comment above.
  3. Peak inside the Alias(Weak) via ~type_of/expand_weak_alias_tys/... but we need to be careful (see comment above)! This more closely mimics the behavior of eager type aliases. We would only imply T: 'a in <'a, T> &'a Weak<T> if <T> Weak<T> "expands" (not normalizes) to a type that would imply T: 'a. We would continue to imply <T as Trait>::Projection: 'a if Weak "expands" to <T as Trait>::Projection and we would continue to imply Opaque<T>: 'a if Weak "expands" to Opaque<T>. This would also happen cross-crate, i.e., we would also expand type aliases from external crates. Seems fine?

@fmease fmease added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 9, 2024
@oli-obk
Copy link
Contributor

oli-obk commented Dec 10, 2024

can we somehow automatically encode this by wrapping weak alias types in a binder that has lifetimes for anything we could possibly want to bind? I guess this would be "precomputing" your option 3 and force every weak alias use site to think about what it wants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-implied-bounds Area: Implied bounds / inferred outlives-bounds F-lazy_type_alias `#![feature(lazy_type_alias)]` S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants