Diagnostics do not update on save

jakcharvat commented 1 month ago

It appears my project doesn't recompile when I save the source files in an sbt project. As a result, only treesitter errors seem to update with the source, but not build diagnostics.

Reproduction steps:

Create a new scala 3 project with sbt new scala/scala3.g8
Open the project in zed, and open Main.scala in the editor
Delete the msg def
- Diagnostics do not update
Restart the language server using editor: restart language server
- bloop: Not found: msg diagnostic appears as expected
Bring def msg = ... back
- The project is compilable and runnable with sbt, but the bloop: Not found: msg error remains until the next language server restart

Looking at lsp and bsp traces from metals-nvim, it appears to send a buildTarget/compile request on file saves, which eventually results in a build/publishDiagnostics response. The zed extension seems to send no such request, and diagnostics only update on language server (re)start.

It seems like I am able to force recompile the project using the "clean compile" button in the web interface of the server, or by manually restarting the language server using zed's editor: restart language server command, but neither of those is very ergonomic.

Am I the only one experiencing this issue? I have metals set up through coursier, using sbt to create projects. Zed metals settings only enable http server:

"metals": {
  "binary": {
    "arguments": ["-Dmetals.http=on"]
  },
  "initialization_options": {
    "isHttpEnabled": true
  }
}

Doctor output:

tgodzik commented 1 month ago

We should be getting didSave notifications when saving the file in Zed, which in turn should invoke the compilation. Are you able to check the .metals/lsp.trace.json file if that happens? If you create the file it should get polluted with traces

jakcharvat commented 1 month ago

Actually I'm not getting any traces there. When I make a bsp.trace.json I get a single build/initialize request with no response, but the lsp.trace.json file stays empty. Maybe zed intercepts them somewhere? When I use metals from neovim they get populated as I would expect.

The LSP features like goto definition, hovers, etc all work though, and I see their messages in the zed LSP Logs / RPC messages view. No didSave notifications there either though.

This is the only entry I get in bsp.trace.json:

[Trace - 05:27:01 PM] Sending request 'build/initialize - (1)'
Params: {
  "displayName": "Metals",
  "version": "1.3.5",
  "bspVersion": "2.2.0-M2",
  "rootUri": "file:///.../testproj/",
  "capabilities": {
    "languageIds": [
      "scala",
      "java"
    ],
    "jvmCompileClasspathReceiver": true
  },
  "data": {
    "javaSemanticdbVersion": "0.10.0",
    "semanticdbVersion": "4.9.9",
    "supportedScalaVersions": [
      "2.13.14",
      "2.12.19",
      "2.12.18",
      "2.12.17",
      "2.12.16",
      "2.13.11",
      "2.13.12",
      "2.13.13",
      "2.11.12"
    ],
    "enableBestEffortMode": false
  }
}

jakcharvat commented 1 month ago

Looks like zed was somehow interfering with the log files, just watching it with tail I do get messages in the lsp trace, but no didSave notifications.

Here's the trace for opening a file, removing the `msg` method, saving and closing the file from the reproduction steps in the original post

I have format on save enabled, so the format notification just before the close is when I saved the file. ```json [Trace - 05:40:20 PM] Received notification 'textDocument/didOpen' Params: { "textDocument": { "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala", "languageId": "scala", "version": 0, "text": "@main def hello(): Unit \u003d\n println(\"Hello world!\")\n println(msg)\n\ndef msg: String \u003d \"Hello, World!\"\n" } } [Trace - 05:40:20 PM] Received request 'textDocument/inlayHint - (121)' Params: { "textDocument": { "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala" }, "range": { "start": { "line": 0, "character": 0 }, "end": { "line": 5, "character": 0 } } } [Trace - 05:40:20 PM] Received request 'textDocument/documentHighlight - (122)' Params: { "textDocument": { "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala" }, "position": { "line": 0, "character": 0 } } [Trace - 05:40:20 PM] Sending response 'textDocument/documentHighlight - (122)'. Processing request took 7ms Result: [ { "range": { "start": { "line": 0, "character": 0 }, "end": { "line": 0, "character": 5 } }, "kind": 2 } ] [Trace - 05:40:20 PM] Received request 'textDocument/codeAction - (123)' Params: { "textDocument": { "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala" }, "range": { "start": { "line": 0, "character": 0 }, "end": { "line": 0, "character": 0 } }, "context": { "diagnostics": [], "only": [ "quickfix", "refactor", "source.organizeImports" ] } } [Trace - 05:40:20 PM] Sending response 'textDocument/codeAction - (123)'. Processing request took 38ms Result: [ { "title": "Organize imports", "kind": "source.organizeImports", "diagnostics": [], "edit": { "changes": { "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala": [] } } } ] [Trace - 05:40:21 PM] Received request 'textDocument/documentHighlight - (124)' Params: { "textDocument": { "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala" }, "position": { "line": 4, "character": 23 } } [Trace - 05:40:21 PM] Sending response 'textDocument/documentHighlight - (124)'. Processing request took 3ms Result: [] [Trace - 05:40:21 PM] Received request 'textDocument/documentHighlight - (125)' Params: { "textDocument": { "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala" }, "position": { "line": 4, "character": 24 } } [Trace - 05:40:21 PM] Sending response 'textDocument/documentHighlight - (125)'. Processing request took 3ms Result: [] [Trace - 05:40:21 PM] Received request 'textDocument/documentHighlight - (126)' Params: { "textDocument": { "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala" }, "position": { "line": 3, "character": 0 } } [Trace - 05:40:21 PM] Sending response 'textDocument/documentHighlight - (126)'. Processing request took 2ms Result: [] [Trace - 05:40:21 PM] Received request 'textDocument/codeAction - (127)' Params: { "textDocument": { "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala" }, "range": { "start": { "line": 3, "character": 0 }, "end": { "line": 4, "character": 24 } }, "context": { "diagnostics": [], "only": [ "quickfix", "refactor", "source.organizeImports" ] } } [Trace - 05:40:21 PM] Sending response 'textDocument/codeAction - (127)'. Processing request took 25ms Result: [ { "title": "Organize imports", "kind": "source.organizeImports", "diagnostics": [], "edit": { "changes": { "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala": [] } } } ] [Trace - 05:40:22 PM] Received notification 'textDocument/didChange' Params: { "textDocument": { "version": 1, "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala" }, "contentChanges": [ { "text": "@main def hello(): Unit \u003d\n println(\"Hello world!\")\n println(msg)\n" } ] } [Trace - 05:40:22 PM] Sending notification 'textDocument/publishDiagnostics' Params: { "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala", "diagnostics": [] } [Trace - 05:40:22 PM] Received request 'textDocument/documentHighlight - (128)' Params: { "textDocument": { "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala" }, "position": { "line": 3, "character": 0 } } [Trace - 05:40:22 PM] Sending response 'textDocument/documentHighlight - (128)'. Processing request took 8ms Result: [] [Trace - 05:40:22 PM] Received request 'textDocument/codeAction - (129)' Params: { "textDocument": { "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala" }, "range": { "start": { "line": 3, "character": 0 }, "end": { "line": 3, "character": 0 } }, "context": { "diagnostics": [], "only": [ "quickfix", "refactor", "source.organizeImports" ] } } [Trace - 05:40:22 PM] Sending response 'textDocument/codeAction - (129)'. Processing request took 39ms Result: [ { "title": "Organize imports", "kind": "source.organizeImports", "diagnostics": [], "edit": { "changes": { "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala": [] } } } ] [Trace - 05:40:22 PM] Received request 'textDocument/formatting - (130)' Params: { "textDocument": { "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala" }, "options": { "tabSize": 2, "insertSpaces": true, "trimTrailingWhitespace": true, "insertFinalNewline": true, "trimFinalNewlines": true } } [Trace - 05:40:22 PM] Sending response 'textDocument/formatting - (130)'. Processing request took 12ms Result: [] [Trace - 05:40:22 PM] Received request 'textDocument/inlayHint - (131)' Params: { "textDocument": { "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala" }, "range": { "start": { "line": 0, "character": 0 }, "end": { "line": 3, "character": 0 } } } [Trace - 05:40:22 PM] Received notification '$/cancelRequest' Params: { "id": 121 } [Trace - 05:40:22 PM] Sending response 'textDocument/inlayHint - (121)'. Processing request took 2693ms Result: null [Trace - 05:40:25 PM] Received notification '$/cancelRequest' Params: { "id": 131 } [Trace - 05:40:25 PM] Sending response 'textDocument/inlayHint - (131)'. Processing request took 2741ms Result: null [Trace - 05:40:25 PM] Received notification 'textDocument/didClose' Params: { "textDocument": { "uri": "file:///Users/jakcharvat/FIT/OOP/testproj/src/main/scala/Main.scala" } } ```

tgodzik commented 1 month ago

That's unexpected, any idea if that is on purpose? we do depend on didSave notification

jakcharvat commented 1 month ago

Could it be related to https://github.com/zed-industries/zed/pull/14412?

I'll see if I can figure out whether the metals server declares support for textDocumentSync.save...

https://github.com/ckipp01/metals/blob/260e9b90b46ba4e5fc790e63467f5c668c1a7bfc/metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala#L1205 Looks like it should, is there a way I can query my metals binary to see what features it declares support for?

WeetHet commented 1 month ago

It seems like zed thinks that watched paths for Metals are empty, weird

WeetHet commented 1 month ago

Oh, there seem to be two issues here, first, metals sends watchers with file://, which zed doesn't handle well. It's a relatively easy fix, strip file:// in zed. I can implement that. The second issue, though, is that metals doesn't send any watchers that match src/main/Scala/*.scala. For me the watchers sent are:

[crates/project/src/lsp_store.rs:4546:63] params.watchers = [
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/*.sbt",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/pom.xml",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/*.sc",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/*?.gradle",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/*.gradle.kts",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/project/*.{scala,sbt}",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/project/project/*.{scala,sbt}",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/project/build.properties",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/.metals/.reports/bloop/*/*",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/**/.bsp/*.json",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/**/BUILD",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/**/BUILD.bazel",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/WORKSPACE",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/WORKSPACE.bazel",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/**/*.bzl",
        ),
        kind: None,
    },
    FileSystemWatcher {
        glob_pattern: String(
            "file:///Users/weethet/Projects/tmp/kyo-pl/*.bazelproject",
        ),
        kind: None,
    },
]

tgodzik commented 1 month ago

Och, we do use our own watchers for most of the stuff, since we had worse experience with in build VS Code ones previously. We could add all Scala paths for Zed at least, though not sure we want to add them in general, which is a much bigger change.

Though I don't believe only watched files should be sent if they are indeed open. At least this is not the experience in other editors,

filipwiech commented 1 month ago

Hey, I'm terribly sorry, due to the lack of time I have completely forgot to link this issue here: https://github.com/zed-industries/zed/issues/18636 You can find more details there and a linked change that explains the source of the problem. We need 2 things to fix this:

First and foremost, Metals has to register more globs for file watching, basically to over all the Scala and Java source code in the workspace, not only a few special build files.
We have to switch the default glob syntax in this plugin to a VS Code one to get rid of the file:// prefix. One can already do it with a custom Zed config like this one:
```
"lsp": {
"metals": {
"initialization_options": {
  "globSyntax": "vscode"
}
}
}
```

Hope that helps. :+1:

WeetHet commented 1 month ago

I've looked a bit into didSave, and I'm unsure if textDocument/didSave should only be sent to the language servers watching the saved file. The spec is unclear, and, for example, helix sends it to everyone

If someone is better than me at reading VSCode files, here's the impl for VSCode as far as I see it also send the notification to all servers, so metals might just be correct here

tgodzik commented 1 month ago

We also have didChangeWatchedFiles notification which is normally what I assumed was decided by the globs.

tgodzik commented 1 month ago

I commented a bit on the issue in Zed and I feel like they arbitrarily decided that LSP spec is saying one thing without consulting anyone, which seems to be the root of the problem. We could try and switch file watching for Scala/Java files to use the default file watching, but I can't promise when it will be. I need to work on a release first

WeetHet commented 1 month ago

I commented a bit on the issue in Zed and I feel like they arbitrarily decided that LSP spec is saying one thing without consulting anyone, which seems to be the root of the problem. We could try and switch file watching for Scala/Java files to use the default file watching, but I can't promise when it will be. I need to work on a release first

I've created a PR reverting the changes to Zed as they seem incorrect, let's see if it goes through

tgodzik commented 1 month ago

At a minimum I think we would be able to force a setting.

Overall, the option to declare files we are interested in would be good to have, but it is simply not there yet.

WeetHet commented 1 month ago

Overall, the option to declare files we are interested in would be good to have, but it is simply not there yet.

I'd prefer that to be a part of the LSP and not a Zed-specific feature

scalameta / metals-zed

Diagnostics do not update on save #28