Rust: Reject bounds in type aliases with edition 2018

[RalfJung]

I propose to make the type_alias_bounds lint deny-by-default with the next edition. See https://github.com/rust-lang/rust/issues/21903 for more information on the issue being linted, and https://github.com/rust-lang/rust/pull/48326 and https://github.com/rust-lang/rust/pull/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 https://github.com/rust-lang/rust/issues/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 (https://github.com/rust-lang/rust/issues/44075#issuecomment-324752149), 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 https://github.com/rust-lang/rust/pull/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 [e-mail]
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
https://github.com/rust-lang/rust/issues/49441#issuecomment-419721907,
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.

[Manishearth]

Talking more with eddyb there's a chance that we _may_ be able to get away
with this with a very basic suggestion if we use crater runs, fix the
crates we find, and use the edition mechanism.

>
Its likely this only turns up in a couple large, common crates. Which is
perfect for the edition mechanism since we can just fix it and the old
versions still work.

[eddyb]

Turns out we have a bit of a problem (e.g. all 30 errors from owning_ref are like this):

type Foo<T> = Box<T>;

It's subtle, but there's an implicit T: Sized, and if we were able to correctly check type aliases, Foo<u8> would compile but Foo<str> wouldn't. But we can't, and currently, both are allowed.

So my checks that Foo<T> and Box<T> have equivalent requirements (correctly) catch the currently-unenforceable T: Sized, and make you change the definition of Foo to this:

type Foo<T: ?Sized> = Box<T>;

Another possibility is we could have T: ?Sized be the default / inferred on type aliases.
That way, you never have to write type Foo<T: ?Sized>.

EDIT: one potential solution is fully automatic migration - see https://github.com/rust-lang/rust/issues/49441#issuecomment-420005996.

[Manishearth]

How would you handle type Foo<T> = (T, u8) with the implicit bound? If that changes, how do we migrate that?

[eddyb]

@Manishearth If we leave it implied, that pretty much just means we keep the current behavior where only the type matters, not declared bounds, but specifically wrt Sized.
In your case, it'd mean T must be Sized, but for type Foo<T> = Box<T>;, it wouldn't.

[eddyb]

Another note on the Sized thing: ironically, it's easier to provide automatic migration for it, because you have to insert ?Sized (assuming the Sized bound that needs to be "removed" is implicit).
So we could have a mix of automatically migrating some things, and manually others, but we need to come to some decision, and gather enough data soon.

[eddyb]

Crater finished for the "reverse WF check" (i.e. "only necessary bounds"): https://github.com/rust-lang/rust/pull/54090#issuecomment-420096131
My analysis, copied from https://github.com/rust-lang/rust/pull/54090#issuecomment-420102434:

Most of the failures come from diesel, and are probably caused by the fact that this is only one half of the check. There are a few legitimate ones. Overall, this seems like not a dent in the ecosystem.

Whether we can do this at a larger scale now rests on "forward WF check" (i.e. "sufficient bounds"), which will likely need to wired through a lint before we can do a crater run for it.

[nikomatsakis]

Hmm. So let's talk through the alternatives. I see two dimensions:

• Settling on the behavior that we ideally want
• Figuring out when we will require this behavior and how to migrate code

Let me dig into those two a bit to try and summarize the status:

What behavior do we want? There seem to be a few questions here:

• Can people specify "extra" bounds, like type IsDebug<T: Debug> = T?
• Do people have to handle Sized correctly? e.g., type Foo<T: ?Sized> = Box<T>

My initial inclination is that, if we are going to fix this, the answer should be "yes" to both, but I suppose that the answers interact with the next question.

When are we going to fix this? There are a few options here:

Treat this as a bug fix and fix for all editions. This would be a future incompatibility lint. We have to settle on a period of time — as this is a long-standing bug, and one where we've gone back and forth, we should probably have a long transition time (6 months or a year). We could probably move to a deny-by-default lint sooner, in order to help push things along.

Treat this as an edition fix. This is what I had assumed we will do, but I'm beginning to rethink that. @eddyb's data suggests that the breakage is relatively limited, and that a migration period might let us patch up the crates in question. Also, the current behavior does seem pretty clearly like a bug, so I think calling it a bugfix is not wrong. Moreover, while I think it's possible to support both behaviors in perpetuity, I'm not dying to do it -- I'd like to move this kind of expansion into the trait system for various reasons (among them: letting us use type aliases in error messages) and it feels like it'd be nicer if we could transition to a single behavior eventually.

These two questions are inter-related, since if we pick stricter rules, that may imply more breakage. @eddyb, I don't really have a clear picture on just which variants you have tested thus far and how the quantities of breakage line up?

[nikomatsakis]

Some complications with treating this as an edition fix:

• As @Manishearth says, we are trying very hard to have automatic migration for anything other than "quite rare" cases
  
  
  
  • and if it is quite rare, maybe we should just fix...
  
  
• It imposes a timing deadline that doesn't exist otherwise

[Manishearth]

Also worth mentioning: the future incompatibility route is not that bad wrt
timing! The main impetus for making it a part of the edition is that we
won't get another chance for a while and having it now lets more cases
accumulate in the wild.

However, as long as we implement the future incompat lint _soon_ (but not
necessarily edition-soon) new cases won't really show up, it's just going
to be old cases we must deal with. We can potentially have the future
incompat lint be around for a long period of time and stuff won't get
_worse_. We can then make it a part of the next edition, or just do a clean
break.

On Tue, Sep 11, 2018, 10:25 PM Niko Matsakis [e-mail]
wrote:

Some complications with treating this as an edition fix:

• As @Manishearth https://github.com/Manishearth says, we are trying
  very hard to have automatic migration for anything other than "quite rare"
  cases
  
  
  
  • and if it is quite rare, maybe we should just fix...
  
  
• It imposes a timing deadline that doesn't exist otherwise

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/49441#issuecomment-420343837,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABivSLd-zXRFd4SOhxYTxxeMZjPyQcjCks5uZ-sYgaJpZM4S-Ov2
.

[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": https://github.com/rust-lang/rust/pull/54033#issuecomment-422251858.
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: https://github.com/rust-lang/rust/issues/55222