Hot code replace succeeds, then displays (incorrect) warning

[jryans]

Describe the bug

When changing code while debugging via hot code replace (lightning bolt), then behaviour change is applied correctly, but then an (incorrect) warning appears saying "No classes were reloaded, please check the logs".

To Reproduce Steps to reproduce the behavior:

1. Debug a program
2. Edit a method
3. Click hot code replace (bolt)
4. Warning appears even though behaviour change was applied correctly

Expected behavior

The warning should not appear.

Installation:

• Operating system: Linux
• VSCode version: 1.97.2
• VSCode extension version: 1.49.0
• Metals version: 1.5.2

[jryans]

I'd like to start contributing to Scala tooling, so I'll attempt to fix this.

[tgodzik]

Great to hear!

The error might be here

metals-vscode/src/debugger/hotCodeReplace.ts

Line 62 in 03b751c

if (!response?.changedClasses?.length) {

or somewhere along the scala-debug-adapter -> Bloop -> Metals path

[tgodzik]

Looks like the response doesn't contain "changedClasses" field. Maybe instead checking for success would be good enough

[tgodzik]

[Trace][03:52:32 PM] Received request:
{
  "type": "request",
  "seq": 18,
  "command": "redefineClasses"
}
[Trace][03:52:32 PM] Sent notification:
{
  "type": "event",
  "seq": 26,
  "event": "hotcodereplace",
  "body": {
    "changeType": "STARTING",
    "message": "Starting hot code replacement...",
    "type": "hotcodereplace"
  }
}
[Trace][03:52:32 PM] Sent notification:
{
  "type": "event",
  "seq": 27,
  "event": "breakpoint",
  "body": {
    "reason": "changed",
    "breakpoint": {
      "id": 3,
      "verified": true,
      "line": 12,
      "message": ""
    },
    "type": "breakpoint"
  }
}
[Trace][03:52:32 PM] Sent notification:
{
  "type": "event",
  "seq": 28,
  "event": "hotcodereplace",
  "body": {
    "changeType": "END",
    "message": "Completed hot code replacement",
    "type": "hotcodereplace"
  }
}
[Trace][03:52:32 PM] Sent response:
{
  "type": "response",
  "seq": 18,
  "request_seq": 18,
  "command": "redefineClasses",
  "success": true
}

[jryans]

Thanks, yes, I am seeing similar messages on my side as well... The part that puzzles me at the moment though is the debug adapter code suggests it should be returning more than that in the message (https://github.com/scalacenter/java-debug/blob/0.34.0%2Bx/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/HotCodeReplaceHandler.java#L63), but perhaps that's not the right bit of adapter code...?

I've noticed that when I set the debug adapter redefineClasses response to a test value (to check that I have a working contribution workflow set up and can actually make changes), I don't see that come through either. The VS Code extension always just gets undefined as a response to the redefineClasses request... 🤔 As a separate test of my contribution workflow, I also modified the stack frame list response, and my modification does appear for that handler. So, at the moment my theory is that the request / response handling for redefineClasses isn't fully wired up quite right, but I haven't fully worked out exactly what's going wrong yet.

I should have a bit more time to look again later this week.

[jryans]

Well, I got a little bit further, but not a lot...

When I modify scala-debug-adapter to log its DAP traffic to a file, the redefineClasses response has a changedClasses field:

{
  "success": true,
  "request_seq": 14,
  "command": "redefineClasses",
  "body": {
    "changedClasses": [
      "Main$package$",
      "loop"
    ]
  },
  "seq": 25,
  "type": "response"
}

but by the time it reaches Metals, the body has been stripped away somehow:

[Trace][09:05:54 pm] Received response:
{
  "type": "response",
  "seq": 25,
  "request_seq": 14,
  "command": "redefineClasses",
  "success": true
}

I've noticed that at least Metals has its own DAP proxy in the mix... Is that the only one, or does Bloop also run a DAP proxy too? (I found a Bloop DAP proxy for testing, but I assume that's not used when debugging.)

At the moment, it feels like one or more DAP proxies are not aware that the redefineClasses response has additional content in the body, and potentially these proxies remove all unexpected fields?

[tgodzik]

At the moment, it feels like one or more DAP proxies are not aware that the redefineClasses response has additional content in the body, and potentially these proxies remove all unexpected fields?

Looks like it, but it's only metals that has the proxy. The feature was added in scalacenter/bloop#2276 and I don't see anything there that could cause it.

[tgodzik]

Maybe it's not worth digging too much into it if it's just a warning. We can check for the success flag and that's all.

[jryans]

I agree we could just work with what we've got and check for success... but, it's a bit sad that the additional info on changed classes is getting lost somehow. I was hoping to use this issue to better understand the Scala tooling pipeline at work here, so it would be a bit more rewarding to dig further and find out which component is changing the messages like this.

Having said that, it's possible I'll get distracted onto other things, so of course someone can feel free to resolve this by just checking for success as you say.