[ty] Fix handling of non-Python text documents#23704
Conversation
|
Two notes about this:
|
|
|
||
| server.close_text_document(foo); | ||
| let diagnostics = server.await_notification::<PublishDiagnostics>(); | ||
| insta::assert_debug_snapshot!(diagnostics); |
There was a problem hiding this comment.
I believe this is is where we should have been awaiting the diagnostic notification before. But since we didn't send one back for the DidOpen call below (because of the aggressive LanguageId filtering), the await_notification below just caught this. But now that we include a publish diagnostics notification for DidOpen (containing no diagnostics, which is I think the important bit), we need to await that.
crates/ty_server/src/session.rs
Outdated
| pub(crate) fn open_notebook_document(&mut self, document: NotebookDocument) -> DocumentHandle { | ||
| let handle = self.index_mut().open_notebook_document(document); | ||
| self.open_document_in_db(&handle); | ||
| self.open_document_in_db(&handle, Some(LanguageId::Python)); |
There was a problem hiding this comment.
Using LanguageId::Python for a notebook feels inaccurate to me. A notebook isn't a Python file. I'd prefer to pass None here and change TextDocument::language_id to LanguageId (remove the Option), as you proposed.
There was a problem hiding this comment.
Yeah I can do that.
Also, in the open_text_document case, do we even need to call open_document_in_db in the case of a non-Python TextDocument? If not, then we don't need to pass a language ID to open_document_in_db at all.
There was a problem hiding this comment.
It's probably safer to keep call open_document_in_db.
I thought there was a filter in ty's setup that defines that ty's only interested in Python files, but I fail to find it. It could be that this filter only exists for VS Code where the extension explicitly limits the files ty's interested to Python files. That's why I'm now a bit unsure if VS Code even sends didOpen request for non-Python files.
Given that I'm not aware of a Python files-only for other clients, the one instance where I think it matters is if a user e.g. opens a .pth file in their editor and starts to make changes to it. It's then important that we update the File's state to ensure ty remains in sync with the filesystem.
In #22449, I added a check to our "did open" handler to effectively ignore notifications for text documents that we were sure weren't Python. This was meant to fix a case where we could return diagnostics for non-Python files, which was undesirable. However, it seems like that might have been too big of a hammer. It seems like we might still want to track non-Python text files in our index but not our project. Otherwise subsequent requests regarding that non-Python file result in log messages saying that ty doesn't know about the file. i.e., a state synchronization issue. Addresses #23121 (comment)
9d5dd5e to
dcc58fa
Compare
|
|
||
| let text_doc = TextDocument::new(uri, text, version, &language_id); | ||
| let document = session.open_text_document(text_doc); | ||
| publish_diagnostics_if_needed(&document, session, client); |
There was a problem hiding this comment.
Nit: We could still skip publishing diagnostics for non-python files (I think?). But IMO, it's not a big deal if we still call it because project.check_file will bail early for non Python files.
There was a problem hiding this comment.
Yeah that was exactly my thought. It seemed fragile to repeat the language ID check here, since there are other reasons why a file might not get added to a project, e.g., when project.is_file_included(db, system_path) returns false. So if we wanted to do this, I think we'd want to bubble up a flag about whether the file was actually added to the project or not. And at that point it didn't seem worth doing.
In #22449, I added a check to our "did open" handler to effectively
ignore notifications for text documents that we were sure weren't
Python. This was meant to fix a case where we could return diagnostics
for non-Python files, which was undesirable.
However, it seems like that might have been too big of a hammer. It
seems like we might still want to track non-Python text files in our
index but not our project. Otherwise subsequent requests regarding that
non-Python file result in log messages saying that ty doesn't know about
the file. i.e., a state synchronization issue.
Addresses #23121 (comment)