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
languages.getDiagnostics doesn't handle case insensitivity correctly #128198
Comments
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 |
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 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). |
Sorry, I didn't read this bit properly:
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! |
@jrieken is there anything that would prevent this working correctly in tests? I'm using latest Insiders:
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: |
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 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! |
Works in latest insiders - thanks! |
@jrieken I just noticed that calling |
Yeah, that is true. The underlaying uri map lower-cases its keys... Shouldn't happen. Pushing another fix |
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):Outputs:
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.
The text was updated successfully, but these errors were encountered: