Skip to content

Add 'Suggestion' diagnostics #22204

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

Merged
2 commits merged into from
Feb 28, 2018
Merged

Add 'Suggestion' diagnostics #22204

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5ed9f3
Add 'info' diagnostics
Feb 27, 2018
fb92a7e
Code review
Feb 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3810,6 +3810,11 @@
"code": 18003
},

"File is a CommonJS module; it may be converted to an ES6 module.": {
"category": "Info",
"code": 80001
},

"Add missing 'super()' call": {
"category": "Message",
"code": 90001
Expand Down
10 changes: 4 additions & 6 deletions src/compiler/program.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,8 +227,7 @@ namespace ts {
}

export function formatDiagnostic(diagnostic: Diagnostic, host: FormatDiagnosticsHost): string {
const category = DiagnosticCategory[diagnostic.category].toLowerCase();
const errorMessage = `${category} TS${diagnostic.code}: ${flattenDiagnosticMessageText(diagnostic.messageText, host.getNewLine())}${host.getNewLine()}`;
const errorMessage = `${diagnosticCategoryName(diagnostic)} TS${diagnostic.code}: ${flattenDiagnosticMessageText(diagnostic.messageText, host.getNewLine())}${host.getNewLine()}`;

if (diagnostic.file) {
const { line, character } = getLineAndCharacterOfPosition(diagnostic.file, diagnostic.start);
Expand All @@ -254,8 +253,9 @@ namespace ts {
const ellipsis = "...";
function getCategoryFormat(category: DiagnosticCategory): string {
switch (category) {
case DiagnosticCategory.Warning: return ForegroundColorEscapeSequences.Yellow;
case DiagnosticCategory.Error: return ForegroundColorEscapeSequences.Red;
case DiagnosticCategory.Warning: return ForegroundColorEscapeSequences.Yellow;
case DiagnosticCategory.Info: return Debug.fail("Should never get an Info diagnostic on the command line.");
case DiagnosticCategory.Message: return ForegroundColorEscapeSequences.Blue;
}
}
Expand Down Expand Up @@ -337,9 +337,7 @@ namespace ts {
output += " - ";
}

const categoryColor = getCategoryFormat(diagnostic.category);
const category = DiagnosticCategory[diagnostic.category].toLowerCase();
output += formatColorAndReset(category, categoryColor);
output += formatColorAndReset(diagnosticCategoryName(diagnostic), getCategoryFormat(diagnostic.category));
output += formatColorAndReset(` TS${ diagnostic.code }: `, ForegroundColorEscapeSequences.Grey);
output += flattenDiagnosticMessageText(diagnostic.messageText, host.getNewLine());

Expand Down
6 changes: 6 additions & 0 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Message
}
/* @internal */
export function diagnosticCategoryName(d: { category: DiagnosticCategory }, lowerCase = true): string {
const name = DiagnosticCategory[d.category];
return lowerCase ? name.toLowerCase() : name;
}

export enum ModuleResolutionKind {
Classic = 1,
Expand Down
28 changes: 22 additions & 6 deletions src/harness/fourslash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,8 +506,11 @@ namespace FourSlash {
}

private getDiagnostics(fileName: string): ts.Diagnostic[] {
return ts.concatenate(this.languageService.getSyntacticDiagnostics(fileName),
this.languageService.getSemanticDiagnostics(fileName));
return [
...this.languageService.getSyntacticDiagnostics(fileName),
...this.languageService.getSemanticDiagnostics(fileName),
...this.languageService.getInfoDiagnostics(fileName),
];
}

private getAllDiagnostics(): ts.Diagnostic[] {
Expand Down Expand Up @@ -578,10 +581,11 @@ namespace FourSlash {
return "global";
}

public verifyNoErrors() {
public verifyNoErrors(options: FourSlashInterface.VerifyNoErrorsOptions) {
ts.forEachKey(this.inputFiles, fileName => {
if (!ts.isAnySupportedFileExtension(fileName)) return;
const errors = this.getDiagnostics(fileName);
let errors = this.getDiagnostics(fileName);
if (options.ignoreInfoDiagnostics) errors = errors.filter(e => e.category !== ts.DiagnosticCategory.Info);
if (errors.length) {
this.printErrorLog(/*expectErrors*/ false, errors);
const error = errors[0];
Expand Down Expand Up @@ -1246,6 +1250,10 @@ Actual: ${stringify(fullActual)}`);
this.testDiagnostics(expected, diagnostics);
}

public getInfoDiagnostics(expected: ReadonlyArray<ts.RealizedDiagnostic>): void {
this.testDiagnostics(expected, this.languageService.getInfoDiagnostics(this.activeFile.fileName));
}

private testDiagnostics(expected: ReadonlyArray<ts.RealizedDiagnostic>, diagnostics: ReadonlyArray<ts.Diagnostic>) {
assert.deepEqual(ts.realizeDiagnostics(diagnostics, ts.newLineCharacter), expected);
}
Expand Down Expand Up @@ -4138,8 +4146,8 @@ namespace FourSlashInterface {
this.state.verifyCurrentSignatureHelpIs(expected);
}

public noErrors() {
this.state.verifyNoErrors();
public noErrors(options: VerifyNoErrorsOptions = {}) {
this.state.verifyNoErrors(options);
}

public numberOfErrorsInCurrentFile(expected: number) {
Expand Down Expand Up @@ -4327,6 +4335,10 @@ namespace FourSlashInterface {
this.state.getSemanticDiagnostics(expected);
}

public getInfoDiagnostics(expected: ReadonlyArray<ts.RealizedDiagnostic>) {
this.state.getInfoDiagnostics(expected);
}

public ProjectInfo(expected: string[]) {
this.state.verifyProjectInfo(expected);
}
Expand Down Expand Up @@ -4665,4 +4677,8 @@ namespace FourSlashInterface {
source?: string;
description: string;
}

export interface VerifyNoErrorsOptions {
ignoreInfoDiagnostics?: boolean;
}
}
4 changes: 2 additions & 2 deletions src/harness/harness.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ namespace Utils {
start: diagnostic.start,
length: diagnostic.length,
messageText: ts.flattenDiagnosticMessageText(diagnostic.messageText, Harness.IO.newLine()),
category: (<any>ts).DiagnosticCategory[diagnostic.category],
category: ts.diagnosticCategoryName(diagnostic, /*lowerCase*/ false),
code: diagnostic.code
};
}
Expand Down Expand Up @@ -1376,7 +1376,7 @@ namespace Harness {
.split("\n")
.map(s => s.length > 0 && s.charAt(s.length - 1) === "\r" ? s.substr(0, s.length - 1) : s)
.filter(s => s.length > 0)
.map(s => "!!! " + ts.DiagnosticCategory[error.category].toLowerCase() + " TS" + error.code + ": " + s);
.map(s => "!!! " + ts.diagnosticCategoryName(error) + " TS" + error.code + ": " + s);
errLines.forEach(e => outputLines += (newLine() + e));
errorsReported++;

Expand Down
3 changes: 3 additions & 0 deletions src/harness/harnessLanguageService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,9 @@ namespace Harness.LanguageService {
getSemanticDiagnostics(fileName: string): ts.Diagnostic[] {
return unwrapJSONCallResult(this.shim.getSemanticDiagnostics(fileName));
}
getInfoDiagnostics(fileName: string): ts.Diagnostic[] {
return unwrapJSONCallResult(this.shim.getInfoDiagnostics(fileName));
}
getCompilerOptionsDiagnostics(): ts.Diagnostic[] {
return unwrapJSONCallResult(this.shim.getCompilerOptionsDiagnostics());
}
Expand Down
1 change: 1 addition & 0 deletions src/harness/unittests/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ namespace ts.server {
CommandNames.GeterrForProject,
CommandNames.SemanticDiagnosticsSync,
CommandNames.SyntacticDiagnosticsSync,
CommandNames.InfoDiagnosticsSync,
CommandNames.NavBar,
CommandNames.NavBarFull,
CommandNames.Navto,
Expand Down
102 changes: 87 additions & 15 deletions src/harness/unittests/tsserverProjectSystem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,12 +467,12 @@ namespace ts.projectSystem {
verifyDiagnostics(actual, []);
}

function checkErrorMessage(session: TestSession, eventName: "syntaxDiag" | "semanticDiag", diagnostics: protocol.DiagnosticEventBody) {
checkNthEvent(session, ts.server.toEvent(eventName, diagnostics), 0, /*isMostRecent*/ false);
function checkErrorMessage(session: TestSession, eventName: protocol.DiagnosticEventKind, diagnostics: protocol.DiagnosticEventBody, isMostRecent = false): void {
checkNthEvent(session, ts.server.toEvent(eventName, diagnostics), 0, isMostRecent);
}

function checkCompleteEvent(session: TestSession, numberOfCurrentEvents: number, expectedSequenceId: number) {
checkNthEvent(session, ts.server.toEvent("requestCompleted", { request_seq: expectedSequenceId }), numberOfCurrentEvents - 1, /*isMostRecent*/ true);
function checkCompleteEvent(session: TestSession, numberOfCurrentEvents: number, expectedSequenceId: number, isMostRecent = true): void {
checkNthEvent(session, ts.server.toEvent("requestCompleted", { request_seq: expectedSequenceId }), numberOfCurrentEvents - 1, isMostRecent);
}

function checkProjectUpdatedInBackgroundEvent(session: TestSession, openFiles: string[]) {
Expand Down Expand Up @@ -3076,8 +3076,13 @@ namespace ts.projectSystem {
host.runQueuedImmediateCallbacks();
assert.isFalse(hasError());
checkErrorMessage(session, "semanticDiag", { file: untitledFile, diagnostics: [] });
session.clearMessages();

host.runQueuedImmediateCallbacks(1);
assert.isFalse(hasError());
checkErrorMessage(session, "infoDiag", { file: untitledFile, diagnostics: [] });
checkCompleteEvent(session, 2, expectedSequenceId);
session.clearMessages();
}

it("has projectRoot", () => {
Expand Down Expand Up @@ -3136,6 +3141,10 @@ namespace ts.projectSystem {

host.runQueuedImmediateCallbacks();
checkErrorMessage(session, "semanticDiag", { file: app.path, diagnostics: [] });
session.clearMessages();

host.runQueuedImmediateCallbacks(1);
checkErrorMessage(session, "infoDiag", { file: app.path, diagnostics: [] });
checkCompleteEvent(session, 2, expectedSequenceId);
session.clearMessages();
}
Expand Down Expand Up @@ -3934,18 +3943,17 @@ namespace ts.projectSystem {
session.clearMessages();

host.runQueuedImmediateCallbacks();
const moduleNotFound = Diagnostics.Cannot_find_module_0;
const startOffset = file1.content.indexOf('"') + 1;
checkErrorMessage(session, "semanticDiag", {
file: file1.path, diagnostics: [{
start: { line: 1, offset: startOffset },
end: { line: 1, offset: startOffset + '"pad"'.length },
text: formatStringFromArgs(moduleNotFound.message, ["pad"]),
code: moduleNotFound.code,
category: DiagnosticCategory[moduleNotFound.category].toLowerCase(),
source: undefined
}]
file: file1.path,
diagnostics: [
createDiagnostic({ line: 1, offset: startOffset }, { line: 1, offset: startOffset + '"pad"'.length }, Diagnostics.Cannot_find_module_0, ["pad"])
],
});
session.clearMessages();

host.runQueuedImmediateCallbacks(1);
checkErrorMessage(session, "infoDiag", { file: file1.path, diagnostics: [] });
checkCompleteEvent(session, 2, expectedSequenceId);
session.clearMessages();

Expand All @@ -3966,6 +3974,63 @@ namespace ts.projectSystem {
host.runQueuedImmediateCallbacks();
checkErrorMessage(session, "semanticDiag", { file: file1.path, diagnostics: [] });
});

it("info diagnostics", () => {
const file: FileOrFolder = {
path: "/a.js",
content: 'require("b")',
};

const host = createServerHost([file]);
const session = createSession(host, { canUseEvents: true });
const service = session.getProjectService();

session.executeCommandSeq<protocol.OpenRequest>({
command: server.CommandNames.Open,
arguments: { file: file.path, fileContent: file.content },
});

checkNumberOfProjects(service, { inferredProjects: 1 });
session.clearMessages();
const expectedSequenceId = session.getNextSeq();
host.checkTimeoutQueueLengthAndRun(2);

checkProjectUpdatedInBackgroundEvent(session, [file.path]);
session.clearMessages();

session.executeCommandSeq<protocol.GeterrRequest>({
command: server.CommandNames.Geterr,
arguments: {
delay: 0,
files: [file.path],
}
});

host.checkTimeoutQueueLengthAndRun(1);

checkErrorMessage(session, "syntaxDiag", { file: file.path, diagnostics: [] }, /*isMostRecent*/ true);
session.clearMessages();

host.runQueuedImmediateCallbacks(1);

checkErrorMessage(session, "semanticDiag", { file: file.path, diagnostics: [] });
session.clearMessages();

host.runQueuedImmediateCallbacks(1);

checkErrorMessage(session, "infoDiag", {
file: file.path,
diagnostics: [
createDiagnostic({ line: 1, offset: 1 }, { line: 1, offset: 13 }, Diagnostics.File_is_a_CommonJS_module_it_may_be_converted_to_an_ES6_module)
],
});
checkCompleteEvent(session, 2, expectedSequenceId);
session.clearMessages();
});

function createDiagnostic(start: protocol.Location, end: protocol.Location, message: DiagnosticMessage, args: ReadonlyArray<string> = []): protocol.Diagnostic {
return { start, end, text: formatStringFromArgs(message.message, args), code: message.code, category: diagnosticCategoryName(message), source: undefined };
}
});

describe("tsserverProjectSystem Configure file diagnostics events", () => {
Expand Down Expand Up @@ -5154,9 +5219,15 @@ namespace ts.projectSystem {

// the semanticDiag message
host.runQueuedImmediateCallbacks();
assert.equal(host.getOutput().length, 2, "expect 2 messages");
assert.equal(host.getOutput().length, 1);
const e2 = <protocol.Event>getMessage(0);
assert.equal(e2.event, "semanticDiag");
session.clearMessages();

host.runQueuedImmediateCallbacks(1);
assert.equal(host.getOutput().length, 2);
const e3 = <protocol.Event>getMessage(0);
assert.equal(e3.event, "infoDiag");
verifyRequestCompleted(getErrId, 1);

cancellationToken.resetToken();
Expand Down Expand Up @@ -5194,6 +5265,7 @@ namespace ts.projectSystem {
return JSON.parse(server.extractMessage(host.getOutput()[n]));
}
});

it("Lower priority tasks are cancellable", () => {
const f1 = {
path: "/a/app.ts",
Expand Down Expand Up @@ -5495,7 +5567,7 @@ namespace ts.projectSystem {
}
type CalledMaps = CalledMapsWithSingleArg | CalledMapsWithFiveArgs;
function createCallsTrackingHost(host: TestServerHost) {
const calledMaps: Record<CalledMapsWithSingleArg, MultiMap<true>> & Record<CalledMapsWithFiveArgs, MultiMap<[ReadonlyArray<string>, ReadonlyArray<string>, ReadonlyArray<string>, number]>> = {
const calledMaps: Record<CalledMapsWithSingleArg, MultiMap<true>> & Record<CalledMapsWithFiveArgs, MultiMap<[ReadonlyArray<string>, ReadonlyArray<string>, ReadonlyArray<string>, number]>> = {
fileExists: setCallsTrackingWithSingleArgFn(CalledMapsWithSingleArg.fileExists),
directoryExists: setCallsTrackingWithSingleArgFn(CalledMapsWithSingleArg.directoryExists),
getDirectories: setCallsTrackingWithSingleArgFn(CalledMapsWithSingleArg.getDirectories),
Expand Down
5 changes: 4 additions & 1 deletion src/harness/virtualFileSystemWithWatch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,7 +708,10 @@ interface Array<T> {}`
}
}

runQueuedImmediateCallbacks() {
runQueuedImmediateCallbacks(checkCount?: number) {
if (checkCount !== undefined) {
assert.equal(this.immediateCallbacks.count(), checkCount);
}
this.immediateCallbacks.invoke();
}

Expand Down
Loading