sbt / zinc

Scala incremental compiler library, used by sbt and other build tools
Apache License 2.0
334 stars 121 forks source link

Parse column/position information from javac error messages #1373

Closed vasilmkd closed 3 months ago

vasilmkd commented 4 months ago

Resolves #1372 .

I've made a binary incompatible change to sbt.internal.inc.javac.JavaPosition. What is the binary compatibility guarantee of this package, given that it has internal in the name?

I can re-add the old constructor to preserve binary compatibility. However, even if I add the constructor once more, one of its parameters is no longer uses in the same way as it was before.

Any advice or preference on this matter is appreciated.

vasilmkd commented 4 months ago

According to https://github.com/sbt/zinc/blob/4eacff2a9bf5c8750bfc5096955065ce67f4e68a/build.sbt#L435

binary incompatible changes are ignored in sbt.internal.*, so the change I made should be fine. MiMa certainly doesn't say anything in the CI.

vasilmkd commented 4 months ago

I'm working on expanding the tests in javaErrorParserSpec.

vasilmkd commented 4 months ago

Can someone please definitively confirm that xsbti.Position#pointer() is 0-based?

eed3si9n commented 4 months ago

I've made a binary incompatible change to sbt.internal.inc.javac.JavaPosition. What is the binary compatibility guarantee of this package, given that it has internal in the name?

The binary compatibility is more for the potential Zinc-as-library users, but Zinc devs may need to worry about potential compatibility issues since there may be Scala compiler interaction via compiler bridge. In this case, it should be ok since the interaction is only with javac.

eed3si9n commented 3 months ago

Can someone please definitively confirm that xsbti.Position#pointer() is 0-based?

I think xsbti.Position started out forwarding Scala 2.x compiler, and here's what Scala 2.13.14 prints with -Xprint-pos:

$ scalac --version
Scala compiler version 2.13.14 -- Copyright 2002-2024, LAMP/EPFL and Lightbend, Inc.

$ scalac -Vprint:parser -Xprint-pos A.scala
[[syntax trees at end of                    parser]] // A.scala
[0:63]package [8:15]example {
  [17:63]object Main extends [29:63][37:40]App {
    [29]def <init>() = [29]{
      [NoPosition][NoPosition][NoPosition]super.<init>();
      [29]()
    };
    [45:61][45:52]println([53:60]"hello")
  }
}

This looks 0-based. For example, [8:15]example:

package example
0123456789012345
vasilmkd commented 3 months ago

The binary compatibility is more for the potential Zinc-as-library users, but Zinc devs may need to worry about potential compatibility issues since there may be Scala compiler interaction via compiler bridge. In this case, it should be ok since the interaction is only with javac.

Do you want me to try and address the problem?

I can re-add the old constructor to preserve binary compatibility. However, even if I add the constructor once more, one of its parameters is no longer uses in the same way as it was before.

As I said, I only need some advice on how to handle this specific problem.

eed3si9n commented 3 months ago

Do you want me to try and address the problem?

Since Zinc should be the only code invoking JavaPosition's apply(...), I don't think we need to do anything further?

vasilmkd commented 3 months ago

Personally, I was quite happy to see the sbt.internal.* MiMa exclusions. 😅