Skip to content

[WIP] feat(diagnostics): ability to ignore some codes #172

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

Closed
wants to merge 2 commits into from
Closed

[WIP] feat(diagnostics): ability to ignore some codes #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1529af
feat(diagnostics): ability to ignore some codes
pyrho Nov 18, 2020
c1666a2
fixup! feat(diagnostics): ability to ignore some codes
pyrho Nov 20, 2020
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
4 changes: 3 additions & 1 deletion server/src/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const program = new Command('typescript-language-server')
.option('--tsserver-log-verbosity <tsserverLogVerbosity>', 'Specify a tsserver log verbosity (terse, normal, verbose). Defaults to `normal`.' +
' example: --tsserver-log-verbosity verbose')
.option('--tsserver-path <path>', `Specify path to tsserver. example: --tsserver-path=${getTsserverExecutable()}`)
.option('--ignored-diagnostic-codes <code...>', 'Specify a diagnostic codes to be ignored. example: --ignored-diagnostic-codes 1337 42')
.parse(process.argv);

if (!(program.stdio || program.socket || program.nodeIpc)) {
Expand All @@ -45,5 +46,6 @@ createLspConnection({
tsserverPath: program.tsserverPath as string,
tsserverLogFile: program.tsserverLogFile as string,
tsserverLogVerbosity: program.tsserverLogVerbosity as string,
showMessageLevel: logLevel as lsp.MessageType
showMessageLevel: logLevel as lsp.MessageType,
ignoredDiagnosticCodes: program.ignoredDiagnosticCodes || [] as number[],
}).listen();
11 changes: 8 additions & 3 deletions server/src/diagnostic-queue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,18 +49,23 @@ export class DiagnosticEventQueue {
constructor(
protected readonly publishDiagnostics: (params: lsp.PublishDiagnosticsParams) => void,
protected readonly documents: LspDocuments,
protected readonly logger: Logger
protected readonly logger: Logger,
protected readonly ignoredDiagnosticCodes: number[],
) { }

updateDiagnostics(kind: EventTypes, event: tsp.DiagnosticEvent): void {
if (!event.body) {
this.logger.error(`Received empty ${event.event} diagnostics.`)
return;
}
const { file } = event.body;
const { file, diagnostics: diagnosticsFromServer } = event.body;

// `d.code` can be undefined
const isDiagnosticAllowed = (d: tsp.Diagnostic) => !d.code || this.ignoredDiagnosticCodes.indexOf(d.code) === -1;
Copy link
Member

Choose a reason for hiding this comment

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

[].includes() instead of indexOf maybe?

Copy link
Author

Choose a reason for hiding this comment

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

I used Array.every in my first version but that's why it wasn't working in neovim.

/lib/diagnostic-queue.js:50 const isDiagnosticAllowed = (d) => this.ignoredDiagnosticCodes.every(c => d.code !== c);
TypeError: this.ignoredDiagnosticCodes.every is not a function

I'm not sure what's going on there, maybe an older nodeJS runtime; but I figured I'd do away with the headache and just stick to es5.

Copy link
Member

Choose a reason for hiding this comment

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

Array.includes is supported since node 6 though so it won't be a problem.

Copy link
Author

Choose a reason for hiding this comment

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

This project currently targets es6 (or 2015), and it looks like Array.includes is only available for es7 (2016) compilation targets.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Then never mind me although that target should probably be updated in a separate PR. That target is very old.

Copy link
Author

Choose a reason for hiding this comment

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

I guess targeting an older standard has the added benefit of maximizing compatibility, if for some reason you're stuck with an older node version.

Copy link
Member

@rchl rchl Nov 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

But ES7 is supported from Node7 (or Node 8 if you prefer "stable" release). It would be reasonable to require at least Node 8 as that is already 3 or 4 years old.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed that targeting the current LTS version would be better.

Copy link
Collaborator

@DonnieWest DonnieWest Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'd lean toward getting the target bumped prior to this PR if either of you are interested. Based on https://nodejs.org/en/about/releases/ we should probably support Node v10 and up

Edit: Created #173 to keep track of it

Copy link
Author

Choose a reason for hiding this comment

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

I agree bumping deps and updating the target JS version is high priority, but it's unrelated to the changes in this PR and shouldn't be a blocker for merging this imo.

const filteredDiagnostics = diagnosticsFromServer.filter(isDiagnosticAllowed);
const uri = pathToUri(file, this.documents);
const diagnostics = this.diagnostics.get(uri) || new FileDiagnostics(uri, this.publishDiagnostics, this.documents);
diagnostics.update(kind, event.body.diagnostics);
diagnostics.update(kind, filteredDiagnostics);
this.diagnostics.set(uri, diagnostics);
}
}
12 changes: 10 additions & 2 deletions server/src/lsp-connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,14 @@ export interface IServerOptions {
tsserverPath: string;
tsserverLogFile?: string;
tsserverLogVerbosity?: string;
showMessageLevel: lsp.MessageType
showMessageLevel: lsp.MessageType;

/**
* Diagnostics code to be omitted in the client response
* See https://github.com/microsoft/TypeScript/blob/master/src/compiler/diagnosticMessages.json
* for a full list of valid codes
*/
ignoredDiagnosticCodes: number[];
}

export function createLspConnection(options: IServerOptions): lsp.IConnection {
Expand All @@ -28,7 +35,8 @@ export function createLspConnection(options: IServerOptions): lsp.IConnection {
lspClient,
tsserverPath: options.tsserverPath,
tsserverLogFile: options.tsserverLogFile,
tsserverLogVerbosity: options.tsserverLogVerbosity
tsserverLogVerbosity: options.tsserverLogVerbosity,
ignoredDiagnosticCodes: options.ignoredDiagnosticCodes,
});

connection.onInitialize(server.initialize.bind(server));
Expand Down
25 changes: 25 additions & 0 deletions server/src/lsp-server.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,31 @@ describe('diagnostics', () => {
await new Promise(resolve => setTimeout(resolve, 200));
assert.equal(diagnosticsForThisTest.length, 2, JSON.stringify(diagnostics));
}).timeout(10000);

it('code 6133 (ununsed variable) is ignored', async () => {
const doc = {
uri: uri('diagnosticsBar2.ts'),
languageId: 'typescript',
version: 1,
text: `
function foo() {
const x = 42;
return 1;
}
`
}
server.didOpenTextDocument({
textDocument: doc
})

server.requestDiagnostics();
await server.requestDiagnostics();
await new Promise(resolve => setTimeout(resolve, 200));
const diagnosticsForThisFile = diagnostics.filter(d => d!.uri === doc.uri);
assert.equal(diagnosticsForThisFile.length, 1, JSON.stringify(diagnostics));
const fileDiagnostics = diagnosticsForThisFile[0]!.diagnostics;
assert.equal(fileDiagnostics.length, 0);
Copy link
Author

Choose a reason for hiding this comment

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

This test passes but only because the sample TS code does not generate a diagnostic.

Copy link
Author

Choose a reason for hiding this comment

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

But in reality, a diagnostic is emitted
Screenshot 2020-11-19 at 12 03 35

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hrm, I'm not getting the same diagnostic error. I wonder what's up

But, switching the code to

x() {
    return Promise.resolve(1).then(a => a + 1);
}

Does give me a diagnostic error:

  fileDiagnostics: [
    {
      range: [Object],
      message: "Cannot find name 'x'.",
      severity: 1,
      code: 2304,
      source: 'typescript',
      relatedInformation: undefined
    }
  ]
}

Which I can ignore by setting ignoredDiagnosticCodes: [2304], and that makes the tests pass

Copy link
Author

Choose a reason for hiding this comment

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

I'll edit the test for this then.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe it has to do with the typescript version in use, or the target; I'm unclear as to which settings are used in the tests.

Copy link
Author

Choose a reason for hiding this comment

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

Pushed the test change on this fixup commit

}).timeout(10000);
});

describe('document symbol', () => {
Expand Down
10 changes: 9 additions & 1 deletion server/src/lsp-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@ export interface IServerOptions {
tsserverLogFile?: string;
tsserverLogVerbosity?: string;
lspClient: LspClient;

/**
* Diagnostics code to be omitted in the client response
* See https://github.com/microsoft/TypeScript/blob/master/src/compiler/diagnosticMessages.json
* for a full list of valid codes
*/
ignoredDiagnosticCodes: number[];
}

export class LspServer {
Expand All @@ -62,7 +69,8 @@ export class LspServer {
this.diagnosticQueue = new DiagnosticEventQueue(
diagnostics => this.options.lspClient.publishDiagnostics(diagnostics),
this.documents,
this.logger
this.logger,
this.options.ignoredDiagnosticCodes,
);
}

Expand Down
7 changes: 7 additions & 0 deletions server/src/test-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ export function lastPosition(document: lsp.TextDocumentItem, match: string): lsp
return positionAt(document, document.text.lastIndexOf(match));
}

/**
* Creates a tsserver instance for testing.
*
* Warning: The diagnostic code `6133` ("'x' is declared but its value is never read.") is ignored
* is ignored for the purpose of testing that ignoring a code works.
*/
export async function createServer(options: {
rootUri: string | null
tsserverLogVerbosity?: string
Expand All @@ -67,6 +73,7 @@ export async function createServer(options: {
applyWorkspaceEdit: () => Promise.reject(new Error('unsupported')),
rename: () => Promise.reject(new Error('unsupported'))
},
ignoredDiagnosticCodes: [6133],
});

await server.initialize({
Expand Down