From 9c3c686b59b4b8fd02c479a534b5ca9b33c5ff40 Mon Sep 17 00:00:00 2001 From: Govind S Date: Mon, 12 Oct 2020 05:55:48 +0530 Subject: [PATCH] fix(eslint-plugin): [ban-ts-comment] support block comments (#2644) --- packages/eslint-plugin/README.md | 2 +- .../docs/rules/ban-ts-comment.md | 24 +- .../eslint-plugin/src/rules/ban-ts-comment.ts | 21 +- .../tests/rules/ban-ts-comment.test.ts | 208 ++++++++++++++++-- 4 files changed, 222 insertions(+), 33 deletions(-) diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 45d6ec7652c..2f6425bf902 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -100,7 +100,7 @@ Pro Tip: For larger codebases you may want to consider splitting our linting int | [`@typescript-eslint/adjacent-overload-signatures`](./docs/rules/adjacent-overload-signatures.md) | Require that member overloads be consecutive | :heavy_check_mark: | | | | [`@typescript-eslint/array-type`](./docs/rules/array-type.md) | Requires using either `T[]` or `Array` for arrays | | :wrench: | | | [`@typescript-eslint/await-thenable`](./docs/rules/await-thenable.md) | Disallows awaiting a value that is not a Thenable | :heavy_check_mark: | | :thought_balloon: | -| [`@typescript-eslint/ban-ts-comment`](./docs/rules/ban-ts-comment.md) | Bans `// @ts-` comments from being used or requires descriptions after directive | :heavy_check_mark: | | | +| [`@typescript-eslint/ban-ts-comment`](./docs/rules/ban-ts-comment.md) | Bans `@ts-` comments from being used or requires descriptions after directive | :heavy_check_mark: | | | | [`@typescript-eslint/ban-tslint-comment`](./docs/rules/ban-tslint-comment.md) | Bans `// tslint:` comments from being used | | :wrench: | | | [`@typescript-eslint/ban-types`](./docs/rules/ban-types.md) | Bans specific types from being used | :heavy_check_mark: | :wrench: | | | [`@typescript-eslint/class-literal-property-style`](./docs/rules/class-literal-property-style.md) | Ensures that literals on classes are exposed in a consistent style | | :wrench: | | diff --git a/packages/eslint-plugin/docs/rules/ban-ts-comment.md b/packages/eslint-plugin/docs/rules/ban-ts-comment.md index 2509793ebd3..bfdd4104751 100644 --- a/packages/eslint-plugin/docs/rules/ban-ts-comment.md +++ b/packages/eslint-plugin/docs/rules/ban-ts-comment.md @@ -1,4 +1,4 @@ -# Bans `// @ts-` comments from being used or requires descriptions after directive (`ban-ts-comment`) +# Bans `@ts-` comments from being used or requires descriptions after directive (`ban-ts-comment`) TypeScript provides several directive comments that can be used to alter how it processes files. Using these to suppress TypeScript Compiler Errors reduces the effectiveness of TypeScript overall. @@ -41,13 +41,19 @@ const defaultOptions: Options = { A value of `true` for a particular directive means that this rule will report if it finds any usage of said directive. -For example, with the defaults above the following patterns are considered warnings: +For example, with the defaults above the following patterns are considered warnings for single line or comment block lines: ```ts if (false) { // @ts-ignore: Unreachable code error console.log('hello'); } +if (false) { + /* + @ts-ignore: Unreachable code error + */ + console.log('hello'); +} ``` The following patterns are not warnings: @@ -63,22 +69,32 @@ if (false) { A value of `'allow-with-description'` for a particular directive means that this rule will report if it finds a directive that does not have a description following the directive (on the same line). -For example, with `{ 'ts-expect-error': 'allow-with-description' }` the following pattern is considered a warning: +For example, with `{ 'ts-expect-error': 'allow-with-description' }` the following patterns are considered a warning: ```ts if (false) { // @ts-expect-error console.log('hello'); } +if (false) { + /* @ts-expect-error */ + console.log('hello'); +} ``` -The following pattern is not a warning: +The following patterns are not a warning: ```ts if (false) { // @ts-expect-error: Unreachable code error console.log('hello'); } +if (false) { + /* + @ts-expect-error: Unreachable code error + */ + console.log('hello'); +} ``` ### `minimumDescriptionLength` diff --git a/packages/eslint-plugin/src/rules/ban-ts-comment.ts b/packages/eslint-plugin/src/rules/ban-ts-comment.ts index f8c68841598..7c2f237cd4c 100644 --- a/packages/eslint-plugin/src/rules/ban-ts-comment.ts +++ b/packages/eslint-plugin/src/rules/ban-ts-comment.ts @@ -21,15 +21,15 @@ export default util.createRule<[Options], MessageIds>({ type: 'problem', docs: { description: - 'Bans `// @ts-` comments from being used or requires descriptions after directive', + 'Bans `@ts-` comments from being used or requires descriptions after directive', category: 'Best Practices', recommended: 'error', }, messages: { tsDirectiveComment: - 'Do not use "// @ts-{{directive}}" because it alters compilation errors.', + 'Do not use "@ts-{{directive}}" because it alters compilation errors.', tsDirectiveCommentRequiresDescription: - 'Include a description after the "// @ts-{{directive}}" directive to explain why the @ts-{{directive}} is necessary. The description must be {{minimumDescriptionLength}} characters or longer.', + 'Include a description after the "@ts-{{directive}}" directive to explain why the @ts-{{directive}} is necessary. The description must be {{minimumDescriptionLength}} characters or longer.', }, schema: [ { @@ -98,7 +98,12 @@ export default util.createRule<[Options], MessageIds>({ }, ], create(context, [options]) { - const tsCommentRegExp = /^\/*\s*@ts-(expect-error|ignore|check|nocheck)(.*)/; + /* + The regex used are taken from the ones used in the official TypeScript repo - + https://github.com/microsoft/TypeScript/blob/master/src/compiler/scanner.ts#L281-L289 + */ + const commentDirectiveRegExSingleLine = /^\/*\s*@ts-(expect-error|ignore|check|nocheck)(.*)/; + const commentDirectiveRegExMultiLine = /^\s*(?:\/|\*)*\s*@ts-(expect-error|ignore|check|nocheck)(.*)/; const sourceCode = context.getSourceCode(); return { @@ -106,12 +111,12 @@ export default util.createRule<[Options], MessageIds>({ const comments = sourceCode.getAllComments(); comments.forEach(comment => { + let regExp = commentDirectiveRegExSingleLine; + if (comment.type !== AST_TOKEN_TYPES.Line) { - return; + regExp = commentDirectiveRegExMultiLine; } - - const [, directive, description] = - tsCommentRegExp.exec(comment.value) ?? []; + const [, directive, description] = regExp.exec(comment.value) ?? []; const fullDirective = `ts-${directive}` as keyof Options; diff --git a/packages/eslint-plugin/tests/rules/ban-ts-comment.test.ts b/packages/eslint-plugin/tests/rules/ban-ts-comment.test.ts index 2d59e1b315a..994da016d5f 100644 --- a/packages/eslint-plugin/tests/rules/ban-ts-comment.test.ts +++ b/packages/eslint-plugin/tests/rules/ban-ts-comment.test.ts @@ -8,11 +8,9 @@ const ruleTester = new RuleTester({ ruleTester.run('ts-expect-error', rule, { valid: [ '// just a comment containing @ts-expect-error somewhere', - '/* @ts-expect-error */', - '/** @ts-expect-error */', ` /* -// @ts-expect-error in a block + @ts-expect-error running with long description in a block */ `, { @@ -50,6 +48,46 @@ ruleTester.run('ts-expect-error', rule, { }, ], }, + { + code: '/* @ts-expect-error */', + options: [{ 'ts-expect-error': true }], + errors: [ + { + data: { directive: 'expect-error' }, + messageId: 'tsDirectiveComment', + line: 1, + column: 1, + }, + ], + }, + { + code: ` +/* +@ts-expect-error +*/ + `, + options: [{ 'ts-expect-error': true }], + errors: [ + { + data: { directive: 'expect-error' }, + messageId: 'tsDirectiveComment', + line: 2, + column: 1, + }, + ], + }, + { + code: '/** @ts-expect-error */', + options: [{ 'ts-expect-error': true }], + errors: [ + { + data: { directive: 'expect-error' }, + messageId: 'tsDirectiveComment', + line: 1, + column: 1, + }, + ], + }, { code: '// @ts-expect-error: Suppress next line', options: [{ 'ts-expect-error': true }], @@ -130,13 +168,6 @@ if (false) { ruleTester.run('ts-ignore', rule, { valid: [ '// just a comment containing @ts-ignore somewhere', - '/* @ts-ignore */', - '/** @ts-ignore */', - ` -/* -// @ts-ignore in a block -*/ - `, { code: '// @ts-ignore', options: [{ 'ts-ignore': false }], @@ -146,6 +177,19 @@ ruleTester.run('ts-ignore', rule, { '// @ts-ignore I think that I am exempted from any need to follow the rules!', options: [{ 'ts-ignore': 'allow-with-description' }], }, + { + code: ` +/* + @ts-ignore running with long description in a block +*/ + `, + options: [ + { + 'ts-ignore': 'allow-with-description', + minimumDescriptionLength: 21, + }, + ], + }, ], invalid: [ { @@ -171,6 +215,46 @@ ruleTester.run('ts-ignore', rule, { }, ], }, + { + code: '/* @ts-ignore */', + options: [{ 'ts-ignore': true }], + errors: [ + { + data: { directive: 'ignore' }, + messageId: 'tsDirectiveComment', + line: 1, + column: 1, + }, + ], + }, + { + code: ` +/* + @ts-ignore +*/ + `, + options: [{ 'ts-ignore': true }], + errors: [ + { + data: { directive: 'ignore' }, + messageId: 'tsDirectiveComment', + line: 2, + column: 1, + }, + ], + }, + { + code: '/** @ts-ignore */', + options: [{ 'ts-ignore': true }], + errors: [ + { + data: { directive: 'ignore' }, + messageId: 'tsDirectiveComment', + line: 1, + column: 1, + }, + ], + }, { code: '// @ts-ignore: Suppress next line', errors: [ @@ -251,13 +335,6 @@ if (false) { ruleTester.run('ts-nocheck', rule, { valid: [ '// just a comment containing @ts-nocheck somewhere', - '/* @ts-nocheck */', - '/** @ts-nocheck */', - ` -/* -// @ts-nocheck in a block -*/ - `, { code: '// @ts-nocheck', options: [{ 'ts-nocheck': false }], @@ -267,6 +344,19 @@ ruleTester.run('ts-nocheck', rule, { '// @ts-nocheck no doubt, people will put nonsense here from time to time just to get the rule to stop reporting, perhaps even long messages with other nonsense in them like other // @ts-nocheck or // @ts-ignore things', options: [{ 'ts-nocheck': 'allow-with-description' }], }, + { + code: ` +/* + @ts-nocheck running with long description in a block +*/ + `, + options: [ + { + 'ts-nocheck': 'allow-with-description', + minimumDescriptionLength: 21, + }, + ], + }, ], invalid: [ { @@ -292,6 +382,46 @@ ruleTester.run('ts-nocheck', rule, { }, ], }, + { + code: '/* @ts-nocheck */', + options: [{ 'ts-nocheck': true }], + errors: [ + { + data: { directive: 'nocheck' }, + messageId: 'tsDirectiveComment', + line: 1, + column: 1, + }, + ], + }, + { + code: ` +/* + @ts-nocheck +*/ + `, + options: [{ 'ts-nocheck': true }], + errors: [ + { + data: { directive: 'nocheck' }, + messageId: 'tsDirectiveComment', + line: 2, + column: 1, + }, + ], + }, + { + code: '/** @ts-nocheck */', + options: [{ 'ts-nocheck': true }], + errors: [ + { + data: { directive: 'nocheck' }, + messageId: 'tsDirectiveComment', + line: 1, + column: 1, + }, + ], + }, { code: '// @ts-nocheck: Suppress next line', errors: [ @@ -348,11 +478,9 @@ if (false) { ruleTester.run('ts-check', rule, { valid: [ '// just a comment containing @ts-check somewhere', - '/* @ts-check */', - '/** @ts-check */', ` /* -// @ts-check in a block + @ts-check running with long description in a block */ `, { @@ -380,6 +508,46 @@ ruleTester.run('ts-check', rule, { }, ], }, + { + code: '/* @ts-check */', + options: [{ 'ts-check': true }], + errors: [ + { + data: { directive: 'check' }, + messageId: 'tsDirectiveComment', + line: 1, + column: 1, + }, + ], + }, + { + code: ` +/* + @ts-check +*/ + `, + options: [{ 'ts-check': true }], + errors: [ + { + data: { directive: 'check' }, + messageId: 'tsDirectiveComment', + line: 2, + column: 1, + }, + ], + }, + { + code: '/** @ts-check */', + options: [{ 'ts-check': true }], + errors: [ + { + data: { directive: 'check' }, + messageId: 'tsDirectiveComment', + line: 1, + column: 1, + }, + ], + }, { code: '// @ts-check: Suppress next line', options: [{ 'ts-check': true }],