From 8cd41a8daee22f2c90a57b95debba14e922edd73 Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Sat, 11 Jan 2025 15:25:15 +0900 Subject: [PATCH 1/4] fix(eslint-plugin): [no-duplicate-type-constituents] handle nested types --- .../rules/no-duplicate-type-constituents.ts | 279 ++++++++++++------ .../no-duplicate-type-constituents.test.ts | 83 +++++- 2 files changed, 258 insertions(+), 104 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts b/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts index c6b700e27ff7..6ffb3ccdbb61 100644 --- a/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts +++ b/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts @@ -22,6 +22,13 @@ export type Options = [ export type MessageIds = 'duplicate' | 'unnecessary'; +export type UnionOrIntersection = 'Intersection' | 'Union'; + +interface DuplicatedConstituentInfo { + duplicatedPrev: TSESTree.TypeNode; + constituentNode: TSESTree.TypeNode; +} + const astIgnoreKeys = new Set(['loc', 'parent', 'range']); const isSameAstNode = (actualNode: unknown, expectedNode: unknown): boolean => { @@ -117,115 +124,192 @@ export default createRule({ const parserServices = getParserServices(context); const { sourceCode } = context; + function report( + messageId: MessageIds, + constituentNode: TSESTree.TypeNode, + data?: Record, + ): void { + const getUnionOrIntersectionToken = ( + where: 'After' | 'Before', + at: number, + ): TSESTree.Token | undefined => + sourceCode[`getTokens${where}`](constituentNode, { + filter: token => + ['&', '|'].includes(token.value) && + constituentNode.parent.range[0] <= token.range[0] && + token.range[1] <= constituentNode.parent.range[1], + }).at(at); + + const beforeUnionOrIntersectionToken = getUnionOrIntersectionToken( + 'Before', + -1, + ); + let afterUnionOrIntersectionToken: TSESTree.Token | undefined; + let bracketBeforeTokens; + let bracketAfterTokens; + if (beforeUnionOrIntersectionToken) { + bracketBeforeTokens = sourceCode.getTokensBetween( + beforeUnionOrIntersectionToken, + constituentNode, + ); + bracketAfterTokens = sourceCode.getTokensAfter(constituentNode, { + count: bracketBeforeTokens.length, + }); + } else { + afterUnionOrIntersectionToken = nullThrows( + getUnionOrIntersectionToken('After', 0), + NullThrowsReasons.MissingToken( + 'union or intersection token', + 'duplicate type constituent', + ), + ); + bracketAfterTokens = sourceCode.getTokensBetween( + constituentNode, + afterUnionOrIntersectionToken, + ); + bracketBeforeTokens = sourceCode.getTokensBefore(constituentNode, { + count: bracketAfterTokens.length, + }); + } + context.report({ + loc: { + start: constituentNode.loc.start, + end: (bracketAfterTokens.at(-1) ?? constituentNode).loc.end, + }, + node: constituentNode, + messageId, + data, + fix: fixer => + [ + beforeUnionOrIntersectionToken, + ...bracketBeforeTokens, + constituentNode, + ...bracketAfterTokens, + afterUnionOrIntersectionToken, + ].flatMap(token => (token ? fixer.remove(token) : [])), + }); + } + + function containsEveryDuplicatedConstituents( + node: TSESTree.TSIntersectionType | TSESTree.TSUnionType, + duplicationInfos: DuplicatedConstituentInfo[], + ): boolean { + const duplicatedConstituentNodes = duplicationInfos.map( + ({ constituentNode }) => constituentNode, + ); + return ( + node.types.length === duplicatedConstituentNodes.length && + node.types.every(constituentNode => + duplicatedConstituentNodes.includes(constituentNode), + ) + ); + } + + function getDuplicateTypeNodes( + unionOrIntersection: UnionOrIntersection, + constituentNode: TSESTree.TypeNode, + uniqueConstituents: TSESTree.TypeNode[], + cachedTypeMap: Map, + forEachNodeType?: (type: Type, node: TSESTree.TypeNode) => void, + ): DuplicatedConstituentInfo[] { + const type = parserServices.getTypeAtLocation(constituentNode); + if (tsutils.isIntrinsicErrorType(type)) { + return []; + } + const duplicatedPrev = + uniqueConstituents.find(ele => isSameAstNode(ele, constituentNode)) ?? + cachedTypeMap.get(type); + + if (duplicatedPrev) { + return [ + { + constituentNode, + duplicatedPrev, + }, + ]; + } + + forEachNodeType?.(type, constituentNode); + cachedTypeMap.set(type, constituentNode); + uniqueConstituents.push(constituentNode); + + if ( + (unionOrIntersection === 'Union' && + constituentNode.type === AST_NODE_TYPES.TSUnionType) || + (unionOrIntersection === 'Intersection' && + constituentNode.type === AST_NODE_TYPES.TSIntersectionType) + ) { + const result: DuplicatedConstituentInfo[] = []; + for (const constituent of constituentNode.types) { + const duplications = getDuplicateTypeNodes( + unionOrIntersection, + constituent, + uniqueConstituents, + cachedTypeMap, + forEachNodeType, + ); + result.push(...duplications); + } + if (containsEveryDuplicatedConstituents(constituentNode, result)) { + return [ + { + constituentNode, + duplicatedPrev: constituentNode, + }, + ]; + } + return result; + } + return []; + } + function checkDuplicate( node: TSESTree.TSIntersectionType | TSESTree.TSUnionType, forEachNodeType?: ( constituentNodeType: Type, - report: (messageId: MessageIds) => void, + constituentNode: TSESTree.TypeNode, ) => void, ): void { const cachedTypeMap = new Map(); - node.types.reduce( - (uniqueConstituents, constituentNode) => { - const constituentNodeType = - parserServices.getTypeAtLocation(constituentNode); - if (tsutils.isIntrinsicErrorType(constituentNodeType)) { - return uniqueConstituents; - } + const uniqueConstituents: TSESTree.TypeNode[] = []; - const report = ( - messageId: MessageIds, - data?: Record, - ): void => { - const getUnionOrIntersectionToken = ( - where: 'After' | 'Before', - at: number, - ): TSESTree.Token | undefined => - sourceCode[`getTokens${where}`](constituentNode, { - filter: token => ['&', '|'].includes(token.value), - }).at(at); - - const beforeUnionOrIntersectionToken = getUnionOrIntersectionToken( - 'Before', - -1, - ); - let afterUnionOrIntersectionToken: TSESTree.Token | undefined; - let bracketBeforeTokens; - let bracketAfterTokens; - if (beforeUnionOrIntersectionToken) { - bracketBeforeTokens = sourceCode.getTokensBetween( - beforeUnionOrIntersectionToken, - constituentNode, - ); - bracketAfterTokens = sourceCode.getTokensAfter(constituentNode, { - count: bracketBeforeTokens.length, - }); - } else { - afterUnionOrIntersectionToken = nullThrows( - getUnionOrIntersectionToken('After', 0), - NullThrowsReasons.MissingToken( - 'union or intersection token', - 'duplicate type constituent', - ), - ); - bracketAfterTokens = sourceCode.getTokensBetween( - constituentNode, - afterUnionOrIntersectionToken, - ); - bracketBeforeTokens = sourceCode.getTokensBefore( - constituentNode, - { - count: bracketAfterTokens.length, - }, - ); - } - context.report({ - loc: { - start: constituentNode.loc.start, - end: (bracketAfterTokens.at(-1) ?? constituentNode).loc.end, - }, - node: constituentNode, - messageId, - data, - fix: fixer => - [ - beforeUnionOrIntersectionToken, - ...bracketBeforeTokens, - constituentNode, - ...bracketAfterTokens, - afterUnionOrIntersectionToken, - ].flatMap(token => (token ? fixer.remove(token) : [])), - }); - }; - const duplicatePrevious = - uniqueConstituents.find(ele => - isSameAstNode(ele, constituentNode), - ) ?? cachedTypeMap.get(constituentNodeType); - if (duplicatePrevious) { - report('duplicate', { - type: - node.type === AST_NODE_TYPES.TSIntersectionType - ? 'Intersection' - : 'Union', - previous: sourceCode.getText(duplicatePrevious), - }); - return uniqueConstituents; - } - forEachNodeType?.(constituentNodeType, report); - cachedTypeMap.set(constituentNodeType, constituentNode); - return [...uniqueConstituents, constituentNode]; - }, - [], - ); + const unionOrIntersection = + node.type === AST_NODE_TYPES.TSIntersectionType + ? 'Intersection' + : 'Union'; + + for (const type of node.types) { + const duplicationInfos = getDuplicateTypeNodes( + unionOrIntersection, + type, + uniqueConstituents, + cachedTypeMap, + forEachNodeType, + ); + duplicationInfos.forEach(({ constituentNode, duplicatedPrev }) => + report('duplicate', constituentNode, { + type: unionOrIntersection, + previous: sourceCode.getText(duplicatedPrev), + }), + ); + } } return { ...(!ignoreIntersections && { - TSIntersectionType: checkDuplicate, + TSIntersectionType(node) { + if (node.parent.type === AST_NODE_TYPES.TSIntersectionType) { + return; + } + checkDuplicate(node); + }, }), ...(!ignoreUnions && { - TSUnionType: (node): void => - checkDuplicate(node, (constituentNodeType, report) => { + TSUnionType: (node): void => { + if (node.parent.type === AST_NODE_TYPES.TSUnionType) { + return; + } + checkDuplicate(node, (constituentNodeType, constituentNode) => { const maybeTypeAnnotation = node.parent; if (maybeTypeAnnotation.type === AST_NODE_TYPES.TSTypeAnnotation) { const maybeIdentifier = maybeTypeAnnotation.parent; @@ -242,11 +326,12 @@ export default createRule({ ts.TypeFlags.Undefined, ) ) { - report('unnecessary'); + report('unnecessary', constituentNode); } } } - }), + }); + }, }), }; }, diff --git a/packages/eslint-plugin/tests/rules/no-duplicate-type-constituents.test.ts b/packages/eslint-plugin/tests/rules/no-duplicate-type-constituents.test.ts index f8490a908f1a..9c510c4f2bd5 100644 --- a/packages/eslint-plugin/tests/rules/no-duplicate-type-constituents.test.ts +++ b/packages/eslint-plugin/tests/rules/no-duplicate-type-constituents.test.ts @@ -598,13 +598,6 @@ type T = A | (A | A); }, messageId: 'duplicate', }, - { - data: { - previous: 'A', - type: 'Union', - }, - messageId: 'duplicate', - }, ], output: ` type A = 'A'; @@ -643,6 +636,72 @@ type D = 'D'; type F = (A | B) | ((C | D) & (A | B)) ; `, }, + { + code: 'type A = (number | string) | number | string;', + errors: [ + { + data: { + previous: 'number', + type: 'Union', + }, + messageId: 'duplicate', + }, + { + data: { + previous: 'string', + type: 'Union', + }, + messageId: 'duplicate', + }, + ], + output: 'type A = (number | string) ;', + }, + { + code: 'type A = (number | (string | null)) | (string | (null | number));', + errors: [ + { + data: { + previous: 'number | (string | null)', + type: 'Union', + }, + messageId: 'duplicate', + }, + ], + output: 'type A = (number | (string | null)) ;', + }, + { + code: 'type A = (number & string) & number & string;', + errors: [ + { + data: { + previous: 'number', + type: 'Intersection', + }, + messageId: 'duplicate', + }, + { + data: { + previous: 'string', + type: 'Intersection', + }, + messageId: 'duplicate', + }, + ], + output: 'type A = (number & string) ;', + }, + { + code: 'type A = number & string & (number & string);', + errors: [ + { + data: { + previous: 'number & string', + type: 'Intersection', + }, + messageId: 'duplicate', + }, + ], + output: 'type A = number & string ;', + }, { code: ` type A = 'A'; @@ -768,6 +827,16 @@ type T = Record; errors: [{ messageId: 'unnecessary' }], output: 'type fn = (a?: string ) => void;', }, + { + code: 'type fn = (a?: string | (undefined | number)) => void;', + errors: [{ messageId: 'unnecessary' }], + output: 'type fn = (a?: string | ( number)) => void;', + }, + { + code: 'type fn = (a?: (undefined | number) | string) => void;', + errors: [{ messageId: 'unnecessary' }], + output: 'type fn = (a?: ( number) | string) => void;', + }, { code: ` abstract class cc { From 9f8864c9cea7196f35f09c24af66c67fab01d0ab Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Sun, 12 Jan 2025 22:51:53 +0900 Subject: [PATCH 2/4] fix --- .../rules/no-duplicate-type-constituents.ts | 59 ++++--------------- .../no-duplicate-type-constituents.test.ts | 49 ++++++++++++++- 2 files changed, 57 insertions(+), 51 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts b/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts index 6ffb3ccdbb61..cb1bd802c6ba 100644 --- a/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts +++ b/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts @@ -24,11 +24,6 @@ export type MessageIds = 'duplicate' | 'unnecessary'; export type UnionOrIntersection = 'Intersection' | 'Union'; -interface DuplicatedConstituentInfo { - duplicatedPrev: TSESTree.TypeNode; - constituentNode: TSESTree.TypeNode; -} - const astIgnoreKeys = new Set(['loc', 'parent', 'range']); const isSameAstNode = (actualNode: unknown, expectedNode: unknown): boolean => { @@ -190,43 +185,27 @@ export default createRule({ }); } - function containsEveryDuplicatedConstituents( - node: TSESTree.TSIntersectionType | TSESTree.TSUnionType, - duplicationInfos: DuplicatedConstituentInfo[], - ): boolean { - const duplicatedConstituentNodes = duplicationInfos.map( - ({ constituentNode }) => constituentNode, - ); - return ( - node.types.length === duplicatedConstituentNodes.length && - node.types.every(constituentNode => - duplicatedConstituentNodes.includes(constituentNode), - ) - ); - } - - function getDuplicateTypeNodes( + function checkDuplicateRecursively( unionOrIntersection: UnionOrIntersection, constituentNode: TSESTree.TypeNode, uniqueConstituents: TSESTree.TypeNode[], cachedTypeMap: Map, forEachNodeType?: (type: Type, node: TSESTree.TypeNode) => void, - ): DuplicatedConstituentInfo[] { + ): void { const type = parserServices.getTypeAtLocation(constituentNode); if (tsutils.isIntrinsicErrorType(type)) { - return []; + return; } const duplicatedPrev = uniqueConstituents.find(ele => isSameAstNode(ele, constituentNode)) ?? cachedTypeMap.get(type); if (duplicatedPrev) { - return [ - { - constituentNode, - duplicatedPrev, - }, - ]; + report('duplicate', constituentNode, { + type: unionOrIntersection, + previous: sourceCode.getText(duplicatedPrev), + }); + return; } forEachNodeType?.(type, constituentNode); @@ -239,28 +218,16 @@ export default createRule({ (unionOrIntersection === 'Intersection' && constituentNode.type === AST_NODE_TYPES.TSIntersectionType) ) { - const result: DuplicatedConstituentInfo[] = []; for (const constituent of constituentNode.types) { - const duplications = getDuplicateTypeNodes( + checkDuplicateRecursively( unionOrIntersection, constituent, uniqueConstituents, cachedTypeMap, forEachNodeType, ); - result.push(...duplications); - } - if (containsEveryDuplicatedConstituents(constituentNode, result)) { - return [ - { - constituentNode, - duplicatedPrev: constituentNode, - }, - ]; } - return result; } - return []; } function checkDuplicate( @@ -279,19 +246,13 @@ export default createRule({ : 'Union'; for (const type of node.types) { - const duplicationInfos = getDuplicateTypeNodes( + checkDuplicateRecursively( unionOrIntersection, type, uniqueConstituents, cachedTypeMap, forEachNodeType, ); - duplicationInfos.forEach(({ constituentNode, duplicatedPrev }) => - report('duplicate', constituentNode, { - type: unionOrIntersection, - previous: sourceCode.getText(duplicatedPrev), - }), - ); } } diff --git a/packages/eslint-plugin/tests/rules/no-duplicate-type-constituents.test.ts b/packages/eslint-plugin/tests/rules/no-duplicate-type-constituents.test.ts index 9c510c4f2bd5..90d32bf0010f 100644 --- a/packages/eslint-plugin/tests/rules/no-duplicate-type-constituents.test.ts +++ b/packages/eslint-plugin/tests/rules/no-duplicate-type-constituents.test.ts @@ -636,6 +636,41 @@ type D = 'D'; type F = (A | B) | ((C | D) & (A | B)) ; `, }, + { + code: ` +type A = 'A'; +type B = 'B'; +type C = (A | B) | A | B | (A | B); + `, + errors: [ + { + data: { + previous: 'A', + type: 'Union', + }, + messageId: 'duplicate', + }, + { + data: { + previous: 'B', + type: 'Union', + }, + messageId: 'duplicate', + }, + { + data: { + previous: 'A | B', + type: 'Union', + }, + messageId: 'duplicate', + }, + ], + output: ` +type A = 'A'; +type B = 'B'; +type C = (A | B) ; + `, + }, { code: 'type A = (number | string) | number | string;', errors: [ @@ -694,13 +729,23 @@ type F = (A | B) | ((C | D) & (A | B)) ; errors: [ { data: { - previous: 'number & string', + previous: 'number', + type: 'Intersection', + }, + messageId: 'duplicate', + }, + { + data: { + previous: 'string', type: 'Intersection', }, messageId: 'duplicate', }, ], - output: 'type A = number & string ;', + output: [ + 'type A = number & string & ( string);', + 'type A = number & string ;', + ], }, { code: ` From 7b3a5e22f041751cb867b72a9de722710451feb0 Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Sun, 12 Jan 2025 22:54:01 +0900 Subject: [PATCH 3/4] Update no-duplicate-type-constituents.ts --- .../eslint-plugin/src/rules/no-duplicate-type-constituents.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts b/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts index cb1bd802c6ba..cddb7e6d6293 100644 --- a/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts +++ b/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts @@ -22,7 +22,7 @@ export type Options = [ export type MessageIds = 'duplicate' | 'unnecessary'; -export type UnionOrIntersection = 'Intersection' | 'Union'; +type UnionOrIntersection = 'Intersection' | 'Union'; const astIgnoreKeys = new Set(['loc', 'parent', 'range']); From 6b055aee580d28b5765eca01d25516305a97de5b Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Thu, 23 Jan 2025 23:03:44 +0900 Subject: [PATCH 4/4] fix naming --- .../src/rules/no-duplicate-type-constituents.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts b/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts index cddb7e6d6293..092db80984b8 100644 --- a/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts +++ b/packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts @@ -196,14 +196,14 @@ export default createRule({ if (tsutils.isIntrinsicErrorType(type)) { return; } - const duplicatedPrev = + const duplicatedPrevious = uniqueConstituents.find(ele => isSameAstNode(ele, constituentNode)) ?? cachedTypeMap.get(type); - if (duplicatedPrev) { + if (duplicatedPrevious) { report('duplicate', constituentNode, { type: unionOrIntersection, - previous: sourceCode.getText(duplicatedPrev), + previous: sourceCode.getText(duplicatedPrevious), }); return; }