Delete files doesn't run compilation

[tanishiking]

Describe the bug

Create two files

// A.scala
package example
object A:
  def aaa = B.bbb

// B.scala
package example
object B:
  def bbb = ???

and then delete B.scala.

Expected behavior

Run compilation and receive an error in A.scala (Not found: B).

---

I guess this is because we don't run compilation when Metals detect the file deletion https://github.com/scalameta/metals/blob/d2ab724848b0c8b5c066694e5cf431c809715d34/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L1410-L1424 Is this an intentional behavior?

Operating system

macOS

Editor/Extension

VS Code

Version of Metals

v0.11.7

Extra context or search terms

FileWatcher delete compile

[tgodzik]

Thanks for finding this! I think we should actually invoke the compilation in such case

[PeuTit]

Hi, I can take this issue. Do you have an example I could use to understand how to trigger the compilation?

[tgodzik]

Great to hear! We would probably need to add additional method:

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_didDeleteFiles

These are represented in MetalsLanguageServer as https://github.com/scalameta/metals/blob/main/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L1249

We would need to add a method also to Compilations that would check which build target the file belonged to and then invoke the compileTarget method.

I think this should do it, let me know if this is clear enough :smiley:

[tanishiking]

@PeuTit Thank you for your interest!

some context

• Metals has a FileWatcher that observes file modification, creation, and deletion under the projects
  • FileWatcher.scala
• MetalsLanguageServer instantiate the file watcher, passing didChangeWatchedFiles as a call back function.
  • https://github.com/scalameta/metals/blob/c80f730d27092e6b1c51552c90bc9c7d5ec05421/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L215-L221
• didChangeWatchedFiles do something based on the received event from FileWatcher.
  • For example, if scala or java files are deleted, we call onChange
  • Where onChange triggers compile against the modified files (more specifically, Metals triggers running compile against the build target (e.g. sbt subproject) that the modified files belong to. This is because build servers such as sbt cannot compile one specific scala files).

That's how, Metals triggers compile when another file has modified.

For example, when we have A.scala and B.scala

// A.scala
package example
object A:
  def aaa = B.bbb

// B.scala
package example
object B:
  def bbb = ???

and modify B.scala to something like

// B.scala
package example
object B:
  def ccc = ???

we will get compile error in A.scala.

Problem

On the other hand, when we remove B.scala in the above setting, we won't get compile error from A.scala, even though object B won't be found from A.scala.

The root cause of this is because we don't do anything when we delete files https://github.com/scalameta/metals/blob/c80f730d27092e6b1c51552c90bc9c7d5ec05421/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L1412-L1415

We may be able to fix this issue by adding onChange(files) in that branch.

The tricky part is, when we delete files, those files won't be there. So I'm not sure onChange(...) properly trigger the project compilation 🤔

[tanishiking]

Oh, we commented on the same timing!

Great to hear! We would probably need to add additional method: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_didDeleteFiles

Metals doesn't use LSP's file watcher because of the following reasons https://github.com/scalameta/metals/blob/c80f730d27092e6b1c51552c90bc9c7d5ec05421/metals/src/main/scala/scala/meta/internal/metals/watcher/FileWatcher.scala#L30-L35

So, I think we won't receive those workspace/didDeleteFiles events from the LSP client. I believe relying on the current FileWatcher mechanism would be handy.

[tgodzik]

So, I think we won't receive those workspace/didDeleteFiles events from the LSP client. I believe relying on the current FileWatcher mechanism would be handy.

Isn't it a different thing though? I think the notification is send when user deletes it themselves. We do rely on didOpen and didSave notifications, which are similar to this. File watching is the one in "workspace/didChangeWatchedFiles, no?

[tgodzik]

The tricky part is, when we delete files, those files won't be there. So I'm not sure onChange(...) properly trigger the project compilation thinking

We should check which target it belonged to and compile that one in that case as I explained above. Though you might be right that we only need to modify the action invoked on delete. So maybe the new LSP method is not needed.

[tanishiking]

Isn't it a different thing though? I think the notification is send when user deletes it themselves.

Ahhh, right, didDelete notification is something unrelated to LSP FileWatcher 😅

We should check which target it belonged to and compile that one in that case as I explained above. Though you might be right that we only need to modify the action invoked on delete. So maybe the new LSP method is not needed.

I agree, either adding a new LSP endpoint or adding some lines in the branch I mentioned in (didChangeWatchedFiles) is fine 👍 but I guess latter would be handy.

[PeuTit]

Hey thanks for the insight on the issue. If I understood correctly, I need to update the branch in the if statement to re-run the compilation on file deletion.

https://github.com/scalameta/metals/blob/c80f730d27092e6b1c51552c90bc9c7d5ec05421/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L1412

I need to call the onChange method to re-run the compilation.

https://github.com/scalameta/metals/blob/c80f730d27092e6b1c51552c90bc9c7d5ec05421/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L1445

Let me know if I've miss anything!

[tanishiking]

@PeuTit Yes, that's perfectly what I was thinking about :100: As @tgodzik mentioned, there might be a problem (I hope there's no problem🤞), but let's see ...

We should check which target it belonged to and compile that one in that case as I explained above. Though you might be right that we only need to modify the action invoked on delete.

[PeuTit]

Hi again!

I tried to do something like this to mimic the behaviour of the createOrModify branch:

if (isScalaOrJava && event.eventType == EventType.Delete) {
    onChange(List(path)).asJava
    Future {
      diagnostics.didDelete(path)
}.asJava

But the issue is that the event that we get hold the path to the deleted file. This means that we cannot pass it to the onChange function as it will throw an error.

What path do I have access to at this point? Can I get a list of all files of the project? Or even better, all the file that reference the deleted file?

Let me know if you have any idea on how to solve this!

[tanishiking]

@PeuTit Thank you for taking a look at this! I think re-using onChange doesn't work because, first of all, we call FileIO.slurp but I guess it will throw an exception because the target file is already deleted (btw, what error did you get?)

https://github.com/scalameta/metals/blob/f7e47862deb39209d5fd24f619e706af081adec9/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L1441-L1443

what we need to do on delete is compiling the project using https://github.com/scalameta/metals/blob/f7e47862deb39209d5fd24f619e706af081adec9/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L1448 which might work with deleted files (because this method lookup the corresponding project and compile the project, we may don't need to have the file on filesystem, hopefully). It might be good idea to create another function something like onDelete which calls compileFiles.

[PeuTit]

Thanks for the insight, I appreciate it!

The exception I was getting was: Exception in thread "metals-watch-callback-thread" java.nio.file.NoSuchFileException: path-of-file.

[tanishiking]

closing for https://github.com/scalameta/metals/pull/4377