rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.49k stars 12.6k forks source link

Proposal: Change syntax of where clauses on type aliases #89122

Closed nikomatsakis closed 2 years ago

nikomatsakis commented 3 years ago

Source

Summary

Proposed: to alter the syntax of where clauses on type aliases so that they appear after the value:

type StringMap<K> = BTreeMap<K, String>
where
    K: PartialOrd

This applies both in top-level modules and in trats (associated types, generic or otherwise).

Background

The current syntax for where to place the "where clause" of a generic associated types is awkward. Consider this example (playground):

trait Iterable {
    type Iter<'a> where Self: 'a;

    fn iter(&self) -> Self::Iter<'_>;
}

impl<T> Iterable for Vec<T> {
    type Iter<'a>
    where 
        Self: 'a = <&'a [T] as IntoIterator>::IntoIter;

    fn iter(&self) -> Self::Iter<'_> {
        self.iter()
    }
}

Note the impl. Most people expect the impl to be written as follows (indeed, the author wrote it this way in the first draft):

impl Iterable for Vec<T> {
    type Iter<'a>  = <&'a [T] as Iterator>::Iter
    where 
        Self: 'a;

    fn iter(&self) -> Self::Iter<'_> {
        self.iter()
    }
}

However, this placement of the where clause is in fact rather inconsistent, since the = <&'a [T] as Iterator>::Iter is in some sense the "body" of the item.

The same current syntax is used for where clauses on type aliases (playground):

type Foo<T> where T: Eq = Vec<T>;

fn main() { }

Top-level type aliases

Currently, we accept where clauses in top-level type aliases, but they are deprecated (warning) and semi-ignored:

type StringMap<K> where
    K: PartialOrd
= BTreeMap<K, String>

Under this proposal, this syntax remains, but is deprecated. The newer syntax for type aliases (with where coming after the type) would remain feature gated until such time as we enforce the expected semantics.

Alternatives

Keep the current syntax.

In this case, we must settle the question of how we expect it to be formatted (surely not as I have shown it above).

impl<T> Iterable for Vec<T> {
    type Iter<'a> where Self: 'a 
        = <&'a [T] as IntoIterator>::IntoIter;

    fn iter(&self) -> Self::Iter<'_> {
        self.iter()
    }
}

Accept either

What do we do if both are supplied?

nikomatsakis commented 3 years ago

@rfcbot fcp merge

@jackh726 and I have discussed this issue and we both agree that changing the syntax here would be correct. I propose that we go ahead and do it.

rfcbot commented 3 years ago

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

cramertj commented 3 years ago

Interesting! I can see why this would be desirable-- the formatting can work out a bit nicer in the new version, and it helps front-load the most important part of the alias (that is, it visually defers the bound until after the main part of the alias has been stated). However, I see two advantages to the previous approach:

  1. It's simpler to copy-paste bounds from the trait definition to the impl. trait Foo { type Bar where X: Y; } -> impl Foo for X { type Bar where X: Y = ...; } both match up visually, and one can create an impl by merely copy-pasting the trait definition and adding the appropriate = .... I appreciate this symmetry, and it would be surprising to me if this were not supported, or if I was given a warning.
  2. Conceptually, the bound is applying to the way the type alias may be used as stated in the trait definition. That is, the bound is not a property specific to the impl. This makes it more similar to the type signature of a trait method than the body of a trait method. Based on this, I'd expect it to belong on the left-hand side of the =. Trait definition / requirements first / on the left, followed by specifics of the impl is a useful pattern to keep, I think.

I don't think I feel particularly strongly either way, but the reasons above contribute to my initial preference for the old syntax.

shepmaster commented 3 years ago

@estebank pointed out to me that the parser might end up parsing both forms anyway, in order to provide useful diagnostics.

nikomatsakis commented 3 years ago

Yes, I expect the parser to parse both forms and offer useful tips for how to transform from one to the other.

joshtriplett commented 3 years ago

FWIW, I can see why the type ... where ... = ... syntax would make sense as well, since it seems analogous to fn ...() where ... { ... }. I do think the proposed change feels more likely to produce readable code, though. On balance, I'd expect a where to get more complex than the RHS of the =.

camelid commented 3 years ago

Note also that trailing where clauses are supported for tuple structs:

// Compiles successfully.
pub struct Struct<T>(T)
where
    T: Eq;
petrochenkov commented 3 years ago

@estebank pointed out to me that the parser might end up parsing both forms anyway, in order to provide useful diagnostics.

I think this is the way to go. If this proposal is accepted, then we'll need to support where clauses in both positions anyway due to compatibility. In that case why not return a non-fatal error in one of the cases, and normalize legal syntax to the single already existing form.

Most people expect the impl to be written as follows

What makes you think so? In functions where clauses are between the header and the body, why in types it should be after the "body"?

In trait aliases, which have similar syntax with =, where is actually a part of the "body". Will it use a second where for where clauses?

trait Alias = Body1 where Self: Body2 where NotABodyAnymore;

?

nikomatsakis commented 3 years ago

What makes you think so?

A fair question! Anecdotal reports. It's hard to quantify this, perhaps I should rewrite to "many people". We could try to get more precise about this, perhaps via a survey.

In that case why not return a non-fatal error in one of the cases, and normalize legal syntax to the single already existing form.

e.g., via rustfmt? That is an option, yes. We could simply union where clauses in the two positions.

rfcbot commented 3 years ago

:bell: This is now entering its final comment period, as per the review above. :bell:

cramertj commented 3 years ago

Ah-- I made a procedural mistake and did not file a concern. I assume we're still considering this pending further discussion?

@rfcbot concern do-we-want-to-make-this-change

nikomatsakis commented 3 years ago

We discussed this in our @rust-lang/lang meeting today. There wasn't a clear consensus one way or the other.

Some points we identified:

There are plenty of cases in Rust's existing grammar where where clauses follow types:

fn foo() -> T
where ...
{ }

impl Foo for Bar
where ...
{}

Therefore, there isn't really a concern about introducing new grammatical ambiguities (this was raised as a concern in the meeting).

One thing we did find is that, when reading type Foo = Bar where Baz, Some people mentally "group" the where clause with Bar, and some mentally group it with Foo. Note that, in all existing instances, where clauses are attached to the surrounding item.

We discussed whether changes to the formatting rules might help, but we didn't discuss that very much.

estebank commented 3 years ago

Third option: support both 😬

We have to support both in the parser for diagnostics, what if we just... accepted both ways?

thorjelly commented 3 years ago

I personally feel as though type Foo<T> = Bar<T> where T: ... is semantically equivalent to fn foo<T>() -> Bar<T> where T: ... and impl Foo<T> for Bar<T> where T: ..., where in each case =, ->, and for sort of act as operators on Foo and Bar. Therefore, I feel like the new proposal is most consistent with the syntax people are most used to.

This is coming from someone who has barely used the current implementation of GATs, so I am unused to its syntax, I would find the new proposal a fair bit more intuitive.

nikomatsakis commented 3 years ago

@estebank I feel like supporting both without a real reason to support both is suboptimal. Then there will be confusion over whether there is a difference, for example, and perhaps two camps (one who prefers one, one who prefers the other). I don't mind more than one way to do things, but it feels like it needs strong justification to me.

One thing I would like is if rustfmt quietly changed from one to the other, though =) that would affect my opinion. But this isn't generally how rustfmt does things.

dtolnay commented 3 years ago

In the context of the FCP proposed in https://github.com/rust-lang/rust/issues/89122#issuecomment-923104269, what behavior is proposed if where is given in both places?

type Thing<T> where T: IntoIterator = T::IntoIter
where
    T::Item: Clone;

Does this union the clauses, or does it reject? From the discussion above I'm unable to tell whether a concrete behavior was proposed. Asking as someone who will need to implement this in a Rust parser.

dtolnay commented 3 years ago

Separately, I feel that https://github.com/rust-lang/rust/issues/89122#issuecomment-923266388 is going to require a response/consensus before completing the FCP.

Here is a concrete example to consider:

#![feature(trait_alias)]

trait Alias<T: Clone> = where T: Default;

// okay, equivalent to `where T: Clone + Default`
fn clone<T: Alias<T> + Clone>() {}

// ERROR, trait bound `T: Clone` not satisfied
fn default<T: Alias<T> + Default>() {}

In this situation, what is proposed to be the where equivalent of the T: Clone bound on Alias? Prior to this proposal, it's natural to expect that it would be:

trait Alias<T> where T: Clone = where T: Default;

This proposal throws a wrench in the works, as @petrochenkov called attention to. It would now seem to be the somewhat bizarre:

trait Alias<T> = where T: Default where T: Clone;

Relatedly, what does the following mean?

trait Alias<T> = Default
where
    T: Clone;

Is this equivalent to trait Alias<T: Clone> = Default, or to (the old interpretation of) trait Alias<T> = Default where T: Clone, which are not the same thing?

trait_alias is unstable (cc tracking issue https://github.com/rust-lang/rust/issues/41517) but since the syntax is heavily inspired by type alias syntax, I think it's worth acknowledging the implications that type alias syntax changes would have on it, and maybe loop in people involved in that proposal if there are specific people.

yasammez commented 3 years ago

Third option: support both 😬

We have to support both in the parser for diagnostics, what if we just... accepted both ways?

This would be my least preferred option: if you support two variants for the same thing, any codebase of sufficient size will eventually contain both, no matter how strict you are trying to be. This increases the mental burden for developers trying to understand existing code and leads to needless discussions about which style to prefer within teams.

nikomatsakis commented 3 years ago

@petrochenkov

In trait aliases, which have similar syntax with =, where is actually a part of the "body".

Remind me, do trait aliases currently permit where clauses in both places?

petrochenkov commented 3 years ago

do trait aliases currently permit where clauses in both places?

Not right now, but both positions have a well-defined meaning in theory, as @dtolnay describes in https://github.com/rust-lang/rust/issues/89122#issuecomment-926252834.

joshtriplett commented 3 years ago

I personally think we should support where in the trailing position as proposed here, and also (later) fully support defining type parameters inline in the angle brackets. That would make these declarations just like functions, where you can either define type parameters in the angle brackets or declare them in the angle brackets and define them in a where clause.

nikomatsakis commented 3 years ago

I agree that this causes a problem for the trait alias syntax, but I also think that the trait alias syntax is potentially quite confusing. In fact, there is an analogous problem with GATs, and we don't really have an answer to it. Consider:

trait Foo {
    type Bar<T: A>: B where T: C;
}

Here, given a type X = <P0 as Foo>::Bar<P1>...

In this situation, we don't have the luxury of a = sign to distinguish the two 'directions'. I expect we might want a kind of keyword, let's say ensures:

trait Foo {
    type Bar<T: A>: B
        where T: C
        ensures T: Foo<Self::Bar<T>>;
}

Well, reading that I would say it's meaning is far from clear, but nonetheless the point stands: the "solution" of having the meaning of where in a trait alias vary depending on whether it comes before or after the = sign isn't a good one, because (a) it is bound to be confusing for users and (b) it doesn't scale to the analogous scenario in a type alias.

nikomatsakis commented 3 years ago

(When it comes to GATs, I have basically felt that ensures is something that we may never find there is much need to express, so I haven't been too worried about it, though I guess ironically trait aliases could address it, but only if we kept the where clause syntax as it was (or found some alternative):

trait RevFoo<T> = where T: Foo<Self>

=)

petrochenkov commented 3 years ago

My main general concern here is a feature creep. We already have a syntax that works -> no need to add a new alternative syntax, especially one that will require even more syntax later on.

jackh726 commented 3 years ago

FWIW, I think the in the trait alias case, having where clauses in two different positions mean two different things is confusing and a recipe for disaster. This opinion might stem from me not fully knowing how trait aliases work and what each of those where clauses do.

To extend upon this: the necessity of where clauses on GATs surely will/does confuse users; particularly since so many are used to being able to use T: Trait and where T: Trait interchangeably basically everywhere. To continue to build on that, and this is a little "bigger picture", I think if a key argument against changing the position of the where clauses is that it will be confusing later on for some other feature, then maybe it would be confusing regardless. Looking back at @dtolnay's comment, it does seem like the "bound" vs "where-clause" question does seem to mirror as in GATs - i.e. they're not actually interchangeable.

In this case, I don't think changing the position of the where clause will change anything as far as any confusion of T: Trait vs where T: Trait. Rather, I think the decision here should be made on where people expect the where clause to be when writing GATs, what feels most natural, and what is most readable. Ultimately, it is subjective, so we might just end up in a situation where we just have to pick one.

I'll try to make a comment fairly shortly to sum up the current arguments for and against each.

nikomatsakis commented 3 years ago

I thought about trait aliases today. I was wondering whether this syntax would work:

trait Foo<T> = T: Ord

i.e., can we just accept full where clauses or bounds in that position? It seems plausible :)

fogti commented 3 years ago

I think that would also have the problem of being confusing (even moreso, because that syntax (... = ...: ...;) is afaik completely new). A new keyword for that would be imo more justified.

estebank commented 3 years ago

The syntactic distance of trait Foo<T> = T: Ord; to other valid constructs feels to me too close for comfort. I prefer when things that have different semantics look clearly different.

nikomatsakis commented 3 years ago

I'm also thinking about the "still unstable" T: Iterator<Item: Debug> syntax. There's an often lot of complexity potentially wound up in where clauses.

nikomatsakis commented 3 years ago

I've opened a @rust-lang/lang design meeting proposal on this topic https://github.com/rust-lang/lang-team/issues/120, to be scheduled some time in October

nikomatsakis commented 2 years ago

This has a pending design meeting scheduled for tomorrow! I'm going to cancel the FCP until we have time to discuss there.

@rfcbot cancel

rfcbot commented 2 years ago

@nikomatsakis proposal cancelled.

jackh726 commented 2 years ago

GATs issue triage: blocking. Though we don't need strictly need to decide to change the syntax, we do have to make a decision before stabilization.

dhardy commented 2 years ago

Reject?

While this proposal sounds initially attractive, it seems we have good grounds to reject it:

  1. where before definition is closer to existing syntax (fn foo<T>(..) where T: A { .. }, impl)
  2. Apparent ambiguity: trait A { type Item = Self where Self: Sized; } reads like where is associated with the default value, but it is not
  3. Actual ambiguities in combination with trait alias, as noted above

Alternative: ensures keyword

The attraction of this proposal (mentioned in passing by @nikomatsakis above) is that it differentiates "input bounds" (on type parameters) from "output bounds" on the result.

Unfortunately, implementing this consistently would be a significant breaking change (albeit one that Editions can handle gracefully). Consider:

// supported today:
trait T where Self: 'static {}
// equivalent to: trait T: 'static {}

This should become:

trait T ensures Self: 'static {}

The advantages here:

  1. The actual ambiguities can be resolved
  2. Possibly better syntax for trait alias: trait DebugDefault ensures Self: Debug + Default;

Alternative: better formatting

For short bounds, the current syntax looks fine given better formatting, as noted in the opening post:

impl<T> Iterable for Vec<T> {
    type Iter<'a> where Self: 'a 
        = <&'a [T] as IntoIterator>::IntoIter;
    // ...
}

For longer lists of bounds things are unfortunately a bit ugly, but perhaps we should just accept that:

pub trait ReadableData {
    type Item;

    // Support std::cell::Ref and std::sync::MutexGuard but default to &'a Self::Item
    type ItemRef<'a>: Deref<Target = Self::Item>
    where
        Self: 'a,
        = &'a Self::Item;

    fn get_ref(&self) -> Self::ItemRef<'a>;

    fn get_cloned(&self) -> Self::Item where Self::Item: Clone;
}

IMHO the latter alternative (better formatting) is preferable.

jackh726 commented 2 years ago

While this proposal sounds initially attractive, it seems we have good grounds to reject it:

I'll go through these points one by one; I disagree that they are good grounds to reject.

  1. where before definition is closer to existing syntax (fn foo<T>(..) where T: A { .. }, impl)

As was pointed out in the lang design meeting, a counter-example is struct Foo<T>(T) where T: Default;; here, the where clause is after the "definition". It was noted that a better "pattern" is that where clauses are before brackets - but that doesn't apply to associated types.

  1. Apparent ambiguity: trait A { type Item = Self where Self: Sized; } reads like where is associated with the default value, but it is not

This is an okay argument, but is subjective.

  1. Actual ambiguities in combination with trait alias, as noted above

After looking more in depth with this, this isn't actually a concern. For example, where clauses before the equals doesn't parse (parser, play).

Additionally, the case noted above, i.e. rewriting a bound as a where clause, indeed is not possible today. Specifically, as discussed in the design meeting, the former gives us some guarantees via implied bounds. There where clauses on GATs and are trait aliases are paralleled, so if we stick with the current syntax for where clauses on GATs, it probably makes sense to switch them for trait aliases (to be before the equals). In fact, if we did want to be able to move "bounds" to a where clause before the equals in trait aliases, shouldn't we expect to be able to do that with GATs? (I bring this up, since it's actually a point in favor of moving it for GATs)

Another counterpoint (in other words, a point in favor of switching) from the meeting: in no other place today (unless we've missed something) do we start a line with an =. You might see something like

let written_predicates: ty::GenericPredicates<'_> =
    tcx.explicit_predicates_of(trait_item.def_id);

with the = at the end of the line though.

Finally, since I've written a PR that does this syntax change, we can actually look at how code will change in practice: https://github.com/rust-lang/rust/pull/90076/files. My favorite example is changing from

    type TRef<'a>
    where
    <Left as HasChildrenOf>::T: 'a,
    <Right as HasChildrenOf>::T: 'a
    = Either<&'a Left::T, &'a Right::T>;

to

    type TRef<'a> = Either<&'a Left::T, &'a Right::T>
    where
        <Left as HasChildrenOf>::T: 'a,
        <Right as HasChildrenOf>::T: 'a;

To me, the second looks a heck of a lot better. Perhaps the most important thing to know is that the "definition" (= Either<&'a Left::T, &'a Right::T>) is inline with the "type declaration" (type TRef<'a>); this frontloads the "important" information of the definition.

nikomatsakis commented 2 years ago

FCP proposed on @jackh726's PR: https://github.com/rust-lang/rust/pull/90076#issuecomment-962173830

The write-up is available here to read, as well:

https://rust-lang.github.io/generic-associated-types-initiative/design-discussions/where-the-where-1.html

calebcartwright commented 2 years ago

This change seems like it would put us in a really tricky position as it relates to rustfmt.

IIUC clauses will be supported in both positions, so is rustfmt supposed to:

fogti commented 2 years ago

@calebcartwright About (2): I think this should be the job of cargo fix or such.

calebcartwright commented 2 years ago

@calebcartwright About (2): I think this should be the job of cargo fix or such.

You still have to account for the formatting piece though, otherwise you'll have cargo fmt and cargo fix dueling and changing the code back and forth

fogti commented 2 years ago

@calebcartwright Thus I think rustfmt should not change the position around, but keep it where it already is. -> (3)

calebcartwright commented 2 years ago

@calebcartwright Thus I think rustfmt should not change the position around, but keep it where it already is. -> (3)

That's not an easy/simple answer either. Just doing that and that alone would introduce a direct contradiction between the style guide that codifies the rules rustfmt's defaults must adhere to, and that would be the first such occurrence afaik.

Apologies if the rustfmt/formatting pieces have already been discussed here/elsewhere, but until a few hours ago I had no idea this change was even being considered, and am trying to play catch up. There's a separate RFC-based process that governs the formatting rules, and that's definitely applicable here as well.

Until the style guide is updated accordingly (and we'll need separate prescriptions for the respective positions), rustfmt shouldn't touch/modify those aliases for which the style guide does not have codified rules.

nikomatsakis commented 2 years ago

I responded to @calebcartwright here

vorot93 commented 2 years ago

I just filed #91883. Couldn't fix w/o asking @jackh726 for help: I instinctively knew that I needed a where clause in associated type in impl, but I could never guess to put it before = - so I thought it's not supported at all.

bombless commented 2 years ago

I just noticed that the syntax is alreay arbitary

struct A<T> (bool, T) where T: Copy;
struct B<T> where T: Copy { a: T }
type C = A<String>;
type D = B<String>;

Even after we decide where should behind something or before anything, we still need to think of a new way to express struct A<T> (bool, T) where T: Copy; I imagine it should be struct A<T> where T: Copy (bool, T); Okay we have a new problem, we don't know the ending of the type part here. So maybe struct A<T> where T: Copy {(bool, T)}. Now, how about we make where always apear before braces. So type alias become type NewAlias <T> where T: Copy { Box<T> };

p-avital commented 2 years ago

While I agree that where clause placement is sometimes a bit awkward, I really don't feel it's worth blocking GATs.

Shouldn't this wait for a later release, so that the decision can be given time without blocking GATs?

jackh726 commented 2 years ago

I just noticed that the syntax is alreay arbitary

struct A<T> (bool, T) where T: Copy;
struct B<T> where T: Copy { a: T }
type C = A<String>;
type D = B<String>;

This was discussed in a lang design meeting a couple months back: https://github.com/rust-lang/lang-team/blob/master/design-meeting-minutes/2021-10-13-where-the-where.md#q-4

While I agree that where clause placement is sometimes a bit awkward, I really don't feel it's worth blocking GATs.

This 100% has to block, given that where clauses on associated types are currently unstable. It doesn't matter too much; this question is resolved and just waiting for the PR to be approved and merged.

swfsql commented 2 years ago

(removed my commend, I changed my mind)