Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ggrossetie opened this issue Oct 3, 2023 · 15 comments · Fixed by #200903
Assignees
Labels
author-verification-requested Issues potentially verifiable by issue author bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders uri verified Verification succeeded

Comments

@ggrossetie
Copy link

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

  • VS Code Version:
Version: 1.82.2
Commit: abd2f3db4bdb28f9e95536dfa84d8479f1eb312d
Date: 2023-09-14T05:51:20.981Z
Electron: 25.8.1
ElectronBuildId: 23779380
Chromium: 114.0.5735.289
Node.js: 18.15.0
V8: 11.4.183.29-electron.0
OS: Linux x64 5.19.0-46-generic
  • OS Version:
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=22.04
DISTRIB_CODENAME=jammy
DISTRIB_DESCRIPTION="Ubuntu 22.04.3 LTS"
PRETTY_NAME="Ubuntu 22.04.3 LTS"
NAME="Ubuntu"
VERSION_ID="22.04"
VERSION="22.04.3 LTS (Jammy Jellyfish)"
VERSION_CODENAME=jammy
ID=ubuntu
ID_LIKE=debian
UBUNTU_CODENAME=jammy

Steps to Reproduce:

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
Copy link
Author

ggrossetie commented Oct 3, 2023

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
Copy link
Author

Regarding my last comment, it seems related to: nodejs/node-v0.x-archive#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.

@bpasero bpasero assigned jrieken and andreamah and unassigned bpasero Oct 3, 2023
@jrieken
Copy link
Member

jrieken commented Oct 5, 2023

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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

#42159 (comment)

Are you brave enough now? 😉

@ggrossetie
Copy link
Author

ggrossetie commented Oct 6, 2023

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 jrieken added the uri label Oct 6, 2023
@jrieken
Copy link
Member

jrieken commented Oct 6, 2023

@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
Copy link
Member

jrieken commented Oct 6, 2023

@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
Copy link
Contributor

@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.

const uri = URI.file(path.join(this.rootFolder, matchPath));
const result = this.createTextSearchMatch(parsedLine.data, uri);

I believe that these root URIs source all the way back here:

const folderResources = this.contextService.getWorkspace().folders;

So I use contextService.getWorkspace().folders.

@jrieken jrieken added this to the December 2023 milestone Oct 23, 2023
@jrieken jrieken added the bug Issue identified by VS Code Team member as probable bug label Dec 5, 2023
@jrieken
Copy link
Member

jrieken commented Dec 5, 2023

@andreamah I believe this is the bug

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

@jrieken jrieken removed their assignment Dec 8, 2023
andreamah added a commit that referenced this issue Dec 15, 2023
andreamah added a commit that referenced this issue Dec 15, 2023
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Dec 15, 2023
@VSCodeTriageBot VSCodeTriageBot added the insiders-released Patch has been released in VS Code Insiders label Dec 15, 2023
@ggrossetie
Copy link
Author

Thanks @andreamah and @jrieken 🤗

@Brayanurbina
Copy link

andreamah/issue194692

@andreamah andreamah added the author-verification-requested Issues potentially verifiable by issue author label Jan 24, 2024
@VSCodeTriageBot
Copy link
Collaborator

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 1091f68 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!

@roblourens roblourens added the verified Verification succeeded label Jan 25, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2024
@aiday-mar aiday-mar added this to the December / January 2024 milestone Feb 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
author-verification-requested Issues potentially verifiable by issue author bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders uri verified Verification succeeded
Projects
None yet
9 participants