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

Change precedence of `+` in type grammar #438

Merged
merged 4 commits into from Nov 18, 2014

Conversation

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Nov 4, 2014

Update type grammar to make + have lower precedence, consistent with the expression grammar, resolving a grammatical ambiguity.

Summary of impact (non-normative, from the RFC):

// Before                             After                         Note
// ~~~~~~                             ~~~~~                         ~~~~
   &Object+Send                       &(Object+Send)
   &'a Object+'a                      &'a (Object+'a)
   Box<Object+Send>                   Box<Object+Send>
   foo::<Object+Send,int>(...)        foo::<Object+Send,int>(...)
   Fn() -> Object+Send                Fn() -> (Object+Send)         // (*)
   Fn() -> &Object+Send               Fn() -> &(Object+Send)

// (*) Must yield a type error

Rendered view


```rust
fn foo<F>(f: F)
where F: FnOnce(&int) -> &Object + Send

This comment has been minimized.

@huonw

huonw Nov 4, 2014
Member

So, under this scheme, a type written like this is (FnOnce(&int) -> &Object) + Send?

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 4, 2014
Author Contributor

On Tue, Nov 04, 2014 at 03:16:07AM -0800, Huon Wilson wrote:

So, under this scheme, a type written like this is (FnOnce(&int) -> &Object) + Send?

Yes, right. That might occur in a context like Box<FnOnce(&int) -> &Object + Send>

@brendanzab
Copy link
Member

@brendanzab brendanzab commented Nov 4, 2014

Looks good to me.

@retep998
Copy link
Member

@retep998 retep998 commented Nov 4, 2014

I didn't know how much I wanted this until I saw this RFC, and now I want it.
Making the order consistent with normal expressions just seems like the right thing to do.

| ...
| '(' SUM ')'
SUM = TYPE { '+' TYPE }
PATH = IDS '<' SUM { ',' SUM } '>' '->' TYPE

This comment has been minimized.

@ben0x539

ben0x539 Nov 5, 2014

the '->' TYPE is spurious here, isn't it?

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 6, 2014
Author Contributor

On Wed, Nov 05, 2014 at 08:26:51AM -0800, Benjamin Herr wrote:

the '->' TYPE is spurious here, isn't it?

Yes, thanks. I was thinking of the () form.

| '(' SUM ')'
SUM = TYPE { '+' TYPE }
PATH = IDS '<' SUM { ',' SUM } '>' '->' TYPE
| IDS '(' SUM { ',' SUM } '>' '->' TYPE

This comment has been minimized.

@ben0x539

ben0x539 Nov 5, 2014

That should be ')' instead of > I assume?

SUM = TYPE { '+' TYPE }
PATH = IDS '<' SUM { ',' SUM } '>' '->' TYPE
| IDS '(' SUM { ',' SUM } '>' '->' TYPE
IDS = ID { :: ID }

This comment has been minimized.

@ben0x539

ben0x539 Nov 5, 2014

This doesn't account for absolute paths, does it? Also probably wants ' ' around the ::.

@comex
Copy link

@comex comex commented Nov 6, 2014

FWIW, &'a Foo+'a is already pretty verbose, and adding parentheses makes it more so.

@nikomatsakis
Copy link
Contributor Author

@nikomatsakis nikomatsakis commented Nov 6, 2014

On Wed, Nov 05, 2014 at 05:28:12PM -0800, comex wrote:

FWIW, &'a Foo+'a is already pretty verbose, and adding parentheses makes it more so.

Yes. I'd prefer to find a way to address this allows one to omit the
+'a altogether, just not sure what is the best way to do that in a
non-ad-hoc way yet.

@zwarich
Copy link
Contributor

@zwarich zwarich commented Nov 18, 2014

It was brought up in last week's Rust meeting that the precedence of + with respect to -> seems unintuitive.

@brson
Copy link
Contributor

@brson brson commented Nov 18, 2014

bors added a commit to rust-lang/rust 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 added a commit to rust-lang/rust 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
@Centril Centril added the A-syntax label Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.