LSP: cleanup when a client has closed all files in a project#5381
LSP: cleanup when a client has closed all files in a project#5381TrippleCCC wants to merge 7 commits intogleam-lang:mainfrom
Conversation
TrippleCCC
changed the title
ankddev
left a comment
ankddev
left a comment
There was a problem hiding this comment.
Hello! I've left some notes inline.
Also, it should clean all diagnsotic when I close all files in project, am I right? For me it doesn't work, I still see diagnostics (Zed):
TrippleCCC
force-pushed
the
lsp-project-cleanup
branch
from
f10f96e to
010ce04
Compare
I initially miss understood the procedure for clearing diagnostics. The latest changes should fix this. |
There was a problem hiding this comment.
Great, thank you! I've left some comments inline.
There's no tests for this yet- how do we verify that it works now and in future?
The language server is quite complex, and it's not clear why things are done, so detailed comments explaining the what and the why of each bit of functionality would be very helpful too.
language-server/src/messages.rs
Outdated
| SourceFileChangedInMemory { path: Utf8PathBuf, text: String }, | ||
| SourceFileChangedInMemory { | ||
| path: Utf8PathBuf, | ||
| opened: bool, |
There was a problem hiding this comment.
What does this represent, and why do we need to track it?
Could you add documentation please 🙏
There was a problem hiding this comment.
Moved this into separate notification enums. Should be a bit more clear
language-server/src/messages.rs
Outdated
| let notification = Notification::SourceFileChangedInMemory { | ||
| path: super::path(¶ms.text_document.uri), | ||
| text: params.content_changes.into_iter().next_back()?.text, | ||
| opened: false, |
There was a problem hiding this comment.
When is it possible to have files edited in memory but not open? Isn't that impossible?
There was a problem hiding this comment.
SourceFileChangedInMemory was used for both open and change notifications; the flag was used to diff between the two. I went ahead an separated like you suguested.
| outside_of_project_feedback: FeedbackBookKeeper, | ||
| router: Router<IO, ConnectionProgressReporter<'a>>, | ||
| changed_projects: HashSet<Utf8PathBuf>, | ||
| opened_files: HashMap<Utf8PathBuf, bool>, |
There was a problem hiding this comment.
Added a comment to explain a bit more. TLDR this allows us to keep diagnostics for closed files up when not all project files are open.
language-server/src/server.rs
Outdated
| let feedback = self.cache_file_in_memory(&path, text); | ||
| if opened && feedback.diagnostics.is_empty() { | ||
| let _ = self.opened_files.insert(path, true); | ||
| } |
There was a problem hiding this comment.
How come we only record the file as opened if there's no diagnostics?
There was a problem hiding this comment.
I removed this check. We actually should still track that the file is open even if we don't return diagnostics.
language-server/src/server.rs
Outdated
| let _ = self | ||
| .opened_files | ||
| .get_mut(&path) | ||
| .map(|is_open| *is_open = false); |
There was a problem hiding this comment.
What's the difference between the value being absent and being false?
There was a problem hiding this comment.
Added a comment to the opened_files field
TrippleCCC
force-pushed
the
lsp-project-cleanup
branch
from
4d9b670 to
6cbf694
Compare
|
Hello @TrippleCCC! If you think the PR is ready to be reviewed again remember to un-draft it |
4 participants
Closes #3205
The language server now keeps track of open files and removes the project engine once all open project files have been closed.
The issue also mentions clearing all diagnostics; I believe this should already be the case since
discard_in_memory_cachereturns an empty feedback.It looks like
LanguageServeris not set up for testing. I would be happy to create tests for this if it is necessary.