syntax: Lower priority of `+` in `impl Trait`/`dyn Trait` by petrochenkov · Pull Request #45294 · rust-lang/rust

petrochenkov · 2017-10-15T00:45:29Z

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

pnkfelix · 2017-10-16T12:09:17Z

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 · 2017-10-16T18:50:48Z

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 · 2017-10-16T18:52:25Z

@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 · 2017-10-17T21:57:20Z

@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 · 2017-10-17T22:09:44Z

@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 · 2017-10-17T22:16:18Z

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 · 2017-10-17T22:26:32Z

@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 · 2017-10-19T19:59:57Z

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 · 2017-10-19T20:44:30Z

@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&dynTrait + y

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

typeA = &A + B;// ERRORtypeB = &dynA + 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 · 2017-10-19T21:02:03Z

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.

petrochenkov · 2017-11-04T01:02:27Z

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

nikomatsakis · 2017-11-06T19:32:46Z

@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 · 2017-11-06T19:36:14Z

@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 · 2017-11-10T23:06:20Z

I'll write an RFC.

cramertj · 2017-11-28T01:38:50Z

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

petrochenkov · 2017-11-28T08:55:37Z

@cramertj
Yes.

petrochenkov · 2017-12-16T16:33:30Z

RFC is submitted
rust-lang/rfcs#2250

cramertj · 2018-01-17T21:00:06Z

@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 · 2018-01-17T22:49:26Z

@cramertj
Tomorrow.

nikomatsakis · 2018-01-19T14:20:52Z

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

nikomatsakis · 2018-01-19T14:21:06Z

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

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.

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

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?

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.

More directly, I think the point was that we know we want this to work:
fnfoo() -> implA + B{}
and that therefore one might very well expect this to work:
whereF:Fn() -> implA + 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.

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").

@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.

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

Agreed, I'll copy my post over there.

nikomatsakis · 2018-01-19T14:21:24Z

Obviously we can't alter this though.

`+` is still disallowed in function types and function-like traits

petrochenkov · 2018-01-27T19:52:58Z

Updated.

nikomatsakis · 2018-01-29T20:22:00Z

@bors r+

Looks nice!

bors · 2018-01-29T20:22:01Z

📌 Commit f57ea7c has been approved by nikomatsakis

bors · 2018-01-29T23:18:25Z

⌛ Testing commit f57ea7c with merge da5ba14...

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 · 2018-01-30T02:21:36Z

💔 Test failed - status-appveyor

kennytm · 2018-01-30T02:51:02Z

@bors retry #46903

bors · 2018-01-30T08:23:48Z

⌛ Testing commit f57ea7c with merge fe7e1a4...

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 · 2018-01-30T11:10:01Z

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

rust-highfive assigned nikomatsakis Oct 15, 2017

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team labels Oct 15, 2017

nikomatsakis added the I-nominated label Oct 17, 2017

carols10cents added S-waiting-on-team and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2017

petrochenkov closed this Nov 10, 2017

petrochenkov mentioned this pull request Dec 16, 2017
mini-RFC: Finalize syntax of impl Trait and dyn Trait with multiple bounds rust-lang/rfcs#2250
Merged

petrochenkov reopened this Jan 18, 2018

petrochenkov force-pushed the prioplus branch 2 times, most recently from 2ef5fe8 to 732a623Compare January 18, 2018 18:00

nikomatsakis reviewed Jan 19, 2018
View reviewed changes

Comment threadsrc/test/parse-fail/impl-trait-plus-priority.rs Outdated
nikomatsakisJan 19, 2018
Copy link
Copy Markdown
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 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 added 4 commits January 27, 2018 22:37

syntax: Lower priority of + in impl Trait/dyn Trait
873b775

syntax: Permit + in return types of function declarations
95d27c3
`+` is still disallowed in function types and function-like traits

Add tests
d79f7cd

Make + in impl/dyn Trait non-associative
f57ea7c

petrochenkov force-pushed the prioplus branch from 732a623 to f57ea7cCompare January 27, 2018 19:51

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 approved these changes Jan 29, 2018
View reviewed changes

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 merged commit f57ea7c into rust-lang:masterJan 30, 2018

aturon mentioned this pull request Feb 27, 2018
Tracking issue for impl Trait (RFC 1522, RFC 1951, RFC 2071) #34511
Closed
29 tasks

petrochenkov deleted the prioplus branch June 5, 2019 16:02

ehuss mentioned this pull request Apr 25, 2020
Simplify rules for GenericArgs nonterminal and other small improvements and fixes rust-lang/reference#805
Closed

ehuss mentioned this pull request Oct 10, 2022
Fix grammar for TypePathFn rust-lang/reference#1281
Merged

Uh oh!

Uh oh!

Conversation

petrochenkov commented Oct 15, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pnkfelix commented Oct 16, 2017

Uh oh!

nikomatsakis commented Oct 16, 2017

Uh oh!

nikomatsakis commented Oct 16, 2017

Uh oh!

petrochenkov commented Oct 17, 2017

Uh oh!

petrochenkov commented Oct 17, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Oct 17, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Oct 17, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshtriplett commented Oct 19, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Oct 19, 2017

Uh oh!

joshtriplett commented Oct 19, 2017

Uh oh!

petrochenkov commented Nov 4, 2017

Uh oh!

nikomatsakis commented Nov 6, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikomatsakis commented Nov 6, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Nov 10, 2017

Uh oh!

cramertj commented Nov 28, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Nov 28, 2017

Uh oh!

petrochenkov commented Dec 16, 2017

Uh oh!

cramertj commented Jan 17, 2018

Uh oh!

petrochenkov commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkovJan 20, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkovJan 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Jan 27, 2018

petrochenkov commented Oct 15, 2017•
edited
Loading

petrochenkov commented Oct 17, 2017•
edited
Loading

petrochenkov commented Oct 17, 2017•
edited
Loading

petrochenkov commented Oct 17, 2017•
edited
Loading

joshtriplett commented Oct 19, 2017•
edited
Loading

nikomatsakis commented Nov 6, 2017•
edited
Loading

nikomatsakis commented Nov 6, 2017•
edited
Loading

cramertj commented Nov 28, 2017•
edited
Loading

petrochenkovJan 20, 2018•
edited
Loading

petrochenkovJan 23, 2018•
edited
Loading