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.

Sign up for GitHub

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

Better error recovery in comma separated lists. #14509

Closed
wants to merge 1 commit into from
Closed

Better error recovery in comma separated lists. #14509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 37 additions & 28 deletions compiler/src/dotty/tools/dotc/parsing/Parsers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,19 +553,28 @@ 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] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I inlined this since it was only ever used for a comma separated list.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That rang a bell. At the beginning of pandemic, I moved Scala 2 trailing comma handling into parser for better handling, so that is why this method still pulls its weight there.

scala/scala#8780

I said on the PR that would forward-port it; apparently I looked and it seemed similar. But obviously I haven't done that yet. It must have slipped my mind during pandemic brain fog.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and wrote up something to get rid of the weird commas. I'll put it up after this PR is in.

/** part { `,` part }
* @param expectedEnd If set to something other than [[EMPTY]],
* assume this comma separated list must be followed by this token.
* If the parser consumes a `part` that is not followed by a comma or this expected
* token, issue a syntax error and try to recover at the next safe point.
*/
def commaSeparated[T](part: () => T, expectedEnd: Token = EMPTY): List[T] = {
val ts = new ListBuffer[T] += part()
while (in.token == separator) {
while (in.token == COMMA) {
in.nextToken()
ts += part()
}
if (expectedEnd != EMPTY && in.token != expectedEnd) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in case we have more that one unexpected token for example foo(1 2 3, 4, 5) this will not try to parse the rest of the param clause?

Maybe it makes sense to try to find the comma or end of line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntaxErrorOrIncomplete skips to the next safe pint as a side effect, so we will see 2 is not a ), issue an error, then jump to , and keep parsing. I'll add a third digit to one of the test cases to be sure.

// As a side effect, will skip to the nearest safe point, which might be a comma
syntaxErrorOrIncomplete(ExpectedTokenButFound(expectedEnd, in.token))
if (in.token == COMMA) {
ts ++= commaSeparated(part, expectedEnd)
}
}
ts.toList
}

def commaSeparated[T](part: () => T): List[T] = tokenSeparated(COMMA, part)

def inSepRegion[T](f: Region => Region)(op: => T): T =
val cur = in.currentRegion
in.currentRegion = f(cur)
Expand Down Expand Up @@ -1509,7 +1518,7 @@ object Parsers {
/** FunParamClause ::= ‘(’ TypedFunParam {‘,’ TypedFunParam } ‘)’
*/
def funParamClause(): List[ValDef] =
inParens(commaSeparated(() => typedFunParam(in.offset, ident())))
inParens(commaSeparated(() => typedFunParam(in.offset, ident()), RPAREN))

def funParamClauses(): List[List[ValDef]] =
if in.token == LPAREN then funParamClause() :: funParamClauses() else Nil
Expand Down Expand Up @@ -1622,7 +1631,7 @@ object Parsers {
else
def singletonArgs(t: Tree): Tree =
if in.token == LPAREN && in.featureEnabled(Feature.dependent)
then singletonArgs(AppliedTypeTree(t, inParens(commaSeparated(singleton))))
then singletonArgs(AppliedTypeTree(t, inParens(commaSeparated(singleton, RPAREN))))
else t
singletonArgs(simpleType1())

Expand All @@ -1638,7 +1647,7 @@ object Parsers {
def simpleType1() = simpleTypeRest {
if in.token == LPAREN then
atSpan(in.offset) {
makeTupleOrParens(inParens(argTypes(namedOK = false, wildOK = true)))
makeTupleOrParens(inParens(argTypes(namedOK = false, wildOK = true, RPAREN)))
}
else if in.token == LBRACE then
atSpan(in.offset) { RefinedTypeTree(EmptyTree, refinement(indentOK = false)) }
Expand Down Expand Up @@ -1722,7 +1731,7 @@ object Parsers {
* | NamedTypeArg {`,' NamedTypeArg}
* NamedTypeArg ::= id `=' Type
*/
def argTypes(namedOK: Boolean, wildOK: Boolean): List[Tree] = {
def argTypes(namedOK: Boolean, wildOK: Boolean, expectedEnd: Token): List[Tree] = {

def argType() = {
val t = typ()
Expand All @@ -1739,7 +1748,7 @@ object Parsers {
val rest =
if (in.token == COMMA) {
in.nextToken()
commaSeparated(arg)
commaSeparated(arg, expectedEnd)
}
else Nil
first :: rest
Expand All @@ -1752,7 +1761,7 @@ object Parsers {
case firstArg =>
otherArgs(firstArg, () => argType())
}
else commaSeparated(() => argType())
else commaSeparated(() => argType(), expectedEnd)
}

/** FunArgType ::= Type | `=>' Type
Expand Down Expand Up @@ -1781,7 +1790,7 @@ object Parsers {
/** TypeArgs ::= `[' Type {`,' Type} `]'
* NamedTypeArgs ::= `[' NamedTypeArg {`,' NamedTypeArg} `]'
*/
def typeArgs(namedOK: Boolean, wildOK: Boolean): List[Tree] = inBrackets(argTypes(namedOK, wildOK))
def typeArgs(namedOK: Boolean, wildOK: Boolean): List[Tree] = inBrackets(argTypes(namedOK, wildOK, RBRACKET))

/** Refinement ::= `{' RefineStatSeq `}'
*/
Expand Down Expand Up @@ -2145,7 +2154,7 @@ object Parsers {
var mods1 = mods
if isErased then mods1 = addModifier(mods1)
try
commaSeparated(() => binding(mods1))
commaSeparated(() => binding(mods1), RPAREN)
finally
accept(RPAREN)
else {
Expand Down Expand Up @@ -2376,7 +2385,7 @@ object Parsers {
/** ExprsInParens ::= ExprInParens {`,' ExprInParens}
*/
def exprsInParensOpt(): List[Tree] =
if (in.token == RPAREN) Nil else commaSeparated(exprInParens)
if (in.token == RPAREN) Nil else commaSeparated(exprInParens, RPAREN)

/** ParArgumentExprs ::= `(' [‘using’] [ExprsInParens] `)'
* | `(' [ExprsInParens `,'] PostfixExpr `*' ')'
Expand All @@ -2386,9 +2395,9 @@ object Parsers {
(Nil, false)
else if isIdent(nme.using) then
in.nextToken()
(commaSeparated(argumentExpr), true)
(commaSeparated(argumentExpr, RPAREN), true)
else
(commaSeparated(argumentExpr), false)
(commaSeparated(argumentExpr, RPAREN), false)
}

/** ArgumentExprs ::= ParArgumentExprs
Expand Down Expand Up @@ -2532,7 +2541,7 @@ object Parsers {
if (leading == LBRACE || in.token == CASE)
enumerators()
else {
val pats = patternsOpt()
val pats = patternsOpt(EMPTY)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why EMPTY in this case? Shouldn't it be something like OUTDENT ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was to handle for x, y in (1,2) do or for x, y in (1,2) yield, so I think the expected end would have to be do or yield? Passing EMPTY preserves the old behavior, so I figured it was safe. I could pass predicate down instead I suppose. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this if fine, the other possibility would be an Option but I don't see it used anywhere in the class, so probably there is a reason for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was maybe being overly cautious but I didn't want to have a perf hit from creating a lot of unnecessary Some wrappers.

val pat =
if (in.token == RPAREN || pats.length > 1) {
wrappedEnums = false
Expand Down Expand Up @@ -2724,7 +2733,7 @@ object Parsers {
case USCORE =>
wildcardIdent()
case LPAREN =>
atSpan(in.offset) { makeTupleOrParens(inParens(patternsOpt())) }
atSpan(in.offset) { makeTupleOrParens(inParens(patternsOpt(RPAREN))) }
case QUOTE =>
simpleExpr(Location.InPattern)
case XMLSTART =>
Expand Down Expand Up @@ -2759,17 +2768,17 @@ object Parsers {

/** Patterns ::= Pattern [`,' Pattern]
*/
def patterns(location: Location = Location.InPattern): List[Tree] =
commaSeparated(() => pattern(location))
def patterns(expectedEnd: Token = EMPTY, location: Location = Location.InPattern): List[Tree] =
commaSeparated(() => pattern(location), expectedEnd)

def patternsOpt(location: Location = Location.InPattern): List[Tree] =
if (in.token == RPAREN) Nil else patterns(location)
def patternsOpt(expectedEnd: Token, location: Location = Location.InPattern): List[Tree] =
if (in.token == RPAREN) Nil else patterns(expectedEnd, location)

/** ArgumentPatterns ::= ‘(’ [Patterns] ‘)’
* | ‘(’ [Patterns ‘,’] PatVar ‘*’ ‘)’
*/
def argumentPatterns(): List[Tree] =
inParens(patternsOpt(Location.InPatternArgs))
inParens(patternsOpt(RPAREN, Location.InPatternArgs))

/* -------- MODIFIERS and ANNOTATIONS ------------------------------------------- */

Expand Down Expand Up @@ -2950,7 +2959,7 @@ object Parsers {
TypeDef(name, lambdaAbstract(hkparams, bounds)).withMods(mods)
}
}
commaSeparated(() => typeParam())
commaSeparated(() => typeParam(), RBRACKET)
}

def typeParamClauseOpt(ownerKind: ParamOwner.Value): List[TypeDef] =
Expand All @@ -2959,7 +2968,7 @@ object Parsers {
/** ContextTypes ::= FunArgType {‘,’ FunArgType}
*/
def contextTypes(ofClass: Boolean, nparams: Int, impliedMods: Modifiers): List[ValDef] =
val tps = commaSeparated(funArgType)
val tps = commaSeparated(funArgType, RPAREN)
var counter = nparams
def nextIdx = { counter += 1; counter }
val paramFlags = if ofClass then Private | Local | ParamAccessor else Param
Expand Down Expand Up @@ -3063,7 +3072,7 @@ object Parsers {
!impliedMods.is(Given)
|| startParamTokens.contains(in.token)
|| isIdent && (in.name == nme.inline || in.lookahead.isColon())
if isParams then commaSeparated(() => param())
if isParams then commaSeparated(() => param(), RPAREN)
else contextTypes(ofClass, nparams, impliedMods)
checkVarArgsRules(clause)
clause
Expand Down Expand Up @@ -3755,7 +3764,7 @@ object Parsers {
val derived =
if (isIdent(nme.derives)) {
in.nextToken()
tokenSeparated(COMMA, () => convertToTypeId(qualId()))
commaSeparated(() => convertToTypeId(qualId()))
}
else Nil
possibleTemplateStart()
Expand Down
36 changes: 36 additions & 0 deletions tests/neg/comma-separated-errors.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
-- [E040] Syntax Error: tests/neg/comma-separated-errors.scala:3:21 ----------------------------------------------------
3 | def foo(x: Int = 5 6, y Int = 7, z: Int 5, x = 5): Unit = () // error // error // error // error
| ^
| ')' expected, but integer literal found
-- [E040] Syntax Error: tests/neg/comma-separated-errors.scala:3:26 ----------------------------------------------------
3 | def foo(x: Int = 5 6, y Int = 7, z: Int 5, x = 5): Unit = () // error // error // error // error
| ^^^
| ':' expected, but identifier found
-- [E040] Syntax Error: tests/neg/comma-separated-errors.scala:3:42 ----------------------------------------------------
3 | def foo(x: Int = 5 6, y Int = 7, z: Int 5, x = 5): Unit = () // error // error // error // error
| ^
| ')' expected, but integer literal found
-- [E040] Syntax Error: tests/neg/comma-separated-errors.scala:3:47 ----------------------------------------------------
3 | def foo(x: Int = 5 6, y Int = 7, z: Int 5, x = 5): Unit = () // error // error // error // error
| ^
| ':' expected, but '=' found
-- [E040] Syntax Error: tests/neg/comma-separated-errors.scala:11:16 ---------------------------------------------------
11 | case Plus(4 1) => // error
| ^
| ')' expected, but integer literal found
-- [E040] Syntax Error: tests/neg/comma-separated-errors.scala:12:16 ---------------------------------------------------
12 | case Plus(4 5 6 7, 1, 2 3) => // error // error
| ^
| ')' expected, but integer literal found
-- [E040] Syntax Error: tests/neg/comma-separated-errors.scala:12:28 ---------------------------------------------------
12 | case Plus(4 5 6 7, 1, 2 3) => // error // error
| ^
| ')' expected, but integer literal found
-- [E040] Syntax Error: tests/neg/comma-separated-errors.scala:14:12 ---------------------------------------------------
14 | val x: A[T=Int, T=Int] = ??? // error // error
| ^
| ']' expected, but '=' found
-- [E040] Syntax Error: tests/neg/comma-separated-errors.scala:14:19 ---------------------------------------------------
14 | val x: A[T=Int, T=Int] = ??? // error // error
| ^
| ']' expected, but '=' found
15 changes: 15 additions & 0 deletions tests/neg/comma-separated-errors.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
class A[T]
object o {
def foo(x: Int = 5 6, y Int = 7, z: Int 5, x = 5): Unit = () // error // error // error // error

case class Plus(a: Int, b: Int)

object Plus {
def unapply(r: Int): Plus = Plus(r - 1, 1)
}
5 match {
case Plus(4 1) => // error
case Plus(4 5 6 7, 1, 2 3) => // error // error
}
val x: A[T=Int, T=Int] = ??? // error // error
}
2 changes: 1 addition & 1 deletion tests/neg/i1679.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
class A[T]
object o {
// Testing compiler crash, this test should be modified when named type argument are completely implemented
val x: A[T=Int, T=Int] = ??? // error: ']' expected, but '=' found // error
val x: A[T=Int, T=Int] = ??? // error: ']' expected, but '=' found // error: ']' expected, but '=' found
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, this error complained about val x not having a body, which doesn't make any sense.

}