Stop parsing incorrect trailing commas and improve error recovery in comma-separated lists. #14534
Conversation
adampauls
force-pushed
the
comma_separated_region_3
branch
from
ebe8baa to
767eb9e
Compare
| @@ -249,8 +249,13 @@ object Parsers { | |||
|
|
|||
| /** Skip on error to next safe point. | |||
| */ | |||
| protected def skip(stopAtComma: Boolean): Unit = | |||
| protected def skip(): Unit = | |||
There was a problem hiding this comment.
Smarter skip, though it is actually a little dumber because it now doesn't handle non-delimited comma-separated lists like for x,y,z <- ...
| /** part { `,` part } | ||
| * @param delimited If true, this comma-separated list must surrounded by brackets, parens, or braces. | ||
| */ | ||
| def commaSeparated[T](part: () => T, delimited: Boolean = true, readFirst: Boolean = true): List[T] = { |
There was a problem hiding this comment.
Main change all here.
adampauls
force-pushed
the
comma_separated_region_3
branch
3 times, most recently
from
08678ab to
58c8da0
Compare
Check for trailing commas in parser instead of scanner Instead of passing down a flag, add a bit to regions to say if we are in a comma-separated region. delimited instead of expectedToken
adampauls
force-pushed
the
comma_separated_region_3
branch
from
58c8da0 to
fc759ec
Compare
adampauls
changed the title
| @@ -655,13 +655,6 @@ object Scanners { | |||
| insert(OUTDENT, offset) | |||
| currentRegion = r.outer | |||
| case _ => | |||
| lookAhead() | |||
There was a problem hiding this comment.
No longer handled in scanner
| @@ -1386,14 +1427,7 @@ object Parsers { | |||
| else | |||
| Function(params, t) | |||
| } | |||
| def funTypeArgsRest(first: Tree, following: () => Tree) = { | |||
There was a problem hiding this comment.
This was really just a comma-separated list.
| @@ -558,19 +563,55 @@ object Parsers { | |||
| def inDefScopeBraces[T](body: => T, rewriteWithColon: Boolean = false): T = | |||
| inBracesOrIndented(body, rewriteWithColon) | |||
|
|
|||
| /** part { `separator` part } | |||
| */ | |||
| def tokenSeparated[T](separator: Int, part: () => T): List[T] = { | |||
There was a problem hiding this comment.
Inlined since it was only ever used with commas.
|
I have already fixed the broken community build projects -- they both had incorrect trailing commas. I'm not sure how to bump them though. |
|
@odersky any chance you're going to have a look at this soon? I was hoping to get it in before 3.1.3. Thanks! |
| @@ -0,0 +1,79 @@ | |||
|
|
|||
There was a problem hiding this comment.
Pulled over from scala/scala#8780.
|
Not an odersky, but I did fetch the branch and look forward to studying it. Today is my birthday, so it won't be today. |
|
I think After looking at the code in depth, I now need to step back from my earlier recommendation to use regions for trailing comma parsing. First the code is not really simpler than in #14517. Second, regions are currently a Scanner only thing. To mix context information from the parser into them would muddle the architecture. So in the end I think I prefer the approach of #14517. |
|
Closing in favor of #14517 |
Instead of handling trailing commas in the scanner, which leads to buggy behavior where trailing commas are accepted in places they shouldn't, handle trailing commas in the parser. Accomplished by adding a
isCommaSeparatedfield to theInParensregion. A redux of scala/scala#8780.A combination of #14509 and #14517, but with some feedback from @odersky. I put the two together because the need to handle trailing commas outside of the scanner is more apparent with both changes.