Finalise FileSystemProvider api

[jrieken]

For April we want to finalise the FileSystemProvider API. This will be mostly simplifying things and fixing how we read/write data. This issue is a bit of a kitchen sink

• support simple readFile and writeFile since that's easy to implement and since it covers most use-cases
• support reading/writing in chunks to enable large file support in remote file systems. This will also include open/close calls and thereby make the server stateful -> Support reading/writing chunks in remote fs #41985
• simplify delete, e.g. don't differentiate between file/folder on the API level
• rename FileSystemProvider#move to FileSystemProvider#rename
• add optional function FileSystemProvider#copy
• polish enum type FileSystemProvide API should declare enums starting with 1 #35449
• 💪 define errors/error-codes
• readDir should return [name, stat] tuples, not [uri, stat] tuples
• polish file system activation event, rename to onFileSystem, e.g. onFileSystem:foo
• should file scheme providers (schemes) be registered in package.json?
• add subscribe-style-functions for file watching, e.g have a watch(resource: Uri, opts:{ recursive:boolean}): Disposable function
• support to signal that a fs is case-sensitive

API Evolution

In order prevent breakage of early adopters we work on a copy of the current proposed API. To prevent a lock-step-approach we will "version" the new API. See below

    export interface FileSystemProvider {
        // current api
    }

    export interface FileSystemProvider2 {
        _version: 1;
        // new api
    }

   export namespace workspace {
        export function registerFileSystemProvider(
          scheme: string, 
          fallback: FileSystemProvider, 
          provider: FileSystemProvider2 // <- new world
       ): Disposable;
    }

• When we make changed to the FileSystemProvider2 the _version-type will change, e.g. from 1 to 2 etc.
• The registerFileSystemProvider-function will check that the new provider implements the proper version of the latest, proposed API, e.g. _version: 2 and only then register that provider. Otherwise it will use the old, fallback, provider.

/cc @siegebell @bolinfest @hansonw

Sample

A sample, memory-based file system provider can be found here: https://github.com/Microsoft/vscode-extension-samples/blob/master/fsprovider-sample/

[IlyaBiryukov]

@jrieken How about a way to get the current SearchProvider, or the default one if none is registered? The scenario is that an extension wants to run find-in-files and get the results off the current workspace, including unsaved changes in the editor buffers.

[jrieken]

@IlyaBiryukov the SearchProvider isn't part of the FileSystemProvider anymore. Both are now separated and will ship independently.

[siegebell]

Great to see this progress!

1. Rather than readFile and writeFile, how about providing an example FSP that implements open/close/read/write/etc. in terms of readFile and writeFile?
2. Simplify delete: this does not simplify the implementation, so what's gained? Since vscode is the only client of the API, simplicity to implement should take precedence over simplicity to use.
3. Could you go into more detail on planned changes for the file system activation event?
4. Is the versioning only until the API is finalized/public?

[siegebell]

@jrieken I want to provide multiple file systems on the same scheme. But I'm not sure if this should be a feature request or if I should just implement the multiplexing myself...

1. Feature: vscode multiplexes file system providers on the scheme and on the authority/hostname. Downside: seems odd to allow different extensions to potentially register for the same scheme.
2. Extensions should each claim their own scheme and register just one provider that multiplexes filesystems (e.g. based on authority/hostname). Downside: redundant work; most of the extensions that provide file systems could end up reimplementing the same multiplexer.

Either way, vscode should (but does not currently) throw an error if registering a conflicting file system provider. The current behavior is to simply ignore subsequent registrations.

[jrieken]

Rather than readFile and writeFile, how about providing an example FSP that implements open/close/read/write/etc. in terms of readFile and writeFile?

That is the plan but raw read/write isn't part of the proposed API yet. The internal variant is ready and can be found here: https://github.com/Microsoft/vscode/blob/f373a15de059b7ed4918797172eeea224a7a939c/src/vs/platform/files/common/files.ts#L206

Simplify delete: this does not simplify the implementation, so what's gained? Since vscode is the only client of the API, simplicity to implement should take precedence over simplicity to use.

Yeah, it will make our implementation simpler ;-). Internally, we just have delete and knowing what to delete would require another stat-call. This API is more loose but allows for more freedom, e.g for something like rm -rf or using something like a trash-can.

Could you go into more detail on planned changes for the file system activation event?

Just name-polish and depending on the scheme vs scheme+authority discussion more fine grained control.

Is the versioning only until the API is finalized/public?

We only have to allow for more rapid change without carnage. Plan is to remove it end of this week.

I want to provide multiple file systems on the same scheme. But I'm not sure if this should be a feature request or if I should just implement the multiplexing myself...

Yeah, tough question. Agreeing on throwing on double registration - that is something we miss today. On the one side knowing how to handle a certain scheme should be enough, e.g. all ftp-servers talk the same protocol and the other side each authority requires special handling (creds etc)... I wonder how often extensions author will run into this?

[siegebell]

@jrieken small bug: vscode.d.ts declares watch as taking excludes, but exclude is actually passed.

[jrieken]

Thanks, fixed that

[siegebell]

@jrieken what does it mean for writeFile to be given option {create: false, overwrite: false}?

[jrieken]

The create flag means create in case the file doesn't exist and overwrite means fail when create is set but the the file exists (like wx+ in nodejs). Having both false means write to file but don't create file (like w).

I see how overwrite might be confusing because it's not clear what to overwrite - content vs file...

[siegebell]

So {create: false, overwrite: false} is equivalent to {create: false, overwrite: true}?

[siegebell]

@jrieken when will watch ever be given recursive: false?

(I'm using fb-watchman, which always does a recursive watch, unless I specify an expression to filter out results from subdirs).

[jrieken]

Yeah, we might call it for files, not just folders. Today, the driver does two things: Call watch recursive and with user-defined-excludes (like /node_modules/) for every (workspace-)folder and watch all files that are open and not inside those folders.

[siegebell]

Sure, but recursive is a no-op for files, right?

[jrieken]

Yep, it should be set to false then. So

• for files watch(Uri.file('/path/to/file.js', { recursive: false, excludes: [] }); -> changes to that file
• for folder watch(Uri.file('/path/to/folder', { recursive: true, excludes: [ **/node_modules/**, .git/**] }); -> changes inside that folder and inside folders of that folder etc
• for folder watch(Uri.file('/path/to/folder', { recursive: false, excludes: [ **/node_modules/**, .git/**] }); -> changes only inside that folder (not recursive). We currently don't make use of that, but we might.

[siegebell]

@jrieken isCaseSensitive won't work if registering a single file system provider that multiplexes over multiple file systems. Either there should be finer-grained control of what hostnames/subdirectories are case sensitive or else vscode will have to handle multiplexing on e.g. hostnames.

[jrieken]

hostnames/subdirectories are case sensitive or else vscode will have to handle multiplexing on e.g. hostnames.

Fair point. Today the option is more or less ignored, see #48258. I tend to keep the option simple and eventually support multiplexing in VS Code, e.g. accepting an object instead of a scheme for fs-providers (similar to document selectors), e.g. registerFileSystemProvider({ scheme: 'foo', authority: 'bi.ba.bum.com', path: /some/prefix' }, myProvider);

[siegebell]

@jrieken maybe add Unavailable/Busy to FileSystemError? E.g. when a remote filesystem cannot be loaded because the network is down.

[siegebell]

@jrieken It's not clear how symlinks should be handled. Do you support both symlinks to files and directories, or just files? (I.e. are FileType fields bit flags?) Should symlink directories be treated like normal directories?

[jrieken]

It's not clear how symlinks should be handled.

Yeah, the symbol is additive to the type-flag. So, be either a file, folder, xor unknown and a symlink.

maybe add Unavailable/Busy to FileSystemError? E.g. when a remote filesystem cannot be loaded because the network is down.

Yeah, I like

[siegebell]

@jrieken Given a chain of symlinks, A -> B -> ... -> foo.txt, should the stats (is-file, is-directory, mtime, etc.) be from A, B, or foo.txt?

[bolinfest]

I believe that in POSIX these flags are mutually exclusive in mode_t of struct stat. The caller effectively decides whether to traverse by using lstat() as opposed to stat().

The VS Code does not have to map 1:1 with these syscalls, but it should be explicit about what it expects.

[jrieken]

@jrieken Given a chain of symlinks, A -> B -> ... -> foo.txt, should the stats (is-file, is-directory, mtime, etc.) be from A, B, or foo.txt?

It should be the stat of foo.txt plus the information about it being a symlink. Yep, this isn't posix but tailored to our use-case. I have added doc-comment about that