diff --git a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.mdx b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.mdx index ab7871fdcba8..e952df715b77 100644 --- a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.mdx +++ b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.mdx @@ -188,6 +188,12 @@ function assertIsString(value: unknown): asserts value is string { } assertIsString(s); // Unnecessary; s is always a string. + +declare function assertIsStringOrNumber( + value: unknown, +): asserts value is string | number; + +assertIsStringOrNumber(s); // Unnecessary; s is assignable to string | number. ``` Whether this option makes sense for your project may vary. diff --git a/packages/eslint-plugin/src/rules/class-methods-use-this.ts b/packages/eslint-plugin/src/rules/class-methods-use-this.ts index f0757f628af1..e5f8e9677945 100644 --- a/packages/eslint-plugin/src/rules/class-methods-use-this.ts +++ b/packages/eslint-plugin/src/rules/class-methods-use-this.ts @@ -173,7 +173,7 @@ export default createRule({ */ function isIncludedInstanceMethod( node: NonNullable, - ): node is NonNullable { + ): boolean { if ( node.static || (node.type === AST_NODE_TYPES.MethodDefinition && diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index b9a1ebb9a4bf..2f727b7ec170 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -19,9 +19,11 @@ import { isPossiblyTruthy, isTypeAnyType, isTypeFlagSet, + isTypeNeverType, isTypeUnknownType, nullThrows, NullThrowsReasons, + toWidenedType, } from '../util'; import { findTruthinessAssertedArgument, @@ -153,7 +155,7 @@ export type MessageId = | 'noOverlapBooleanExpression' | 'noStrictNullCheck' | 'suggestRemoveOptionalChain' - | 'typeGuardAlreadyIsType'; + | 'typeAssertionArgumentAlreadyAssignable'; export default createRule({ name: 'no-unnecessary-condition', @@ -186,8 +188,8 @@ export default createRule({ noStrictNullCheck: 'This rule requires the `strictNullChecks` compiler option to be turned on to function correctly.', suggestRemoveOptionalChain: 'Remove unnecessary optional chain', - typeGuardAlreadyIsType: - 'Unnecessary conditional, expression already has the type being checked by the {{typeGuardOrAssertionFunction}}.', + typeAssertionArgumentAlreadyAssignable: + 'Unnecessary conditional, expression is already assignable to the type being checked by {{typeGuardOrAssertionFunction}}.', }, schema: [ { @@ -595,14 +597,50 @@ export default createRule({ node, ); if (typeGuardAssertedArgument != null) { - const typeOfArgument = getConstrainedTypeAtLocation( - services, - typeGuardAssertedArgument.argument, - ); - if (typeOfArgument === typeGuardAssertedArgument.type) { + const shouldReport = (() => { + const argumentType = services.getTypeAtLocation( + typeGuardAssertedArgument.argument, + ); + + const assertedType = typeGuardAssertedArgument.type; + if (isTypeAnyType(argumentType) && isTypeAnyType(assertedType)) { + return true; + } + + if (isTypeAnyType(argumentType)) { + return false; + } + + if ( + isTypeNeverType(argumentType) && + isTypeNeverType(assertedType) + ) { + return true; + } + + if (isTypeNeverType(argumentType)) { + return false; + } + + if ( + checker.isTypeAssignableTo( + // Use the widened type to bypass excess property checking + toWidenedType(checker, argumentType), + assertedType, + ) + ) { + // ... unless the asserted type actually does narrow the argument, + // for example by granting it additional optional properties? + return true; + } + + return false; + })(); + + if (shouldReport) { context.report({ node: typeGuardAssertedArgument.argument, - messageId: 'typeGuardAlreadyIsType', + messageId: 'typeAssertionArgumentAlreadyAssignable', data: { typeGuardOrAssertionFunction: typeGuardAssertedArgument.asserts ? 'assertion function' diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-type-parameters.ts b/packages/eslint-plugin/src/rules/no-unnecessary-type-parameters.ts index 3a486639dff7..cc0e34748c9d 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-type-parameters.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-type-parameters.ts @@ -452,6 +452,7 @@ function collectTypeParameterUsageCounts( const properties = type.getProperties(); visitSymbolsListOnce(properties, false); + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- TODO revisit this if (isMappedType(type)) { visitType(type.typeParameter, false); if (properties.length === 0) { diff --git a/packages/eslint-plugin/src/rules/no-unsafe-type-assertion.ts b/packages/eslint-plugin/src/rules/no-unsafe-type-assertion.ts index 988a140d60df..04797ac036c9 100644 --- a/packages/eslint-plugin/src/rules/no-unsafe-type-assertion.ts +++ b/packages/eslint-plugin/src/rules/no-unsafe-type-assertion.ts @@ -1,14 +1,16 @@ import type { TSESTree } from '@typescript-eslint/utils'; +import type * as ts from 'typescript'; import * as tsutils from 'ts-api-utils'; -import * as ts from 'typescript'; import { createRule, getParserServices, + isObjectLiteralType, isTypeAnyType, isTypeUnknownType, isUnsafeAssignment, + toWidenedType, } from '../util'; export default createRule({ @@ -42,13 +44,6 @@ export default createRule({ return tsutils.isIntrinsicErrorType(type) ? 'error typed' : '`any`'; } - function isObjectLiteralType(type: ts.Type): boolean { - return ( - tsutils.isObjectType(type) && - tsutils.isObjectFlagSet(type, ts.ObjectFlags.ObjectLiteral) - ); - } - function checkExpression( node: TSESTree.TSAsExpression | TSESTree.TSTypeAssertion, ): void { @@ -110,11 +105,8 @@ export default createRule({ return; } - // Use the widened type in case of an object literal so `isTypeAssignableTo()` - // won't fail on excess property check. - const expressionWidenedType = isObjectLiteralType(expressionType) - ? checker.getWidenedType(expressionType) - : expressionType; + // Use the widened type to bypass excess property checking + const expressionWidenedType = toWidenedType(checker, expressionType); const isAssertionSafe = checker.isTypeAssignableTo( expressionWidenedType, diff --git a/packages/eslint-plugin/src/util/index.ts b/packages/eslint-plugin/src/util/index.ts index 035fca0cec4a..d727bd0ec3bf 100644 --- a/packages/eslint-plugin/src/util/index.ts +++ b/packages/eslint-plugin/src/util/index.ts @@ -29,6 +29,7 @@ export * from './getValueOfLiteralType'; export * from './isHigherPrecedenceThanAwait'; export * from './skipChainExpression'; export * from './truthinessUtils'; +export * from './isObjectType.js'; // this is done for convenience - saves migrating all of the old rules export * from '@typescript-eslint/type-utils'; diff --git a/packages/eslint-plugin/src/util/isObjectType.ts b/packages/eslint-plugin/src/util/isObjectType.ts new file mode 100644 index 000000000000..8084aa3be5cc --- /dev/null +++ b/packages/eslint-plugin/src/util/isObjectType.ts @@ -0,0 +1,37 @@ +import * as tsutils from 'ts-api-utils'; +import * as ts from 'typescript'; + +/** + * Returns whether the type is a fresh object literal, which will be subject to + * excess property checking. + * + * For example, in the following snippet, the first function call has an object + * literal type, and therefore is disallowed by excess property checking, + * whereas the second function call is allowed, because the argument is not an + * object literal type: + * + * ```ts + * declare function f(x: { a: string }); + * + * f({ a: 'foo', excess: 'property' }); // TS error + * + * const allowed = { a: 'foo', excess: 'property' }; + * + * f(allowed); // allowed + * ``` + */ +export function isObjectLiteralType(type: ts.Type): boolean { + return ( + tsutils.isObjectType(type) && + tsutils.isObjectFlagSet(type, ts.ObjectFlags.ObjectLiteral) + ); +} + +/** + * Maps object literal types into ordinary types, in order to be able to avoid + * spurious results from `checker.isTypeAssignableTo()` due to excess property + * checking. + */ +export function toWidenedType(checker: ts.TypeChecker, type: ts.Type): ts.Type { + return isObjectLiteralType(type) ? checker.getWidenedType(type) : type; +} diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-unnecessary-condition.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-unnecessary-condition.shot index 18aaf55898c5..eef075560bfc 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-unnecessary-condition.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-unnecessary-condition.shot @@ -132,7 +132,7 @@ declare const s: string; // Unnecessary; s is always a string. if (isString(s)) { - ~ Unnecessary conditional, expression already has the type being checked by the type guard. + ~ Unnecessary conditional, expression is already assignable to the type being checked by type guard. } function assertIsString(value: unknown): asserts value is string { @@ -142,7 +142,14 @@ function assertIsString(value: unknown): asserts value is string { } assertIsString(s); // Unnecessary; s is always a string. - ~ Unnecessary conditional, expression already has the type being checked by the assertion function. + ~ Unnecessary conditional, expression is already assignable to the type being checked by assertion function. + +declare function assertIsStringOrNumber( + value: unknown, +): asserts value is string | number; + +assertIsStringOrNumber(s); // Unnecessary; s is assignable to string | number. + ~ Unnecessary conditional, expression is already assignable to the type being checked by assertion function. diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts index 80d81bce079c..d10b3037079d 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -1115,22 +1115,7 @@ isString(a); `, options: [{ checkTypePredicates: false }], }, - { - // Technically, this has type 'falafel' and not string. - code: ` -declare function assertString(x: unknown): asserts x is string; -assertString('falafel'); - `, - options: [{ checkTypePredicates: true }], - }, - { - // Technically, this has type 'falafel' and not string. - code: ` -declare function isString(x: unknown): x is string; -isString('falafel'); - `, - options: [{ checkTypePredicates: true }], - }, + ` type A = { [name in Lowercase]?: A }; declare const a: A; @@ -1159,6 +1144,70 @@ declare const t: T; t.a.a.a.value; t.A?.A?.A?.VALUE; `, + { + code: ` +type T1 = { a: string }; +type T2 = { b: number }; + +declare function isIntersection(x: unknown): x is T1 & T2; + +declare const u: T1 | T2; + +isUnion(u); + `, + options: [{ checkTypePredicates: true }], + }, + { + // TODO - what do we do with this case? + // + // The two types are mutually assignable, but the type predicate does have + // an observable effect. + skip: true, + code: ` +interface Wider { + a: string; +} + +interface Narrower { + a: string; + b?: number; +} + +declare function isNarrower(x: unknown): x is Narrower; + +declare const w: Wider; + +if (isNarrower(w)) { + w.b?.toFixed(); +} + `, + options: [{ checkTypePredicates: true }], + }, + { + // any is still narrowed, but it's pathologically assignable to anything + code: ` +declare const a: any; + +declare function isNumber(x: number | string): x is number; + +if (isNumber(a)) { +} + `, + options: [{ checkTypePredicates: true }], + }, + { + // never is badly behaved + code: ` +declare const _never: never; + +declare function isNumber(x: unknown): x is number; + +if (isNumber(_never)) { + // _never still has type never +} + `, + options: [{ checkTypePredicates: true }], + }, ], invalid: [ @@ -3478,7 +3527,7 @@ assertsString(a); errors: [ { line: 4, - messageId: 'typeGuardAlreadyIsType', + messageId: 'typeAssertionArgumentAlreadyAssignable', }, ], options: [{ checkTypePredicates: true }], @@ -3492,7 +3541,7 @@ isString(a); errors: [ { line: 4, - messageId: 'typeGuardAlreadyIsType', + messageId: 'typeAssertionArgumentAlreadyAssignable', }, ], options: [{ checkTypePredicates: true }], @@ -3506,7 +3555,7 @@ isString('fa' + 'lafel'); errors: [ { line: 4, - messageId: 'typeGuardAlreadyIsType', + messageId: 'typeAssertionArgumentAlreadyAssignable', }, ], options: [{ checkTypePredicates: true }], @@ -3846,5 +3895,109 @@ if (arr[42] && arr[42]) { }, }, }, + { + code: ` +declare function assertString(x: unknown): asserts x is string; +assertString('falafel'); + `, + errors: [ + { + column: 14, + endColumn: 23, + endLine: 3, + line: 3, + messageId: 'typeAssertionArgumentAlreadyAssignable', + }, + ], + options: [{ checkTypePredicates: true }], + }, + { + code: ` +declare function isString(x: unknown): x is string; +isString('falafel'); + `, + errors: [ + { + column: 10, + endColumn: 19, + endLine: 3, + line: 3, + messageId: 'typeAssertionArgumentAlreadyAssignable', + }, + ], + options: [{ checkTypePredicates: true }], + }, + { + code: ` +type T1 = { a: string }; +type T2 = { b: number }; + +declare function isUnion(x: unknown): x is T1 | T2; + +isUnion({ a: 'foo' }); + `, + errors: [ + { + messageId: 'typeAssertionArgumentAlreadyAssignable', + }, + ], + options: [{ checkTypePredicates: true }], + }, + { + code: ` +type Foo = { a: string }; + +declare function isUnion(x: unknown): x is Foo; + +isUnion({ a: 'foo', excess: 'property' }); + `, + errors: [ + { + messageId: 'typeAssertionArgumentAlreadyAssignable', + }, + ], + options: [{ checkTypePredicates: true }], + }, + { + code: ` +type T1 = { a: string }; +type T2 = { b: number }; + +declare function isUnion(x: unknown): x is T1 | T2; + +declare const u: T1 | T2; + +isUnion(u); + `, + errors: [ + { + messageId: 'typeAssertionArgumentAlreadyAssignable', + }, + ], + options: [{ checkTypePredicates: true }], + }, + { + code: ` +interface T1 { + a: string; +} + +interface T2 { + a: string; +} + +declare function isT1(x: unknown): x is T1; + +declare const t2: T2; + +isT1(t2); + `, + errors: [ + { + messageId: 'typeAssertionArgumentAlreadyAssignable', + }, + ], + options: [{ checkTypePredicates: true }], + }, ], }); diff --git a/packages/eslint-plugin/typings/typescript.d.ts b/packages/eslint-plugin/typings/typescript.d.ts index 4b1d1a6b643a..196eb2da3183 100644 --- a/packages/eslint-plugin/typings/typescript.d.ts +++ b/packages/eslint-plugin/typings/typescript.d.ts @@ -14,6 +14,9 @@ declare module 'typescript' { getContextualTypeForArgumentAtIndex(node: Node, argIndex: number): Type; + // TODO - technically this is not a valid type predicate, since the + // contrapositive is not correct. + /** * @returns `true` if the given type is an array type: * - `Array` diff --git a/packages/rule-tester/src/utils/flat-config-schema.ts b/packages/rule-tester/src/utils/flat-config-schema.ts index 1b631070a3e0..68af6714b2a7 100644 --- a/packages/rule-tester/src/utils/flat-config-schema.ts +++ b/packages/rule-tester/src/utils/flat-config-schema.ts @@ -1,6 +1,7 @@ // Forked from https://github.com/eslint/eslint/blob/4b23ffd6454cfb1a269430f5fe28e7d1c37b9d3e/lib/config/flat-config-schema.js import type { + Linter, Processor, SharedConfig, } from '@typescript-eslint/utils/ts-eslint'; @@ -498,6 +499,7 @@ const rulesSchema = { assertIsRuleOptions(ruleId, ruleOptions); + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- the type of `ruleOptions` is incorrect. if (Array.isArray(ruleOptions)) { assertIsRuleSeverity(ruleId, ruleOptions[0]); } else {