ruff

ruff copied to clipboard

Summary

This PR is mainly a renaming PR to address the confusion around the existing FileSystem trait.

Today, Red Knot uses a FileSystem trait to abstract the system-dependent file-level operations. They're system-dependent because reading a file differs between the CLI (always goes to disk), the LSP (may return unsaved content), and WASM (has no file system). So far, this makes sense. However, it all became very confusing when we introduced the VendoredFileSystem, which doesn't implement FileSystem because the logic isn't system-dependent. Still, its name suggests that it should implement the FileSystem trait (it is a filesystem-like interface, after all).

This PR renames the FileSystem trait to System (and VfsPath::FileSystem to VfsPath::System, FileSystemPath to SystemPath, and FileSystemPathBuf to SystemPathBuf) to make the distinction more clear. The trait isn't about abstracting a way a file-system like API; it's solely for abstracting system-dependent logic, which, for now, is mainly IO-related, but I can envision more:

• Reading a file as a notebook rather than a string
• Getting information about the runtime environment: CLI/LSP (OS, architecture, etc), WASM: wasm engine in addition to the OS architecture, etc
• Setting up watch tasks (I'm not 100% sure about this one yet, mainly because it requires tight integration into the host's main loop and, therefore, might not be possible to abstract away)
• Traversing a directory
• Getting the current working directory (again, WASM doesn't know the concept of current working directory)
• Getting the configuration directory

The second rename that I've done as part of this PR is to rename Vfs to Files, VfsFile to File, and VfsPath to FilePath. While it is correct that Files emulates a virtual file system by supporting both vendors and system paths, it seemed to me that it over-emphasizes that fact. As far as all upstream code is concerned, these are just files that Red Knot can operate on. That's why I opted for a simpler name.

Should VendoredFileSystem and System have a shared ReadOnlyFileSystem trait?

I mulled over this for a long time. It is very intriguing: Both implement a file system-like API. However, I’m concluding that they should not have a shared base trait because:

• We have no use case today for a generic operation over a file system (either using impl or dyn). Such an operation would have the form of fs.read_to_string(path) where fs can either be vendored or system.
• Even if the two have a shared base trait, they couldn’t be used interchangeably because VendoredFileSystem uses VendoredPath and System uses SystemPath. We could remove the distinction between VendoredPath and SystemPath, but I think this distinction is more valuable than having a shared base trait. A shared base trait would widen the API of VendoredFileSystem because it suddenly must support FileType::Symlink and permissions.

I’m open to reconsidering introducing a shared base trait when we have a use case for it.

Exposing the VendoredFileSystem

Today, VendoredFileSystem is a field on Files (Vfs). This PR introduces a new vendored method on the Db. A trait method is necessary because the ruff_db function source_text and the Files struct operate on vendored files. but the vendored typeshed stubs only ship with red_knot_module_resolver.

For tests, I decided that each test stores its own VendoredFileSystem instance. This allows replacing the VendoredFileSystem with an ad-hoc built zip file and also guarantees full isolation between tests.

Should Db implement System?

An alternative to what's done in this PR is to remove the system method on Db and instead make Db extend System. The main advantage is that it removes one additional dynamic-dispatch (system() returns dyn System).

I opted for keeping the function because:

• It's not entirely clear to me yet if we'll have code that needs to be generic over system that runs before constructing Program.
• It's easier to share code between the different runtime environments. We can have a single Program that takes a &dyn System as argument instead of having a custom Program for each environment (although we may still want to do that, this is unclear to me)

Overall, I think this can easily be changed later if we decide that Db extending System is the right way.

Should it be named Host?

An alternative to System is Host. I prefer System but don't have a strong reasoning for it. I'm open to renaming it to Host (or any other name)` if someone feels strongly. I prefer not to rename it if we all think both are about as good, just to safe some time.

Test Plan

cargo test