From 0ae067de4f4a3269f361a79144b8458e222f39ca Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger <53019676+kirkwaiblinger@users.noreply.github.com> Date: Wed, 26 Nov 2025 00:31:09 -0700 Subject: [PATCH 1/5] [no-unnecessary-condition] use assignability for checkTypePredicates --- .../src/rules/no-unnecessary-condition.ts | 16 ++- .../src/rules/no-unsafe-type-assertion.ts | 18 +-- packages/eslint-plugin/src/util/index.ts | 1 + .../eslint-plugin/src/util/isObjectType.ts | 38 +++++ .../rules/no-unnecessary-condition.test.ts | 134 +++++++++++++++--- 5 files changed, 174 insertions(+), 33 deletions(-) create mode 100644 packages/eslint-plugin/src/util/isObjectType.ts diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index b9a1ebb9a4bf..33f88af2a347 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -22,6 +22,7 @@ import { isTypeUnknownType, nullThrows, NullThrowsReasons, + toWidenedType, } from '../util'; import { findTruthinessAssertedArgument, @@ -595,11 +596,18 @@ export default createRule({ node, ); if (typeGuardAssertedArgument != null) { - const typeOfArgument = getConstrainedTypeAtLocation( - services, - typeGuardAssertedArgument.argument, + // Use the widened type to bypass excess property checking + const argumentType = toWidenedType( + checker, + services.getTypeAtLocation(typeGuardAssertedArgument.argument), ); - if (typeOfArgument === typeGuardAssertedArgument.type) { + + if ( + checker.isTypeAssignableTo( + argumentType, + typeGuardAssertedArgument.type, + ) + ) { context.report({ node: typeGuardAssertedArgument.argument, messageId: 'typeGuardAlreadyIsType', 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..7cfc8e343b18 --- /dev/null +++ b/packages/eslint-plugin/src/util/isObjectType.ts @@ -0,0 +1,38 @@ +import { checkServerIdentity } from 'node:tls'; +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/rules/no-unnecessary-condition.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts index 80d81bce079c..b6018d68cfb3 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,19 @@ 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 }], + }, ], invalid: [ @@ -3846,5 +3844,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: 'typeGuardAlreadyIsType', + }, + ], + options: [{ checkTypePredicates: true }], + }, + { + code: ` +declare function isString(x: unknown): x is string; +isString('falafel'); + `, + errors: [ + { + column: 10, + endColumn: 19, + endLine: 3, + line: 3, + messageId: 'typeGuardAlreadyIsType', + }, + ], + 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: 'typeGuardAlreadyIsType', + }, + ], + options: [{ checkTypePredicates: true }], + }, + { + code: ` +type Foo = { a: string }; + +declare function isUnion(x: unknown): x is Foo; + +isUnion({ a: 'foo', excess: 'property' }); + `, + errors: [ + { + messageId: 'typeGuardAlreadyIsType', + }, + ], + 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: 'typeGuardAlreadyIsType', + }, + ], + 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: 'typeGuardAlreadyIsType', + }, + ], + options: [{ checkTypePredicates: true }], + }, ], }); From 5cf27d89d4e12a33e2247fd137af8481303bcac7 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger <53019676+kirkwaiblinger@users.noreply.github.com> Date: Wed, 26 Nov 2025 00:35:11 -0700 Subject: [PATCH 2/5] tweak wording --- .../src/rules/no-unnecessary-condition.ts | 8 ++++---- .../rules/no-unnecessary-condition.test.ts | 18 +++++++++--------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 33f88af2a347..dea640d7c398 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -154,7 +154,7 @@ export type MessageId = | 'noOverlapBooleanExpression' | 'noStrictNullCheck' | 'suggestRemoveOptionalChain' - | 'typeGuardAlreadyIsType'; + | 'typeAssertionArgumentAlreadyAssignable'; export default createRule({ name: 'no-unnecessary-condition', @@ -187,8 +187,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: [ { @@ -610,7 +610,7 @@ export default createRule({ ) { context.report({ node: typeGuardAssertedArgument.argument, - messageId: 'typeGuardAlreadyIsType', + messageId: 'typeAssertionArgumentAlreadyAssignable', data: { typeGuardOrAssertionFunction: typeGuardAssertedArgument.asserts ? '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 b6018d68cfb3..41cb9d321df6 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -3476,7 +3476,7 @@ assertsString(a); errors: [ { line: 4, - messageId: 'typeGuardAlreadyIsType', + messageId: 'typeAssertionArgumentAlreadyAssignable', }, ], options: [{ checkTypePredicates: true }], @@ -3490,7 +3490,7 @@ isString(a); errors: [ { line: 4, - messageId: 'typeGuardAlreadyIsType', + messageId: 'typeAssertionArgumentAlreadyAssignable', }, ], options: [{ checkTypePredicates: true }], @@ -3504,7 +3504,7 @@ isString('fa' + 'lafel'); errors: [ { line: 4, - messageId: 'typeGuardAlreadyIsType', + messageId: 'typeAssertionArgumentAlreadyAssignable', }, ], options: [{ checkTypePredicates: true }], @@ -3855,7 +3855,7 @@ assertString('falafel'); endColumn: 23, endLine: 3, line: 3, - messageId: 'typeGuardAlreadyIsType', + messageId: 'typeAssertionArgumentAlreadyAssignable', }, ], options: [{ checkTypePredicates: true }], @@ -3871,7 +3871,7 @@ isString('falafel'); endColumn: 19, endLine: 3, line: 3, - messageId: 'typeGuardAlreadyIsType', + messageId: 'typeAssertionArgumentAlreadyAssignable', }, ], options: [{ checkTypePredicates: true }], @@ -3887,7 +3887,7 @@ isUnion({ a: 'foo' }); `, errors: [ { - messageId: 'typeGuardAlreadyIsType', + messageId: 'typeAssertionArgumentAlreadyAssignable', }, ], options: [{ checkTypePredicates: true }], @@ -3902,7 +3902,7 @@ isUnion({ a: 'foo', excess: 'property' }); `, errors: [ { - messageId: 'typeGuardAlreadyIsType', + messageId: 'typeAssertionArgumentAlreadyAssignable', }, ], options: [{ checkTypePredicates: true }], @@ -3920,7 +3920,7 @@ isUnion(u); `, errors: [ { - messageId: 'typeGuardAlreadyIsType', + messageId: 'typeAssertionArgumentAlreadyAssignable', }, ], options: [{ checkTypePredicates: true }], @@ -3943,7 +3943,7 @@ isT1(t2); `, errors: [ { - messageId: 'typeGuardAlreadyIsType', + messageId: 'typeAssertionArgumentAlreadyAssignable', }, ], options: [{ checkTypePredicates: true }], From fa3a3b2d40ccc88b70a21d790021aaee9d7da141 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger <53019676+kirkwaiblinger@users.noreply.github.com> Date: Wed, 26 Nov 2025 00:41:58 -0700 Subject: [PATCH 3/5] update documentation --- .../docs/rules/no-unnecessary-condition.mdx | 6 ++++++ .../no-unnecessary-condition.shot | 11 +++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) 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/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. From dc700c80025c26da8b96a746474d8f2af2de0ef0 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger <53019676+kirkwaiblinger@users.noreply.github.com> Date: Wed, 26 Nov 2025 00:47:17 -0700 Subject: [PATCH 4/5] remove accidental import --- packages/eslint-plugin/src/util/isObjectType.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/eslint-plugin/src/util/isObjectType.ts b/packages/eslint-plugin/src/util/isObjectType.ts index 7cfc8e343b18..8084aa3be5cc 100644 --- a/packages/eslint-plugin/src/util/isObjectType.ts +++ b/packages/eslint-plugin/src/util/isObjectType.ts @@ -1,4 +1,3 @@ -import { checkServerIdentity } from 'node:tls'; import * as tsutils from 'ts-api-utils'; import * as ts from 'typescript'; From c17df5c62ccffe425ed3fefc17fffec2c72597d0 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger <53019676+kirkwaiblinger@users.noreply.github.com> Date: Wed, 26 Nov 2025 01:36:20 -0700 Subject: [PATCH 5/5] big think --- .../src/rules/class-methods-use-this.ts | 2 +- .../src/rules/no-unnecessary-condition.ts | 52 +++++++++++++++---- .../rules/no-unnecessary-type-parameters.ts | 1 + .../rules/no-unnecessary-condition.test.ts | 51 ++++++++++++++++++ .../eslint-plugin/typings/typescript.d.ts | 3 ++ .../src/utils/flat-config-schema.ts | 2 + 6 files changed, 99 insertions(+), 12 deletions(-) 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 dea640d7c398..2f727b7ec170 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -19,6 +19,7 @@ import { isPossiblyTruthy, isTypeAnyType, isTypeFlagSet, + isTypeNeverType, isTypeUnknownType, nullThrows, NullThrowsReasons, @@ -596,18 +597,47 @@ export default createRule({ node, ); if (typeGuardAssertedArgument != null) { - // Use the widened type to bypass excess property checking - const argumentType = toWidenedType( - checker, - services.getTypeAtLocation(typeGuardAssertedArgument.argument), - ); + const shouldReport = (() => { + const argumentType = services.getTypeAtLocation( + typeGuardAssertedArgument.argument, + ); - if ( - checker.isTypeAssignableTo( - argumentType, - typeGuardAssertedArgument.type, - ) - ) { + 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: 'typeAssertionArgumentAlreadyAssignable', 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/tests/rules/no-unnecessary-condition.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts index 41cb9d321df6..d10b3037079d 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -1157,6 +1157,57 @@ 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: [ 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 {