zinc icon indicating copy to clipboard operation
zinc copied to clipboard

Parse column/position information from javac error messages

Open vasilmkd opened this issue 1 year ago • 3 comments

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 avatar Jul 03 '24 16:07 vasilmkd

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 avatar Jul 03 '24 17:07 vasilmkd

I'm working on expanding the tests in javaErrorParserSpec.

vasilmkd avatar Jul 03 '24 17:07 vasilmkd

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

vasilmkd avatar Jul 03 '24 19:07 vasilmkd

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 avatar Jul 05 '24 05:07 eed3si9n

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

eed3si9n avatar Jul 06 '24 22:07 eed3si9n

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.

vasilmkd avatar Jul 07 '24 07:07 vasilmkd

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?

eed3si9n avatar Jul 07 '24 18:07 eed3si9n

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

vasilmkd avatar Jul 07 '24 19:07 vasilmkd