Generalise associative operator parsing by nagisa · Pull Request #29072 · rust-lang/rust

nagisa · 2015-10-15T13:02:45Z

This commit generalises parsing of associative operators from left-associative
only (with some ugly hacks to support right-associative assignment) to properly
left/right-associative operators.

Parsing is still is not general enough to handle non-associative,
non-highest-precedence prefix or non-highest-precedence
postfix operators (e.g. .. range syntax) and should be made to be.

Lastly, this commit adds support for parsing right-associative <- (left arrow)
operator with precedence higher than assignment as the operator for placement-in
feature.

This PR still needs various non-parser changes (e.g. src/grammar and tests) and I’m still working on these; the meat of the PR can already be reviewed, though, I think.

Please review carefully. I made sure that quirks I have discovered so far are preserved (see e.g. #29071) and am looking for more corner cases as I continue to work on tests et al, but there may be something I haven’t noticed or accounted for.

EDIT: I’m also not sure I managed to preserve all the semantics with the range operator inside non-trivial expressions since these are a mess at the moment. Crater runs would be nice.

rust-highfive · 2015-10-15T13:02:57Z

r? @brson

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

nagisa · 2015-10-15T15:16:46Z

Something I found is that x = 2..3 = y now parses as (x = 2..3) = y while it used to fail before. The expression still fails to compile later with E0070 anyway, though.

Something that doesn’t parse anymore and is a bug is x = ..y and x = ... I’ll tackle these soon.

brson · 2015-10-15T16:56:12Z

r? @pnkfelix

nagisa · 2015-10-15T21:08:41Z

Something that also needs to be fixed:

test.rs:45:5: 45:13 warning: unnecessary parentheses around assigned value, #[warn(unused_parens)] on by default
test.rs:45     (x <- y) <- z;
               ^~~~~~~~

alexcrichton · 2015-10-15T21:24:13Z

Running a crater build to evaluate these changes

pnkfelix · 2015-10-16T10:25:14Z

Something that doesn’t parse anymore and is a bug is x = ..y and x = ... I’ll tackle these soon.

@nagisa So this is now fixed with your latest commit ?

nagisa · 2015-10-16T10:51:33Z

I believe it is.
On Oct 16, 2015 1:25 PM, "Felix S Klock II" notifications@github.com
wrote:

Something that doesn’t parse anymore and is a bug is x = ..y and x = ...
I’ll tackle these soon.

@nagisa https://github.com/nagisa So this is now fixed with your latest
commit ?

—
Reply to this email directly or view it on GitHub
#29072 (comment).

alexcrichton · 2015-10-16T17:06:31Z

Crater reports two regressions one of which I believe is spurious and one of which I believe is legitimate.

nagisa · 2015-10-16T17:14:37Z

Thanks, @alexcrichton!

Short update on unused_parens lint: it has been fixed by @nrc’s changes to make EarlyPassLint to actually run pre-expansion.

nagisa · 2015-10-16T21:12:13Z

This PR is now in a pretty good spot, I think. The underlying issue behind the regression was just me forgetting to move restriction filtering over from the old code. This also helped to fix issues I had with semi-statement things I hacked around previously.

pnkfelix · 2015-10-16T23:12:04Z

(i won't get to this review until monday; hopefully that's okay.)

nagisa · 2015-10-27T18:01:51Z

NB: this changes the previous bahaviour of .. from

This has the same precedence as assignment expressions
(much lower than other prefix expressions) to be consistent
// with the postfix-form 'expr..'”

To having more precedence than <- and = but still less than any other binary operator.

EDIT: I believe it doesn’t change the semantics of prefix .., though, since the operator is not associative.

nagisa · 2015-10-27T18:03:18Z

NB: this still needs changes to src/grammar and I had planned to add more tests for quirky cases such as x..y=z not parsing (it did not parse before, and AFAIR it does not parse after changes).

Got distracted by university things, sadly.

This commit generalises parsing of associative operators from left-associative only (with some ugly hacks to support right-associative assignment) to properly left/right-associative operators. Parsing still is not general enough to handle non-associative, non-highest-precedence prefix or non-highest-precedence postfix operators (e.g. `..` range syntax), though. That should be fixed in the future. Lastly, this commit adds support for parsing right-associative `<-` (left arrow) operator with precedence higher than assignment as the operator for placement-in feature.

Fixes rust-lang#29071

Also add some (regression) tests for discovered parser oddities

nagisa · 2015-10-27T20:09:20Z

I pushed src/grammar change, but honestly have no idea what I’m doing since I’m just following grammar for similarly behaving equalities.

Going from master to after the change, make check-grammar reports 2 less mismatches (from 132 down to 130).

EDIT: ah also, the in PLACE {EXPR} warning is removed in nagisa@58c299f

pnkfelix · 2015-10-27T21:11:12Z

EDIT: ah also, the in PLACE {EXPR} warning is removed in nagisa/rust@58c299f

Oh, okay, sorry. Sometimes the commit-by-commit reviewing strategy works well, but I need to learn to look ahead in the queue for things like this.

nagisa · 2015-10-27T21:13:02Z

Oh, okay, sorry. Sometimes the commit-by-commit reviewing strategy works well, but I need to learn to look ahead in the queue for things like this.

I removed it after your comment, just made a note that I fixed it.

pnkfelix · 2015-10-27T21:16:21Z

lgtm

pnkfelix · 2015-10-27T21:16:25Z

@bors r+

bors · 2015-10-27T21:16:26Z

📌 Commit 99f9bb1 has been approved by pnkfelix

bors · 2015-10-27T22:56:40Z

⌛ Testing commit 99f9bb1 with merge e0e2627...

This commit generalises parsing of associative operators from left-associative only (with some ugly hacks to support right-associative assignment) to properly left/right-associative operators. Parsing is still is not general enough to handle non-associative, non-highest-precedence prefix or non-highest-precedence postfix operators (e.g. `..` range syntax) and should be made to be. Lastly, this commit adds support for parsing right-associative `<-` (left arrow) operator with precedence higher than assignment as the operator for placement-in feature. --- This PR still needs various non-parser changes (e.g. src/grammar and tests) and I’m still working on these; the meat of the PR can already be reviewed, though, I think. Please review carefully. I made sure that quirks I have discovered so far are preserved (see e.g. #29071) and am looking for more corner cases as I continue to work on tests et al, but there may be something I haven’t noticed or accounted for. EDIT: I’m also not sure I managed to preserve all the semantics with the range operator inside non-trivial expressions since these are a mess at the moment. Crater runs would be nice.

bors · 2015-10-28T00:46:30Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned brson Oct 15, 2015

nagisa force-pushed the place-arrow branch 2 times, most recently from 429ad12 to ae2bb9e Compare October 15, 2015 14:39

rust-highfive assigned pnkfelix and unassigned brson Oct 15, 2015

nagisa force-pushed the place-arrow branch from 9e76459 to 388fd34 Compare October 15, 2015 19:09

nagisa force-pushed the place-arrow branch from 388fd34 to e1c2ae6 Compare October 16, 2015 07:30

nagisa force-pushed the place-arrow branch from 9193d7d to 00ba7ce Compare October 16, 2015 17:14

nagisa force-pushed the place-arrow branch from 68f2365 to 5a3f42b Compare October 16, 2015 21:06

nagisa force-pushed the place-arrow branch from 5a3f42b to 7564c68 Compare October 17, 2015 10:07

nagisa reviewed Oct 27, 2015
View reviewed changes

nagisa added 6 commits October 27, 2015 21:55

Add a test for rust-lang#29071

884b589

Fixes rust-lang#29071

Fix prefix range expressions being not parsed

58c299f

Fix restrictions when parsing rhs of equalities

972c1c6

Update unused_parens lint for placement-in arrow

662082c

Add tests for newly introduced syntax

c1a238c

Also add some (regression) tests for discovered parser oddities

nagisa force-pushed the place-arrow branch from 7564c68 to c1a238c Compare October 27, 2015 19:56

nagisa force-pushed the place-arrow branch from be66959 to f986cbe Compare October 27, 2015 21:06

Adjust src/grammar for the introduced <- op

99f9bb1

nagisa force-pushed the place-arrow branch from f986cbe to 99f9bb1 Compare October 27, 2015 21:06

bors merged commit 99f9bb1 into rust-lang:master Oct 28, 2015

Conversation

nagisa commented Oct 15, 2015

Uh oh!

rust-highfive commented Oct 15, 2015

Uh oh!

nagisa commented Oct 15, 2015

Uh oh!

brson commented Oct 15, 2015

Uh oh!

nagisa commented Oct 15, 2015

Uh oh!

alexcrichton commented Oct 15, 2015

Uh oh!

pnkfelix commented Oct 16, 2015

Uh oh!

nagisa commented Oct 16, 2015

Uh oh!

alexcrichton commented Oct 16, 2015

Uh oh!

nagisa commented Oct 16, 2015

Uh oh!

nagisa commented Oct 16, 2015

Uh oh!

pnkfelix commented Oct 16, 2015

Uh oh!

nagisa Oct 27, 2015

Choose a reason for hiding this comment

Uh oh!

nagisa commented Oct 27, 2015

Uh oh!

nagisa commented Oct 27, 2015

Uh oh!

pnkfelix commented Oct 27, 2015

Uh oh!

nagisa commented Oct 27, 2015

Uh oh!

pnkfelix commented Oct 27, 2015

Uh oh!

pnkfelix commented Oct 27, 2015

Uh oh!

bors commented Oct 27, 2015

Uh oh!

bors commented Oct 27, 2015

Uh oh!

bors commented Oct 28, 2015

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants