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

Parse empty arguments in (invalid) Apply more often #14463

Merged
merged 1 commit into from
Feb 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 7 additions & 6 deletions compiler/src/dotty/tools/dotc/parsing/Parsers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -249,11 +249,11 @@ object Parsers {

/** Skip on error to next safe point.
*/
protected def skip(): Unit =
protected def skip(stopAtComma: Boolean): Unit =
val lastRegion = in.currentRegion
def atStop =
in.token == EOF
|| skipStopTokens.contains(in.token) && (in.currentRegion eq lastRegion)
|| ((stopAtComma && in.token == COMMA) || skipStopTokens.contains(in.token)) && (in.currentRegion eq lastRegion)
while !atStop do
in.nextToken()
lastErrorOffset = in.offset
Expand All @@ -278,7 +278,7 @@ object Parsers {
if (in.token == EOF) incompleteInputError(msg)
else
syntaxError(msg, offset)
skip()
skip(stopAtComma = true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line makes it so we skip to the next argument if we encounter an error in a comma separated list.


/** Consume one token of the specified type, or
* signal an error if it is not there.
Expand Down Expand Up @@ -346,7 +346,7 @@ object Parsers {
false // it's a statement that might be legal in an outer context
else
in.nextToken() // needed to ensure progress; otherwise we might cycle forever
skip()
skip(stopAtComma=false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, we are explicitly looking to skip to a place that should end a statement, which a , never will.

true

in.observeOutdented()
Expand Down Expand Up @@ -881,7 +881,8 @@ object Parsers {
val next = in.lookahead.token
next == LBRACKET || next == LPAREN

/** Is current ident a `*`, and is it followed by a `)` or `, )`? */
/** Is current ident a `*`, and is it followed by a `)`, `, )`, `,EOF`? The latter two are not
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 think this is a correct use of this function, or at least does not make it worse than it was before. Without this, the special handling for EOF in the comma lookahead is unnecessary, and also leads to a better error when a comma is followed by an EOF.

syntactically valid, but we need to include them here for error recovery. */
def followingIsVararg(): Boolean =
in.isIdent(nme.raw.STAR) && {
val lookahead = in.LookaheadScanner()
Expand All @@ -890,7 +891,7 @@ object Parsers {
|| lookahead.token == COMMA
&& {
lookahead.nextToken()
lookahead.token == RPAREN
lookahead.token == RPAREN || lookahead.token == EOF
}
}

Expand Down
2 changes: 0 additions & 2 deletions compiler/src/dotty/tools/dotc/parsing/Scanners.scala
Original file line number Diff line number Diff line change
Expand Up @@ -659,8 +659,6 @@ object Scanners {
&& (token == RPAREN || token == RBRACKET || token == RBRACE || token == OUTDENT)
then
() /* skip the trailing comma */
else if token == EOF then // e.g. when the REPL is parsing "val List(x, y, _*,"
() /* skip the trailing comma */
else
reset()
case END =>
Expand Down
1 change: 1 addition & 0 deletions compiler/src/dotty/tools/repl/Main.scala
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package dotty.tools.repl

/** Main entry point to the REPL */
// To test, run bin/scala
object Main {
def main(args: Array[String]): Unit =
new ReplDriver(args).tryRunning
Expand Down
3 changes: 3 additions & 0 deletions tests/neg/arg-eof.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
object Test:
case class Widget(name: String, other: Int = 5)
Widget(name = "foo", // error // error
tgodzik marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion tests/neg/i1679.scala
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
val x: A[T=Int, T=Int] = ??? // error: ']' expected, but '=' found // error
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here, there seems to be no message accompanying the message

Copy link
Contributor Author

@adampauls adampauls Feb 16, 2022

Choose a reason for hiding this comment

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

The second message here is not ideal:

Declaration of value x not allowed here: only classes can have declared but undefined members

This is arguably a regression. I'm not quite sure what's happening, but I noticed there were a bunch of untagged, less-than-ideal error messages elsewhere so I left it. Think it's worth fixing here? Leaving a TODO?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, if there are other wierd errors then I guess we can create a Todo with an issue to improve this.

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 looked into it a bit more. When parsing a comma-separated list, if thing you're looking for (in this case, a type tree) ends before you hit a comma, the parser thinks you're done the list, so the parser reads T then thinks it's done the list because it doesn't see a ,. It's then expecting to see a ], and issues the first error. Then it skips to the next safe point, which in the past was the next ], but now is the ,. So I guess the parser sees this as val x: A[T], T = Int] = ???, so that explains why the parser produces a val x: A[T] and hence the new error. I don't know why there aren't errors resulting from the , T = Int] = ??? suffix, but presumably there are and they are just suppressed?

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 pushed a change that gets rid of this (bad) error by changing skip to look for a comma only if we're not explicitly trying to find a statement separator.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove the added error then?

}
8 changes: 8 additions & 0 deletions tests/neg/t1625.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
-- [E040] Syntax Error: tests/neg/t1625.scala:2:20 ---------------------------------------------------------------------
2 | def foo(x: String*, y: String*, c: String*): Int // error: an identifier expected, but ',' found // error: an identifier expected, but ',' found
| ^
| an identifier expected, but ',' found
-- [E040] Syntax Error: tests/neg/t1625.scala:2:32 ---------------------------------------------------------------------
2 | def foo(x: String*, y: String*, c: String*): Int // error: an identifier expected, but ',' found // error: an identifier 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.

This is an example of better error messages IMHO: you get an error on both invalid occurrences.

| an identifier expected, but ',' found
4 changes: 2 additions & 2 deletions tests/neg/t1625.scala
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
trait T3 {
def foo(x: String*, y: String*, c: String*): Int // error: an identifier expected, but ',' found
}
def foo(x: String*, y: String*, c: String*): Int // error: an identifier expected, but ',' found // error: an identifier expected, but ',' found
}
44 changes: 44 additions & 0 deletions tests/neg/t5702-neg-bad-and-wild.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
-- [E032] Syntax Error: tests/neg/t5702-neg-bad-and-wild.scala:10:22 ---------------------------------------------------
10 | case List(1, _*,) => // error: pattern expected // error
| ^
| pattern expected
|
| longer explanation available when compiling with `-explain`
-- [E032] Syntax Error: tests/neg/t5702-neg-bad-and-wild.scala:12:23 ---------------------------------------------------
12 | case List(1, _*3,) => // error: pattern expected // error // error
| ^
| pattern expected
|
| longer explanation available when compiling with `-explain`
-- [E032] Syntax Error: tests/neg/t5702-neg-bad-and-wild.scala:15:18 ---------------------------------------------------
15 | case List(x*, 1) => // error: pattern expected
| ^
| pattern expected
|
| longer explanation available when compiling with `-explain`
-- [E031] Syntax Error: tests/neg/t5702-neg-bad-and-wild.scala:17:18 ---------------------------------------------------
17 | case (1, x: _*) => // error: bad use of _* (sequence pattern not allowed)
| ^
| * can be used only for last argument
|
| longer explanation available when compiling with `-explain`
-- [E032] Syntax Error: tests/neg/t5702-neg-bad-and-wild.scala:23:17 ---------------------------------------------------
23 | val K(ns @ _*, x) = k // error: pattern expected
| ^
| pattern expected
|
| longer explanation available when compiling with `-explain`
-- Error: tests/neg/t5702-neg-bad-and-wild.scala:10:21 -----------------------------------------------------------------
10 | case List(1, _*,) => // error: pattern expected // error
| ^
| Values of types Null and Int cannot be compared with == or !=
-- [E006] Not Found Error: tests/neg/t5702-neg-bad-and-wild.scala:12:20 ------------------------------------------------
12 | case List(1, _*3,) => // error: pattern expected // error // error
| ^
| Not found: *
|
| longer explanation available when compiling with `-explain`
-- Error: tests/neg/t5702-neg-bad-and-wild.scala:12:22 -----------------------------------------------------------------
12 | case List(1, _*3,) => // error: pattern expected // error // error
| ^
| Values of types Null and Int cannot be compared with == or !=
10 changes: 10 additions & 0 deletions tests/neg/t5702-neg-bad-brace.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
-- [E032] Syntax Error: tests/neg/t5702-neg-bad-brace.scala:8:21 -------------------------------------------------------
8 | case List(1, _*} => // error: pattern expected
| ^
| pattern expected
|
| longer explanation available when compiling with `-explain`
-- [E040] Syntax Error: tests/neg/t5702-neg-bad-brace.scala:11:0 -------------------------------------------------------
11 |} // error: eof expected, but '}' found
|^
|eof expected, but '}' found
tgodzik marked this conversation as resolved.
Show resolved Hide resolved
11 changes: 11 additions & 0 deletions tests/neg/t5702-neg-bad-brace.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@

object Test {

def main(args: Array[String]) = {
val is = List(1,2,3)

is match {
case List(1, _*} => // error: pattern expected
}
}
} // error: eof expected, but '}' found
6 changes: 6 additions & 0 deletions tests/neg/trailing-comma-pattern.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
-- [E032] Syntax Error: tests/neg/trailing-comma-pattern.scala:3:8 -----------------------------------------------------
3 |// error
| ^
| pattern expected
|
| longer explanation available when compiling with `-explain`
3 changes: 3 additions & 0 deletions tests/neg/trailing-comma-pattern.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
object Test:
val List(x, y, _*,
// error
10 changes: 10 additions & 0 deletions tests/neg/trailing-comma-pattern2.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
-- [E032] Syntax Error: tests/neg/trailing-comma-pattern2.scala:2:21 ---------------------------------------------------
2 | val List(x, y, _*, ) // error
| ^
| pattern expected
|
| longer explanation available when compiling with `-explain`
-- [E040] Syntax Error: tests/neg/trailing-comma-pattern2.scala:3:8 ----------------------------------------------------
3 |// error
| ^
| '=' expected, but unindent found
3 changes: 3 additions & 0 deletions tests/neg/trailing-comma-pattern2.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
object Test:
val List(x, y, _*, ) // error
// error
3 changes: 3 additions & 0 deletions tests/pos/trailing-comma-pattern.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
object Test:
val List(x, y, _*,
) = List(1, 2, 3)
28 changes: 0 additions & 28 deletions tests/untried/neg/t5702-neg-bad-and-wild.check

This file was deleted.

10 changes: 0 additions & 10 deletions tests/untried/neg/t5702-neg-bad-brace.check

This file was deleted.

17 changes: 0 additions & 17 deletions tests/untried/neg/t5702-neg-bad-brace.scala

This file was deleted.