From 2c281c6f9be965b5c253cb418504f125331e2d35 Mon Sep 17 00:00:00 2001 From: auvred Date: Mon, 5 Feb 2024 09:05:54 +0300 Subject: [PATCH 1/2] fix(eslint-plugin): [ban-ts-comment] more accurate handling of multiline comments --- .../eslint-plugin/src/rules/ban-ts-comment.ts | 89 +++- .../tests/rules/ban-ts-comment.test.ts | 487 ++++++++++++++---- .../tools/generate-breaking-changes.mts | 2 +- 3 files changed, 463 insertions(+), 115 deletions(-) diff --git a/packages/eslint-plugin/src/rules/ban-ts-comment.ts b/packages/eslint-plugin/src/rules/ban-ts-comment.ts index 5c70c7b9179b..32674c1ba365 100644 --- a/packages/eslint-plugin/src/rules/ban-ts-comment.ts +++ b/packages/eslint-plugin/src/rules/ban-ts-comment.ts @@ -1,7 +1,8 @@ -import { AST_TOKEN_TYPES, type TSESLint } from '@typescript-eslint/utils'; +import type { TSESLint, TSESTree } from '@typescript-eslint/utils'; +import { AST_TOKEN_TYPES } from '@typescript-eslint/utils'; import { getSourceCode } from '@typescript-eslint/utils/eslint-utils'; -import { createRule, getStringLength } from '../util'; +import { createRule, getStringLength, nullThrows } from '../util'; type DirectiveConfig = | boolean @@ -16,7 +17,7 @@ interface Options { minimumDescriptionLength?: number; } -export const defaultMinimumDescriptionLength = 3; +const defaultMinimumDescriptionLength = 3; type MessageIds = | 'tsDirectiveComment' @@ -25,6 +26,11 @@ type MessageIds = | 'tsDirectiveCommentRequiresDescription' | 'replaceTsIgnoreWithTsExpectError'; +interface MatchedTSDirective { + directive: string; + description: string; +} + export default createRule<[Options], MessageIds>({ name: 'ban-ts-comment', meta: { @@ -44,7 +50,7 @@ export default createRule<[Options], MessageIds>({ tsDirectiveCommentDescriptionNotMatchPattern: 'The description for the "@ts-{{directive}}" directive must match the {{format}} format.', replaceTsIgnoreWithTsExpectError: - 'Replace "@ts-ignore" with "@ts-expect-error".', + 'Replace "@ts-ignore" with "@ts-expect-error" to ensure an error is actually being suppressed.', }, hasSuggestions: true, schema: [ @@ -95,14 +101,18 @@ export default createRule<[Options], MessageIds>({ }, ], create(context, [options]) { + // https://github.com/microsoft/TypeScript/blob/6f1ad5ad8bec5671f7e951a3524b62d82ec4be68/src/compiler/parser.ts#L10591 + const singleLinePragmaRegEx = + /^\/\/\/?\s*@ts-(?check|nocheck)(?.*)$/; + /* The regex used are taken from the ones used in the official TypeScript repo - - https://github.com/microsoft/TypeScript/blob/408c760fae66080104bc85c449282c2d207dfe8e/src/compiler/scanner.ts#L288-L296 + https://github.com/microsoft/TypeScript/blob/6f1ad5ad8bec5671f7e951a3524b62d82ec4be68/src/compiler/scanner.ts#L340-L348 */ const commentDirectiveRegExSingleLine = - /^\/*\s*@ts-(?expect-error|ignore|check|nocheck)(?.*)/; + /^\/*\s*@ts-(?expect-error|ignore)(?.*)/; const commentDirectiveRegExMultiLine = - /^\s*(?:\/|\*)*\s*@ts-(?expect-error|ignore|check|nocheck)(?.*)/; + /^\s*(?:\/|\*)*\s*@ts-(?expect-error|ignore)(?.*)/; const sourceCode = getSourceCode(context); const descriptionFormats = new Map(); @@ -118,21 +128,66 @@ export default createRule<[Options], MessageIds>({ } } + function execDirectiveRegEx( + regex: RegExp, + str: string, + ): MatchedTSDirective | null { + const match = regex.exec(str); + if (!match) { + return null; + } + + const { directive, description } = nullThrows( + match.groups, + 'RegExp should contain groups', + ); + return { + directive: nullThrows( + directive, + 'RegExp should contain "directive" group', + ), + description: nullThrows( + description, + 'RegExp should contain "description" group', + ), + }; + } + + function findDirectiveInComment( + comment: TSESTree.Comment, + ): MatchedTSDirective | null { + if (comment.type === AST_TOKEN_TYPES.Line) { + const matchedPragma = execDirectiveRegEx( + singleLinePragmaRegEx, + `//${comment.value}`, + ); + if (matchedPragma) { + return matchedPragma; + } + + return execDirectiveRegEx( + commentDirectiveRegExSingleLine, + comment.value, + ); + } + + const commentLines = comment.value.split('\n'); + return execDirectiveRegEx( + commentDirectiveRegExMultiLine, + commentLines[commentLines.length - 1], + ); + } + return { Program(): void { const comments = sourceCode.getAllComments(); comments.forEach(comment => { - const regExp = - comment.type === AST_TOKEN_TYPES.Line - ? commentDirectiveRegExSingleLine - : commentDirectiveRegExMultiLine; - - const match = regExp.exec(comment.value); + const match = findDirectiveInComment(comment); if (!match) { return; } - const { directive, description } = match.groups!; + const { directive, description } = match; const fullDirective = `ts-${directive}` as keyof Options; @@ -174,12 +229,10 @@ export default createRule<[Options], MessageIds>({ option === 'allow-with-description' || (typeof option === 'object' && option.descriptionFormat) ) { - const { - minimumDescriptionLength = defaultMinimumDescriptionLength, - } = options; + const { minimumDescriptionLength } = options; const format = descriptionFormats.get(fullDirective); if ( - getStringLength(description.trim()) < minimumDescriptionLength + getStringLength(description.trim()) < minimumDescriptionLength! ) { context.report({ data: { directive, minimumDescriptionLength }, 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 640f618200f8..51447bfe6130 100644 --- a/packages/eslint-plugin/tests/rules/ban-ts-comment.test.ts +++ b/packages/eslint-plugin/tests/rules/ban-ts-comment.test.ts @@ -13,6 +13,24 @@ ruleTester.run('ts-expect-error', rule, { /* @ts-expect-error running with long description in a block */ + `, + ` +/* @ts-expect-error not on the last line + */ + `, + ` +/** + * @ts-expect-error not on the last line + */ + `, + ` +/* not on the last line + * @ts-expect-error + */ + `, + ` +/* @ts-expect-error + * not on the last line */ `, { code: '// @ts-expect-error', @@ -26,6 +44,17 @@ ruleTester.run('ts-expect-error', rule, { }, ], }, + { + code: ` +/* + * @ts-expect-error here is why the error is expected */ + `, + options: [ + { + 'ts-expect-error': 'allow-with-description', + }, + ], + }, { code: '// @ts-expect-error exactly 21 characters', options: [ @@ -35,6 +64,18 @@ ruleTester.run('ts-expect-error', rule, { }, ], }, + { + code: ` +/* + * @ts-expect-error exactly 21 characters*/ + `, + options: [ + { + 'ts-expect-error': 'allow-with-description', + minimumDescriptionLength: 21, + }, + ], + }, { code: '// @ts-expect-error: TS1234 because xyz', options: [ @@ -46,6 +87,20 @@ ruleTester.run('ts-expect-error', rule, { }, ], }, + { + code: ` +/* + * @ts-expect-error: TS1234 because xyz */ + `, + options: [ + { + 'ts-expect-error': { + descriptionFormat: '^: TS\\d+ because .+$', + }, + minimumDescriptionLength: 10, + }, + ], + }, { code: noFormat`// @ts-expect-error ๐Ÿ‘จโ€๐Ÿ‘ฉโ€๐Ÿ‘งโ€๐Ÿ‘ฆ๐Ÿ‘จโ€๐Ÿ‘ฉโ€๐Ÿ‘งโ€๐Ÿ‘ฆ๐Ÿ‘จโ€๐Ÿ‘ฉโ€๐Ÿ‘งโ€๐Ÿ‘ฆ`, options: [ @@ -83,8 +138,37 @@ ruleTester.run('ts-expect-error', rule, { { code: ` /* -@ts-expect-error -*/ +@ts-expect-error */ + `, + options: [{ 'ts-expect-error': true }], + errors: [ + { + data: { directive: 'expect-error' }, + messageId: 'tsDirectiveComment', + line: 2, + column: 1, + }, + ], + }, + { + code: ` +/** on the last line + @ts-expect-error */ + `, + options: [{ 'ts-expect-error': true }], + errors: [ + { + data: { directive: 'expect-error' }, + messageId: 'tsDirectiveComment', + line: 2, + column: 1, + }, + ], + }, + { + code: ` +/** on the last line + * @ts-expect-error */ `, options: [{ 'ts-expect-error': true }], errors: [ @@ -96,6 +180,109 @@ ruleTester.run('ts-expect-error', rule, { }, ], }, + { + code: ` +/** + * @ts-expect-error: TODO */ + `, + options: [ + { + 'ts-expect-error': 'allow-with-description', + minimumDescriptionLength: 10, + }, + ], + errors: [ + { + data: { directive: 'expect-error', minimumDescriptionLength: 10 }, + messageId: 'tsDirectiveCommentRequiresDescription', + line: 2, + column: 1, + }, + ], + }, + { + code: ` +/** + * @ts-expect-error: TS1234 because xyz */ + `, + options: [ + { + 'ts-expect-error': { + descriptionFormat: '^: TS\\d+ because .+$', + }, + minimumDescriptionLength: 25, + }, + ], + errors: [ + { + data: { directive: 'expect-error', minimumDescriptionLength: 25 }, + messageId: 'tsDirectiveCommentRequiresDescription', + line: 2, + column: 1, + }, + ], + }, + { + code: ` +/** + * @ts-expect-error: TS1234 */ + `, + options: [ + { + 'ts-expect-error': { + descriptionFormat: '^: TS\\d+ because .+$', + }, + }, + ], + errors: [ + { + data: { directive: 'expect-error', format: '^: TS\\d+ because .+$' }, + messageId: 'tsDirectiveCommentDescriptionNotMatchPattern', + line: 2, + column: 1, + }, + ], + }, + { + code: ` +/** + * @ts-expect-error : TS1234 */ + `, + options: [ + { + 'ts-expect-error': { + descriptionFormat: '^: TS\\d+ because .+$', + }, + }, + ], + errors: [ + { + data: { directive: 'expect-error', format: '^: TS\\d+ because .+$' }, + messageId: 'tsDirectiveCommentDescriptionNotMatchPattern', + line: 2, + column: 1, + }, + ], + }, + { + code: ` +/** + * @ts-expect-error ๐Ÿ‘จโ€๐Ÿ‘ฉโ€๐Ÿ‘งโ€๐Ÿ‘ฆ */ + `, + options: [ + { + 'ts-expect-error': 'allow-with-description', + }, + ], + errors: [ + { + data: { directive: 'expect-error', minimumDescriptionLength: 3 }, + messageId: 'tsDirectiveCommentRequiresDescription', + line: 2, + column: 1, + }, + ], + }, { code: '/** @ts-expect-error */', options: [{ 'ts-expect-error': true }], @@ -280,6 +467,29 @@ ruleTester.run('ts-ignore', rule, { }, ], }, + ` +/* + @ts-ignore +*/ + `, + ` +/* @ts-ignore not on the last line + */ + `, + ` +/** + * @ts-ignore not on the last line + */ + `, + ` +/* not on the last line + * @ts-expect-error + */ + `, + ` +/* @ts-ignore + * not on the last line */ + `, { code: '// @ts-ignore: TS1234 because xyz', options: [ @@ -299,6 +509,52 @@ ruleTester.run('ts-ignore', rule, { }, ], }, + { + code: ` +/* + * @ts-ignore here is why the error is expected */ + `, + options: [ + { + 'ts-ignore': 'allow-with-description', + }, + ], + }, + { + code: '// @ts-ignore exactly 21 characters', + options: [ + { + 'ts-ignore': 'allow-with-description', + minimumDescriptionLength: 21, + }, + ], + }, + { + code: ` +/* + * @ts-ignore exactly 21 characters*/ + `, + options: [ + { + 'ts-ignore': 'allow-with-description', + minimumDescriptionLength: 21, + }, + ], + }, + { + code: ` +/* + * @ts-ignore: TS1234 because xyz */ + `, + options: [ + { + 'ts-ignore': { + descriptionFormat: '^: TS\\d+ because .+$', + }, + minimumDescriptionLength: 10, + }, + ], + }, ], invalid: [ { @@ -373,8 +629,7 @@ ruleTester.run('ts-ignore', rule, { { code: ` /* - @ts-ignore -*/ + @ts-ignore */ `, options: [{ 'ts-ignore': true }], errors: [ @@ -387,8 +642,53 @@ ruleTester.run('ts-ignore', rule, { messageId: 'replaceTsIgnoreWithTsExpectError', output: ` /* - @ts-expect-error -*/ + @ts-expect-error */ + `, + }, + ], + }, + ], + }, + { + code: ` +/** on the last line + @ts-ignore */ + `, + options: [{ 'ts-ignore': true }], + errors: [ + { + messageId: 'tsIgnoreInsteadOfExpectError', + line: 2, + column: 1, + suggestions: [ + { + messageId: 'replaceTsIgnoreWithTsExpectError', + output: ` +/** on the last line + @ts-expect-error */ + `, + }, + ], + }, + ], + }, + { + code: ` +/** on the last line + * @ts-ignore */ + `, + options: [{ 'ts-ignore': true }], + errors: [ + { + messageId: 'tsIgnoreInsteadOfExpectError', + line: 2, + column: 1, + suggestions: [ + { + messageId: 'replaceTsIgnoreWithTsExpectError', + output: ` +/** on the last line + * @ts-expect-error */ `, }, ], @@ -412,6 +712,64 @@ ruleTester.run('ts-ignore', rule, { }, ], }, + { + code: ` +/** + * @ts-ignore: TODO */ + `, + options: [ + { + 'ts-expect-error': 'allow-with-description', + minimumDescriptionLength: 10, + }, + ], + errors: [ + { + messageId: 'tsIgnoreInsteadOfExpectError', + line: 2, + column: 1, + suggestions: [ + { + messageId: 'replaceTsIgnoreWithTsExpectError', + output: ` +/** + * @ts-expect-error: TODO */ + `, + }, + ], + }, + ], + }, + { + code: ` +/** + * @ts-ignore: TS1234 because xyz */ + `, + options: [ + { + 'ts-expect-error': { + descriptionFormat: '^: TS\\d+ because .+$', + }, + minimumDescriptionLength: 25, + }, + ], + errors: [ + { + messageId: 'tsIgnoreInsteadOfExpectError', + line: 2, + column: 1, + suggestions: [ + { + messageId: 'replaceTsIgnoreWithTsExpectError', + output: ` +/** + * @ts-expect-error: TS1234 because xyz */ + `, + }, + ], + }, + ], + }, { code: '// @ts-ignore: Suppress next line', errors: [ @@ -623,6 +981,19 @@ ruleTester.run('ts-nocheck', rule, { }, ], }, + '//// @ts-nocheck - pragma comments may contain 2 or 3 leading slashes', + ` +/** + @ts-nocheck +*/ + `, + ` +/* + @ts-nocheck +*/ + `, + '/** @ts-nocheck */', + '/* @ts-nocheck */', ], invalid: [ { @@ -648,46 +1019,6 @@ 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: [ @@ -699,17 +1030,6 @@ ruleTester.run('ts-nocheck', rule, { }, ], }, - { - code: '/////@ts-nocheck: Suppress next line', - errors: [ - { - data: { directive: 'nocheck' }, - messageId: 'tsDirectiveComment', - line: 1, - column: 1, - }, - ], - }, { code: ` if (false) { @@ -849,31 +1169,17 @@ ruleTester.run('ts-check', rule, { }, ], }, - ], - invalid: [ { - code: '// @ts-check', + code: '//// @ts-check - pragma comments may contain 2 or 3 leading slashes', options: [{ 'ts-check': true }], - errors: [ - { - data: { directive: 'check' }, - messageId: 'tsDirectiveComment', - line: 1, - column: 1, - }, - ], }, { - code: '/* @ts-check */', + code: ` +/** + @ts-check +*/ + `, options: [{ 'ts-check': true }], - errors: [ - { - data: { directive: 'check' }, - messageId: 'tsDirectiveComment', - line: 1, - column: 1, - }, - ], }, { code: ` @@ -882,29 +1188,19 @@ ruleTester.run('ts-check', rule, { */ `, 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', + code: '/* @ts-check */', + options: [{ 'ts-check': true }], + }, + ], + invalid: [ + { + code: '// @ts-check', options: [{ 'ts-check': true }], errors: [ { @@ -916,9 +1212,8 @@ ruleTester.run('ts-check', rule, { ], }, { - code: '/////@ts-check: Suppress next line', + code: '// @ts-check: Suppress next line', options: [{ 'ts-check': true }], - errors: [ { data: { directive: 'check' }, diff --git a/packages/eslint-plugin/tools/generate-breaking-changes.mts b/packages/eslint-plugin/tools/generate-breaking-changes.mts index 05f4d8d5485a..0eb15a26617c 100644 --- a/packages/eslint-plugin/tools/generate-breaking-changes.mts +++ b/packages/eslint-plugin/tools/generate-breaking-changes.mts @@ -10,7 +10,7 @@ async function main(): Promise { { default: { default: Rules }} instead of just { default: Rules } - @ts-expect-error */ + @ts-expect-error: ^ read above */ const rules = rulesImport.default as TypeScriptESLintRules; // Annotate which rules are new since the last version From c0f500735b9e835099c7d68cb1cc6abd4224a56a Mon Sep 17 00:00:00 2001 From: auvred Date: Fri, 9 Feb 2024 09:45:05 +0300 Subject: [PATCH 2/2] chore: revert suggesstion message --- packages/eslint-plugin/src/rules/ban-ts-comment.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/ban-ts-comment.ts b/packages/eslint-plugin/src/rules/ban-ts-comment.ts index eac06d8ca7cf..b9f6cc59d4a8 100644 --- a/packages/eslint-plugin/src/rules/ban-ts-comment.ts +++ b/packages/eslint-plugin/src/rules/ban-ts-comment.ts @@ -49,7 +49,7 @@ export default createRule<[Options], MessageIds>({ tsDirectiveCommentDescriptionNotMatchPattern: 'The description for the "@ts-{{directive}}" directive must match the {{format}} format.', replaceTsIgnoreWithTsExpectError: - 'Replace "@ts-ignore" with "@ts-expect-error" to ensure an error is actually being suppressed.', + 'Replace "@ts-ignore" with "@ts-expect-error".', }, hasSuggestions: true, schema: [