Add FS implementation #80
Conversation
| ) | ||
|
|
||
| // FS is a file system abstraction. | ||
| type FS interface { |
There was a problem hiding this comment.
Given there's only one concrete implementation of this interface (which then itself wraps a more generic abstraction), I may just eliminate this entirely.
jakebailey
requested review from
EricCornelson,
andrewbranch,
gabritto and
weswigham
andrewbranch
left a comment
andrewbranch
left a comment
There was a problem hiding this comment.
What are all the Path related !!!?
| type ResolutionHost interface { | ||
| FileExists(fileName string) bool | ||
| ReadFile(fileName string) string | ||
| FS() vfs.FS |
There was a problem hiding this comment.
Just curious, why add a method instead of embed?
There was a problem hiding this comment.
That would then imply that we have to create a struct which embeds a vfs.FS value and then assign the object there in order to make something which implements ResolutionHost. Then, if you need to construct another host object that references the FS of another host, you can't just pass the previous FS by value, you'd have to pass in the entire previous interface and do an "I to I conversion". If it's an accessor like this, then all you do is grab one host's FS and then pass it as part of the next one.
But, maybe that's not the right approach, and we actually do want to construct giant hosts which implement loads of methods. We have a lot of hosts, and that may help us determine "optional" functionality by doing speculative type conversions...
There was a problem hiding this comment.
It's nice to implement these big hosts when you only need a subset of functionality, but I'm not against this change.
There was a problem hiding this comment.
Yeah, declaring that you only need an FS with ReadFile is definitely something not handled here. It's pretty easy to change this, though.
| v.readSema <- struct{}{} | ||
| defer func() { <-v.readSema }() |
There was a problem hiding this comment.
I didn’t get this far in the Go book yet 😆
There was a problem hiding this comment.
This is semaphore implemented in terms of a channel; it allows a fixed number of concurrent work. Normally this is not a problem in Go, but performance benchmarking shows that not storming the filesystem with reads improves performance and that's also what gopls does.
They're all places where I find it mega awkward that I have to convert to string and back, or where I am unsure if I even did it right. Are our path helpers designed to operate on func ConvertToRelativePath[S ~string](p S, opt ComparePathsOptions) S { ... }So that it worked with both strings and |
internal/vfs/vfs.go
Outdated
| UseCaseSensitiveFileNames() bool | ||
|
|
||
| // GetCurrentDirectory returns the current directory. | ||
| GetCurrentDirectory() string |
There was a problem hiding this comment.
I actually think I should drop these methods entirely; they only carry state which I think is actually better passed through other hosts? The values are not used internally at all. Thoughts?
There was a problem hiding this comment.
FWIW TS's system has GetCurrentDirectory, I am wondering if we should restore it just so paths are relative? But, I kinda want us to actually name a NormalizedPath type or something... It's probably ok right now.
It depends. Unfortunately, you just have to reason about whether the outputs maintain the Pathyness of the inputs. I think this example may be safe, but you have to check whether there’s any way that the |
internal/vfs/vfs.go
Outdated
| func rootLength(p string) int { | ||
| l := tspath.GetEncodedRootLength(p) | ||
| if l <= 0 { | ||
| panic("expected absolute path, got: " + p) |
There was a problem hiding this comment.
I added this assert; the FS doesn't have a CWD so cannot itself resolve paths. That's a downside of it not having a CWD...
|
I believe the PR is now fully ready for a review; I know @andrewbranch already approved it but I pushed one last change that fixed error reporting paths to now use the non-lowercased paths which I think is correct but want eyes on. |
|
Realized I did not add unit tests, will do that quick. |
This adds a filesystem implementation, wrapping
io/fs.FS.The filesystem works on "normalized paths", which in our parlance means "absolute paths, normalized to
/slashes regardless of OS". These paths look like:/home/jake/mycode/wow.tsc:/Users/Jake/project/hello.tsio/fs.FSis not a "traditional" abstraction. All paths must be/separated. Good news, that's how we already did it. But, eachfs.FSitself is already rooted and expects to be given paths relative to that root.. For example, I can doos.DirFS("/")oros.DirFS("c:/"), then pass in"home/jake/mycode/wow.ts". This poses a problem on Windows machines because there's no common root between all drives. But, good news again, we already haveGetEncodedRootLengthwhich is able to perform the splitting we need.The FS in this PR just splits paths with
GetEncodedRootLength(rejecting relative paths), and then opens the relevant FS by root, passing in the rest of the path to perform file operations.The FS also supports wrapping a single
fs.FS, which is useful for usingtesting/fstest.Map. Since this is not multi-root, paths likec:/blahare treated as/c:/blah.This FS differs from the original TS "System" interface in a few ways:
ReadFilereturns(contents string, ok bool)instead ofstring | undefined.readDirectory, which in TS is a recursive function that also performs exclude/include globs, I'm just givingWalkDir, which is the same asfs.WalkDir,filepath.WalkDir, etc. Globbing can be done externally.Realpathis implemented by hand and is a noop in the virtual side of the FS. I think my implementation is okay, but, the real thing to use would befs.Readlinkwhich is not yet available in Go.This PR also eliminates all uses of
pathandpath/filepathoutside of the VFS,tspath, tests, and code gen.