microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.87k stars 29.52k forks source link

Prefer to use `IURIIdentityService.extUri` over raw `extpath` #140970

Open bpasero opened 2 years ago

bpasero commented 2 years ago

I am tracing calls to extpath.isEqual / extpath.isEqualOrParent and found some in testing:

https://github.com/microsoft/vscode/blob/dba74b1923061e1f81aa223c756b07d18f9f89c3/src/vs/workbench/services/search/common/search.ts#L435

https://github.com/microsoft/vscode/blob/a865bed813e9cf01ebbc3c6ee3a05d672bb366f3/src/vs/workbench/services/search/node/fileSearch.ts#L94

https://github.com/microsoft/vscode/blob/f74ad40a3e98ebd8f2a70df093560e835d92cdaa/src/vs/workbench/services/search/worker/localFileSearch.ts#L315

That seem to compare with fsPath from existing URIs. We have better tools to compare URIs for this: IURIIdentityService.extUri which also include support for ignoring path casing based on the platform.

roblourens commented 2 years ago

The third one is @JacksonKearl

roblourens commented 2 years ago

@bpasero Is there no IUriIdentityService in the ext host? NativeExtHostSearch depends on UNKNOWN service IUriIdentityService.

bpasero commented 2 years ago

Probably not, @jrieken might know. I think the service is only present in the workbench because it depends on registered file system providers. Though the extension registered ones should actually be available in the extension host.

JacksonKearl commented 2 years ago

Removed uses in the search worker files. Had to pass in the path sensitivity from the outside and create a new extUri from it in the worker.

jrieken commented 2 years ago

In the extension host you can use ExtHostFileSystemInfo#extUri but there is this a bigger "but". You should generally not be required to use IURIIdentityService.extUri, the default extUri-util should be sufficient in 99% of the cases.

This is why: all places that feed uris into the system, like receiving diagnostics from extensions, open files, and (maybe?) search results must use IUriIdentityService#asCanonicalUri. That function guarantees that only one valid form of a uri is used and in consequence the default and simpler/faster extUri-util can be used.

So, can we take a detailed look at what kind of uris we are dealing with? Does the extension host feed uris into the system or is it some mainThreadSearch-type (which should use asCanonicalUri). Are these uris that users type and therefore should also be made canonical?

JacksonKearl commented 2 years ago

In my case I'm forming the URI's from URI.file while walking through the FileSystemDirectoryHandle's in the local file search worker (the web version of ripgrep). https://github.com/microsoft/vscode/blob/236a7162831248af4981dcfa2dc06223effd6640/src/vs/workbench/services/search/worker/localFileSearch.ts#L193

jrieken commented 2 years ago

And is that done to feed the search results into vscode or an internal thing to know in what folders to search? Also, this is done in an entirely different context, like a separate web worker, right?

JacksonKearl commented 2 years ago

Yes this is all in the web worker's context, this is for filtering the directories we walk based on include/excludes. We create the URI's like this then send them over to the main thread for actual rendering.