Add 'Suggestion' diagnostics · Pull Request #22204 · microsoft/TypeScript

ghost · 2018-02-27T18:56:28Z

Fixes #19392

Previously we had just syntax and semantic diagnostics. This adds a third layer of "info" diagnostics. They are only a part of services, so do not impact command line compilation.

Now when the editor sends us a "geterr" request, we will send 3 responses for "syntaxDiag", "semanticDiag", and "infoDiag". (See changes to updateErrorCheck.) All return a Diagnostic[].

For use from Visual Studio, this also adds a "infoDiagnosticsSync" command. (@amcasey or would it be easier to just send it along with "semanticDiagnosticsSync"? Might impact performance as we add more info diagnostics though.)

For now there is only one info diagnostic, for a file that could be converted to commonJs. Moved the convertToEs6Module refactoring to a codefix.

CC @mjbvz

mhegazy · 2018-02-27T22:14:36Z

src/compiler/types.ts

    export enum DiagnosticCategory {
        Warning,
        Error,
+        Info,


I would call it Suggession. Info and Message are pretty much the same thing.

Suggession -> Suggestion

FWIW, Roslyn calls it Info. What is the difference between Info and Message? Is Message actually Telemetry?

Message is like description of a quick fix. user never sees that as a Diagnostic really.

This isn't quite what Roslyn calls Info, so I'm fine with either Info or Suggestion (or Hint, which appears to be the VS Code name).

I'd rather be consistent, but if we plan to have more-specific categories than Info in the future, Suggestion is fine with me.

mhegazy · 2018-02-27T22:19:36Z

src/server/protocol.ts


        /**
-         * The category of the diagnostic message, e.g. "error" vs. "warning"
+         * The category of the diagnostic message. This is the lower-case of a DiagnosticCategory member.


the generated protocol.d.ts does not have DiagnosticCategory. so I would keep the comment in there.

One other thing also is we allow extensions to set it, so it is possible that there are other values here. so a union of string literals would be too restrictive for clients.

mhegazy · 2018-02-27T22:29:55Z

src/server/session.ts

-                if (diags) {
-                    const bakedDiags = diags.map((diag) => formatDiag(file, project, diag));
-                    this.event<protocol.DiagnosticEventBody>({ file, diagnostics: bakedDiags }, "syntaxDiag");
+                // TODO: why do we check for diagnostics existence here, but in semanticCheck send unconditionally?


we should report both unconditionally

FYI @aozgaa

mhegazy · 2018-02-27T22:35:39Z

src/services/infoDiagnostics.ts

@@ -0,0 +1,8 @@
+/* @internal */
+namespace ts {
+    export function infoDiagnostics(sourceFile: SourceFile): Diagnostic[] {


consider naming it compute*Diagnostics

mhegazy · 2018-02-27T22:37:05Z

Just a few comments about the name of the enum value. @amcasey any comments?

mhegazy

With the enum name change

amcasey · 2018-02-27T23:39:03Z

src/compiler/types.ts

    export enum DiagnosticCategory {
        Warning,
        Error,
+        Info,


This isn't quite what Roslyn calls Info, so I'm fine with either Info or Suggestion (or Hint, which appears to be the VS Code name).

amcasey · 2018-02-27T23:40:32Z

src/server/protocol.ts

+        arguments: InfoDiagnosticsSyncRequestArgs;
+    }
+
+    export interface InfoDiagnosticsSyncRequestArgs extends FileRequestArgs {


Can/should interface declarations be shared across the various kinds of diagnostics?

amcasey · 2018-02-27T23:41:11Z

src/server/session.ts

-                if (diags) {
-                    const bakedDiags = diags.map((diag) => formatDiag(file, project, diag));
-                    this.event<protocol.DiagnosticEventBody>({ file, diagnostics: bakedDiags }, "syntaxDiag");
+                // TODO: why do we check for diagnostics existence here, but in semanticCheck send unconditionally?


FYI @aozgaa

amcasey · 2018-02-27T23:41:43Z

src/services/codefixes/convertToEs6Module.ts

+    const errorCodes = [Diagnostics.File_is_a_CommonJS_module_it_may_be_converted_to_an_ES6_module.code];
+    registerCodeFix({
+        errorCodes,
+        getCodeActions(context) {


This seems much simpler. 😄

amcasey · 2018-02-27T23:43:20Z

tests/cases/fourslash/javascriptModules22.ts

 verify.completionListContains("eggs");
 edit.insert("eggs;");
-verify.noErrors();
+verify.noErrors({ ignoreInfoDiagnostics: true });


Personally, I would expect noDiagnostics to mean "no diagnostics of any severity" and noErrors to mean "no diagnostics of error severity".

amcasey · 2018-02-28T00:07:34Z

I'm going to try hacking up my local VS build to confirm that I can consume this. I don't expect issues.

amcasey · 2018-02-28T01:50:45Z

I've created an internal PR that makes VS consume the new API. LGTM

ghost · 2018-02-28T16:08:20Z

@amcasey @mhegazy good to go? (Note that since name has changed to "suggestion", VS will need to change too.)

amcasey · 2018-02-28T18:16:59Z

I'm fine with either "Info" or "Suggestion" and I've already confirmed that I can consume the new API from VS.

Add 'info' diagnostics

f5ed9f3

ghost requested review from amcasey and sheetalkamat February 27, 2018 18:56

mhegazy reviewed Feb 27, 2018

View reviewed changes

mhegazy approved these changes Feb 27, 2018

View reviewed changes

amcasey approved these changes Feb 27, 2018

View reviewed changes

Code review

fb92a7e

ghost force-pushed the info branch from 65e8f56 to fb92a7e Compare February 28, 2018 16:07

ghost merged commit fa4619c into master Feb 28, 2018

ghost deleted the info branch February 28, 2018 19:16

mhegazy changed the title ~~Add 'info' diagnostics~~ Add 'Suggestion' diagnostics Feb 28, 2018

mjbvz mentioned this pull request Mar 9, 2018

Support for TypeScript's suggestion diagnostics microsoft/vscode#45378

Closed

ghost mentioned this pull request Apr 9, 2018

Rename infoCheck to suggestionCheck #23286

Merged

microsoft locked and limited conversation to collaborators Jul 31, 2018

This pull request was closed.

Conversation

ghost commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mhegazy commented Feb 27, 2018

Uh oh!

mhegazy left a comment

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!

amcasey commented Feb 28, 2018

Uh oh!

amcasey commented Feb 28, 2018

Uh oh!

ghost commented Feb 28, 2018

Uh oh!

amcasey commented Feb 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DanielRosenwasser Feb 27, 2018 •

edited

Loading