Skip to content

Commit f44da95

Browse files
abrahamguoabrahamguo
authored
feat(eslint-plugin): [no-duplicate-type-constituents] prevent unnecessary | undefined for optional parameters (#9479)
* make the feature * small refactor * update docs * apply lint rule * add issue comment * update snapshot * revert accidental formatting changes * handle first constituent of union * lint * use nullThrows * add failing test * WIP * finish refactor * cleanup * clean up nits * add more node types * remove only * remove now-unnecessary check
1 parent 4bc801e commit f44da95

File tree

5 files changed

+235
-76
lines changed

5 files changed

+235
-76
lines changed

‎packages/eslint-plugin/docs/rules/no-duplicate-type-constituents.mdx‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@ This rule disallows duplicate union or intersection constituents.
1717
We consider types to be duplicate if they evaluate to the same result in the type system.
1818
For example, given `type A = string` and `type T = string | A`, this rule would flag that `A` is the same type as `string`.
1919

20+
This rule also disallows explicitly listing `undefined` in a type union when a function parameter is marked as optional.
21+
Doing so is unnecessary.
22+
2023
<Tabs>
2124
<TabItem value="❌ Incorrect">
2225

@@ -32,6 +35,8 @@ type T4 = [1, 2, 3] | [1, 2, 3];
3235
type StringA = string;
3336
type StringB = string;
3437
type T5 = StringA | StringB;
38+
39+
const fn = (a?: string | undefined) => {};
3540
```
3641

3742
</TabItem>
@@ -49,6 +54,8 @@ type T4 = [1, 2, 3] | [1, 2, 3, 4];
4954
type StringA = string;
5055
type NumberB = number;
5156
type T5 = StringA | NumberB;
57+
58+
const fn = (a?: string) => {};
5259
```
5360

5461
</TabItem>

‎packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts‎

Lines changed: 115 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,15 @@ import type { TSESTree } from '@typescript-eslint/utils';
22
import { AST_NODE_TYPES } from '@typescript-eslint/utils';
33
import * as tsutils from 'ts-api-utils';
44
import type { Type } from 'typescript';
5+
import * as ts from 'typescript';
56

6-
import { createRule, getParserServices } from '../util';
7+
import {
8+
createRule,
9+
getParserServices,
10+
isFunctionOrFunctionType,
11+
nullThrows,
12+
NullThrowsReasons,
13+
} from '../util';
714

815
export type Options = [
916
{
@@ -12,7 +19,7 @@ export type Options = [
1219
},
1320
];
1421

15-
export type MessageIds = 'duplicate';
22+
export type MessageIds = 'duplicate' | 'unnecessary';
1623

1724
const astIgnoreKeys = new Set(['range', 'loc', 'parent']);
1825

@@ -79,6 +86,8 @@ export default createRule<Options, MessageIds>({
7986
fixable: 'code',
8087
messages: {
8188
duplicate: '{{type}} type constituent is duplicated with {{previous}}.',
89+
unnecessary:
90+
'Explicit undefined is unnecessary on an optional parameter.',
8291
},
8392
schema: [
8493
{
@@ -105,9 +114,14 @@ export default createRule<Options, MessageIds>({
105114
],
106115
create(context, [{ ignoreIntersections, ignoreUnions }]) {
107116
const parserServices = getParserServices(context);
117+
const { sourceCode } = context;
108118

109119
function checkDuplicate(
110120
node: TSESTree.TSIntersectionType | TSESTree.TSUnionType,
121+
forEachNodeType?: (
122+
constituentNodeType: Type,
123+
report: (messageId: MessageIds) => void,
124+
) => void,
111125
): void {
112126
const cachedTypeMap = new Map<Type, TSESTree.TypeNode>();
113127
node.types.reduce<TSESTree.TypeNode[]>(
@@ -118,94 +132,120 @@ export default createRule<Options, MessageIds>({
118132
return uniqueConstituents;
119133
}
120134

121-
const duplicatedPreviousConstituentInAst = uniqueConstituents.find(
122-
ele => isSameAstNode(ele, constituentNode),
123-
);
124-
if (duplicatedPreviousConstituentInAst) {
125-
reportDuplicate(
126-
{
127-
duplicated: constituentNode,
128-
duplicatePrevious: duplicatedPreviousConstituentInAst,
129-
},
130-
node,
135+
const report = (
136+
messageId: MessageIds,
137+
data?: Record<string, unknown>,
138+
): void => {
139+
const getUnionOrIntersectionToken = (
140+
where: 'Before' | 'After',
141+
at: number,
142+
): TSESTree.Token | undefined =>
143+
sourceCode[`getTokens${where}`](constituentNode, {
144+
filter: token => ['|', '&'].includes(token.value),
145+
}).at(at);
146+
147+
const beforeUnionOrIntersectionToken = getUnionOrIntersectionToken(
148+
'Before',
149+
-1,
131150
);
132-
return uniqueConstituents;
133-
}
134-
const duplicatedPreviousConstituentInType =
135-
cachedTypeMap.get(constituentNodeType);
136-
if (duplicatedPreviousConstituentInType) {
137-
reportDuplicate(
138-
{
139-
duplicated: constituentNode,
140-
duplicatePrevious: duplicatedPreviousConstituentInType,
151+
let afterUnionOrIntersectionToken: TSESTree.Token | undefined;
152+
let bracketBeforeTokens;
153+
let bracketAfterTokens;
154+
if (beforeUnionOrIntersectionToken) {
155+
bracketBeforeTokens = sourceCode.getTokensBetween(
156+
beforeUnionOrIntersectionToken,
157+
constituentNode,
158+
);
159+
bracketAfterTokens = sourceCode.getTokensAfter(constituentNode, {
160+
count: bracketBeforeTokens.length,
161+
});
162+
} else {
163+
afterUnionOrIntersectionToken = nullThrows(
164+
getUnionOrIntersectionToken('After', 0),
165+
NullThrowsReasons.MissingToken(
166+
'union or intersection token',
167+
'duplicate type constituent',
168+
),
169+
);
170+
bracketAfterTokens = sourceCode.getTokensBetween(
171+
constituentNode,
172+
afterUnionOrIntersectionToken,
173+
);
174+
bracketBeforeTokens = sourceCode.getTokensBefore(
175+
constituentNode,
176+
{
177+
count: bracketAfterTokens.length,
178+
},
179+
);
180+
}
181+
context.report({
182+
data,
183+
messageId,
184+
node: constituentNode,
185+
loc: {
186+
start: constituentNode.loc.start,
187+
end: (bracketAfterTokens.at(-1) ?? constituentNode).loc.end,
141188
},
142-
node,
143-
);
189+
fix: fixer =>
190+
[
191+
beforeUnionOrIntersectionToken,
192+
...bracketBeforeTokens,
193+
constituentNode,
194+
...bracketAfterTokens,
195+
afterUnionOrIntersectionToken,
196+
].flatMap(token => (token ? fixer.remove(token) : [])),
197+
});
198+
};
199+
const duplicatePrevious =
200+
uniqueConstituents.find(ele =>
201+
isSameAstNode(ele, constituentNode),
202+
) ?? cachedTypeMap.get(constituentNodeType);
203+
if (duplicatePrevious) {
204+
report('duplicate', {
205+
type:
206+
node.type === AST_NODE_TYPES.TSIntersectionType
207+
? 'Intersection'
208+
: 'Union',
209+
previous: sourceCode.getText(duplicatePrevious),
210+
});
144211
return uniqueConstituents;
145212
}
213+
forEachNodeType?.(constituentNodeType, report);
146214
cachedTypeMap.set(constituentNodeType, constituentNode);
147215
return [...uniqueConstituents, constituentNode];
148216
},
149217
[],
150218
);
151219
}
152-
function reportDuplicate(
153-
duplicateConstituent: {
154-
duplicated: TSESTree.TypeNode;
155-
duplicatePrevious: TSESTree.TypeNode;
156-
},
157-
parentNode: TSESTree.TSIntersectionType | TSESTree.TSUnionType,
158-
): void {
159-
const beforeTokens = context.sourceCode.getTokensBefore(
160-
duplicateConstituent.duplicated,
161-
{ filter: token => token.value === '|' || token.value === '&' },
162-
);
163-
const beforeUnionOrIntersectionToken =
164-
beforeTokens[beforeTokens.length - 1];
165-
const bracketBeforeTokens = context.sourceCode.getTokensBetween(
166-
beforeUnionOrIntersectionToken,
167-
duplicateConstituent.duplicated,
168-
);
169-
const bracketAfterTokens = context.sourceCode.getTokensAfter(
170-
duplicateConstituent.duplicated,
171-
{ count: bracketBeforeTokens.length },
172-
);
173-
const reportLocation: TSESTree.SourceLocation = {
174-
start: duplicateConstituent.duplicated.loc.start,
175-
end:
176-
bracketAfterTokens.length > 0
177-
? bracketAfterTokens[bracketAfterTokens.length - 1].loc.end
178-
: duplicateConstituent.duplicated.loc.end,
179-
};
180-
context.report({
181-
data: {
182-
type:
183-
parentNode.type === AST_NODE_TYPES.TSIntersectionType
184-
? 'Intersection'
185-
: 'Union',
186-
previous: context.sourceCode.getText(
187-
duplicateConstituent.duplicatePrevious,
188-
),
189-
},
190-
messageId: 'duplicate',
191-
node: duplicateConstituent.duplicated,
192-
loc: reportLocation,
193-
fix: fixer => {
194-
return [
195-
beforeUnionOrIntersectionToken,
196-
...bracketBeforeTokens,
197-
duplicateConstituent.duplicated,
198-
...bracketAfterTokens,
199-
].map(token => fixer.remove(token));
200-
},
201-
});
202-
}
220+
203221
return {
204222
...(!ignoreIntersections && {
205223
TSIntersectionType: checkDuplicate,
206224
}),
207225
...(!ignoreUnions && {
208-
TSUnionType: checkDuplicate,
226+
TSUnionType: (node): void =>
227+
checkDuplicate(node, (constituentNodeType, report) => {
228+
const maybeTypeAnnotation = node.parent;
229+
if (maybeTypeAnnotation.type === AST_NODE_TYPES.TSTypeAnnotation) {
230+
const maybeIdentifier = maybeTypeAnnotation.parent;
231+
if (
232+
maybeIdentifier.type === AST_NODE_TYPES.Identifier &&
233+
maybeIdentifier.optional
234+
) {
235+
const maybeFunction = maybeIdentifier.parent;
236+
if (
237+
isFunctionOrFunctionType(maybeFunction) &&
238+
maybeFunction.params.includes(maybeIdentifier) &&
239+
tsutils.isTypeFlagSet(
240+
constituentNodeType,
241+
ts.TypeFlags.Undefined,
242+
)
243+
) {
244+
report('unnecessary');
245+
}
246+
}
247+
}
248+
}),
209249
}),
210250
};
211251
},

‎packages/eslint-plugin/src/rules/prefer-find.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,7 @@ function isStaticMemberAccessOfValue(
332332
| TSESTree.MemberExpressionComputedName
333333
| TSESTree.MemberExpressionNonComputedName,
334334
value: string,
335-
scope?: Scope.Scope | undefined,
335+
scope?: Scope.Scope,
336336
): boolean {
337337
if (!memberExpression.computed) {
338338
// x.memberName case.

‎packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-duplicate-type-constituents.shot‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)