microsoft / vscode

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

vscode.workspace.findFiles and WorkspaceFolder.uri inconsistency regarding Windows driver letter #194692

Closed ggrossetie closed 10 months ago

ggrossetie commented 1 year ago

Does this issue occur when all extensions are disabled?: Yes

Calling .path on WorkspaceFolder.uri returns the Windows drive letter "as is" (in my case, the drive letter is uppercase). For instance:

console.log(vscode.workspace.workspaceFolders[0].uri.path) 
// /D:/a/asciidoctor-vscode/asciidoctor-vscode/test-workspace

However, vscode.workspace.findFiles returns a list of URI with the Windows drive letter as lowercase. For instance:

console.log((await vscode.workspace.findFiles('**/antora.yml')).map((uri) => uri.path).join(', ')) 
// /d:/a/asciidoctor-vscode/asciidoctor-vscode/test-workspace/antora.yml
// /d:/a/asciidoctor-vscode/asciidoctor-vscode/test-workspace/cli/antora.yml

Windows is case-insensitive but I think it would be better to return normalized URIs (or at least return a consistent value across APIs)

ggrossetie commented 1 year ago

I just noticed that vscode.Uri.joinPath() will apply lowercase on path segments (on Windows):

console.log(vscode.Uri.joinPath(workspaceUri, ['multiComponents'])) 
// d:/a/asciidoctor-vscode/asciidoctor-vscode/test-workspace/multicomponents

Is that expected?

ggrossetie commented 1 year ago

Regarding my last comment, it seems related to: https://github.com/nodejs/node-v0.x-archive/issues/7031 since VS Code is using https://github.com/microsoft/vscode-uri (which relies on posix path):

All util use posix path-math as defined by the node.js path module.

jrieken commented 1 year ago

Windows is case-insensitive but I think it would be better to return normalized URIs

The URI implementation preserves the case of the path component, a normalized variant is being offered via fsPath.

Tho, I wonder why this is needed or how this came up? @ggrossetie Do you use path to feed data into the UI or why is this an issue for you?

ggrossetie commented 1 year ago

I'm using path and I want to know if one URI is a child of another URI (i.e., path starts with). Since string comparison is case-sensitive, that's an issue. Basically, I have a list of URIs that comes from vscode.workspace.findFiles(), vscode.Uri.joinPath() and vscode.workspace.workspaceFolders[0].uri. I would expect path to preserve the case of the path component but that's not the case.

I read somewhere that paths should only be compared after calling normalize on them (https://nodejs.org/api/path.html#pathnormalizepath) that's why I was suggesting to normalize path. However, since all URIs come from the vscode API, it would be great if the case was preserved on all URI.path.

As an alternative, the URI module could provide functions to compare URIs (which will be case-insensitive on Windows).

ggrossetie commented 1 year ago

In summary, the following function does the right thing (i.e., it preserves the case of the path component):

console.log(vscode.workspace.workspaceFolders[0].uri.path) 
// /D:/a/asciidoctor-vscode/asciidoctor-vscode/test-workspace

The following function normalizes Windows drive letter to lowercase:

console.log((await vscode.workspace.findFiles('**/antora.yml')).map((uri) => uri.path)
// /d:/a/asciidoctor-vscode/asciidoctor-vscode/test-workspace/antora.yml

In my opinion, vscode.workspace.findFiles should preserve Windows driver letter case. In this case, d should be D.

The following function normalizes path segments:

console.log(vscode.Uri.joinPath(workspaceUri, ['multiComponents'])) 
// /d:/a/asciidoctor-vscode/asciidoctor-vscode/test-workspace/multicomponents

I think it would be better to preserve case on path segments.

ggrossetie commented 1 year ago

It's worth noting that I can reproduce this issue on GitHub Actions but on my local machine (where my drive is C:) I get lowercase c: when calling vscode.workspace.workspaceFolders[0].uri.path 🤔

ggrossetie commented 1 year ago

We lower-case it for some historic reason... I am not brave enough to change that TBH

https://github.com/microsoft/vscode/issues/42159#issuecomment-360791384

Are you brave enough now? 😉

ggrossetie commented 1 year ago

The following function returns the driver letter as is:

vscode.workspace.getWorkspaceFolder()

Since the Windows driver letter is already in lowercase in other parts or the API, I think it would be great if the Windows driver letter returned by vscode.workspace.workspaceFolders[0].uri.path and vscode.workspace.getWorkspaceFolder().uri.path was also in lowercase.

In the meantime, I will use a wrapper around the vscode.workspace API to lowercase Windows drive letter:

import vscode, { Uri, WorkspaceFolder } from 'vscode'
import os from 'os'

export function getWorkspaceFolder (uri: Uri): WorkspaceFolder | undefined {
  const workspaceFolder = vscode.workspace.getWorkspaceFolder(uri)
  if (workspaceFolder && os.platform() === 'win32') {
    return {
      uri: workspaceFolder.uri.with({ path: workspaceFolder.uri.path.replace(/^\/([A-Z]):.*/, (driverLetter) => driverLetter.toLowerCase()) }),
      name: workspaceFolder.name,
      index: workspaceFolder.index,
    }
  }
  return workspaceFolder
}
jrieken commented 1 year ago

@andreamah How and where does search construct its result uris? Do you use the uri identify service and do you create uri from absolute (file) paths or does rg return relative paths that you join with some uri?

jrieken commented 1 year ago

@ggrossetie Fiddling with the drive letter casing will only help you so much. All parts of the path can be in funny and expected casings and that's totally valid (wrt the OS). What we try to do is that the first appearance of an uri defines its shape and latter appearances are normalised to that. However, it only works with full uris and not containment, e.g a:/FOLDER defines the appearance and a:/Folder becomes that, however a:/Folder/Path is included by that logic

andreamah commented 1 year ago

@andreamah How and where does search construct its result uris? Do you use the uri identify service and do you create uri from absolute (file) paths or does rg return relative paths that you join with some uri?

We seem to join a path from rg to another root URI. https://github.com/microsoft/vscode/blob/af0f0398694cb1411318615383db98933ea24cf6/src/vs/workbench/services/search/node/ripgrepTextSearchEngine.ts#L256-L257

I believe that these root URIs source all the way back here: https://github.com/microsoft/vscode/blob/3a5f66460f3a81242df4beb63a9d6fd94a59d25d/src/vs/workbench/contrib/search/browser/searchView.ts#L1494

So I use contextService.getWorkspace().folders.

jrieken commented 11 months ago

@andreamah I believe this is the bug

https://github.com/microsoft/vscode/blob/d77b83519912be96325dcc12f7d681427a804dc9/src/vs/workbench/services/search/node/ripgrepTextSearchEngine.ts#L43

The cwd variable is used as rootFolder when creating RipgrepParser. Because it is created via fsPath the drive letter is lower-cased. This shouldn't be done. The simplest is to create the parser with the URI and to use URI#joinPath to create the resulting uri values here and here

ggrossetie commented 10 months ago

Thanks @andreamah and @jrieken 🤗

Brayanurbina commented 10 months ago

andreamah/issue194692

vscodenpa commented 9 months ago

This bug has been fixed in the latest release of VS Code Insiders!

@ggrossetie, you can help us out by commenting /verified if things are now working as expected.

If things still don't seem right, please ensure you're on version 1091f68d834974d78cd4f9769034b4ae4581ff2a of Insiders (today's or later - you can use Help: About in the command palette to check), and leave a comment letting us know what isn't working as expected.

Happy Coding!