languages.getDiagnostics doesn't handle case insensitivity correctly

[DanTup]

The languages.getDiagnostics API doesn't seem to take case sensitivity into account. Calling it with a different casing to that which VS Code is using (for example a path from a language server that's in the natural casing, while VS Code is using a previous casing - #121106):

console.log(`#####`);
console.log(JSON.stringify(vs.languages.getDiagnostics(vs.Uri.file(path.join(projectPath, filename)))));
console.log(JSON.stringify(vs.languages.getDiagnostics(vs.Uri.file(path.join(projectPath, filename.toUpperCase())))));
console.log(JSON.stringify(vs.languages.getDiagnostics(vs.Uri.file(path.join(projectPath, filename.toLowerCase())))));
console.log(`#####`);

Outputs:

#####
[{"severity":"Information","message":"Name source files using `lowercase_with_underscores`.","range":[{"line":0,"character":0},{"line":0,"character":0}],"source":"dart","code":"file_names"}]
[]
[]
#####

My understanding from the discussions in the related cases (#121106, #123660) is that VS Code will treat all file URIs case-insensitively when the host is macOS or Windows.

[jrieken]

is that VS Code will treat all file URIs case-insensitively when the host is macOS or Windows.

Only internally, e.g. when associating diagnostics to documents, but not in the extension host. For this specific case it will use the format as used by extensions. What we should probably do is to use the canonical representation of the URI so that diagnostics aren't spread across different casings. Tho, that will not mean that getDiagnostics can be called with "random" casings

[DanTup]

Tho, that will not mean that getDiagnostics can be called with "random" casings

What do you mean by "random" here? If a language server returns a path that is cased differently to what VS code is using (for example because VS Code was launched with code foo and the language server read the folder from disk as Foo), is it not valid for an extension to use that path in this API?

My understanding from previous discussions is that extensions and language servers were expected to handle casing "correctly" in all cases, and VS Code would do the same. If this is not the case, it really should be documented exactly what responsibilities each side has. I have many case-related bugs I'm struggling to fix because the VS Code behaviour is not well documented and is inconsistent (for example, VS Code makes assumptions about actual case-sensitivity based on platform, rather than the actual file system - #123660).

[DanTup]

Sorry, I didn't read this bit properly:

For this specific case it will use the format as used by extensions

Although I still don't think it's clear. If the user renames a file by only case, then the diagnostics I first gave to VS Code are using one casing, but my language server is now using the updated casing. If a language server should be detecting a rename of only casing and clearing the diagnostics and then sending again for the new casing, I think that should be made clear.

Right now, renaming files outside of VS code still seems to cause some weird issues (like the UI doesn't update). It's hard to make things work correctly when it's not clear who should be doing what.

Thanks!

[DanTup]

@jrieken is there anything that would prevent this working correctly in tests? I'm using latest Insiders:

Version: 1.60.0-insider
Commit: e7d7e9a9348e6a8cc8c03f877d39cb72e5dfb1ff
Date: 2021-09-01T10:39:32.559Z (1 day ago)
Electron: 13.1.8
Chrome: 91.0.4472.164
Node.js: 14.16.0
V8: 9.1.269.39-electron.0
OS: Darwin x64 20.6.0

And I see the error appear and disappear in the dev host when the test is running, but the test still see different results depending on whether I use the filename casing from before a rename versus after:

[DanTup]

Here's a repro outside of tests:

const lowercase = vs.Uri.file('/Users/danny/Desktop/dart_sample/bin/dannytest.dart');
const uppercase = vs.Uri.file('/Users/danny/Desktop/dart_sample/bin/DANNYTEST.dart');
const diagnostics = vs.languages.createDiagnosticCollection("dart");
diagnostics.set(lowercase, [new vs.Diagnostic(new vs.Range(new vs.Position(1, 1), new vs.Position(1, 1)), "lowercase error")]);
diagnostics.set(uppercase, []);
console.log(diagnostics.get(lowercase)?.length); // prints 1, expected 0

A diagnostic is produced with one casing. The file is then renamed, and then the error is fixed, so the update comes with a different casing. However it seems like the original error is hanging around. It's still not clear to me how this is intended to work. If servers need to send specific casing for case-insensitive file systems, I think it needs to be clearer where. Or if renaming a file should trigger a server to clear all diagnostics for the old casing and then re-send them with the new, I think that should also be documented.

[jrieken]

@DanTup

[DanTup]

@jrieken I thought that just meant to stable, since insiders is usually updated daily. I'm not familiar with what Insiders is building from or what branches fixes are pushed to (I usually test things in Insiders and verify they're fixed before the released) - can you clarify what does/doesn't go into Insiders builds?

Edit: I realise we're close to a release, so I presume Insiders is tracking a branch that will become a stable release right now, and this new work is in a branch that will come later. Presumably Insiders will switch to that branch once the stable release goes out?

Thanks!

[DanTup]

Works in latest insiders - thanks!

[DanTup]

@jrieken I just noticed that calling getDiagnostics now returns the URI fully in lowercase no matter what was recorded. This shouldn't be a problem but I'm not sure if it was an intended change or not (it did trip up some code I had that assumed the casing was the same as the error was recorded with).

[michelkaporin]

@jrieken observed the same what @DanTup described on Mac OS. If DiagnosticsColection.set() called with URI having different casing within its path, running .forEach over collection returns all URIs lower cased. Is it expected behaviour now?

P.S. hi to the great team 😉

[jrieken]

Yeah, that is true. The underlaying uri map lower-cases its keys... Shouldn't happen. Pushing another fix