scalameta / metals

Scala language server with rich IDE features 🚀
https://scalameta.org/metals/
Apache License 2.0
2.1k stars 334 forks source link

New files when using scala-cli aren't registered #4741

Closed ckipp01 closed 1 year ago

ckipp01 commented 1 year ago

Describe the bug

I'm not 100% sure if this is something we can solve on our end or if there would need to be changes in scala-cli but one thing I noticed today was the following.

Let's say you have a Main.scala that you've already ran setup-ide for via scala-cli setup-ide . Then inside of that file you try to use Foo, but Foo doesn't exist yet. So Metals offers to create this new Foo for you and you choose a case class for it. Metals then facilitates the create of Foo, but your Main.scala still doesn't detect it. Also when you look at the Source Directories you still only see main:

Source Directories
  /Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Main.scala

Expected behavior

I'd expect that if a user is using Metals to create a new file it should be able to "reload" the build or let scala-cli know that there is another source. Either that or scala-cli should know that another source was added to the workspace.

Operating system

macOS

Editor/Extension

Nvim (nvim-metals)

Version of Metals

0.11.9+211-b679d25f-SNAPSHOT

Extra context or search terms

No response

tgodzik commented 1 year ago

Thanks for reporting! When I create a file in Metals it's correctly picked up and compiles fine. I can see 2022.12.19 15:46:44 INFO Refreshed build after change in the logs. Are you sure you are using the newest scala-cli? Maybe you scala-cli setup-ide Main.scala ?

ckipp01 commented 1 year ago

Weird that's not what I'm seeing at all for example, here is the LSP trace

Full trace
``` [Trace - 04:37:33 pm] Received response 'metals/quickPick - (2)' in 3125ms Result: { "itemId": "scala-case-class" } Error: null [Trace - 04:37:33 pm] Sending notification 'metals/executeClientCommand' Params: { "command": "metals-goto-location", "arguments": [ { "uri": "file:///Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala", "range": { "start": { "line": 0, "character": 21 }, "end": { "line": 0, "character": 21 } }, "otherWindow": false } ] } [Trace - 04:37:33 pm] Sending response 'workspace/executeCommand - (12)'. Processing request took 3143ms Result: {} [Trace - 04:37:33 pm] Received notification 'textDocument/didOpen' Params: { "textDocument": { "uri": "file:///Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala", "languageId": "scala", "version": 0, "text": "final case class Foo()\n" } } [Trace - 04:37:33 pm] Received request 'textDocument/codeLens - (13)' Params: { "textDocument": { "uri": "file:///Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala" } } [Trace - 04:37:33 pm] Received notification 'metals/didFocusTextDocument' Params: "file:///Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala" [Trace - 04:37:33 pm] Sending notification 'window/logMessage' Params: { "type": 4, "message": "2022.12.19 16:37:33 WARN no build target for: /Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala" } [Trace - 04:37:33 pm] Sending notification 'metals/publishDecorations' Params: { "uri": "file:///Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala", "options": [] } [Trace - 04:37:33 pm] Sending notification 'metals/publishDecorations' Params: { "uri": "file:///Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala", "options": [] } [Trace - 04:37:33 pm] Sending response 'textDocument/codeLens - (13)'. Processing request took 67ms Result: [] [Trace - 04:37:33 pm] Received request 'textDocument/documentHighlight - (14)' Params: { "textDocument": { "uri": "file:///Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala" }, "position": { "line": 0, "character": 21 } } [Trace - 04:37:33 pm] Received request 'textDocument/codeLens - (15)' Params: { "textDocument": { "uri": "file:///Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala" } } [Trace - 04:37:33 pm] Sending response 'textDocument/codeLens - (15)'. Processing request took 2ms Result: [] [Trace - 04:37:33 pm] Sending response 'textDocument/documentHighlight - (14)'. Processing request took 7ms Result: [ { "range": { "start": { "line": 0, "character": 20 }, "end": { "line": 1, "character": 1 } }, "kind": 3 } ] [Trace - 04:37:35 pm] Received notification 'textDocument/didSave' Params: { "textDocument": { "uri": "file:///Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala" }, "text": "final case class Foo()\n" } [Trace - 04:37:35 pm] Sending notification 'window/logMessage' Params: { "type": 4, "message": "2022.12.19 16:37:35 WARN no build target for: /Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala" } [Trace - 04:37:35 pm] Received request 'textDocument/documentHighlight - (16)' Params: { "textDocument": { "uri": "file:///Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala" }, "position": { "line": 0, "character": 21 } } [Trace - 04:37:35 pm] Received request 'textDocument/codeLens - (17)' Params: { "textDocument": { "uri": "file:///Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala" } } [Trace - 04:37:35 pm] Sending response 'textDocument/documentHighlight - (16)'. Processing request took 2ms Result: [ { "range": { "start": { "line": 0, "character": 20 }, "end": { "line": 1, "character": 1 } }, "kind": 3 } ] [Trace - 04:37:35 pm] Sending response 'textDocument/codeLens - (17)'. Processing request took 2ms Result: [] [Trace - 04:37:36 pm] Sending notification 'metals/status' Params: { "text": "", "hide": true } ```

This is from the moment I select to create a new case class, save it, etc. Is there maybe a notification missing from the nvim-metals side that Metals is expecting here? What is actually triggering the reload for scala-cli?

Here is what I'm using:

  - Metals Server version: 0.11.9+211-b679d25f-SNAPSHOT
  - Build server currently being used is scala-cli v0.1.18.
tgodzik commented 1 year ago

Weird that's not what I'm seeing at all for example, here is the LSP trace

Full trace This is from the moment I select to create a new case class, save it, etc. Is there maybe a notification missing from the nvim-metals side that Metals is expecting here? What is actually triggering the reload for scala-cli?

That should be entirely on the Metals side, there is onBuildTargetChanged notification, but that is never forwarded to clients.

ckipp01 commented 1 year ago

Interesting so I see inverseSources requests but no onBuildTargetChanged happening in the BSP logs. For example here is from the original diagnostic through me created and saving Foo.scala:

BSP logs
``` [Trace - 05:24:50 pm] Received notification 'build/publishDiagnostics' Params: { "textDocument": { "uri": "file:///Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Main.scala" }, "buildTarget": { "uri": "file:/Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/.scala-build/?id\u003dproject_bd2c96d2de-1f78fb3332" }, "diagnostics": [ { "range": { "start": { "line": 2, "character": 10 }, "end": { "line": 2, "character": 13 } }, "severity": 1, "code": "6", "source": "bloop", "message": "Not found: Foo" } ], "reset": true } [Trace - 05:24:50 pm] Received notification 'build/taskFinish' Params: { "taskId": { "id": "4" }, "eventTime": 1671467090602, "message": "Compiled \u0027project_bd2c96d2de-1f78fb3332\u0027", "status": 2, "dataKind": "compile-report", "data": { "target": { "uri": "file:/Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/.scala-build/?id\u003dproject_bd2c96d2de-1f78fb3332" }, "errors": 1, "warnings": 0, "isNoOp": false, "isLastCycle": true } } [Trace - 05:24:50 pm] Received response 'buildTarget/compile - (14)' in 270ms Result: { "statusCode": 2 } Error: null [Trace - 05:24:50 pm] Sending request 'buildTarget/scalaMainClasses - (15)' Params: { "targets": [ { "uri": "file:/Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/.scala-build/?id\u003dproject_bd2c96d2de-1f78fb3332" } ] } [Trace - 05:24:50 pm] Sending request 'buildTarget/scalaTestClasses - (16)' Params: { "targets": [ { "uri": "file:/Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/.scala-build/?id\u003dproject_bd2c96d2de-1f78fb3332" } ] } [Trace - 05:24:50 pm] Received response 'buildTarget/scalaMainClasses - (15)' in 2ms Result: { "items": [ { "target": { "uri": "file:/Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/.scala-build/?id\u003dproject_bd2c96d2de-1f78fb3332" }, "classes": [] } ] } Error: null [Trace - 05:24:50 pm] Received response 'buildTarget/scalaTestClasses - (16)' in 2ms Result: { "items": [] } Error: null [Trace - 05:25:02 pm] Sending request 'buildTarget/inverseSources - (17)' Params: { "textDocument": { "uri": "file:///Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala" } } [Trace - 05:25:02 pm] Received response 'buildTarget/inverseSources - (17)' in 3ms Result: { "targets": [] } Error: null [Trace - 05:25:03 pm] Sending request 'buildTarget/inverseSources - (18)' Params: { "textDocument": { "uri": "file:///Users/ckipp/Documents/scratch-workspace/new-file-scala-cli-test/Foo.scala" } } [Trace - 05:25:03 pm] Received response 'buildTarget/inverseSources - (18)' in 2ms Result: { "targets": [] } Error: null ```
kasiaMarek commented 1 year ago

In conclusion:

  1. If you run scala-cli setup-ide . the files are discovered correctly.
  2. If you run scala-cli run Main.scala the files are not discovered, which is not technically a bug but can be confusing. It is even more confusing since running scala-cli run . will not update the config. So if no build target is found for a file for scala-cli we should ask the user if they want to amend the config and restart the build server?
tgodzik commented 1 year ago

So if no build target is found for a file for scala-cli we should ask the user if they want to amend the config and restart the build server?

Ideally yes, I wonder whether even to do it automatically :thinking:

kasiaMarek commented 1 year ago

Turns out @ckipp01 is right. If you do scala-cli setup-ide . new files are not properly registered if you use new scala file.... Build targets are not found for those newly created files, which makes sense, since Source Directories for scala-cli will be a list of the files in that build target. However, if you add a file using a different method than new scala file... it will cause scala-cli bsp to reconnect and recreate build targets, so they contain also the new file. Which makes is look like things work properly. @tgodzik, any thoughts?

tgodzik commented 1 year ago

That's super weird, this should work the same irrelevant of how the file is created :thinking:

kasiaMarek commented 1 year ago

After some digging. It seems it's not metals that causes the recreation of the build targets but the build server. And refresh in metals is just the consequence of bs changes. You can see it when you start scala-cli bsp from terminal and add a new scala file. So I'm thinking that whatever action the build sever reacts to is not triggered when metals is the one to create the file. So, yeah @tgodzik from metals point of view the way you create the file doesn't matter.

tgodzik commented 1 year ago

This should ork correctly with the amend options.Closing for now, do let u know if it breks again.

ckipp01 commented 1 year ago

Just confirming that I did just try the same workflow as I did when I reported this and it does indeed work 🎉