Scala support broken in v0.156.x

[filipwiech]

Check for existing issues

• Completed

Describe the bug / provide steps to reproduce it

After PR #17756 support for the Scala LSP became broken: server is no longer notified about saved files.

@osiewicz I have included more details as a comment in you pull request:
#17756 (comment) 👍

Environment

Zed: v0.156.0 (Zed Preview)
OS: Linux Wayland ubuntu 24.04
Memory: 31 GiB
Architecture: x86_64
GPU: Intel(R) UHD Graphics (CML GT2) || Intel open-source Mesa driver || Mesa 24.0.9-0ubuntu0.1

If applicable, add mockups / screenshots to help explain present your vision of the feature

No response

If applicable, attach your Zed.log file to this issue.

Zed.log

[osiewicz]

Uh oh. Based on your comment I do believe that Metals is registering for scala files too?
Aren't the following globs matching Scala source files?

   {
      "globPattern": {
        "left": "file:///to/workspace/*.sc"
      }
    },
    {
      "globPattern": {
        "left": "file:///to/workspace/project/*.{scala,sbt}"
      }
    },

If so, the problem seems to be that these patterns should be root/**/*.{scala,sbt} and not root/*.{scala,sbt}, as the latter are not recursive.

I don't think we should be bringing back the old behaviour, as overnotifying breaks RA. Gotta pick our poison. :P

[filipwiech]

Unfortunately all the patterns are about the "special" definition files only, which describe the project structure and settings for different Scala build tools (like SBT, Mill, etc.). They don't cover the normal source code. I imagine that probably Metals could (should?) register for these as well. 🤔 Will try to create an issue there and get back to you. 👍

[osiewicz]

Yeah well, I imagine it should be as "easy" as including that /**/ bit in these globs.

[tgodzik]

Hey, we can add those patterns probably, though I am not sure if it's not bigger change as file events would get duplicated with our own file watching

However, isn't file watching only for workspace/didChangeWatchedFiles notification? There is nothing about didSave being configured this way. Also, both VS Code and Helix do send didSave notifications without the additional globs.

The spec also says it's perfectly valid to implement our own file watchers (we had different experience from different clients), so it seems any server that does it will have the same issue as we don't want to duplicate the file events.

[osiewicz]

@tgodzik

However, isn't file watching only for workspace/didChangeWatchedFiles notification? There is nothing about didSave being configured this way. Also, both VS Code and Helix do send didSave notifications without the additional globs.

This is where the ambiguity kicks in; we used to match what Helix/VSC does and that caused problems for us with RA.
RA was explicitly tripping up on us sending didSave for files it didn't ask for, so right now our implementation is consistent with them.

I do think however that the client-side file watching is intended for the whole lifecycle of the document, as explained in this issue: microsoft/language-server-protocol#939

[tgodzik]

Why doesn't RA break on VS Code then? Is Zed sending different files that VS Code does? Though if there is an ambiguity isn't it better to do the same as the other clients do?

The current behaviour feels like forcing every server to use file watching, while the specification explicitly says that server can implement their own file watching. If they can, why would they have to also use the one from the client always.

Why only use these globs on didSave and not didChange, which is still being sent. That is highly inconsistent.

Also, why would we have both workspace/didChangeWatchedFiles and textDocument/didSave ? If we always register for files then the first method would be enough.

microsoft/language-server-protocol#939

There is nothing there about file watching though. It's only about didSave/didOpen notifications. Their server might just be filtering the files to what they need.

Overall, I feel like a change like that should be first changed in the spec, otherwise this can cause issues and it already has. Is it possible to revert this change, maybe add a setting?

[tgodzik]

Thinking a bit further I don't believe there is any ambiguity. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_didSave doesn't say anything about didChangeWatchedFiles files capability, which means that it's not connected to it. LSP spec is very explicit about everything and the connection between didChangeWatchedFiles and didSave is just not there however I try to find it.

It looks like the RA folks decided it must be like that (it does make sense to have such capability), but there is really nothing to back it up. Every notification has a connected capability, the same with FileOperationOptions, which again have separate globs to define what files we are interested in. We would need didSave to have that defined in the it's options, and there is nothing there

I see three options here:

1. Revert this change
2. Add a setting for each extension to use
3. Leave as is and force every extension to follow the same unspecified option.

I can live with each ( 3. feels worst for me), but I strongly believe there is a big misconception about LSP here, which might cause more issues.

[osiewicz]

Why doesn't RA break on VS Code then? Is Zed sending different files that VS Code does? Though if there is an ambiguity isn't it better to do the same as the other clients do?

It works in VSC by happenstance; the RA extension itself registers to watch just Rust files.
Same goes for Helix, which registers RA as a language server just for Rust: https://github.com/helix-editor/helix/blob/a7651f5bf027ec98645d571ab05a685d97e1b772/languages.toml#L248

By that merit, neither our old nor new behaviour matched what they did.

Why only use these globs on didSave and not didChange, which is still being sent. That is highly inconsistent.

We do respect globs for didChange as well. didSave was the odd one.

There is nothing there about file watching though. It's only about didSave/didOpen notifications. Their server might just be filtering the files to what they need.

I do believe it is related to file watching. Namely, it says that the content of didSave notification should match the state of the document after the sequence of edits made by file watching. This is not a property we can uphold when the didSave notification is sent unconditionally, as then it would be possible to have a document excluded from edit watching for which we'd send no edit events; yet, we'd still send didSave that would not match the state of the document on the server side.

Also, why would we have both workspace/didChangeWatchedFiles and textDocument/didSave ? If we always register for files then the first method would be enough.

I agree, I imagine that it would be possible to reuse didChangeWatchedFiles for that with an extra WatchKind of Save.

It looks like the RA folks decided it must be like that (it does make sense to have such capability), but there is really nothing to back it up.

I think we may want to ask LSP folks directly which way is preferred.

[WeetHet]

Why only use these globs on didSave and not didChange, which is still being sent. That is highly inconsistent.

We do respect globs for didChange as well. didSave was the odd one.

It does not seem so:

zed/crates/project/src/lsp_store.rs

Lines 2872 to 2889 in f73a076

	let next_version = previous_snapshot.version + 1;
	buffer_snapshots.push(LspBufferSnapshot {
	version: next_version,
	snapshot: next_snapshot.clone(),
	});
	language_server
	.notify::<lsp::notification::DidChangeTextDocument>(
	lsp::DidChangeTextDocumentParams {
	text_document: lsp::VersionedTextDocumentIdentifier::new(
	uri.clone(),
	next_version,
	),
	content_changes,
	},
	)
	.log_err();
	}

Also, why would we have both workspace/didChangeWatchedFiles and textDocument/didSave? If we always register for files then the first method would be enough.

A discussion from the LSP repo

[tgodzik]

From microsoft/language-server-protocol#237 from the LSP repo

What is the difference to didChange: didChange events are only send for document for which a didOpen got send. didOpen, didChange, didClose is the live cycle of a document buffer and the buffer must not necessarily exist on disk.

It looks like that extends to didSave. So this could even be a totally virtual document, which would not have to exist on filesystem. This all only related to the text buffer, nothing else.

There is a mention also that it's a missed opportunity to be able to declare which files we want:

I like the idea of adding a client capability to signal if a client can provide these events at all. And one thing that I missed to spec and implement is that a server can dynamically register for file events or ask for file events on the initialize result. We should add both to the spec and implementation.

So it looks like it's not really been even thought of in the spec.