REPL - fix collective extensions

[dos65]

Fixes #9374

[griggt]

Suggested change

	m.isDefined && statSepOrEnd(meths, what = "extension method")
	if !m.isDefined then syntaxErrorOrIncomplete("`def` expected")
	statSepOrEnd(meths, what = "extension method")

I think this may fix the failure in tests/neg/extension-methods.scala

Or maybe a bit cleaner as

        extMethodOpt(numLeadParams) match
          case Some(m) => meths += m
          case None    => syntaxErrorOrIncomplete("`def` expected")

[dos65]

Thanks for help! I played with your suggestion and found even more compact way to fix this problem

[som-snytt]

I was going to help out, but it's too bad dotty testing is more complicated than scala 2 partest/junit.

Every test is a research project.

Locally,

[info] Test dotty.tools.dotc.CompilationTests.negAll started
Error reported in tests/neg/extension-methods.scala, but no annotation found

Errors found on incorrect row numbers when compiling out/compileNeg/neg/extension-methods
-> following the errors:
at 15: Not found: type T
 at 10: value l1 is not a member of Int
 at 9: value l2 is not a member of String
 at 17: eof expected, but '}' found
 at 13: `def` expected
(PC2.apply(10): P) match {
  case c @ (c: PC1) =>
    ()
  case c @ (`c₂`: PC2) =>
    ()
}
[=======================================] completed (1285/1285, 1 failed, 39s)
[error] Test dotty.tools.dotc.CompilationTests.negAll failed: java.lang.AssertionError: Neg test should have failed, but did not, took 39.748 sec
[error]     at dotty.tools.vulpix.ParallelTesting$CompilationTest.checkExpectedErrors(ParallelTesting.scala:919)
[error]     at dotty.tools.dotc.CompilationTests.negAll(CompilationTests.scala:181)
[error]     at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[error]     at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
[error]     at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[error]     at java.lang.reflect.Method.invoke(Method.java:568)
[error]     ...

[griggt]

Some thoughts on the current implementation:

scala> extension (x: Int) { ; def foo = 42 }
def foo(x: Int): Int

The above is now accepted, but I believe it should be disallowed by the grammar (before this PR it gives an error on the ; character)

Also, a little more subjective:

file.scala

extension (x: Int)
  private val foo = 10
  def bar = foo

With this PR:

$ ./bin/scalac file.scala 
-- Error: file.scala:2:10 ----------------------------------------------
2 |  private val foo = 10
  |          ^^^
  |          end of extension method expected but 'val' found
-- [E040] Syntax Error: file.scala:4:0 ---------------------------------
4 |
  |^
  |eof expected, but unindent found
-- [E006] Not Found Error: file.scala:3:12 -----------------------------
3 |  def bar = foo
  |            ^^^
  |            Not found: foo

Previously:

$ scalac  file.scala
-- [E040] Syntax Error: file.scala:2:10 ----------------------------------------
2 |  private val foo = 10
  |          ^^^
  |          'def' expected, but 'val' found
-- [E006] Not Found Error: file.scala:3:12 -------------------------------------
3 |  def bar = foo
  |            ^^^
  |            Not found: foo

In this case, the existing error messages seem clearer to me than those with this PR.

[som-snytt]

I was suspicious of Option[Tree], which isn't idiomatic; usually it's an EmptyTree. I added an EmptyDefDef, but that might be overkill. Here's a diff:

https://github.com/lampepfl/dotty/compare/master...som-snytt:dos65/repl-latest?expand=1

I wasted some time comparing trees because I didn't realize extensions had changed recently. So I won't push it further. The gist is that extMethods is sensitive to EOF. Maybe it can be simplified, but I'm all tweaked out at the moment.

This version preserves the edge behavior mentioned above.

[dos65]

@griggt oops! That for pointing at this, will look how to fix that.

[som-snytt]

That is, my branch preserves the messaging mentioned by @griggt .

[dos65]

@som-snytt Oh, I've just took a look at your changes - your version is better especially because of introducing EmptyDefDef.
Are you going to submit PR? Or should I add your commit to this PR?

[som-snytt]

@dos65 I suspect the new empty tree does not pull its weight; it's basically just to signal from extMethod instead of option.

The extMethods should show something useful, but I was coding at midnight, and it night look different with fresh eyes.

Feel free to steal whatever is useful, or if you are bored with it, I can make another pass and PR.

[som-snytt]

Actually I have a few minutes where I can either PR or go work on remodelling the bedroom, so I'll do a quick PR.

[som-snytt]

The linked PR tweaks this commit, so this can be closed. I can't push to this branch, but I kept the commit which records dos65's contribution IIUC.

Also apologies for my frustrated comment from a week ago. I don't remember what snag I hit in trying out the failed test? but although I know testing frameworks is thankless work, barriers to trivial contributions are also not good. "But if contributing was easy, everyone would do it!"

[dos65]

@som-snytt Thanks!

What about the moment with test output, I agree that it might be done better but don't think that is critical.