"error: ')' expected but eof found." after Scala 2.12.13

[eed3si9n]

reproduction steps

Locally build sbt using Scala 2.12.13 and run:

lazy val root = (project in file("."))

lazy val sub1 = (project in file("sub1"))

lazy val sub2 = (project in file("sub2"))

lazy val sub3 = (project in file("sub3"))

Partest for this is https://gist.github.com/eed3si9n/5cb9572eac9f2be11ae3a42cd25174ec

problem

This causes

/private/tmp/hello/build.sbt:24: error: ')' expected but eof found.
lazy val sub2 = (project in file("sub2")
                                        ^

expectation

No error.

notes

The relevant place in sbt code is https://github.com/sbt/sbt/blob/4b71de0098b1f28b3d8ec6fa93e87cc5c9d50809/main-actions/src/main/scala/sbt/compiler/Eval.scala#L401-L408. It's possible I messed some other thing while upgrading to Scala 2.12.13, but figured I'd report this in case this pops up in a different way elsewhere.

[lrytz]

do you think this is a bug in scalac? a change/bug in the API that sbt uses to call into the compiler?

[eed3si9n]

Yea. If I understand correctly sbt uses the parser to figure out the expression boundary, and it seems something changed that it's now off by one?

[dwijnand]

Perhaps it's configurable warnings that's tripping up sbt's parsing.

[SethTisue]

Perhaps the next thing here would be to bisect it using Scala nightlies to figure out what scala/scala PR caused the change. That almost always either makes it obvious what happened, or at least makes it obvious where to look.

[melezov]

I started the bisect, and compiling new Scala versions was pretty simple - but when it came to compiling the SBT with said bisect (random SNAPSHOT version) I wasn't smart to figure out what to do.

Bumping the Scala version requires bumping the linked dependencies which require an exact Scala version (e.g. kind-projector and others) and this didn't resolve.
I can try faking it (replacing just the compiler/library, leaving other deps)...

[SethTisue]

You don't need to compile the Scala versions yourself, you can use our published nightlies.

But yeah, CrossVersion.full is a big problem. If you just have one or two such dependencies, it can work to publishLocal those dependencies for each Scala version you want to test, but beyond one or two, that becomes cumbersome. Or if the dependencies in question are explicitly listed in build.sbt, you can edit them not to use the full Scala version and just use the 2.12.13 versions. I don't know about the sbt repo in particular, but there have definitely been repos where I've gotten into situations where I wasn't smart enough to figure out what to do, either.

The alternative is to use the community build, which builds everything from source. That route works well but does require learning the community build ropes a bit. It's reasonably well documented. You can ./narrow sbt so just build sbt and its dependencies.

One thing I don't know is why the community build didn't already detect this problem for us. Is the version we have in https://github.com/scala/community-build/blob/2.12.x/proj/sbt.conf too old? Or does sbt's test suite not detect the problem?

[SethTisue]

the version we have in https://github.com/scala/community-build/blob/2.12.x/proj/sbt.conf too old?

Oh wow it really is very old (1.2.1). Tell you what: I will try to bring that up-to-date myself. I may hit some roadblock but there's a good chance I won't. I'll report back here.

[SethTisue]

@eed3si9n do you have a branch somewhere with your changes for 2.12.13? Or was it just a version number bump, no other changes?

[eed3si9n]

My PR that I originally ran into this is here - sbt/sbt#6261

Within sbt you can run:

> scripted project/console

which should trigger the error.

I'm guessing you should be able to get the error also by just changing project/Dependencies.scala and main/src/main/scala/sbt/PluginCross.scala from 2.12.12 to 2.12.13 (or your local SNAPSHOT) without all the @silent to @nowarn changes.

[SethTisue]

I have now merged scala/community-build#1345, but it didn't reproduce the bug

@eed3si9n is it clear to you why? do I just need to enable an additional subproject or something? (and is this worth pursuing further in the context of the community build? I'm happy to have done the 1345 work, but let's only keep going, in the community build context, if doing so seems worth it)

[eed3si9n]

I'll check to see if I can reproduce this with just the Scala version bump.

[eed3si9n]

I created a smaller PR sbt/sbt#6291, and I'm still seeing the issue.

[griggt]

Assuming I've done the bisection correctly, the culprit looks to be scala/scala#9248

[good] c30130b Merge pull request #9224 from dwijnand/partest-norm-file-dir-and-testIdent
[bad ] 2d4985d Merge pull request #9298 from dwijnand/2.12/repl-summarizes-warnings
[good] 95a569b Merge pull request #8794 from mkeskells/2.12.x_RedBlackMutable
[bad ] 7b4f3c8 Merge pull request #8948 from retronym/topic/group-by-2.12.x
[bad ] 7577c7a Merge pull request #9259 from lrytz/t12096
[bad ] 262f5f2 Merge pull request #9248 from dwijnand/2.12/conf-warnings

[eed3si9n]

To provide a bit more context on sbt side, we first try to parse the whole source, then try to split them into individual val expressions in https://github.com/sbt/sbt/blob/v1.4.7/main/src/main/scala/sbt/internal/parser/SbtParser.scala#L220 using -Yrangepos. There's another bug (#8859?) and the parser returns positions for:

lazy val root = project.in(file("."))
lazy val sub1 = project.in(file("sub1"))
lazy val sub2 = project.in(file("sub2"))
lazy val sub3 = project.in(file("sub3"))

even though the input was

lazy val root = (project in file("."))
lazy val sub1 = (project in file("sub1"))
lazy val sub2 = (project in file("sub2"))
lazy val sub3 = (project in file("sub3"))

so when sbt tries to reconstruct the expression it becomes off-by-one lazy val sub1 = (project in file("sub1") etc. To work around this, sbt then sends the expression back to the parser to see if it still parses, and when it doesn't keep adding more bits of text till the next ) or ]. This is how the sausage is made sometimes.

My guess of what's happening is that lazy val sub1 = (project in file("sub1") fails to parse correctly, but lazy val sub2 = (project in file("sub2") is no longer failing to parse during parseStatementAgain stage for some reason.

scala/scala#9248 being the cause makes sense since that's the only change made to Parser / Scanner on 2.12.13?

[som-snytt]

The scalac 2 parser doesn't preserve parens and text. If you want the line of source, you can pos.lineContent. I see it is deprecated in the public API FSR.

My other thought is that REPL parses progressively because it's trying to detect when it has enough to compile. sbt could also do that, with the advantage of always knowing the next line, so it can re-parse with the next line to detect whether it is agglutinative. (After if (b) expr, REPL can't tell if there is an else to come.)

I haven't looked at the sbt code, ergo sorry if this observation isn't relevant.

(That is, this issue makes me want to look at sbt code.)

[eed3si9n]

Since build.sbt is not line oriented, I don't think pos.lineContent would work.

My sort-of minimized partest is https://gist.github.com/eed3si9n/5cb9572eac9f2be11ae3a42cd25174ec, and after manually binary searching scala/scala#9248 it seems like the first bad commit is scala/scala@8ee5e80.

da684e0c76 good
12917a4492 bad
f3a4305012 good
8ee5e80689 bad
b5adfbc5ae good
f1e35cba8a good

Changing the parent class from Reporter to FilteringReporter or overriding doReport did not make any difference. The critical hit was:

-  def warning(pos: Position, msg: String): Unit = info0(pos, msg, WARNING, force = false)
-  def error(pos: Position, msg: String): Unit   = info0(pos, msg, ERROR, force = false)
+  def warning(pos: Position, msg: String): Unit = {
+    val f = filter(pos, msg, WARNING)
+    if (f <= 1) increment(WARNING)
+    if (f == 0) info0(pos, msg, WARNING, force = false)
+  }
+
+  def error(pos: Position, msg: String): Unit = {
+    val f = filter(pos, msg, ERROR)
+    if (f <= 1) increment(ERROR)
+    if (f == 0) info0(pos, msg, ERROR, force = false)
+  }

Previously, aspiring reporter aggregators like sbt could override just info0 to delegate to other reporters, but now error calls 3 methods, so it needs to override warning and error:

+    override def warning(pos: Position, msg: String): Unit =
+      getReporter(pos.source.file.name).warning(pos, msg)
+
+    override def error(pos: Position, msg: String): Unit =
+      getReporter(pos.source.file.name).error(pos, msg)

[eed3si9n]

Overriding error didn't work for 2.13 since def error on 2.13 is final. Here's a weird workaround:

    override def filter(pos: Position, msg: String, severity: Severity): Int = {
      val reporter = getReporter(pos.source.file.name)
      val result = reporter.filter(pos, msg, severity)
      if (result <= 1) reporter.increment(severity)
      if (result == 0) reporter.doReport(pos, msg, severity)
      result
    }

It effectively reimplements filteredInfo because filteredInfo is private. Having error final AND it delegating to a private method is not subclass friendly. Assuming either reporters are going to change much from here, I'm ok with closing this issue.