microsoft / vscode

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

Incorrect URI casing passed to extensions on Windows after file rename #137256

Open Colengms opened 3 years ago

Colengms commented 3 years ago

Version: 1.62.2 (user setup) Commit: 3a6960b964327f0e3882ce18fcebd07ed191b316 Date: 2021-11-11T20:56:38.428Z Electron: 13.5.2 Chrome: 91.0.4472.164 Node.js: 14.16.0 V8: 9.1.269.39-electron.0 OS: Windows_NT x64 10.0.22000

On Linux, I'm seeing that a file gets a didClose/didOpen when renamed while open. On Windows, if the rename only alters the casing of the filename, no new didClose/didOpen occurs. VS Code will continue to refer to the file using the old casing. If the workspace is reloaded with the file still open, the new session continues to refer to the file using the old casing.

Our extension (C/C++) relies on VS Code referring to the same file consistently with the same URI, and that URI matching the actual case of the file in the file system. We may need to match paths not received from VS Code, with URIs received from VS Code. We can work around the issue by always going to disk to detect the file system casing before doing any comparison against a URI received from VS Code. Though, I suspect this scenario is a bug and the intention is for VS Code URI's to refer to files consistently with casing that matches the file system.

A potential fix would be to issue didClose/didOpen for files on Windows even if only casing was changed.

DanTup commented 3 years ago

There's some related discussions about this in https://github.com/microsoft/vscode/issues/121106 and https://github.com/microsoft/language-server-protocol/issues/1203 (and https://github.com/microsoft/language-server-protocol/issues/1263).

jrieken commented 2 years ago

@bpasero Are you saying that you do close and reopen the file (editor model) on windows and the API and/or LSP integration looses that somewhere further down?

bpasero commented 2 years ago

@jrieken no, there is no update to text models when the URI changes ever since you introduced canonical URIs. Since I cannot update the URI of a text model (without disposing and recreating it), there is a label update and an update to the "preferred" URI of my workbench model. I think the ask here is to transport that "preferred" URI to extensions.

jrieken commented 2 years ago

I think the ask here is to transport that "preferred" URI to extensions.

I am not seeing that being asked for but I take it as your proposal to tackle this. Tho, it would mean we have TextDocument#alternativeUris: Uri[] and I am unsure that helps to reduce confusion.

@Colengms Our point of view is that we do not "normalize" paths on case-insensitive file systems but that the first appearance of a URI defines its "canonical" form. Doing the normalization would be too expensive for us, think of an extension that delivers thousands of diagnostics or references. Our promise is that the uris of text document do not change and that "lookup function", like getDiagnostics honor different case strategies.

DanTup commented 2 years ago

Since it caught me out, it may be worth noting that VS Code is using the platform to decide if a filesystem is case-sensitive rather than the actual filesystem (see https://github.com/microsoft/vscode/issues/123660). This can result in some weird issues if your filesystem sensitivity doesn't match the assumed default.

Colengms commented 2 years ago

Hi @jrieken . We may read file paths from the file system that we then need to deliver to VS Code as URIs. For example, a ReferenceProvider will return Locations containing URIs, some of those URIs may refer to files that are currently open (with different casing). (Or, RenameProvider, WorkspaceSymbolProvider, DefinitionProvider, DeclarationProvider, or the LSP equivalents). If we deliver a URI to VS Code that does not match the casing it's using for an open file, is VS Code going to correctly match it?

jrieken commented 2 years ago

If we deliver a URI to VS Code that does not match the casing it's using for an open file, is VS Code going to correctly match it?

Yes, that's our promise. The first usage/opening of an uri defines its canonical form. Then subsequent usages will be using the canonical form as well. Eg. the editor shows file:///c:/foo/a.c and a language service returns two references like so: file:///c:/FOO/A.c and file:///c:/FOO/B.c. The first will be referring to file:///c:/foo/a.c but the latter is kept (granted it is the first usage). If later someone opens file:///c:/foo/b.c it will opened as file:///c:/FOO/B.c

dbaeumer commented 2 years ago

I had similar issue in the ESLint extension and they only occurred when I passed URIs to other libraries (in my case ESLint). As long it only was inside my code VS Code's model of keeping one canonical form of an URI stable works for me.

For passing URIs to ESLint I found out that they use the disk representation as their canonical form. So before passing it on I do a fs.realpath and use the new path if only the casing has changed (real path does some symbolic link / subst resolving as well which you might not to see happening)

Colengms commented 2 years ago

Not all of our URIs originate from VS Code. IMHO, it would be best if VS Code always provided URIs to extensions in the same canonicalized form an extension could itself generate from the file system directly, so comparisons and Maps would not break.

Short of that, could we get a notification when a URI in circulation no longer matches its canonicalized form? That would be an imperfect solution, as it would require us to also remap those URIs (to proper canonical form) as we receive them from VS Code. But it would allow us to update our Maps and other stored URIs, so subsequent comparisons and lookups would succeed. (Otherwise, I'd had to re-canonicalize all URIs in a map every time it's accessed, as I don't know when canonicalization was broken).

Since the behavior on non-Windows platforms is to close and re-open the file when renamed (as the previous canonical path is no longer valid, despite still being the same file), would it really be unacceptable to do the same across all platforms any time the canonical representation for a file has changed (even if still 'valid')?

jrieken commented 2 years ago

to do the same across all platforms any time the canonical representation for a file has changed (even if still 'valid')?

Let's make sure that we are on the same page here: For us a canonical representation never changes. It's the one form that VS Code will be using until (renderer) restarted. Also, only when we know or think that a file system is case insensitive we map different casing variants onto one canonical representation (sample in https://github.com/microsoft/vscode/issues/137256#issuecomment-971323669).

But it would allow us to update our Maps and other stored URIs, so subsequent comparisons and lookups would succeed. (Otherwise, I'd had to re-canonicalize all URIs in a map every time it's accessed, as I don't know when canonicalization was broken).

I believe what would be most helpful here is to expose (a subset of) our IExtUri-utility to extensions. It knows the case-rules for each scheme and comes with identify-related utils, like compare, isEqual, or getComparisonKey

Colengms commented 2 years ago

Hi @jrieken . That would not be particularly helpful for us, as we have native components and that would require making synchronous round-trips to the TypeScript side whenever we needed to compare a URI in native code.

Let's make sure that we are on the same page here: For us a canonical representation never changes.

I'm saying that if the file no longer resolves to the same canonical string when we processes it ourselves (in native code), directly based on the file system, then yes, the canonical representation has changed. I'm suggesting that whenever that happens, VS Code should trigger a didClose/didOpen with the new canonical path, similar to what happens on (what VS Code currently considers to be) case sensitive OSes.

dbaeumer commented 2 years ago

@Colengms IMO that will not work since the whole communication is async. It could still happen that VS Code uses a different canonical path then the language extensions.

IMO we should expose whether VS Code treats a FileSystem as case sensitive or insensitive via the FileSystem API similar to isWritableFileSystem. Then extensions could lower case URIs if the corresponding file system is case insensitive.

Colengms commented 2 years ago

Upon further consideration, maybe this is a non-issue. We should be able to address this in the extension by always converting URIs for case-insensitive files systems to all upper case (or all lower case) when we receive them from VS Code (or based directly on the file system), before putting them into maps, comparing, etc..

Some platforms support a mixture of case-sensitive and case-insensitive file systems - even potentially in the same path. If support were added for that scenario, we would need a more comprehensive solution.

jrieken commented 2 years ago

If support were added for that scenario, we would need a more comprehensive solution.

Yes, the IExtUri utility is for that, e.g extUri.getComparisonKey(someUri) knows the rules per scheme and returns a string that can be used as map key

chrisdothtml commented 2 years ago

I recently ran into this on my Mac and was getting frustrated that I couldn't fix an error even after reloading vscode.

I did, however, discover that if I delete the file (after copying its contents), then reload vscode, then re-create the file, it fixed the issue.

Would be nice if vscode offered a way to reset the info it has stored about files (or even just automatically re-populate the info about any file that gets renamed in the UI), but at least there is a workaround (in case you want to instruct users of your extension)