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

adampauls · 2022-02-13T00:29:51Z

Attempts to improve parsing of incomplete Apply nodes. Makes two improvements:

foo(x,<EOF> will now parse as foo(x, <errorTermTree>) instead of foo(x)
foo(x = , <EOF|)> will now parse as foo(x = <errorTermTree>, <errorTermTree>) instead of foo(x = <errorTermTree>).

As a side effect, makes the parser more robust to errors in comma-separated lists by adding , to the list of "safe points" that the parser will skip to in the face of an error. That is, foo(x, , y) will parse as foo(x, <errorTermTree>, y) instead of foo(x, <errorTermTree>). I am unsure if there are more ramifications than I'm seeing for that last change. I'm also not sure how best to test the parser when there are errors.

som-snytt · 2022-02-13T00:56:59Z

compiler/test/dotty/tools/vulpix/ParallelTesting.scala

-            val key = s"${relativize(srcpos.source.file.toString)}:${srcpos.line + 1}"
+            // If an annotated // error is placed on the last line of the file, EOF errors can show up past the last.
+            // Without this adjustment, it's not possible to annotate // error on errors whose position is EOF
+            // in some cases.


I think I add anypos-error for positions that can't be expressed in comment.

dff31fb

(Too tired to think about off-by-one just now.)

You did, and I'll happily use that if it's better. I think it's a little unfortunate not to be able to add a // error at the end of a file to indicate that the error should be at the end of the file though? I agree this isn't great, but I'd rather not use anypos all else equal. Happy to if this is what gets the PR through!

Yes, absolutely. There is the endless thing on github whether the file ends in a newline, and therefore what is the true line count. This came up in a scala 2 issue where the line number mattered. As a vi user, my core belief is always terminate and there is not an empty line after that, it's just EOF. But I recognize that reasonable people may use tabs instead of spaces. I meant to say stubborn. Now I'm curious to follow your issue.

(I reverted this change because I realized how to get the existing reporting mechanism to be happy).

adampauls · 2022-02-13T01:32:47Z

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

@@ -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


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.

adampauls · 2022-02-13T01:33:24Z

compiler/src/dotty/tools/dotc/parsing/Tokens.scala

@@ -285,7 +285,7 @@ object Tokens extends TokensCommon {

  final val endMarkerTokens = identifierTokens | BitSet(IF, WHILE, FOR, MATCH, TRY, NEW, THROW, GIVEN, VAL, THIS)

-  final val skipStopTokens = BitSet(SEMI, NEWLINE, NEWLINES, RBRACE, RPAREN, RBRACKET, OUTDENT)
+  final val skipStopTokens = BitSet(SEMI, NEWLINE, NEWLINES, RBRACE, RPAREN, RBRACKET, OUTDENT, COMMA)


Possibly controversial. This makes it so that foo(bar, <some error>, baz) still properly parses baz.

I'm not sure what the downside of this is though. Only two tests were affected, and IMHO the extra errors are an improvement.

compiler/test/dotty/tools/vulpix/ParallelTesting.scala

som-snytt · 2022-02-13T02:48:01Z

There are some old tests in the repo due to the similar thing in scala 2

➜  dotty git:(test/current) find . -name "*5702*"
./tests/neg/t5702-neg-bad-and-wild.scala
./tests/pos/t5702-pos-infix-star.scala
./tests/untried/neg/t5702-neg-bad-xbrace.scala
./tests/untried/neg/t5702-neg-ugly-xbrace.scala
./tests/untried/neg/t5702-neg-ugly-xbrace.check
./tests/untried/neg/t5702-neg-bad-xbrace.check
./tests/untried/neg/t5702-neg-bad-and-wild.check
./tests/untried/neg/t5702-neg-bad-brace.scala
./tests/untried/neg/t5702-neg-bad-brace.check

adampauls · 2022-02-13T04:39:03Z

I added the tests from t5702 that didn't involve XML. I think the errors are acceptable?

adampauls · 2022-02-13T04:39:44Z

tests/untried/neg/t5702-neg-bad-and-wild.check

@@ -1,28 +0,0 @@
-t5702-neg-bad-and-wild.scala:10: error: bad simple pattern: bad use of _* (a sequence pattern must be the last pattern)


Not deleted but moved. The diff is too big for GH to realize it though.

adampauls · 2022-02-13T04:41:39Z

tests/untried/neg/t5702-neg-bad-brace.scala

@@ -1,17 +0,0 @@
-
-object Test {


Not deleted, just moved. Diff too big.

adampauls · 2022-02-13T04:50:09Z

tests/neg/t1625.check

+  |                    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
+  |                                ^


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

adampauls · 2022-02-14T09:27:41Z

Cc @tgodzik, might be related to scalameta/metals#3625.

tgodzik · 2022-02-14T09:48:32Z

Cc @tgodzik, might be related to scalameta/metals#3625.

It might be! I will take a look at the PR!

tgodzik

It would be super useful to have this 🎉

We could workaround it with an artificial cursor, but it would probably be more efficient to handle it in the compiler.

I left a couple of comments, but we would need to get someone from the core compiler team to take a look as I am not confident enough about the impact of the changes.

Maybe @odersky ?

tests/neg/arg-eof.scala

tgodzik · 2022-02-16T11:17:22Z

tests/neg/i1679.scala

@@ -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


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

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?

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

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?

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.

Should we remove the added error then?

tests/neg/t5702-neg-bad-brace.check

adampauls · 2022-02-16T20:08:41Z

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

@@ -278,7 +278,7 @@ object Parsers {
      if (in.token == EOF) incompleteInputError(msg)
      else
        syntaxError(msg, offset)
-        skip()
+        skip(stopAtComma = true)


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

adampauls · 2022-02-16T20:09:06Z

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

@@ -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)


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

odersky

Looks like a net improvement to me.

adampauls force-pushed the missing-argument-parsing branch from 6682ee2 to 64a3c97 Compare February 13, 2022 00:53

som-snytt reviewed Feb 13, 2022

View reviewed changes

adampauls force-pushed the missing-argument-parsing branch from 64a3c97 to 2e004b8 Compare February 13, 2022 01:29

adampauls commented Feb 13, 2022

View reviewed changes

som-snytt reviewed Feb 13, 2022

View reviewed changes

compiler/test/dotty/tools/vulpix/ParallelTesting.scala Outdated Show resolved Hide resolved

adampauls changed the title ~~[WIP] Parse empty arguments in (invalid) Apply more often~~ Parse empty arguments in (invalid) Apply more often Feb 13, 2022

adampauls commented Feb 13, 2022

View reviewed changes

adampauls force-pushed the missing-argument-parsing branch from 95eea24 to b00da66 Compare February 13, 2022 04:41

adampauls commented Feb 13, 2022

View reviewed changes

tests/untried/neg/t5702-neg-bad-brace.scala

@@ -1,17 +0,0 @@

object Test {

Copy link

Contributor Author

adampauls Feb 13, 2022

Choose a reason for hiding this comment

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

Not deleted, just moved. Diff too big.

adampauls commented Feb 13, 2022

View reviewed changes

tgodzik mentioned this pull request Feb 15, 2022

Scala3 emptyline completions scalameta/metals#3629

Merged

adampauls force-pushed the missing-argument-parsing branch from ff1a73b to fb73f4b Compare February 15, 2022 19:06

tgodzik reviewed Feb 16, 2022

View reviewed changes

adampauls commented Feb 16, 2022

View reviewed changes

Create errorTermTrees for more missing arguments to Apply nodes.

e2537c5

adampauls force-pushed the missing-argument-parsing branch from 2c63ee3 to e2537c5 Compare February 16, 2022 20:18

tgodzik requested a review from odersky February 17, 2022 15:27

odersky approved these changes Feb 17, 2022

View reviewed changes

odersky merged commit 0e479ee into scala:main Feb 17, 2022

adampauls mentioned this pull request Feb 17, 2022

Better error recovery in comma separated lists. #14509

Closed

adampauls mentioned this pull request May 27, 2022

Interactive.pathTo contains Literal(Constant(null)) as a head of path for incomplete program #15294

Closed

Kordyjan added this to the 3.1.3 milestone Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adampauls commented Feb 13, 2022 •

edited

som-snytt Feb 13, 2022

som-snytt Feb 13, 2022

adampauls Feb 13, 2022

som-snytt Feb 13, 2022

adampauls Feb 15, 2022

adampauls Feb 13, 2022

adampauls Feb 13, 2022 •

edited

adampauls Feb 13, 2022 •

edited

som-snytt commented Feb 13, 2022

adampauls commented Feb 13, 2022

adampauls Feb 13, 2022

adampauls Feb 13, 2022

adampauls Feb 13, 2022

adampauls commented Feb 14, 2022

tgodzik commented Feb 14, 2022

tgodzik left a comment

tgodzik Feb 16, 2022

adampauls Feb 16, 2022 •

edited

tgodzik Feb 16, 2022

adampauls Feb 16, 2022

adampauls Feb 16, 2022

tgodzik Feb 17, 2022

adampauls Feb 16, 2022

adampauls Feb 16, 2022

odersky left a comment

		@@ -1,28 +0,0 @@
		t5702-neg-bad-and-wild.scala:10: error: bad simple pattern: bad use of _* (a sequence pattern must be the last pattern)

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

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

Conversation

adampauls commented Feb 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adampauls Feb 13, 2022 • edited

Choose a reason for hiding this comment

adampauls Feb 13, 2022 • edited

Choose a reason for hiding this comment

som-snytt commented Feb 13, 2022

adampauls commented Feb 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adampauls commented Feb 14, 2022

tgodzik commented Feb 14, 2022

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adampauls Feb 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky left a comment

Choose a reason for hiding this comment

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

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

adampauls commented Feb 13, 2022 •

edited

adampauls Feb 13, 2022 •

edited

adampauls Feb 13, 2022 •

edited

adampauls Feb 16, 2022 •

edited