Reject bounds in type aliases with edition 2024

[RalfJung]

I propose to make the type_alias_bounds lint deny-by-default with the next edition. See #21903 for more information on the issue being linted, and #48326 and #48909 for the PRs implementing the lint.

Cc @nikomatsakis @Manishearth (and Manish told me to also Cc someone else but I forgot whom)

[RalfJung]

Also maybe in the light of this, the lint message should be extended to also link to #21903 or something else providing further information?

[Manishearth]

cc @withoutboats

We can make it a hard error instead, fwiw.

[RalfJung]

We can make it a hard error instead, fwiw.

I thought deny-by-default-lint was as hard as we wanted to make things in the next edition, but yeah -- there is no good reason to still allow this code, other than to ease porting to the next edition by allowing the lint.

Would that mean this is a topic for rustfix?

[Manishearth]

Yes -- edition breakages work by taking a warn lint in a certain group and making it a hard error; and having a good suggestion that rustfix can apply

[RalfJung]

@nikomatsakis you mentioned some plans to fix where clauses in type aliases properly, so that they are added as obligations when type aliases are expanded. What is the status of that? I think this lint should become an error in the next edition, but whether it becomes Deny-by-default or a hard error I guess depends on whether we need this to be rule out for a transition in the 2021 edition.

[RalfJung]

After some talking with @Centril, I think ideally we want a lint that

• complains about type SendVec<T: Send> = Vec<T> because the bound is ignored
• complains about type Baz<I> = <I as Iterator>::Item because associated items should have a bound
• does not complain about type Baz<I: Iterator> = <I as Iterator>::Item because the bound is, de facto, enforced after expansion.

What makes this hard is that associated items are not the only way that a bound can be used; using a type like struct Foo<T: Send>(T) is another way -- but @eddyb said he had ideas.

[eddyb]

My idea is to diff what WF needs (i.e. the things that would error if not satisfied) with the predicates (bounds) written by the user.

Without Chalk this will be tricky, but we can manually run trait selection (or just plug something optional into the fulfillment context) to peek at how each WF requirement is satisfied, and mark every predicate that actually ends up used as part of that process, as useful - everything else, we'll warn (we can even have a consistency check, that if we remove the "unused bounds", WF still passes).

EDIT: Hang on, we might be able to do "difference" much easier by trying every declared bound with WF bounds as assumed: if they fail, that means the WF conditions enforced onto the user of the type alias aren't enough to subsume it, which is really what we want to check for (equivalence)!

[eddyb]

We actually have a very good reason to check "WF of RHS implies bounds" and not "used":

type DEIItem<T: DoubleEndedIterator> = T::Item;

The DoubleEndedIterator bound won't be checked in uses, only WF(T::Item) i.e. T: Iterator will.
And whenever we can start checking uses, then we don't really care if the bounds are used.

[eddyb]

I guess this is the same problem I recognized last year (#44075 (comment)), but now I have a plan.

[RalfJung]

@eddyb What is the deadline for this so that it could actually be an error in the next edition? Seeing #54057 makes me think it is already too late.

[Manishearth]

It's been too late for ages, there's significant work required to make an edition breakage robust.

[eddyb]

@Manishearth Ugh, really? I already have half of it implemented, the hard part is actually warning on Rust 2015, erroring on Rust 2018 is pretty easy.
What if I finish both halves that error on Rust 2018 today? I wish there were less things to do for Rust 2018, and we had noticed enough things in time...

[Manishearth]

I mean, I guess you could make it work but you need docs and testing and some time for it to bake. And you need the teams to approve this.

The "hard part" about warning on 2015 is precisely the problem, edition migration lints are tricky.

Make it a non breaking backwards compatible lint and we can eventually break it (it's not that major a breakage) or include it in the edition.

[eddyb]

@Manishearth What I'm working is doing the proper checks, which bring type aliases in line with other types, reverting the "force people to remove almost all bounds" approach (which I think was a complete mistake caused by miscommunication).
It looks like a bug fix, and the errors make sense the same way they do for anything but type aliases.

[Centril]

I for one really want to take the opportunity to fix this glaring and large hole in the language in Rust 2018.

We fixed the anon types in traits and made it a hard error in 2018 not too long ago, so I think we should be able to do the same with this. We have different people doing different things; so I can for example work on the documentation side of this in the edition guide.

I don't think it is too late to do this sort of thing; RC2 lands on 2018-10-25 for example - if it needs to be extended a bit (which I don't think it does), then so be it - having this hole in the language for several extra years cause we couldn't muster a few extra days to fix it seems bad to me.

[Mark-Simulacrum]

Yes, I don't think it's necessarily too late for lints to be added. However, we don't (I believe) have a great story for "migration" post-migration today which could be a blocker there - I'd want to see this lint as rustfix-able on both 2015 and 2018 codebases for those who migrate early (as we're asking them to).

[Manishearth]

Honestly I feel like a month is not enough time for this. I worked on the migration of a lot of the other edition breakages, and invariably it involved a LOT more work than anticipated. Nobody was on the same page, nobody had thought through the migration process, and there wasn't compiler dev input on the feasibility of the lints (let alone on the feasibility of emitting machine applicable suggestions). It took a lot of work to get this in place. Once the edition settles down I'm considering putting up an RFC that sets out baseline requirements for edition breakage RFCs that prevent these things in the future. I have _some_ ideas on the feasibility of the suggestion for this particular bug -- the spans of generics are tricky -- but when I mentioned the necessity of suggestions for this to @eddyb they felt it was impossible (?). We don't even have the code for the _lint_ for this yet, and eddyb also has path RFC lints to write. This is _not_ a matter of a couple of days, not in a time where there are a lot of other time-constrained edition priorities. I don't think we even have consensus on whether or not this _should_ be in the edition (regardless of whether or not it can make it) The alternative isn't too bad, we can stick a future incompatibility lint in there at our leisure and in a while evaluate if we can just straight out break it or stick it in the next edition.

[eddyb]

I'd want to see this lint as rustfix-able on both 2015 and 2018 codebases for those who migrate early (as we're asking them to).

But does it really need to be? What I want to do is figure out a way to measure the breakage. I suspect it's not as bad in practice (I already implemented the second half - the "reverse WF" - and I'm working on getting it up as a PR).

For the record, up to this point I haven't been informed about any requirement of automatic migration wrt the edition, for anything that's not universally used.

[Manishearth]

I'm pretty sure smooth migration was required in the edition RFC. If it's uncommon enough that you wouldn't need migration lints, then you won't need the edition _anyway_, we break stuff like this via future incompatibility lints all the time. Then it's best to postpone this till after the edition when folks have more time.

…

On Sun, Sep 9, 2018, 8:32 PM Eduard-Mihai Burtescu ***@***.***> wrote: I'd want to see this lint as rustfix-able on both 2015 and 2018 codebases for those who migrate early (as we're asking them to). But does it *really* need to be? What I want to do is figure out a way to measure the breakage. I suspect it's not as bad in practice (I already implemented the second half - the "reverse WF" - and I'm working on getting it up as a PR). For the record, up to this point I haven't been informed about any *requirement* of automatic migration wrt the edition, for anything that's not universally used. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49441 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABivSCywY8gJh7fX0vkb9YoECxL4v6UVks5uZS2UgaJpZM4S-Ov2> .

[eddyb]

@Manishearth The problem is that while it breaks somewhat rare, it breaks hard.
It's not rarely used code that does the things being broken, but commonly used code, just rarely so.

serde, for example, has one or two errors on #54033, that are trivially fixable (the error tells you to add a trait bound) - but we can't really break the entire ecosystem like that.

[Centril]

I feel this is too big to break via future incompat lints... If we can figure out smooth migration here to do hard errors in 2018, I'd love that, and I feel we should try to do it. It feels like a priority to me because it is too important a part of the language to leave broken in 2018. Anyways; I have nominated this for the next lang team meeting to see where the consensus lies.

[Manishearth]

If it's too big to break via future incompatibility, then you need good migration lints, and both I and eddyb are saying that there are issues with the implementation of those. The lang team isn't really the bottleneck here. Feel free to discuss it, but _please_ loop in the actual implementors (eddyb?) during the decision, a lot of the past issues around edition migration was a lack of communication around implementation. The suggestion infrastructure isn't amazing and it's nontrivial to improve, till then we have a lot of constraints on what the lints can actually _do_. 2018 isn't the last stage of Rust, I don't really understand the argument of "this seems too important to be broken in 2018". We've put plenty of important things on the back burner so that we can make this edition. If anything this seems super trivial -- it blocks the implementation of a relatively minor QoL feature and is inconsistent, (but most folks won't even encounter it so the inconsistency doesn't matter that much)

[Centril]

Feel free to discuss it, but please loop in the actual implementors (eddyb?) during the decision, a lot of the past issues around edition migration was a lack of communication around implementation.

Hopefully eddyb is in that meeting =P

We've put plenty of important things on the back burner so that we can make this edition.

Important things involving breakage?

My view is that we should try to fix inconsistencies earlier rather than later because delaying it will just make the problem worse; in addition, delaying it also adds more technical debt -- I'd rather aim for having incrementally less and less breakage each edition because problems were fixed early, instead of spreading breakage out.

[nikomatsakis]

One clarification on what I wrote:

I was getting ahead of myself when I wrote that we should support "extra" bounds.

I do think we should do that, but the current check that @eddyb implemented can't handle that -- the extra bounds would be ignored -- so for now we should not allow those.

[nikomatsakis]

We discussed in the @rust-lang/lang team meeting and decided we would:

• Make it a FCP warning in 2015
• Make it a hard error in 2018

where "it" refers to having "incorrect bounds" -- i.e., too many, or too few. We may in fact land the 2018 Hard Error first, since that is logistically easier, and 2018 code is unstable.

Ideally, we would do a migration for adding the ?Sized bound.

This FCW for 2015 may or may not ever become a hard error, it depends on how effective we are at removing older code and/or whether we find better alternatives. This is always true for future-compatibility warnings.

This should fit out general migration story:

• We believe that this formulation affects very few crates, therefore it is acceptable (although not ideal) that we don't have a fully automatic migration
• However, the formulation affects crates with many dependencies, so changing 2015 behavior must be done with care.

It's basically a mildly stronger variant of what @Manishearth was saying: we are doing the FCW, because we think the impact is small, but we are also tightening the noose a bit for 2018 code.

Seem reasonable?

[eddyb]

I should also note that the "too many" bounds warning/error, we can eventually turn off, when we have implemented the "check bounds at use sites" mechanism (probably involving injecting type aliases into the type-system, as projections, after we get lazy normalization).

Ignoring Sized, very few crates are affected by the "too many bounds" check (from the crater data we have so far), and it's plausible/likely that the bounds actually hold at the use sites, so we would simply let the code compile.

Whereas the "not enough bounds" warning can stay forever a warning on Rust 2015, since it only affects definition-site checking, not interactions between the type aliases and the typesystem.

---

As for automatic migration, we can probably do "add (un)bound to type-parameter" much easier than anything else, especially in simple cases like ?Sized.
Automatically removing bounds would be especially difficult (but ideally a non-problem).

[eddyb]

We have the crater report for "not enough bounds": #54033 (comment).
Aaaand it's 379 regressions. A random sample looks entirely legitimate to me.
Who'd've thought people use type aliases so much!

The lint from #48326 and #48909 didn't help (in that it was telling people to remove all bounds that aren't needed to resolve T::Assoc), but it couldn't have been responsible for all of those.

Based on this, I think we can keep it a warn-by-default lint in both editions, with no plans to make missing bounds an error. We don't have the infrastructure to do automatically applicable suggestions (i.e. scope-aware path printing) - maybe we will by 2021, but maybe we won't.

---

This does make our job easier, because "too many bounds" is the only future-compat hazard and it affects much fewer crates (and none of them are widely-used dependencies AFAICT).

[nikomatsakis]

This does make our job easier, because "too many bounds" is the only future-compat hazard and it affects much fewer crates (and none of them are widely-used dependencies AFAICT).

Say more about this? I guess you are thinking that "too few bounds" would break too many things, so we shouldn't warn about it?

(FWIW, I suspect that we could use an implied bound setup to accept the existing crates in a principled way, as well.)

[eddyb]

Well, we could warn, but we can't ever make it an error, or at least not in the near future.
And, yes, I agree about implied bounds, but it's almost meaningless/pointless until we're switching to the lazy normalization projection implementation.
Then it'd be a way to check the projection before/without expanding to the aliased type, by getting the implied bounds.

[RalfJung]

Related: #55222

[jpost-collegenet]

Since I see this being considered for edition 2024, I'll note that this lint fires on lifetime bounds which, while not enforced, are also not inert. In summary, if you want the below alias to behave like &T behaves with regards to dyn lifetime defaults, you need the bound (and removing the bound can break code relying on it).

pub type Borrow<'a, T: 'a> = &'a T;
// With the bound, `Borrow<'a, dyn Trait>` is `&'a (dyn Trait + 'a)`
// Without the bound, it is `&'a (dyn Trait + 'static)`

I.e. this applies to alias definitions in addition to struct (etc) definitions.

[fmease]

Superseded by #112792.