red-knot: Add directory support to MemoryFileSystem
#11825
Conversation
MichaReiser
changed the title
MemoryFileSystemMemoryFileSystem
MichaReiser
force-pushed
the
salsa-memory-fs-directories
branch
2 times, most recently
from
bee11e7 to
0a7a310
Compare
|
MichaReiser
force-pushed
the
salsa-source
branch
from
2fb8508 to
0b6f5b2
Compare
MichaReiser
force-pushed
the
salsa-memory-fs-directories
branch
from
0a7a310 to
c5e4ec0
Compare
| /// The implementation doesn't aim at fully capturing the behavior of a real file system. | ||
| /// The implementation intentionally doesn't support: | ||
| /// * symlinks | ||
| /// * hardlinks | ||
| /// * permissions: All files and directories have the permission 0755. | ||
| /// | ||
| /// Use a tempdir with the real file system to test these advanced file system features and complex file system behavior. |
There was a problem hiding this comment.
+1 to this approach. We should use in-memory file system for fast and easy testing of semantic analysis, not for testing filesystem behaviors.
| struct MemoryFileSystemInner { | ||
| files: FxDashMap<FileSystemPathBuf, FileData>, | ||
| by_path: RwLock<FxHashMap<Utf8PathBuf, Entry>>, |
There was a problem hiding this comment.
Why the switch from FxDashMap to an FxHashMap wrapped in a RwLock?
There was a problem hiding this comment.
Good question. I don't think it was necessary as part of this PR. I first thought I would need a BTreeMap. I'll leave it this way because I do need a BTreeMap when adding support for removing directories.
| /// Normalizes the path by removing `.` and `..` components and transform the path into an absolute path. | ||
| /// |
There was a problem hiding this comment.
I am very surprised that we have to implement such a thing ourselves instead of just using a battle-tested library method for this.
There was a problem hiding this comment.
I think it exists, it's just not stabalized. @BurntSushi knows more ;)
MichaReiser
force-pushed
the
salsa-source
branch
from
0b6f5b2 to
99cbb93
Compare
MichaReiser
force-pushed
the
salsa-memory-fs-directories
branch
from
c5e4ec0 to
671d556
Compare
MichaReiser
changed the title
MemoryFileSystemMemoryFileSystem
CodSpeed Performance ReportMerging #11825 will improve performances by 13.32%Comparing Summary
Benchmarks breakdown
|
Summary
This PR adds support for handling directories to the
MemoryFileSystem.I'm adding this now because the module resolver contains logic that branches if
pathis a directory.I used this PR to remove permissions support from the
MemoryFileSystem. I think having support for it is misleading, it gives the impression that the file system would check permissions when reading/writing which it does not and I don't plan on adding.The
MemoryFileSystemis mainly intended for testing. We should use the real file system to test more advanced or even platform specific filesystem behavior. That's the only way to get that right.Test Plan
Added tests