[ty] Track open files in the server by dhruvmanila · Pull Request #19264 · astral-sh/ruff

dhruvmanila · 2025-07-10T16:38:53Z

Summary

This PR updates the server to keep track of open files both system and virtual files.

This is done by updating the project by adding the file in the open file set in didOpen notification and removing it in didClose notification.

This does mean that for workspace diagnostics, ty will only check open files because the behavior of different diagnostic builder is to first check is_file_open and only add diagnostics for open files. So, this required updating the is_file_open model to be should_check_file model which validates whether the file needs to be checked based on the CheckMode. If the check mode is open files only then it will check whether the file is open. If it's all files then it'll return true by default.

Closes: astral-sh/ty#619

Test Plan

Before

There are two files in the project: __init__.py and diagnostics.py.

In the video, I'm demonstrating the old behavior where making changes to the (open) diagnostics.py file results in re-parsing the file:

Screen.Recording.2025-07-15.at.11.07.01.mov

After

Same setup as above.

In the video, I'm demonstrating the new behavior where making changes to the (open) diagnostics.py file doesn't result in re-parting the file:

Screen.Recording.2025-07-15.at.11.07.44.mov

github-actions · 2025-07-10T16:49:18Z

`mypy_primer` results

No ecosystem changes detected ✅
No memory usage changes detected ✅

crates/ty_project/src/lib.rs

MichaReiser · 2025-07-10T17:11:39Z

crates/ty_server/src/server/api/notifications/did_open.rs

+                // TODO: Why do we require this? Because this doesn't do anything as `File` doesn't
+                // exists for the system path and this event will not create it either.


I think we need it to bump the file revision. Before the event: the file's revision is the timestamp on disk. After opening it, the revision is the document version.

github-actions · 2025-07-14T10:05:29Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

dhruvmanila · 2025-07-14T10:09:59Z

crates/ty_project/src/lib.rs

-    Indexed(files::Indexed<'a>),
+    Indexed {
+        files: files::Indexed<'a>,
+        virtual_files: Option<Box<[File]>>,


This does not seem useful at all because VS Code also sends us the textDocument/diagnostic request for open files and virtual files are always open. So, we could possibly just remove this field and all should still be fine.

Hmm, that's interesting. It would certainly simplify things a bit. Can you try if skipping over the virtual files still works as expected?

I've removed this special handling for virtual files.

I tested out Zed which also supports virtual files but it seems that it doesn't send any notification to the client about it's existence.

Screen.Recording.2025-07-16.at.17.32.44.mov

Zed seems to be sending a didClose for the new other.py and then a didOpen which is what caused the error in the above video. I'm not sure why though. But, it does send a FileChangeType::Created which seems fine but then why is it sending a didClose event?

// Send: {"jsonrpc":"2.0","method":"workspace/didChangeWatchedFiles","params":{"changes":[{"uri":"file:///Users/dhruv/playground/ty-server/src/ty_server/other.py","type":1}]}} // Send: {"jsonrpc":"2.0","method":"textDocument/didClose","params":{"textDocument":{"uri":"file:///Users/dhruv/playground/ty-server/src/ty_server/other.py"}}} // Send: {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///Users/dhruv/playground/ty-server/src/ty_server/other.py","languageId":"python","version":0,"text":"x"}}}

Neovim also has virtual file support but it has it's own issues: #15392

## Summary Previously, the virtual files were being added to the default database that's present on the session. This is wrong because the default database is for any files that don't belong to any project i.e., they're outside of any projects managed by the server. Virtual files are neither part of the project nor it is outside the projects. This was not the intention as in the initial version, virtual files were being added to the only project database managed by the server. This PR fixes this by reverting back to the original behavior where virtual files will be added to the only project database present. When support for multiple workspace and project is added, this will require updating (astral-sh/ty#794). This is required for #19264 because workspace diagnostics doesn't check the default project database yet. Ideally, the default db should be checked as well. The implementation of this PR means that virtual files are now being included for workspace diagnostics but it doesn't work completely e.g., if I save an untitled file the diagnostics disappears but it doesn't appear back for the (now) saved file on disk as shown in the following video demonstration: https://github.com/user-attachments/assets/123e8d20-1e95-4c7d-b7eb-eb65be8c476e

MichaReiser

Thank you. I think the should_check_file implementation is currently incorrect as it also returns true for third party files, which we don't want. It might be worth adding a CLI test for it.

I also think that we should remove the implicit behavior where setting open_files to Some changes the check mode. I think this is confusing now where we also have an explicit check mode. Instead, check should always do whatever is configured by CheckMode. This may require updating the benchmarks and ty_wasm to explicitly set CheckMode

crates/ty_project/src/lib.rs

MichaReiser · 2025-07-15T09:36:22Z

crates/ty_project/src/lib.rs

-    Indexed(files::Indexed<'a>),
+    Indexed {
+        files: files::Indexed<'a>,
+        virtual_files: Option<Box<[File]>>,


Hmm, that's interesting. It would certainly simplify things a bit. Can you try if skipping over the virtual files still works as expected?

crates/ty_server/src/server/api/notifications/did_open.rs

crates/ty_project/src/lib.rs

dhruvmanila · 2025-07-16T11:59:04Z

Post review changes

Update should_check_file to check whether the file exists in the indexed file set in AllFiles check mode. This will avoid any third party files to be checked in this mode
Remove the check_mode field from the ProjectMetadata, instead expose a method set_check_mode from ProjectDatabase that updates the Project's check mode
Update the default check mode to be AllFiles and update the server code to set the check mode as per the diagnostic mode, update the wasm and benchmark code to always use the OpenFiles check mode
Update the logic to check if a file is open or not to only check whether it's explicitly in the open file set and avoid checking the indexed file set
Remove virtual file handling in ProjectFiles as it isn't required because editors send a textDocument/diagnostic request for open files and virtual files are always open
Use ChangeEvent::Created in the didOpen handler for system path to notify the Project that there might be a new file that's added in the project so the Indexed file set should be updated accordingly

crates/ty_project/src/lib.rs

crates/ty_server/src/server/api/notifications/did_open.rs

- Update `should_check_file` to check whether the file exists in the indexed file set in `AllFiles` check mode. This will avoid any third party files to be checked in this mode - Remove the `check_mode` field from the `ProjectMetadata`, instead expose a method `set_check_mode` from `ProjectDatabase` that updates the `Project`'s check mode - Update the default check mode to be `AllFiles` and update the server code to set the check mode as per the diagnostic mode, update the wasm and benchmark code to always use the `OpenFiles` check mode - Update the logic to check if a file is open or not to only check whether it's explicitly in the open file set and avoid checking the indexed file set - Expand comments

crates/ty_server/src/server/api/notifications/did_open.rs

dhruvmanila · 2025-07-18T09:56:29Z

Test cases

Open a virtual file, save it as system file
Delete the system file while it's open
Delete the system file while it's open but the editor focus is in a different file
Add an import that references a non-existant file, create a virtual file and save it with the same name as the import references

Screen.Recording.2025-07-18.at.15.24.30.mov

MichaReiser

Uff, that was tricky. Thank you for seeing this through!

dhruvmanila added server Related to the LSP server ty Multi-file analysis & type inference labels Jul 10, 2025

dhruvmanila force-pushed the dhruv/workspace-diagnostics-in-virtual-files branch 3 times, most recently from 6f768c0 to 1f21e75 Compare July 10, 2025 16:45

MichaReiser reviewed Jul 10, 2025

View reviewed changes

crates/ty_project/src/lib.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Jul 10, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

dhruvmanila force-pushed the dhruv/workspace-diagnostics-in-virtual-files branch 2 times, most recently from 1cc96b9 to bf3212e Compare July 12, 2025 03:17

This was referenced Jul 14, 2025

[ty] Use python version and path from Python extension #19012

Merged

[ty] Add virtual files to the only project database #19322

Merged

dhruvmanila force-pushed the dhruv/workspace-diagnostics-in-virtual-files branch from bf3212e to 8d5ef98 Compare July 14, 2025 08:48

dhruvmanila changed the base branch from main to dhruv/virtual-files-in-project-db July 14, 2025 08:48

dhruvmanila changed the title ~~[ty] Consider virtual files for workspace diagnostics~~ [ty] Track open files in the server Jul 14, 2025

dhruvmanila commented Jul 14, 2025

View reviewed changes

dhruvmanila force-pushed the dhruv/virtual-files-in-project-db branch from c14535c to 993d565 Compare July 14, 2025 13:57

dhruvmanila force-pushed the dhruv/workspace-diagnostics-in-virtual-files branch from 854decd to e649866 Compare July 14, 2025 14:04

Base automatically changed from dhruv/virtual-files-in-project-db to main July 14, 2025 14:47

dhruvmanila force-pushed the dhruv/workspace-diagnostics-in-virtual-files branch from e649866 to 93a44b3 Compare July 14, 2025 14:48

dhruvmanila marked this pull request as ready for review July 15, 2025 05:39

dhruvmanila requested review from AlexWaygood, carljm, dcreager and sharkdp as code owners July 15, 2025 05:39

dhruvmanila added the internal An internal refactor or improvement label Jul 15, 2025

AlexWaygood removed their request for review July 15, 2025 07:17

MichaReiser reviewed Jul 15, 2025

View reviewed changes

sharkdp removed their request for review July 16, 2025 06:10

dhruvmanila requested a review from MichaReiser July 16, 2025 12:14

MichaReiser reviewed Jul 17, 2025

View reviewed changes

dhruvmanila added 7 commits July 17, 2025 14:06

[ty] Consider virtual files for workspace diagnostics

0c1409f

Fix docs

6729f40

Include file error in trace logs

3b0a0d1

Remove virtual file special handling from ProjectFiles

912fc64

Use Created change event in didOpen handler

edc8573

Remove Option<Arc<...>> from open file set

b4d826b

dhruvmanila force-pushed the dhruv/workspace-diagnostics-in-virtual-files branch from 398bbb7 to b4d826b Compare July 17, 2025 15:03

dhruvmanila added 2 commits July 18, 2025 09:28

Apply Created/Opened change event based on path exists

49ef67e

Merge branch 'main' into dhruv/workspace-diagnostics-in-virtual-files

d7d2b2c

MichaReiser reviewed Jul 18, 2025

View reviewed changes

crates/ty_server/src/server/api/notifications/did_open.rs Outdated Show resolved Hide resolved

Check the file existence using try_system

4d0df12

dhruvmanila force-pushed the dhruv/workspace-diagnostics-in-virtual-files branch from 29831c6 to 4d0df12 Compare July 18, 2025 09:45

dhruvmanila requested a review from MichaReiser July 18, 2025 10:03

MichaReiser approved these changes Jul 18, 2025

View reviewed changes

dhruvmanila merged commit 99d0ac6 into main Jul 18, 2025
37 checks passed

dhruvmanila deleted the dhruv/workspace-diagnostics-in-virtual-files branch July 18, 2025 14:03

		// TODO: Why do we require this? Because this doesn't do anything as `File` doesn't
		// exists for the system path and this event will not create it either.

Comments

Conversation

dhruvmanila commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Before

After

Uh oh!

github-actions bot commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

Uh oh!

MichaReiser Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

github-actions bot commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

dhruvmanila Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

dhruvmanila Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MichaReiser Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dhruvmanila commented Jul 16, 2025

Post review changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dhruvmanila commented Jul 18, 2025

Test cases

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dhruvmanila commented Jul 10, 2025 •

edited

Loading

github-actions bot commented Jul 10, 2025 •

edited

Loading

`mypy_primer` results

github-actions bot commented Jul 14, 2025 •

edited

Loading

`ruff-ecosystem` results