Skip to content

Commit a916ff2

Browse files
feat(eslint-plugin): [no-unnecessary-condition] add checkTypePredicates (#10009)
* [no-unnecessary-condition]: add checkTruthinessAssertions * test -u * change internal reports * added some coverage * hugely simplify * uge * yooj * changes * some changes * test changes * finishing touches * snapshots * fixup * remove type predicate
1 parent 032918a commit a916ff2

16 files changed

+525
-187
lines changed

eslint.config.mjs

+1-1
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ export default tseslint.config(
129129
'no-constant-condition': 'off',
130130
'@typescript-eslint/no-unnecessary-condition': [
131131
'error',
132-
{ allowConstantLoopConditions: true },
132+
{ allowConstantLoopConditions: true, checkTypePredicates: true },
133133
],
134134
'@typescript-eslint/no-unnecessary-type-parameters': 'error',
135135
'@typescript-eslint/no-unused-expressions': 'error',

packages/eslint-plugin/docs/rules/no-unnecessary-condition.mdx

+40
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,46 @@ for (; true; ) {}
9090
do {} while (true);
9191
```
9292

93+
### `checkTypePredicates`
94+
95+
Example of additional incorrect code with `{ checkTypePredicates: true }`:
96+
97+
```ts option='{ "checkTypePredicates": true }' showPlaygroundButton
98+
function assert(condition: unknown): asserts condition {
99+
if (!condition) {
100+
throw new Error('Condition is falsy');
101+
}
102+
}
103+
104+
assert(false); // Unnecessary; condition is always falsy.
105+
106+
const neverNull = {};
107+
assert(neverNull); // Unnecessary; condition is always truthy.
108+
109+
function isString(value: unknown): value is string {
110+
return typeof value === 'string';
111+
}
112+
113+
declare const s: string;
114+
115+
// Unnecessary; s is always a string.
116+
if (isString(s)) {
117+
}
118+
119+
function assertIsString(value: unknown): asserts value is string {
120+
if (!isString(value)) {
121+
throw new Error('Value is not a string');
122+
}
123+
}
124+
125+
assertIsString(s); // Unnecessary; s is always a string.
126+
```
127+
128+
Whether this option makes sense for your project may vary.
129+
Some projects may intentionally use type predicates to ensure that runtime values do indeed match the types according to TypeScript, especially in test code.
130+
Often, it makes sense to use eslint-disable comments in these cases, with a comment indicating why the condition should be checked at runtime, despite appearing unnecessary.
131+
However, in some contexts, it may be more appropriate to keep this option disabled entirely.
132+
93133
### `allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing`
94134

95135
:::danger Deprecated

packages/eslint-plugin/src/rules/no-unnecessary-condition.ts

+49
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@ import {
1818
nullThrows,
1919
NullThrowsReasons,
2020
} from '../util';
21+
import {
22+
findTruthinessAssertedArgument,
23+
findTypeGuardAssertedArgument,
24+
} from '../util/assertionFunctionUtils';
2125

2226
// Truthiness utilities
2327
// #region
@@ -71,6 +75,7 @@ export type Options = [
7175
{
7276
allowConstantLoopConditions?: boolean;
7377
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing?: boolean;
78+
checkTypePredicates?: boolean;
7479
},
7580
];
7681

@@ -81,6 +86,8 @@ export type MessageId =
8186
| 'alwaysTruthy'
8287
| 'alwaysTruthyFunc'
8388
| 'literalBooleanExpression'
89+
| 'typeGuardAlreadyIsType'
90+
| 'replaceWithTrue'
8491
| 'never'
8592
| 'neverNullish'
8693
| 'neverOptionalChain'
@@ -111,6 +118,11 @@ export default createRule<Options, MessageId>({
111118
'Whether to not error when running with a tsconfig that has strictNullChecks turned.',
112119
type: 'boolean',
113120
},
121+
checkTypePredicates: {
122+
description:
123+
'Whether to check the asserted argument of a type predicate function for unnecessary conditions',
124+
type: 'boolean',
125+
},
114126
},
115127
additionalProperties: false,
116128
},
@@ -129,18 +141,22 @@ export default createRule<Options, MessageId>({
129141
'Unnecessary conditional, left-hand side of `??` operator is always `null` or `undefined`.',
130142
literalBooleanExpression:
131143
'Unnecessary conditional, both sides of the expression are literal values.',
144+
replaceWithTrue: 'Replace always true expression with `true`.',
132145
noOverlapBooleanExpression:
133146
'Unnecessary conditional, the types have no overlap.',
134147
never: 'Unnecessary conditional, value is `never`.',
135148
neverOptionalChain: 'Unnecessary optional chain on a non-nullish value.',
136149
noStrictNullCheck:
137150
'This rule requires the `strictNullChecks` compiler option to be turned on to function correctly.',
151+
typeGuardAlreadyIsType:
152+
'Unnecessary conditional, expression already has the type being checked by the {{typeGuardOrAssertionFunction}}.',
138153
},
139154
},
140155
defaultOptions: [
141156
{
142157
allowConstantLoopConditions: false,
143158
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: false,
159+
checkTypePredicates: false,
144160
},
145161
],
146162
create(
@@ -149,6 +165,7 @@ export default createRule<Options, MessageId>({
149165
{
150166
allowConstantLoopConditions,
151167
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing,
168+
checkTypePredicates,
152169
},
153170
],
154171
) {
@@ -463,6 +480,38 @@ export default createRule<Options, MessageId>({
463480
}
464481

465482
function checkCallExpression(node: TSESTree.CallExpression): void {
483+
if (checkTypePredicates) {
484+
const truthinessAssertedArgument = findTruthinessAssertedArgument(
485+
services,
486+
node,
487+
);
488+
if (truthinessAssertedArgument != null) {
489+
checkNode(truthinessAssertedArgument);
490+
}
491+
492+
const typeGuardAssertedArgument = findTypeGuardAssertedArgument(
493+
services,
494+
node,
495+
);
496+
if (typeGuardAssertedArgument != null) {
497+
const typeOfArgument = getConstrainedTypeAtLocation(
498+
services,
499+
typeGuardAssertedArgument.argument,
500+
);
501+
if (typeOfArgument === typeGuardAssertedArgument.type) {
502+
context.report({
503+
node: typeGuardAssertedArgument.argument,
504+
messageId: 'typeGuardAlreadyIsType',
505+
data: {
506+
typeGuardOrAssertionFunction: typeGuardAssertedArgument.asserts
507+
? 'assertion function'
508+
: 'type guard',
509+
},
510+
});
511+
}
512+
}
513+
}
514+
466515
// If this is something like arr.filter(x => /*condition*/), check `condition`
467516
if (
468517
isArrayMethodCallWithPredicate(context, services, node) &&

packages/eslint-plugin/src/rules/prefer-optional-chain.ts

+1-6
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import type { TSESTree } from '@typescript-eslint/utils';
22
import { AST_NODE_TYPES } from '@typescript-eslint/utils';
33
import type { RuleFix } from '@typescript-eslint/utils/ts-eslint';
4-
import * as ts from 'typescript';
54

65
import {
76
createRule,
@@ -130,14 +129,10 @@ export default createRule<
130129

131130
function isLeftSideLowerPrecedence(): boolean {
132131
const logicalTsNode = parserServices.esTreeNodeToTSNodeMap.get(node);
133-
134132
const leftTsNode = parserServices.esTreeNodeToTSNodeMap.get(leftNode);
135-
const operator = ts.isBinaryExpression(logicalTsNode)
136-
? logicalTsNode.operatorToken.kind
137-
: ts.SyntaxKind.Unknown;
138133
const leftPrecedence = getOperatorPrecedence(
139134
leftTsNode.kind,
140-
operator,
135+
logicalTsNode.operatorToken.kind,
141136
);
142137

143138
return leftPrecedence < OperatorPrecedence.LeftHandSide;

packages/eslint-plugin/src/rules/prefer-string-starts-ends-with.ts

+1-5
Original file line numberDiff line numberDiff line change
@@ -535,11 +535,7 @@ export default createRule<Options, MessageIds>({
535535
const callNode = getParent(node) as TSESTree.CallExpression;
536536
const parentNode = getParent(callNode) as TSESTree.BinaryExpression;
537537

538-
if (
539-
!isEqualityComparison(parentNode) ||
540-
!isNull(parentNode.right) ||
541-
!isStringType(node.object)
542-
) {
538+
if (!isNull(parentNode.right) || !isStringType(node.object)) {
543539
return;
544540
}
545541

packages/eslint-plugin/src/rules/strict-boolean-expressions.ts

+2-123
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
getWrappingFixer,
1414
isTypeArrayTypeOrUnionOfArrayTypes,
1515
} from '../util';
16+
import { findTruthinessAssertedArgument } from '../util/assertionFunctionUtils';
1617

1718
export type Options = [
1819
{
@@ -267,134 +268,12 @@ export default createRule<Options, MessageId>({
267268
}
268269

269270
function traverseCallExpression(node: TSESTree.CallExpression): void {
270-
const assertedArgument = findAssertedArgument(node);
271+
const assertedArgument = findTruthinessAssertedArgument(services, node);
271272
if (assertedArgument != null) {
272273
traverseNode(assertedArgument, true);
273274
}
274275
}
275276

276-
/**
277-
* Inspect a call expression to see if it's a call to an assertion function.
278-
* If it is, return the node of the argument that is asserted.
279-
*/
280-
function findAssertedArgument(
281-
node: TSESTree.CallExpression,
282-
): TSESTree.Expression | undefined {
283-
// If the call looks like `assert(expr1, expr2, ...c, d, e, f)`, then we can
284-
// only care if `expr1` or `expr2` is asserted, since anything that happens
285-
// within or after a spread argument is out of scope to reason about.
286-
const checkableArguments: TSESTree.Expression[] = [];
287-
for (const argument of node.arguments) {
288-
if (argument.type === AST_NODE_TYPES.SpreadElement) {
289-
break;
290-
}
291-
292-
checkableArguments.push(argument);
293-
}
294-
295-
// nothing to do
296-
if (checkableArguments.length === 0) {
297-
return undefined;
298-
}
299-
300-
// Game plan: we're going to check the type of the callee. If it has call
301-
// signatures and they _ALL_ agree that they assert on a parameter at the
302-
// _SAME_ position, we'll consider the argument in that position to be an
303-
// asserted argument.
304-
const calleeType = getConstrainedTypeAtLocation(services, node.callee);
305-
const callSignatures = tsutils.getCallSignaturesOfType(calleeType);
306-
307-
let assertedParameterIndex: number | undefined = undefined;
308-
for (const signature of callSignatures) {
309-
const declaration = signature.getDeclaration();
310-
const returnTypeAnnotation = declaration.type;
311-
312-
// Be sure we're dealing with a truthiness assertion function.
313-
if (
314-
!(
315-
returnTypeAnnotation != null &&
316-
ts.isTypePredicateNode(returnTypeAnnotation) &&
317-
// This eliminates things like `x is string` and `asserts x is T`
318-
// leaving us with just the `asserts x` cases.
319-
returnTypeAnnotation.type == null &&
320-
// I think this is redundant but, still, it needs to be true
321-
returnTypeAnnotation.assertsModifier != null
322-
)
323-
) {
324-
return undefined;
325-
}
326-
327-
const assertionTarget = returnTypeAnnotation.parameterName;
328-
if (assertionTarget.kind !== ts.SyntaxKind.Identifier) {
329-
// This can happen when asserting on `this`. Ignore!
330-
return undefined;
331-
}
332-
333-
// If the first parameter is `this`, skip it, so that our index matches
334-
// the index of the argument at the call site.
335-
const firstParameter = declaration.parameters.at(0);
336-
const nonThisParameters =
337-
firstParameter?.name.kind === ts.SyntaxKind.Identifier &&
338-
firstParameter.name.text === 'this'
339-
? declaration.parameters.slice(1)
340-
: declaration.parameters;
341-
342-
// Don't bother inspecting parameters past the number of
343-
// arguments we have at the call site.
344-
const checkableNonThisParameters = nonThisParameters.slice(
345-
0,
346-
checkableArguments.length,
347-
);
348-
349-
let assertedParameterIndexForThisSignature: number | undefined;
350-
for (const [index, parameter] of checkableNonThisParameters.entries()) {
351-
if (parameter.dotDotDotToken != null) {
352-
// Cannot assert a rest parameter, and can't have a rest parameter
353-
// before the asserted parameter. It's not only a TS error, it's
354-
// not something we can logically make sense of, so give up here.
355-
return undefined;
356-
}
357-
358-
if (parameter.name.kind !== ts.SyntaxKind.Identifier) {
359-
// Only identifiers are valid for assertion targets, so skip over
360-
// anything like `{ destructuring: parameter }: T`
361-
continue;
362-
}
363-
364-
// we've found a match between the "target"s in
365-
// `function asserts(target: T): asserts target;`
366-
if (parameter.name.text === assertionTarget.text) {
367-
assertedParameterIndexForThisSignature = index;
368-
break;
369-
}
370-
}
371-
372-
if (assertedParameterIndexForThisSignature == null) {
373-
// Didn't find an assertion target in this signature that could match
374-
// the call site.
375-
return undefined;
376-
}
377-
378-
if (
379-
assertedParameterIndex != null &&
380-
assertedParameterIndex !== assertedParameterIndexForThisSignature
381-
) {
382-
// The asserted parameter we found for this signature didn't match
383-
// previous signatures.
384-
return undefined;
385-
}
386-
387-
assertedParameterIndex = assertedParameterIndexForThisSignature;
388-
}
389-
390-
// Didn't find a unique assertion index.
391-
if (assertedParameterIndex == null) {
392-
return undefined;
393-
}
394-
395-
return checkableArguments[assertedParameterIndex];
396-
}
397-
398277
/**
399278
* Inspects any node.
400279
*

0 commit comments

Comments
 (0)