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

syntax: Lower priority of + in impl Trait/dyn Trait #45294

Merged
merged 4 commits into from
Jan 30, 2018

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Oct 15, 2017

Now you have to write Fn() -> (impl A + B) instead of Fn() -> impl A + B, this is consistent with priority of + in trait objects (Fn() -> A + B means (Fn() -> A) + B).

To make this viable I changed the syntax to also permit + in return types in function declarations

fn f() -> dyn A + B { ... } // OK, don't have to write `-> (dyn A + B)`

// This is acceptable, because `dyn A + B` here is an isolated type and
// not part of a larger type with various operator priorities in play
// like `dyn A + B` in `Fn() -> dyn A + B` despite syntax similarities.

but you still have to use -> (dyn A + B) in function types and function-like trait object types (see this PR's tests for examples).

This can be a breaking change for code using impl Trait on nightly. The thing that is most likely to break is &impl A + B, it needs to be rewritten as &(impl A + B).

cc #34511 #44662 rust-lang/rfcs#438

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 15, 2017
@pnkfelix
Copy link
Member

While I understand the idea of trying to be as flexible as possible, I do wonder if it breaks the principle of least surprise to accept non-parenthesized dyn A + Bin a fn-definition, but to also reject it in a fn-type or Fn-type.

Put another way: will users be more likely to write the correct thing for case of fn-type if we force them to use parentheses when writing an `fn-definition?

@nikomatsakis
Copy link
Contributor

I'm not sure yet what I think about (impl A + B), but my instinct is that impl and & ought behave somewhat analogously. In other words, since we write &(Debug + 'static), I would expect to at least be able to write impl (Debug + 'static). Similarly, I would expect that if (impl Debug + 'static) works, then (&Debug + 'static) ought to work, no?

(On the other hand, since & is a sigil and impl is a keyword, it seems not completely out of the question that their precedence might differ...but I feel like I'd prefer to avoid it if possible.)

@nikomatsakis
Copy link
Contributor

@pnkfelix

While I understand the idea of trying to be as flexible as possible, I do wonder if it breaks the principle of least surprise

I had a similar reaction.

(I also wonder if this is something we might consider tweaking in a Rust 2.0 Epoch -- i.e., we could encourage Rust 1.x users to write where F: (FnMut() -> Debug) + Send, while still accepting it without parentheses, and then change the precedence in Rust 2.0, so that where F: FnMut() -> Debug + Send parses as where F: FnMut() -> (Debug + Send).)

@petrochenkov
Copy link
Contributor Author

@pnkfelix

I do wonder if it breaks the principle of least surprise to accept non-parenthesized dyn A + Bin a fn-definition, but to also reject it in a fn-type or Fn-type.
Put another way: will users be more likely to write the correct thing for case of fn-type if we force them to use parentheses when writing an `fn-definition?

There's certainly a trade-off here between "least surprise" and "least annoyance".
So far return types of function items is the place where impl Trait is used so always requiring fn f() -> (impl A + B) { ... } would be pretty unpleasant, especially given that the parens are not technically necessary. But we can try and look at people's reaction :D

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Oct 17, 2017

@nikomatsakis

Similarly, I would expect that if (impl Debug + 'static) works, then (&Debug + 'static) ought to work, no?

Why? The () here are usual type parentheses (TYPE), not some special new syntax.

impl (Debug + 'static) on the other hand does add new syntax.
If I understand the idea correctly, in impl (A + B) parens are always required for multiple bounds and impl A is parsed as a single bound regardless of its context and following tokens? I.e.

IMPL_TRAIT = `impl` BOUND
    | `impl` `(` BOUNDS `)`
BOUNDS = Ø | BOUND (`+` BOUND)* `+`? 

This is certainly a viable alternative, maybe even more intuitive, even if it requires writing strictly more parens.
I'd also say requiring parens in fn f() -> impl (A + B) { ... } looks subjectively less annoying than in fn f() -> (impl A + B) { ... }.

Unnecessary parentheses for all sums are still pretty bad though. If I weren't followed the language closely and suddenly discovered that I need to change my Box<Write + Send>s into Box<dyn (Write + Send)> I'd start suspecting language designers in being evil.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Oct 17, 2017

Oh, also dyn (A + B) is a breaking change, pedantically speaking.

In #45175 it is disambiguated in favor of a function-like trait dyn with one argument A + B, see the test in https://github.com/petrochenkov/rust/blob/9d373204a5e67c91953f0080d4f93ebdcdcfc402/src/test/compile-fail/dyn-trait-compatibility.rs#L23.
But it is something that we certainly can (and probably should) break because function-like traits are unstable and we don't define any such traits named dyn ourselves.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Oct 17, 2017

@nikomatsakis

(I also wonder if this is something we might consider tweaking in a Rust 2.0 Epoch -- i.e., we could encourage Rust 1.x users to write where F: (FnMut() -> Debug) + Send, while still accepting it without parentheses, and then change the precedence in Rust 2.0, so that where F: FnMut() -> Debug + Send parses as where F: FnMut() -> (Debug + Send).)

If dyn Trait does become the new preferred syntax, then the current behavior will actually be preferable, because with switched rules disambiguation would happen exactly in favor of the deprecated dyn-less syntax ((Debug + Send)).

@joshtriplett
Copy link
Member

joshtriplett commented Oct 19, 2017

Rather than changing the precedence, personally, if we decided to change this I'd just as soon make it non-associative, requiring you to write parentheses for either meaning. Because I think where F: FnMut() -> Debug + Send is really awful to read, no matter which parse it has.

@petrochenkov
Copy link
Contributor Author

@joshtriplett
Warning on the ambiguity with Fn and maybe deprecating it may be a good idea (@nikomatsakis suggested it as well in #45294 (comment)), but precedence affects other cases as well, for example:

x as &Trait + y
x as &dyn Trait + y

This is currently parsed as (x as &Trait) + y without dyn, but x as &(dyn Trait + y) with dyn.
Similarly,

type A = &A + B; // ERROR
type B = &dyn A + B; // OK

Even if it's technically parseable and explainable, the inconsistency and the only precedent of binary "operator" having higher precedence than unary ones still annoy me.

@joshtriplett
Copy link
Member

Parsing &Trait + y as (&Trait) + y seems correct to me. Parsing &dyn Trait + y as (&dyn Trait) + y seems more correct, as well. Unary operators should almost always bind tighter.

@carols10cents carols10cents added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2017
@petrochenkov
Copy link
Contributor Author

Maybe I should write a mini-RFC describing possible alternatives, to get some wider feedback?

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 6, 2017

@petrochenkov sorry, I've been meaning to reply to this PR for a while now. I'd be in favor of a mini-RFC, or at least a good write-up. What I really want is an official rust grammar to express these changes in terms of. I'm sad we don't have that yet, though I think there are some unofficial ones that have progress quite far.

Let me at least clarify how I expected it to work. My expectation was that we had a setup like this:

T = T + U
U = Path
  | &U
  | impl U
  | dyn U
  | Box<T>
  | (T)
  | fn(T...) -> U 

// bounds
B = C | B + C
C = Path "<" T... ">"
  | Path "(" T... ")" -> U

This is kinda' what we have now, or at least how I think of it, and it means that I can do (e.g.) where F: Fn() -> Foo + Send without ambiguity, because after an -> operator, we only accept a type U, which excludes + outside of a parents. This does mean that (&Trait + Send) would not parse, but &(Trait + Send) does.

However, I realize that this doesn't account for bounds that are not types, like 'static. I'll have to think a bit more about what you wrote. =)

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 6, 2017

@petrochenkov

Oh, also dyn (A + B) is a breaking change, pedantically speaking.

Can you elaborate on this? A breaking change to stable syntax, or unstable?

OK, I see. You mean that right now dyn(A+B) would parse as some trait dyn applied to A + B; if we changed it to parse as a trait object, that's breaking. That seems true though this is almost certainly an error today, since () is unstable except when applied to FnOnce. I suppose you could do use std::ops::FnOnce as dyn or something.

@petrochenkov
Copy link
Contributor Author

I'll write an RFC.

@cramertj
Copy link
Member

cramertj commented Nov 28, 2017

@petrochenkov are you still planning to write something up for this?

@petrochenkov
Copy link
Contributor Author

@cramertj
Yes.

@petrochenkov
Copy link
Contributor Author

RFC is submitted
rust-lang/rfcs#2250

@cramertj
Copy link
Member

@petrochenkov I'm trying to get impl Trait issues closed out ASAP so that I can start FCP to stabilize before the next branching. Are you able to get this rebased today/tomorrow, or would you like me to do it?

@petrochenkov
Copy link
Contributor Author

@cramertj
Tomorrow.

@petrochenkov petrochenkov reopened this Jan 18, 2018
@petrochenkov petrochenkov force-pushed the prioplus branch 2 times, most recently from 2ef5fe8 to 732a623 Compare January 18, 2018 18:00
type A = fn() -> A + B;
//~^ ERROR expected a path on the left-hand side of `+`, not `fn() -> A`

type A = Fn() -> impl A + B; // OK, interpreted as `(Fn() -> impl A) + B`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get a separate test showing that this errors? (Hmm, well, I suppose such a test already exists, right @cramertj ?)

//~^ ERROR expected a path on the left-hand side of `+`, not `fn() -> A`

type A = Fn() -> impl A + B; // OK, interpreted as `(Fn() -> impl A) + B`
type A = Fn() -> dyn A + B; // OK, interpreted as `(Fn() -> dyn A) + B`
Copy link
Contributor

Choose a reason for hiding this comment

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

But this case probably doesn't error -- I sort of think it should.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we might be able to target it by linting. I did think though there was some desire to "put off" the question of just how Fn and friends works when combined with dyn and impl.

Copy link
Member

Choose a reason for hiding this comment

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

+1-- I think users should have to manually specify parentheses here, at least for the time-being.

Copy link
Contributor

Choose a reason for hiding this comment

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

It'll be a bit of a pain to do, I guess, since saying dyn A + B and A + B have same priority means that this case just kind of "falls out" this way.

But I still feel like we should keep future flexibility here.

@petrochenkov what do you think?

Copy link
Contributor Author

@petrochenkov petrochenkov Jan 20, 2018

Choose a reason for hiding this comment

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

Clippy lint?

To put this into perspective - few people remember priorities of operators in expressions beyond * vs + and && vs ||, but we still don't require parentheses everywhere.
Here if you get the priorities of Fn and + wrong, you won't even get runtime bugs, like with expressions, you'll get a type checking error about unsatisfied bounds, giving you opportunity to learn what priority is right.

Copy link
Contributor

Choose a reason for hiding this comment

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

More directly, I think the point was that we know we want this to work:

fn foo() -> impl A + B { }

and that therefore one might very well expect this to work:

where F: Fn() -> impl A + B

Now, historically, as @petrochenkov correctly points out, we opted not to make the where clause case "work" (that is, where F: Fn() -> A + B parses two bounds on F) because returning a value of type A + B was almost certainly not what you wanted. However, introducing keywords like dyn and impl gives us a chance to revisit this decision backwards compatibly, and it is not clear that -- in this new context -- this is the correct behavior.

As @cramertj noted, I don't think that anyone feels the correct interpretation of where F: Fn() -> impl A + B would be that B is a bound on F. (It's just a bit too surprising.) And moreover it seems clear that dyn ought to behave the same as impl. However, there were also those -- notably @joshtriplett -- who felt that there is no correct interpretation, and we should just avoid guessing. (I think that @joshtriplett would also be happy to have required parentheses in other cases where we do not today, e.g. amongst confusing operators.)

Given this, it does seem like we ought to try to disallow -> impl A + B and -> dyn A + B for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me try to clarify what I think we want. We already have two grammatical precedence levels for types. I'll them T0 and T1. T0 includes all types, including the "n-ary" operators like dyn A + B and A + B. T1 excludes those n-ary operators, and is used primarily for & types (which take a T1 argument).

Currently, in the parser, if we are parsing a T1 type and we see a + we stop, leaving it to be consumed from some enclosing context.

I think that we want to have a rule such that T1 = dyn Identifier parses, but if we see a + after that, we error out (in the parser). This means that &dyn A + B is a parse error, as is where F: Fn() -> dyn A + B, and (I imagine) dyn Fn() -> dyn A + B.

I think of this as being analogous to non-associativity: there are basically multiple interpretations of the + operator. It can be adding bounds to a type parameter in a where-clause list; it can be adding types in a (old-style, A+B) sum-type; and it can be adding bounds to a dyn or impl type.

In a fn item context, or in the T in Box<T>, once we see dyn or impl, there is only one valid interpretation (since we are not in a where clause list) -- . These are exactly the cases (I believe) where we use the grammatical nonterminal T0. We can parse eagerly there.

In other contexts, there are multiple contending interpretations. We are aiming not to choose between them (hence "non-associative").

Copy link
Contributor Author

@petrochenkov petrochenkov Jan 23, 2018

Choose a reason for hiding this comment

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

@nikomatsakis
Thanks for the detailed write up, this is better than just "lang team decided so".
I agree that consistency with fn f() -> A + B { ... } is a good argument in favor of potentially swapping priorities for Fn and + in the future.
I'll implement the conservative "refuse to disambiguate" solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This discussion should've probably been happening in rust-lang/rfcs#2250, because the RFC needs to be updated as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, I'll copy my post over there.


type A = Fn() -> impl A + B; // OK, interpreted as `(Fn() -> impl A) + B`
type A = Fn() -> dyn A + B; // OK, interpreted as `(Fn() -> dyn A) + B`
type A = Fn() -> A + B; // OK, interpreted as `(Fn() -> A) + B`
Copy link
Contributor

Choose a reason for hiding this comment

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

Obviously we can't alter this though.

@petrochenkov petrochenkov 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 Jan 25, 2018
@petrochenkov
Copy link
Contributor Author

Updated.

@petrochenkov petrochenkov 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 Jan 27, 2018
@nikomatsakis
Copy link
Contributor

@bors r+

Looks nice!

@bors
Copy link
Contributor

bors commented Jan 29, 2018

📌 Commit f57ea7c has been approved by nikomatsakis

@nikomatsakis nikomatsakis added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 29, 2018
@bors
Copy link
Contributor

bors commented Jan 29, 2018

⌛ Testing commit f57ea7c with merge da5ba14...

bors added a commit that referenced this pull request Jan 29, 2018
syntax: Lower priority of `+` in `impl Trait`/`dyn Trait`

Now you have to write `Fn() -> (impl A + B)` instead of `Fn() -> impl A + B`, this is consistent with priority of `+` in trait objects (`Fn() -> A + B` means `(Fn() -> A) + B`).

To make this viable I changed the syntax to also permit `+` in return types in function declarations
```
fn f() -> dyn A + B { ... } // OK, don't have to write `-> (dyn A + B)`

// This is acceptable, because `dyn A + B` here is an isolated type and
// not part of a larger type with various operator priorities in play
// like `dyn A + B` in `Fn() -> dyn A + B` despite syntax similarities.
```
but you still have to use `-> (dyn A + B)` in function types and function-like trait object types (see this PR's tests for examples).

This can be a breaking change for code using `impl Trait` on nightly. The thing that is most likely to break is `&impl A + B`, it needs to be rewritten as `&(impl A + B)`.

cc #34511 #44662 rust-lang/rfcs#438
@bors
Copy link
Contributor

bors commented Jan 30, 2018

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

kennytm commented Jan 30, 2018

@bors retry #46903

@bors
Copy link
Contributor

bors commented Jan 30, 2018

⌛ Testing commit f57ea7c with merge fe7e1a4...

bors added a commit that referenced this pull request Jan 30, 2018
syntax: Lower priority of `+` in `impl Trait`/`dyn Trait`

Now you have to write `Fn() -> (impl A + B)` instead of `Fn() -> impl A + B`, this is consistent with priority of `+` in trait objects (`Fn() -> A + B` means `(Fn() -> A) + B`).

To make this viable I changed the syntax to also permit `+` in return types in function declarations
```
fn f() -> dyn A + B { ... } // OK, don't have to write `-> (dyn A + B)`

// This is acceptable, because `dyn A + B` here is an isolated type and
// not part of a larger type with various operator priorities in play
// like `dyn A + B` in `Fn() -> dyn A + B` despite syntax similarities.
```
but you still have to use `-> (dyn A + B)` in function types and function-like trait object types (see this PR's tests for examples).

This can be a breaking change for code using `impl Trait` on nightly. The thing that is most likely to break is `&impl A + B`, it needs to be rewritten as `&(impl A + B)`.

cc #34511 #44662 rust-lang/rfcs#438
@bors
Copy link
Contributor

bors commented Jan 30, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing fe7e1a4 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants