diff --git a/README.md b/README.md index 853b14e40..8cdf434aa 100644 --- a/README.md +++ b/README.md @@ -394,9 +394,10 @@ Manually fixable by ### Requires Type Checking -| Name           | Description | 💼 | ⚠️ | 🔧 | 💡 | -| :--------------------------------------------- | :----------------------------------------------------------- | :-- | :-- | :-- | :-- | -| [unbound-method](docs/rules/unbound-method.md) | Enforce unbound methods are called with their expected scope | | | | | +| Name                     | Description | 💼 | ⚠️ | 🔧 | 💡 | +| :----------------------------------------------------------------- | :----------------------------------------------------------- | :-- | :-- | :-- | :-- | +| [no-unnecessary-assertion](docs/rules/no-unnecessary-assertion.md) | Disallow unnecessary assertions based on types | | | | | +| [unbound-method](docs/rules/unbound-method.md) | Enforce unbound methods are called with their expected scope | | | | | diff --git a/docs/rules/no-unnecessary-assertion.md b/docs/rules/no-unnecessary-assertion.md new file mode 100644 index 000000000..6452a9f5a --- /dev/null +++ b/docs/rules/no-unnecessary-assertion.md @@ -0,0 +1,35 @@ +# Disallow unnecessary assertions based on types (`no-unnecessary-assertion`) + +💭 This rule requires +[type information](https://typescript-eslint.io/linting/typed-linting). + + + +When using TypeScript, runtime assertions based on types can often be omitted +provided that the types are accurate. + +## Rule details + +This rule warns when you do an assertion about being `null` or `undefined` on +something that cannot be those types, as that indicates either the assertion can +be removed or the types need to be adjusted. + +The following patterns are considered warnings: + +```ts +expect('hello world'.match('sunshine') ?? []).toBeNull(); + +expect(User.findOrThrow(1)).toBeDefined(); + +expect(map.getOrInsert('key', 'default')).not.toBeUndefined(); +``` + +The following patterns are not considered warnings: + +```ts +expect('hello world'.match('sunshine')).toBeNull(); + +expect(User.findOrNull(1)).toBeDefined(); + +expect(map.get('key')).not.toBeUndefined(); +``` diff --git a/src/__tests__/__snapshots__/rules.test.ts.snap b/src/__tests__/__snapshots__/rules.test.ts.snap index 307015837..b993b0a7d 100644 --- a/src/__tests__/__snapshots__/rules.test.ts.snap +++ b/src/__tests__/__snapshots__/rules.test.ts.snap @@ -36,6 +36,7 @@ exports[`rules should export configs that refer to actual rules 1`] = ` "jest/no-standalone-expect": "error", "jest/no-test-prefixes": "error", "jest/no-test-return-statement": "error", + "jest/no-unnecessary-assertion": "error", "jest/no-unneeded-async-expect-function": "error", "jest/no-untyped-mock-factory": "error", "jest/padding-around-after-all-blocks": "error", @@ -132,6 +133,7 @@ exports[`rules should export configs that refer to actual rules 1`] = ` "jest/no-standalone-expect": "error", "jest/no-test-prefixes": "error", "jest/no-test-return-statement": "error", + "jest/no-unnecessary-assertion": "error", "jest/no-unneeded-async-expect-function": "error", "jest/no-untyped-mock-factory": "error", "jest/padding-around-after-all-blocks": "error", diff --git a/src/__tests__/rules.test.ts b/src/__tests__/rules.test.ts index 763bff5ac..04fe70af8 100644 --- a/src/__tests__/rules.test.ts +++ b/src/__tests__/rules.test.ts @@ -2,7 +2,7 @@ import { existsSync } from 'fs'; import { resolve } from 'path'; import plugin from '../'; -const numberOfRules = 67; +const numberOfRules = 68; const ruleNames = Object.keys(plugin.rules); const deprecatedRules = Object.entries(plugin.rules) .filter(([, rule]) => rule.meta.deprecated) diff --git a/src/rules/__tests__/no-unnecessary-assertion.test.ts b/src/rules/__tests__/no-unnecessary-assertion.test.ts new file mode 100644 index 000000000..54bc7ac91 --- /dev/null +++ b/src/rules/__tests__/no-unnecessary-assertion.test.ts @@ -0,0 +1,480 @@ +import path from 'path'; +import type { TSESLint } from '@typescript-eslint/utils'; +import dedent from 'dedent'; +import type { MessageIds, Options } from '../no-unnecessary-assertion'; +import { FlatCompatRuleTester as RuleTester } from './test-utils'; + +function getFixturesRootDir(): string { + return path.join(__dirname, 'fixtures'); +} + +const rootPath = getFixturesRootDir(); + +const ruleTester = new RuleTester({ + parser: require.resolve('@typescript-eslint/parser'), + parserOptions: { + sourceType: 'module', + tsconfigRootDir: rootPath, + project: './tsconfig.json', + disallowAutomaticSingleRunInference: true, + }, +}); + +const fixtureFilename = path.join(rootPath, 'file.ts'); + +const requireRule = (throwWhenRequiring: boolean) => { + jest.resetModules(); + + TSESLintPluginRef.throwWhenRequiring = throwWhenRequiring; + + // eslint-disable-next-line @typescript-eslint/no-require-imports + return require('../no-unnecessary-assertion').default; +}; + +const TSESLintPluginRef: { throwWhenRequiring: boolean } = { + throwWhenRequiring: false, +}; + +jest.mock('typescript', () => { + if (TSESLintPluginRef.throwWhenRequiring) { + throw new (class extends Error { + public code; + + constructor(message?: string) { + super(message); + this.code = 'MODULE_NOT_FOUND'; + } + })(); + } + + return jest.requireActual('typescript'); +}); + +describe('error handling', () => { + afterAll(() => { + TSESLintPluginRef.throwWhenRequiring = false; + }); + + it('does not require typescript until the rule is actually created', () => { + expect(() => requireRule(true)).not.toThrow(); + }); +}); + +const withFixtureFilename = < + T extends Array< + | (TSESLint.ValidTestCase | string) + | TSESLint.InvalidTestCase + >, +>( + cases: T, +): T extends Array> + ? Array> + : Array> => { + // @ts-expect-error this is fine, and will go away later once we upgrade + return cases.map(code => { + const test = typeof code === 'string' ? { code } : code; + + return { filename: fixtureFilename, ...test }; + }); +}; + +ruleTester.run('no-unnecessary-assertion (general)', requireRule(false), { + valid: withFixtureFilename([ + 'expect', + 'expect.hasAssertions', + 'expect.hasAssertions()', + 'expect(a).toBe(b)', + ]), + invalid: [ + { + filename: path.join(rootPath, 'unstrict', 'file.ts'), + code: 'expect(x).toBe(y);', + parserOptions: { + tsconfigRootDir: path.join(rootPath, 'unstrict'), + }, + errors: [ + { + messageId: 'noStrictNullCheck', + line: 0, + }, + ], + }, + ], +}); + +const generateValidCases = ( + matcher: 'toBeNull' | 'toBeUndefined' | 'toBeDefined', + thing: 'null' | 'undefined', +) => { + return [ + `expect.${matcher}`, + `expect.${matcher}()`, + dedent` + const add = (a, b) => a + b; + + expect(add(1, 1)).toBe(2); + `, + dedent` + const add = (a: number, b: number) => a + b; + + expect(add(1, 1)).toBe(2); + `, + dedent` + const add = (a: number, b: number): number => a + b; + + expect(add(1, 1)).toBe(2); + `, + dedent` + const add = (a: number, b: number): number | ${thing} => a + b; + + expect(add(1, 1)).not.${matcher}(); + `, + dedent` + declare function mx(): string | ${thing}; + + expect(mx()).${matcher}(); + expect(mx()).not.${matcher}(); + `, + dedent` + declare function mx(p: T): T | ${thing}; + + expect(mx(${thing})).${matcher}(); + expect(mx(${thing})).not.${matcher}(); + + expect(mx('hello')).${matcher}(); + expect(mx('world')).not.${matcher}(); + `, + `expect(${thing}).not.${matcher}()`, + `expect("hello" as ${thing}).${matcher}();`, + ]; +}; + +const generateInvalidCases = ( + matcher: 'toBeNull' | 'toBeUndefined' | 'toBeDefined', + thing: 'null' | 'undefined', +): Array> => { + return [ + { + code: `expect(0).${matcher}()`, + errors: [ + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 1, + }, + ], + }, + { + code: `expect("hello world").${matcher}()`, + errors: [ + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 1, + }, + ], + }, + { + code: `expect({}).${matcher}()`, + errors: [ + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 1, + }, + ], + }, + { + code: `expect([]).not.${matcher}()`, + errors: [ + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 1, + }, + ], + }, + { + code: dedent` + const x = 0; + + expect(x).${matcher}() + expect(x).not.${matcher}() + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 3, + }, + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 4, + }, + ], + }, + { + code: dedent` + const add = (a: number, b: number) => a + b; + + expect(add(1, 1)).${matcher}(); + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 3, + }, + ], + }, + { + code: dedent` + const add = (a: number, b: number): number => a + b; + + expect(add(1, 1)).${matcher}(); + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 3, + }, + ], + }, + { + code: dedent` + const add = (a: number, b: number): number => a + b; + + expect(add(1, 1)).not.${matcher}(); + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 3, + }, + ], + }, + + { + code: dedent` + const result = "hello world".match("sunshine") ?? []; + + expect(result).not.${matcher}(); + expect(result).${matcher}(); + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 3, + }, + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 4, + }, + ], + }, + { + code: dedent` + const result = "hello world".match("sunshine") || []; + + expect(result).not.${matcher}(); + expect(result).${matcher}(); + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 3, + }, + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 4, + }, + ], + }, + + // todo: ideally support these + // { + // code: dedent` + // let result = "hello world".match("sunshine"); + // + // result ??= [] + // + // expect(result).not.${matcher}(); + // expect(result).${matcher}(); + // `, + // errors: [ + // { + // messageId: 'unnecessaryAssertion', + // data: { thing }, + // line: 5, + // }, + // { + // messageId: 'unnecessaryAssertion', + // data: { thing }, + // line: 6, + // }, + // ], + // }, + // { + // code: dedent` + // let result = "hello world".match("sunshine"); + // + // result ||= [] + // + // expect(result).not.${matcher}(); + // expect(result).${matcher}(); + // `, + // errors: [ + // { + // messageId: 'unnecessaryAssertion', + // data: { thing }, + // line: 5, + // }, + // { + // messageId: 'unnecessaryAssertion', + // data: { thing }, + // line: 6, + // }, + // ], + // }, + + { + code: dedent` + declare function mx(): string | number; + + expect(mx()).${matcher}(); + expect(mx()).not.${matcher}(); + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 3, + }, + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 4, + }, + ], + }, + { + code: dedent` + declare function mx(p: T): T; + + expect(mx(${thing})).${matcher}(); + expect(mx(${thing})).not.${matcher}(); + + expect(mx('hello')).${matcher}(); + expect(mx('world')).not.${matcher}(); + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 6, + }, + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 7, + }, + ], + }, + { + code: dedent` + declare function mx(p: T): T extends string ? ${thing} : T; + + expect(mx('hello')).${matcher}(); + expect(mx('world')).not.${matcher}(); + + expect(mx({})).${matcher}(); + expect(mx({})).not.${matcher}(); + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 6, + }, + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 7, + }, + ], + }, + + { + code: dedent` + declare function mx(): string | ${thing}; + + expect(mx()!).${matcher}(); + expect(mx()!).not.${matcher}(); + + expect(mx() as string).${matcher}(); + expect(mx() as string).not.${matcher}(); + + expect(mx() as number).${matcher}(); + expect(mx() as number).not.${matcher}(); + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 3, + }, + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 4, + }, + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 6, + }, + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 7, + }, + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 9, + }, + { + messageId: 'unnecessaryAssertion', + data: { thing }, + line: 10, + }, + ], + }, + ]; +}; + +ruleTester.run('no-unnecessary-assertion (toBeNull)', requireRule(false), { + valid: withFixtureFilename(generateValidCases('toBeNull', 'null')), + invalid: withFixtureFilename(generateInvalidCases('toBeNull', 'null')), +}); + +ruleTester.run('no-unnecessary-assertion (toBeDefined)', requireRule(false), { + valid: withFixtureFilename(generateValidCases('toBeDefined', 'undefined')), + invalid: withFixtureFilename( + generateInvalidCases('toBeDefined', 'undefined'), + ), +}); + +ruleTester.run('no-unnecessary-assertion (toBeUndefined)', requireRule(false), { + valid: withFixtureFilename(generateValidCases('toBeUndefined', 'undefined')), + invalid: withFixtureFilename( + generateInvalidCases('toBeUndefined', 'undefined'), + ), +}); diff --git a/src/rules/no-unnecessary-assertion.ts b/src/rules/no-unnecessary-assertion.ts new file mode 100644 index 000000000..090a9e418 --- /dev/null +++ b/src/rules/no-unnecessary-assertion.ts @@ -0,0 +1,92 @@ +import { AST_NODE_TYPES, ESLintUtils } from '@typescript-eslint/utils'; +import type ts from 'typescript'; +import { createRule, getAccessorValue, parseJestFnCall } from './utils'; + +const canBe = (firstArgumentType: ts.Type, flag: ts.TypeFlags) => { + if (firstArgumentType.isUnion()) { + return firstArgumentType.types.some(typ => typ.flags & flag); + } + + return firstArgumentType.flags & flag; +}; + +export type MessageIds = 'unnecessaryAssertion' | 'noStrictNullCheck'; + +export type Options = []; + +export default createRule({ + name: __filename, + meta: { + docs: { + description: 'Disallow unnecessary assertions based on types', + requiresTypeChecking: true, + }, + messages: { + unnecessaryAssertion: + 'Unnecessary assertion, subject cannot be {{ thing }}', + noStrictNullCheck: + 'This rule requires the `strictNullChecks` compiler option to be turned on to function correctly.', + }, + type: 'suggestion', + schema: [], + }, + defaultOptions: [], + create(context) { + const services = ESLintUtils.getParserServices(context); + + const compilerOptions = services.program.getCompilerOptions(); + + const isStrictNullChecks = + compilerOptions.strictNullChecks || + (compilerOptions.strict && compilerOptions.strictNullChecks !== false); + + if (!isStrictNullChecks) { + context.report({ + loc: { + start: { column: 0, line: 0 }, + end: { column: 0, line: 0 }, + }, + messageId: 'noStrictNullCheck', + }); + } + + // eslint-disable-next-line @typescript-eslint/no-require-imports + const { TypeFlags } = require('typescript') as typeof ts; + + return { + CallExpression(node) { + const jestFnCall = parseJestFnCall(node, context); + + if ( + jestFnCall?.type !== 'expect' || + jestFnCall.head.node.parent.type !== AST_NODE_TYPES.CallExpression + ) { + return; + } + + const matcherName = getAccessorValue(jestFnCall.matcher); + + if ( + !['toBeNull', 'toBeDefined', 'toBeUndefined'].includes(matcherName) + ) { + return; + } + + const [argument] = jestFnCall.head.node.parent.arguments; + + const isNullable = canBe( + services.getTypeAtLocation(argument), + matcherName === 'toBeNull' ? TypeFlags.Null : TypeFlags.Undefined, + ); + + if (!isNullable) { + context.report({ + messageId: 'unnecessaryAssertion', + data: { thing: matcherName === 'toBeNull' ? 'null' : 'undefined' }, + node, + }); + } + }, + }; + }, +}); diff --git a/src/rules/utils/misc.ts b/src/rules/utils/misc.ts index da91249d5..374cb4966 100644 --- a/src/rules/utils/misc.ts +++ b/src/rules/utils/misc.ts @@ -16,7 +16,9 @@ import { type ParsedExpectFnCall, isTypeOfJestFnCall } from './parseJestFnCall'; const REPO_URL = 'https://github.com/jest-community/eslint-plugin-jest'; -export const createRule = ESLintUtils.RuleCreator(name => { +export const createRule = ESLintUtils.RuleCreator<{ + requiresTypeChecking?: boolean; +}>(name => { const ruleName = parsePath(name).name; return `${REPO_URL}/blob/v${version}/docs/rules/${ruleName}.md`;