Bounds parsing refactoring 2 #39158

petrochenkov · 2017-01-18T17:09:18Z

See #37511 for previous discussion.
cc @matklad

Relaxed parsing rules:

zero bounds after : are allowed in all contexts.
zero predicates are allowed after where.
trailing separator , is allowed after predicates in where clauses not followed by {.

Other parsing rules:

trailing separator + is still allowed in all bound lists.

Code is also cleaned up and tests added.

I haven't touched parsing of trait object types yet, I'll do it later.

rust-highfive · 2017-01-18T17:09:30Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

durka · 2017-01-18T18:01:44Z

Why disallow trailing + when trailing , is allowed? Does it create an ambiguity?

petrochenkov · 2017-01-18T18:28:40Z

@durka
No ambiguity, it's a lang team decision.

(I'd personally keep it though, because 1) uniformity (other trailing separators are allowed), 2) it was accepted since 1.0 until now, 3) it's "naturally" allowed and you have to add extra code to the parser to prohibit it)

petrochenkov · 2017-01-18T18:31:15Z

@durka
Do you have an example of trailing + being useful in macros?

durka · 2017-01-18T18:32:07Z

Yeah, those three reasons make sense to me. This should be cratered then, WRT #2.

…

On Wed, Jan 18, 2017 at 1:29 PM, Vadim Petrochenkov < ***@***.***> wrote: @durka <https://github.com/durka> No ambiguity, it's a lang team decision <#37511 (comment)>. (I'd personally keep it though, because 1) uniformity (other trailing separators are allowed), 2) it was accepted since 1.0 until now, 3) it's "naturally" allowed and you have to add extra code to the parser to prohibit it) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#39158 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3n5mUeuO3uFj1On8BN5wcj7OzoCK4ks5rTlnsgaJpZM4LnJLp> .

durka · 2017-01-18T18:32:59Z

I don't think it's useful in macros, because you can just do `$($bounds)++` and it's no problem. I was thinking of consistency between various types of separators. But then again you don't want "1 + 1 +" to be a valid arithmetic expression.

…

On Wed, Jan 18, 2017 at 1:31 PM, Vadim Petrochenkov < ***@***.***> wrote: @durka <https://github.com/durka> Do you have an example of trailing + being useful in macros? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#39158 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3n9kaqguUTZiZyDI3GUOpgy_ahvJiks5rTlqHgaJpZM4LnJLp> .

petrochenkov · 2017-01-18T18:44:17Z

But then again you don't want "1 + 1 +" to be a valid
arithmetic expression.

Expression + and type + are actually different! 🙂
Expression + is a binary operator and lists like 1 + 1 + 1 + 1 are desugared into several operator applications ((1 + 1) + 1) + 1.
Type + is not a binary operator and summands in lists like Trait + Trait + Trait can't be grouped. (Trait + Trait) + Trait and ArbitraryType + Trait in general are not valid.

nikomatsakis · 2017-01-19T07:25:49Z

@petrochenkov I feel like I would keep it too -- which is probably why it's parsed in the first place. I wonder if it's worth revisiting this. I found your three points convincing, and I'm not sure that we focus on the backwards compatibility point in the meeting.

cc @rust-lang/lang -- we had previously decided to not accept T: Foo + Bar + (where + acts as a trailing separator) because it "looks weird" (which is indisputably true). However, @petrochenkov points out that it is accepted today (also indisputably true). "Looking weird" doesn't feel like reason enough to break backwards compatibility to me, particularly since accepting trailing separators is known to be useful for macros. So I think I'd like to revisit this decision. What do people think?

withoutboats · 2017-01-19T08:40:56Z

I think when we agreed on that we thought we were talking about expanding what is accepted to include trailing +. I agree we shouldn't break the language.

joshtriplett · 2017-01-19T19:53:37Z

@nikomatsakis If we accept this today, we should continue doing so, and document it for potential use by macros. Not worth breaking backward compatibility over.

aturon · 2017-01-21T12:59:12Z

@nikomatsakis I agree with your reasoning; let's keep it.

nikomatsakis · 2017-01-23T20:51:37Z

@petrochenkov care to update the PR to accept trailing +?

@nrc you were the main one who objected, I think, so I'd like to get your input in particular

(Does anyone feel we need to use rfcbot here?)

nrc · 2017-01-23T21:06:53Z

@nikomatsakis yeah, I agree its not worth breaking for.

petrochenkov · 2017-01-24T20:41:25Z

Updated with trailing +s permitted.

nikomatsakis · 2017-01-25T17:21:55Z

@bors r+

Thanks @petrochenkov !

bors · 2017-01-25T17:21:56Z

📌 Commit 65aeafa has been approved by nikomatsakis

nikomatsakis · 2017-01-25T17:22:04Z

@bors r-

nikomatsakis · 2017-01-25T17:22:13Z

Oops :) had one thing I wanted to ask

nikomatsakis · 2017-01-25T16:01:40Z

src/librustc_passes/ast_validation.rs

+            }
+        }
+        for predicate in &g.where_clause.predicates {
+            if let WherePredicate::EqPredicate(ref predicate) = *predicate {


heh, we should really remove this variant from the compiler (and parser)

to be clear, not as part of this PR =)

There are no plans to implement this?
Then #20041 should be officially closed or something before removing WherePredicate::EqPredicate.

Well, eventually perhaps, but at the moment there are some theoretical limitations that I do not yet know how to overcome. I would say "we may implement it eventually, but in that case we can add that stuff back in to the compiler".

nikomatsakis · 2017-01-25T16:12:11Z

src/libsyntax/parse/parser.rs

            // BORROWED POINTER
            self.expect_and()?;
            self.parse_borrowed_pointee()?
        } else if self.check_keyword(keywords::For) {
+            // FIXME plus priority


Can you clarify what this FIXME means? Ideally, if there's an actual bug we care about here, we'd open an issue describing what is going on. Otherwise, maybe just leave it out. =)

+ still has high priority in some cases.

The first case is when trait object type starts with for:

trait Tr<'a> {} fn f(arg: &for<'a> Tr<'a> + Send){} // Parsed as &(for<'a> Tr<'a> + Send)

I'm going to fix this in upcoming PR so I didn't create an issue.

The second case is impl Trait1 + Trait2 and it seems to be known.

Updated with better comments for FIXMEs.

nikomatsakis · 2017-01-26T20:23:57Z

@bors r+

bors · 2017-01-26T20:23:59Z

📌 Commit bd4d5ec has been approved by nikomatsakis

bors · 2017-01-26T21:47:32Z

⌛ Testing commit bd4d5ec with merge d68ec42...

bors · 2017-01-26T23:26:24Z

💔 Test failed - status-travis

alexcrichton · 2017-01-26T23:35:14Z

@bors: retry * android emulator wedged? Haven't seen that before...

…

On Thu, Jan 26, 2017 at 3:26 PM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/rust/builds/195685787> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#39158 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95Aq3XBj-ktJQQjbFe66c8DYzA6Y0ks5rWSuigaJpZM4LnJLp> .

bors · 2017-01-27T00:39:15Z

⌛ Testing commit bd4d5ec with merge 989cc5d...

alexcrichton · 2017-01-27T01:26:24Z

@bors: retry

fiddling with homu

bors · 2017-01-27T01:27:17Z

⌛ Testing commit bd4d5ec with merge 23a9469...

@matklad

Bounds parsing refactoring 2 See #37511 for previous discussion. cc @matklad Relaxed parsing rules: - zero bounds after `:` are allowed in all contexts. - zero predicates are allowed after `where`. - trailing separator `,` is allowed after predicates in `where` clauses not followed by `{`. Other parsing rules: - trailing separator `+` is still allowed in all bound lists. Code is also cleaned up and tests added. I haven't touched parsing of trait object types yet, I'll do it later.

bors · 2017-01-27T04:16:58Z

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

See rust-lang/rust#39158

bors · 2017-04-12T14:11:36Z

💥 Test timed out

rust-highfive assigned nikomatsakis Jan 18, 2017

petrochenkov mentioned this pull request Jan 18, 2017

Bounds parsing refactoring #37511

Closed

petrochenkov force-pushed the bounds branch from 064c7c6 to 7fa72ae Compare January 18, 2017 18:45

petrochenkov added 4 commits January 24, 2017 22:56

Refactor parsing of generic arguments/parameters and where clauses

b795abe

Add tests

a8f5047

Improve some expected/found error messages from parser

375cb2e

parser: Permit trailing +'s in bound lists

65aeafa

petrochenkov force-pushed the bounds branch from 7fa72ae to 65aeafa Compare January 24, 2017 20:40

nikomatsakis requested changes Jan 25, 2017

View reviewed changes

petrochenkov mentioned this pull request Jan 26, 2017

syntax: + has incorrect priority in trait object types starting with for #39317

Closed

Better comments for FIXMEs

bd4d5ec

nikomatsakis approved these changes Jan 26, 2017

View reviewed changes

bors merged commit bd4d5ec into rust-lang:master Jan 27, 2017

petrochenkov deleted the bounds branch March 16, 2017 19:43

matklad added a commit to intellij-rust/intellij-rust that referenced this pull request Mar 21, 2017

GRAM: support obscure syntax for trait bounds

d695b9e

See rust-lang/rust#39158

petrochenkov mentioned this pull request Apr 8, 2017

Trailing + in bounds are allowed #41158

Closed

petrochenkov mentioned this pull request Apr 15, 2017

Bound and supertrait bounds can be empty #30106

Closed

varkor mentioned this pull request Jan 30, 2018

Parse and accept type equality constraints in where clauses #20041

Open

Bounds parsing refactoring 2 #39158

Bounds parsing refactoring 2 #39158

Conversation

petrochenkov commented Jan 18, 2017 • edited Loading

rust-highfive commented Jan 18, 2017

durka commented Jan 18, 2017

petrochenkov commented Jan 18, 2017

petrochenkov commented Jan 18, 2017

durka commented Jan 18, 2017 via email

durka commented Jan 18, 2017 via email • edited Loading

petrochenkov commented Jan 18, 2017

nikomatsakis commented Jan 19, 2017

withoutboats commented Jan 19, 2017

joshtriplett commented Jan 19, 2017

aturon commented Jan 21, 2017

nikomatsakis commented Jan 23, 2017

nrc commented Jan 23, 2017

petrochenkov commented Jan 24, 2017

nikomatsakis commented Jan 25, 2017

bors commented Jan 25, 2017

nikomatsakis commented Jan 25, 2017

nikomatsakis commented Jan 25, 2017

nikomatsakis Jan 25, 2017

Choose a reason for hiding this comment

nikomatsakis Jan 25, 2017

Choose a reason for hiding this comment

petrochenkov Jan 25, 2017

Choose a reason for hiding this comment

nikomatsakis Jan 26, 2017

Choose a reason for hiding this comment

nikomatsakis Jan 25, 2017

Choose a reason for hiding this comment

petrochenkov Jan 25, 2017

Choose a reason for hiding this comment

petrochenkov Jan 26, 2017

Choose a reason for hiding this comment

nikomatsakis commented Jan 26, 2017

bors commented Jan 26, 2017

bors commented Jan 26, 2017

bors commented Jan 26, 2017

alexcrichton commented Jan 26, 2017 via email

bors commented Jan 27, 2017

alexcrichton commented Jan 27, 2017

bors commented Jan 27, 2017

bors commented Jan 27, 2017

bors commented Apr 12, 2017

petrochenkov commented Jan 18, 2017 •

edited

Loading

durka commented Jan 18, 2017 via email •

edited

Loading