Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add 'Suggestion' diagnostics #22204

Merged
2 commits merged into from
Feb 28, 2018
Merged

Add 'Suggestion' diagnostics #22204

2 commits merged into from
Feb 28, 2018

Conversation

ghost
Copy link

@ghost ghost commented Feb 27, 2018

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

@ghost ghost requested review from sheetalkamat and amcasey February 27, 2018 18:56
mhegazy
mhegazy reviewed Feb 27, 2018
View reviewed changes
src/compiler/types.ts Outdated
@@ -4017,8 +4017,14 @@ namespace ts {
export enum DiagnosticCategory {
Warning,
Error,
Info,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@DanielRosenwasser DanielRosenwasser Feb 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggession -> Suggestion

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
mhegazy reviewed Feb 27, 2018
View reviewed changes
src/server/protocol.ts Outdated
@@ -2121,7 +2135,7 @@ namespace ts.server.protocol {
text: string;

/**
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
mhegazy reviewed Feb 27, 2018
View reviewed changes
src/server/session.ts Outdated
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?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should report both unconditionally

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @aozgaa

mhegazy
mhegazy reviewed Feb 27, 2018
View reviewed changes
src/services/infoDiagnostics.ts Outdated
@@ -0,0 +1,8 @@
/* @internal */
namespace ts {
export function infoDiagnostics(sourceFile: SourceFile): Diagnostic[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider naming it compute*Diagnostics

@mhegazy
Copy link
Contributor

mhegazy commented Feb 27, 2018

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

mhegazy
mhegazy approved these changes Feb 27, 2018
View reviewed changes
Copy link
Contributor

@mhegazy mhegazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the enum name change

amcasey
amcasey approved these changes Feb 27, 2018
View reviewed changes
src/compiler/types.ts Outdated
@@ -4017,8 +4017,14 @@ namespace ts {
export enum DiagnosticCategory {
Warning,
Error,
Info,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

src/server/protocol.ts Outdated
arguments: InfoDiagnosticsSyncRequestArgs;
}

export interface InfoDiagnosticsSyncRequestArgs extends FileRequestArgs {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/server/session.ts Outdated
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?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @aozgaa

src/services/codefixes/convertToEs6Module.ts Outdated
const errorCodes = [Diagnostics.File_is_a_CommonJS_module_it_may_be_converted_to_an_ES6_module.code];
registerCodeFix({
errorCodes,
getCodeActions(context) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems much simpler. 😄

tests/cases/fourslash/javascriptModules22.ts Outdated
@@ -29,4 +29,4 @@ verify.completionListContains("name");
edit.insert("name;\nsausages.");
verify.completionListContains("eggs");
edit.insert("eggs;");
verify.noErrors();
verify.noErrors({ ignoreInfoDiagnostics: true });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@amcasey
Copy link
Member

amcasey commented Feb 28, 2018

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

@amcasey
Copy link
Member

amcasey commented Feb 28, 2018

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

@ghost
Copy link
Author

ghost commented Feb 28, 2018

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

@amcasey
Copy link
Member

amcasey commented Feb 28, 2018

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

@ghost ghost merged commit fa4619c into master Feb 28, 2018
@ghost ghost deleted the info branch February 28, 2018 19:16
@mhegazy mhegazy changed the title Add 'info' diagnostics Add 'Suggestion' diagnostics Feb 28, 2018
@mjbvz mjbvz mentioned this pull request Mar 9, 2018
Closed
@ghost ghost mentioned this pull request Apr 9, 2018
Merged
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sheetalkamat sheetalkamat Awaiting requested review from sheetalkamat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

codeFixes for things that aren't diagnostics
3 participants
@mhegazy @amcasey @DanielRosenwasser