Bump zinc and account for diagnostic code

[ckipp01]

At the moment this is just testing a bit. I see you already have a test
PR to bump zinc in #1845 but one
of the things that this brings in is the changes to Problem I made in
sbt/sbt#6874 that expose the diagnostic code of
the diagnostic coming from dotc. I have been doing some work on that on
the compiler side in scala/scala3#15565 and
wanted to try it out with Mill.

I tried to mimic the way you currently have it set up, so let me know if
it's not the direction you'd want to go. However, the idea here would be
that the diagnostic code is forwarded when diagnostics are published via
BSP so that Metals could then capture that code and know what code
actions to offer. You can see more of the big picture in scala/scala3#14904.

[lefou]

Hi @ckipp01, what's the state of this? Could you bump this to zinc 1.7.0?

I updated #1845 to zinc 1.7.0 and now it fails three CI workflows. The CaffeineTests to be specific. Do you have any idea what's changed, that is causing this?

[ckipp01]

Hi @ckipp01, what's the state of this? Could you bump this to zinc 1.7.0?

Yea, later this week I should have some time to look back at this, but don't let it block updating to 1.7.0 in the other pr. Plus, this won't really matter until Dotty starts giving this information, and I'm still stuck on that since when testing with the locally published version of dotty that gives those, they are alway None so I'm missing something that I need to figure out.

As for the failures in the other PR, no idea.

[ckipp01]

I honestly have no idea what's going on with this. I was able to get this added to sbt by just mapping the diagnostic code pretty easily in https://github.com/sbt/sbt/pull/6998/files, but when trying to do the same thing here the code is just never there. I'm not sure if I'm misunderstanding something with how the compiler bridge works with Mill, but I can't get this to work. Do you by chance have any insight into what I might be doing wrong here or why this might not be working? You should be able to test this with 3.2.1-RC1-bin-20220805-e560c2d-NIGHTLY as your scalaVersion and then the Problem should end up having what was added in scala/scala3#15565, but it just doesn't seem to ever be set. At first I thought I did something wrong with my Dotty pr, but after seeing it work in sbt I must just be missing something here. Any insight would be greatly appreciated.

[lefou]

I honestly have no idea what's going on with this. I was able to get this added to sbt by just mapping the diagnostic code pretty easily in https://github.com/sbt/sbt/pull/6998/files, but when trying to do the same thing here the code is just never there. I'm not sure if I'm misunderstanding something with how the compiler bridge works with Mill, but I can't get this to work. Do you by chance have any insight into what I might be doing wrong here or why this might not be working? You should be able to test this with 3.2.1-RC1-bin-20220805-e560c2d-NIGHTLY as your scalaVersion and then the Problem should end up having what was added in lampepfl/dotty#15565, but it just doesn't seem to ever be set. At first I thought I did something wrong with my Dotty pr, but after seeing it work in sbt I must just be missing something here. Any insight would be greatly appreciated.

I can't promise anything, but just from looking at the single line change in sbt, one potential difference between sbt and Mill could be the way we feed the source files. I don't know if we do it differently, but zinc has introduced some VirtualFile concept and it could be we use a different implementation which does not meet some assumptions which are fulfilled by sbt. This is just a guess though. It could also be caused by the use of different APIs, as we can't call all the sbt-private zinc API as sbt does. Maybe, we need to enable something before zinc/scalac is producing this info?

[ckipp01]

I can't promise anything, but just from looking at the single line change in sbt, one potential difference between sbt and Mill could be the way we feed the source files. I don't know if we do it differently, but zinc has introduced some VirtualFile concept and it could be we use a different implementation which does not meet some assumptions which are fulfilled by sbt. This is just a guess though. It could also be caused by the use of different APIs, as we can't call all the sbt-private zinc API as sbt does. Maybe, we need to enable something before zinc/scalac is producing this info?

I don't think this should really have anything to do with it since this change touches a very small area, and is only reflected in the diagnosticCode being present. So I can tell that the version of Zinc we have here is correct since we have those fields, but I just can't figure out why it's always empty. When it's reported from Dotty, the sbt-bridge that is being used by that Scala version (3.2.1-RC1-bin-20220805-e560c2d-NIGHTLY) for example should be reporting it. So it should be there just like it is in sbt. I'm going to try to figure it out in Bloop first since maybe there I'll figure out something I'm missing here.

[ckipp01]

btw update on this, I need sbt/sbt#7006 merged in first, and then I need to do an update to Zinc https://github.com/sbt/zinc/blob/5804d50afd960330f34af35e4367fdb171204f5a/internal/zinc-compile-core/src/main/scala/sbt/internal/inc/LoggedReporter.scala#L133-L142 and then hopefully this should be working here.

[ckipp01]

So Mima is failing, but this is now working as expected. For example given the following code in Scala 3:

package other

object Main extends App {
  val a = "hi"
  def test(a: Int) = if (a < 0) "negative" else "positive"
  test(a)
}

If you run it with Mill you'll see:

❯ ./mill-release minimal3.compile
[32/32] minimal3.compile
[info] compiling 1 Scala source to /Users/ckipp/Documents/scala-workspace/minimal/out/minimal3/compile.dest/classes ...
[error] -- [E007] Type Mismatch Error: /Users/ckipp/Documents/scala-workspace/minimal/minimal3/src/Main.scala:6:7
[error] 6 |  test(a)
[error]   |       ^
[error]   |       Found:    (other.Main.a : String)
[error]   |       Required: Int
[error]   |
[error]   | longer explanation available when compiling with `-explain`
[error] one error found
1 targets failed
minimal3.compile Compilation failed

And then when used as a BSP server Mill will be correctly reporting the code:

[Trace - 08:26:06 am] Received notification 'build/publishDiagnostics'
Params: {
  "textDocument": {
    "uri": "file:/Users/ckipp/Documents/scala-workspace/minimal/minimal3/src/Main.scala"
  },
  "buildTarget": {
    "uri": "file:///Users/ckipp/Documents/scala-workspace/minimal/minimal3?id\u003dminimal3"
  },
  "diagnostics": [
    {
      "range": {
        "start": {
          "line": 5,
          "character": 7
        },
        "end": {
          "line": 5,
          "character": 8
        }
      },
      "severity": 1,
      "code": "7",
      "source": "mill",
      "message": "Found:    (other.Main.a : String)\nRequired: Int"
    }
  ],
  "reset": true
}

[lefou]

Thanks for updating this PR. I wonder if there is an easy way to test this?

There is a binary incompatibility in mill.api.Problem, which needs to be fixed when this PR shall make it into Mill 0.10 series. I think, one possible solution could be to let the new method diagnosticCode have a default implementation, which could return None, so that older code inheriting this trait won't fail to load. We can make that change temporary and remove it once we are in 0.11 development scope. (It would be fine to already open a draft PR with the removal of the default value.)

[ckipp01]

I wonder if there is an easy way to test this?

It depends I guess. Testing it at which phase, during the actual BSP reporting? I looked a bit and didn't see any tests regarding diagnostic reporting, but I may have missed them. Did I?

I think, one possible solution could be to let the new method diagnosticCode have a default implementation, which could return None, so that older code inheriting this trait won't fail to load.

Ah yea, should have thought of this. I went ahead and added a default and then also added a TODO. I can create another draft after the merge to remove this so we don't forget.

[lefou]

I wonder if there is an easy way to test this?

It depends I guess. Testing it at which phase, during the actual BSP reporting? I looked a bit and didn't see any tests regarding diagnostic reporting, but I may have missed them. Did I?

You are right. We currently don't have any thorough BSP workflow testing. I guess, we lack some easy BSP client test toolkit. As we currently don't activate reporting in normal compile runs, there is no existing test into which we can hook easily. I'm fine with the PR without any new test.

[lefou]

Looks good to me. Thank you!