Manage information diagnostic severity on the client-side. #7984

JoeRobich · 2025-02-10T22:47:06Z

A server-side change is being made to report diagnostics with their reported severity (Return LSP diagnostics with their reported severity. roslyn#77145).
This adds client-side middleware to allow users who prefer to not see information diagnostics in their editor have those diagnostics be reported as hint.
Middleware required updated vscode-languageclient packages which caused many imported to be updated due changes in what types are exported from which packages.

Resolves #6723

- A server-side change is being made to report diagnostics with their reported severity. - This adds client-side middleware to allow users who prefer to not see information diagnostics in their editor have those diagnostics be reproted as hint.

JoeRobich · 2025-02-10T22:49:26Z

src/lsptoolshost/autoInsert/onAutoInsertFeature.ts

        }
    }
-    dispose(): void {
+    clear(): void {


JoeRobich · 2025-02-10T22:51:48Z

src/lsptoolshost/diagnostics/fixAllCodeAction.ts

                if (response.edit) {
                    const uriConverter: URIConverter = (value: string): vscode.Uri => UriConverter.deserialize(value);
-                    const protocolConverter = createConverter(uriConverter, true, true);
+                    const protocolConverter = createConverter(uriConverter, true, true, true);


createConverter added support for Theme Icons

JoeRobich · 2025-02-10T22:52:49Z

src/lsptoolshost/server/roslynLanguageClient.ts

-    MessageSignature,
-    ServerOptions,
-} from 'vscode-languageclient/node';
+import { CancellationToken, MessageSignature } from 'vscode-jsonrpc';


Lots of places where types are only being exported from their implementation package.

JoeRobich · 2025-02-10T22:54:06Z

src/lsptoolshost/utils/combineDocumentSelectors.ts

+export function combineDocumentSelectors(
+    ...selectors: (DocumentSelector | vscode.DocumentSelector)[]
+): vscode.DocumentSelector {
+    return selectors.reduce<(string | vscode.DocumentFilter)[]>((acc, selector) => acc.concat(<any>selector), []);


DocumentSelector types do not quite match up anymore. I expect this will be resolved in a future "@types/vscode" package update.

Shouldn't we be using the vscode document selector types here? And not the LSP versions?

we weren't previously but that doesn't mean that we can't. Will look at it.

Long story short, we need to use both types of DocumentSelector. ClientLanguageOptions requires the LSP type and various calls require the vscode type. When vscode moves to the newer vscode-languageserver package, I expect these to become equivalent again.

JoeRobich · 2025-02-10T22:55:16Z

src/shared/options.ts

    'componentPaths',
    'enableXamlTools',
    'useServerGC',
+    'reportInformationAsHint',


Changing 'reportInformationAsHint' will prompt for the user to reload the workspace

JoeRobich · 2025-02-10T22:56:19Z

tsconfig.json

        "lib": [
            "ES2021"
        ],
+        "paths": {


Exports have changed in these packages. I think if we were able to use a different module resolution behavior they would not be necessary. Added alias paths here and in webpack config.

src/lsptoolshost/server/roslynLanguageServer.ts

For VSCode we were adjusting information diagnostics severity so that they would be reported as hints. We are moving this behavior to the client and allowing users to opt-in. Client-side PR: dotnet/vscode-csharp#7984 Part of dotnet/vscode-csharp#6723

dibarbet

fine with the majority of the changes - but definitely think we should avoid changing the default for now here.

package.json

dibarbet · 2025-02-11T20:37:14Z

src/lsptoolshost/extensions/roslynLanguageServerExportChannel.ts

 *--------------------------------------------------------------------------------------------*/
 import * as vscode from 'vscode';
-import { RequestType } from 'vscode-languageclient/node';
+import { RequestType } from 'vscode-jsonrpc';


What happened here - did types get moved around? Would have thought we would be using the language client API

So these packages are layered dependencies. Packages higher up are not re-exporting all these types from those beneath anymore.

dibarbet · 2025-02-11T20:39:57Z

src/lsptoolshost/utils/combineDocumentSelectors.ts

+export function combineDocumentSelectors(
+    ...selectors: (DocumentSelector | vscode.DocumentSelector)[]
+): vscode.DocumentSelector {
+    return selectors.reduce<(string | vscode.DocumentFilter)[]>((acc, selector) => acc.concat(<any>selector), []);


Shouldn't we be using the vscode document selector types here? And not the LSP versions?

dibarbet · 2025-02-11T20:44:31Z

package.json

-    "vscode-languageclient": "8.2.0-next.1",
-    "vscode-languageserver-protocol": "3.17.4-next.1",
-    "vscode-languageserver-textdocument": "^1.0.5",
+    "vscode-jsonrpc": "9.0.0-next.7",


I think the new server package version is available now, we should probably include it in this PR

I also need to update the diagnostic integration tests for this change.

…avior.

TLDR: This was always wrong. The spec doesn't say anything, but VS Code always threw in converting these diagnostics (silently) and VS seems to do the same. VS Code had a lot of version bumps in this change dotnet/vscode-csharp#7984 Unfortunately, that changed vscode-languageclient from 8.2.0-next.1 to 10.0.0-next.14. Functionally this wasn't a problem, but it did add a sneaky line here to helpfully let extension developers know they are doing something wrong very loudly microsoft/vscode-languageserver-node@release/client/8.2.0-next.1...release/client/10.0.0-next.14#diff-1442f78812 This change meant that diagnostics we sent prior that could not be converted correctly (such as a negative line number) would silently bubble up and be dropped. Now there is an error reported that matches exactly what devdiv.visualstudio.com/DevDiv/_workitems/edit/2397532?view=edit reports. From testing VS also drops these diagnostics so there's no real point in keeping them. I'd rather find bugs where a user expects diagnostics that aren't there than yell at a user for our code doing bad things.

mikes-gh · 2025-03-03T22:31:40Z

@JoeRobich Thank you for giving us the choice. Been waiting a long time to get my infos back.
I can confirm my infos are now listed in the problems pane and I can navigate to them from there.

JoeRobich requested a review from dibarbet February 10, 2025 22:47

JoeRobich requested review from a team as code owners February 10, 2025 22:47

Remove unused dev depenedency

252c0e5

JoeRobich force-pushed the dev/jorobich/report-all-diagnostics branch from 0c57903 to 252c0e5 Compare February 10, 2025 22:57

JoeRobich commented Feb 10, 2025

View reviewed changes

JoeRobich mentioned this pull request Feb 10, 2025

Return LSP diagnostics with their reported severity. dotnet/roslyn#77145

Merged

davidwengier reviewed Feb 10, 2025

View reviewed changes

src/lsptoolshost/server/roslynLanguageServer.ts Show resolved Hide resolved

Add report information diagnostic as hint middleware to Razor client

fbb88aa

dibarbet requested changes Feb 11, 2025

View reviewed changes

dibarbet reviewed Feb 11, 2025

View reviewed changes

JoeRobich and others added 5 commits February 11, 2025 16:14

Update Roslyn to 4.14.0-2.25110.11

5b9d4f5

Add integration tests for reportInformationAsHint setting

a273bb3

Update changelog

245ef1e

Change default for reportInformationAsHint to match the current beh…

80187d6

…avior.

Merge branch 'main' into dev/jorobich/report-all-diagnostics

f092266

dibarbet approved these changes Feb 21, 2025

View reviewed changes

Update Roslyn to 4.14.0-2.25120.5

d91acae

dibarbet approved these changes Feb 21, 2025

View reviewed changes

JoeRobich enabled auto-merge February 21, 2025 21:31

JoeRobich merged commit b5f32c4 into main Feb 21, 2025
16 checks passed

JoeRobich deleted the dev/jorobich/report-all-diagnostics branch February 21, 2025 22:24

ryzngard mentioned this pull request Feb 26, 2025

Don't send invalid ranges for diagnostics if they do not map dotnet/razor#11555

Merged

Manage information diagnostic severity on the client-side. #7984

Manage information diagnostic severity on the client-side. #7984

Uh oh!

Conversation

JoeRobich commented Feb 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dibarbet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikes-gh commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mikes-gh commented Mar 3, 2025 •

edited

Loading