From a8e8d6b803d0e218d87617f59d235585f392ff6c Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Wed, 29 Dec 2021 09:41:47 +1300 Subject: [PATCH 1/6] fix(prefer-to-contain): support square bracket accessors --- src/rules/__tests__/prefer-to-contain.test.ts | 25 +++++-- src/rules/prefer-to-contain.ts | 67 ++++++++++++------- src/rules/utils.ts | 11 +++ 3 files changed, 72 insertions(+), 31 deletions(-) diff --git a/src/rules/__tests__/prefer-to-contain.test.ts b/src/rules/__tests__/prefer-to-contain.test.ts index 387d2c2f1..79dfa43e5 100644 --- a/src/rules/__tests__/prefer-to-contain.test.ts +++ b/src/rules/__tests__/prefer-to-contain.test.ts @@ -33,11 +33,26 @@ ruleTester.run('prefer-to-contain', rule, { errors: [{ messageId: 'useToContain', column: 23, line: 1 }], }, // todo: support this, as it's counted by isSupportedAccessor - // { - // code: "expect(a['includes'](b)).toEqual(true);", - // errors: [{ messageId: 'useToContain', column: 23, line: 1 }], - // output: 'expect(a).toContain(b);', - // }, + { + code: "expect(a['includes'](b)).toEqual(true);", + output: 'expect(a).toContain(b);', + errors: [{ messageId: 'useToContain', column: 26, line: 1 }], + }, + { + code: "expect(a['includes'](b)).toEqual(false);", + output: 'expect(a).not.toContain(b);', + errors: [{ messageId: 'useToContain', column: 26, line: 1 }], + }, + { + code: "expect(a['includes'](b)).not.toEqual(false);", + output: 'expect(a).toContain(b);', + errors: [{ messageId: 'useToContain', column: 26, line: 1 }], + }, + { + code: "expect(a['includes'](b))['not'].toEqual(false);", + output: 'expect(a).toContain(b);', + errors: [{ messageId: 'useToContain', column: 26, line: 1 }], + }, { code: 'expect(a.includes(b)).toEqual(false);', output: 'expect(a).not.toContain(b);', diff --git a/src/rules/prefer-to-contain.ts b/src/rules/prefer-to-contain.ts index b13439f81..252e51ffd 100644 --- a/src/rules/prefer-to-contain.ts +++ b/src/rules/prefer-to-contain.ts @@ -11,6 +11,7 @@ import { NotNegatableParsedModifier, ParsedEqualityMatcherCall, ParsedExpectMatcher, + assertNotNull, createRule, followTypeAssertionChain, hasOnlyOneArgument, @@ -67,28 +68,29 @@ const isFixableIncludesCallExpression = ( node.type === AST_NODE_TYPES.CallExpression && node.callee.type === AST_NODE_TYPES.MemberExpression && isSupportedAccessor(node.callee.property, 'includes') && - node.callee.property.type === AST_NODE_TYPES.Identifier && + // node.callee.property.type === AST_NODE_TYPES.Identifier && hasOnlyOneArgument(node); const buildToContainFuncExpectation = (negated: boolean) => negated ? `${ModifierName.not}.toContain` : 'toContain'; /** - * Finds the first `.` character token between the `object` & `property` of the given `member` expression. + * Finds the first "accessor start" token (either a dot or opening square bracket) + * between the `object` & `property` of the given `member` expression. * * @param {TSESTree.MemberExpression} member * @param {SourceCode} sourceCode * * @return {Token | null} */ -const findPropertyDotToken = ( +const findAccessorStartToken = ( member: TSESTree.MemberExpression, sourceCode: TSESLint.SourceCode, ) => sourceCode.getFirstTokenBetween( member.object, member.property, - token => token.value === '.', + token => token.value === '.' || token.value === '[', ); const getNegationFixes = ( @@ -100,25 +102,35 @@ const getNegationFixes = ( fileName: string, ) => { const [containArg] = node.arguments; - const negationPropertyDot = findPropertyDotToken(modifier.node, sourceCode); + const negationAccessorStart = findAccessorStartToken( + modifier.node, + sourceCode, + ); const toContainFunc = buildToContainFuncExpectation( followTypeAssertionChain(matcher.arguments[0]).value, ); - /* istanbul ignore if */ - if (negationPropertyDot === null) { - throw new Error( - `Unexpected null when attempting to fix ${fileName} - please file a github issue at https://github.com/jest-community/eslint-plugin-jest`, - ); - } + assertNotNull(negationAccessorStart, fileName); - return [ - fixer.remove(negationPropertyDot), + const fixers = [ + fixer.remove(negationAccessorStart), fixer.remove(modifier.node.property), fixer.replaceText(matcher.node.property, toContainFunc), fixer.replaceText(matcher.arguments[0], sourceCode.getText(containArg)), ]; + + if (negationAccessorStart.value === '[') { + const negationAccessorStop = sourceCode.getTokenAfter( + modifier.node.property, + ); + + assertNotNull(negationAccessorStop, fileName); + + fixers.push(fixer.remove(negationAccessorStop)); + } + + return fixers; }; const getCommonFixes = ( @@ -129,29 +141,32 @@ const getCommonFixes = ( const [containArg] = node.arguments; const includesCallee = node.callee; - const propertyDot = findPropertyDotToken(includesCallee, sourceCode); + const accessorStartToken = findAccessorStartToken(includesCallee, sourceCode); const closingParenthesis = sourceCode.getTokenAfter(containArg); const openParenthesis = sourceCode.getTokenBefore(containArg); - /* istanbul ignore if */ - if ( - propertyDot === null || - closingParenthesis === null || - openParenthesis === null - ) { - throw new Error( - `Unexpected null when attempting to fix ${fileName} - please file a github issue at https://github.com/jest-community/eslint-plugin-jest`, - ); - } + assertNotNull(accessorStartToken, fileName); + assertNotNull(closingParenthesis, fileName); + assertNotNull(openParenthesis, fileName); - return [ + const fixers = [ containArg, includesCallee.property, - propertyDot, + accessorStartToken, closingParenthesis, openParenthesis, ]; + + if (accessorStartToken.value === '[') { + const accessorStopToken = sourceCode.getTokenBefore(openParenthesis); + + assertNotNull(accessorStopToken, fileName); + + fixers.push(accessorStopToken); + } + + return fixers; }; // expect(array.includes()[not.]{toBe,toEqual}() diff --git a/src/rules/utils.ts b/src/rules/utils.ts index 35635f60d..2f4313c22 100644 --- a/src/rules/utils.ts +++ b/src/rules/utils.ts @@ -15,6 +15,17 @@ export const createRule = ESLintUtils.RuleCreator(name => { return `${REPO_URL}/blob/v${version}/docs/rules/${ruleName}.md`; }); +export function assertNotNull( + value: T | null, + fileName: string, +): asserts value is T { + if (value === null) { + throw new Error( + `Unexpected null when attempting to fix ${fileName} - please file a github issue at https://github.com/jest-community/eslint-plugin-jest`, + ); + } +} + export type MaybeTypeCast = | TSTypeCastExpression | Expression; From 482ad45028960d690128bd6966c56c81c11ff886 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Wed, 29 Dec 2021 09:42:41 +1300 Subject: [PATCH 2/6] refactor(prefer-to-contain): simplify fixer --- src/rules/prefer-to-contain.ts | 125 +++++---------------------------- src/rules/utils.ts | 11 --- 2 files changed, 16 insertions(+), 120 deletions(-) diff --git a/src/rules/prefer-to-contain.ts b/src/rules/prefer-to-contain.ts index 252e51ffd..a88fcfb95 100644 --- a/src/rules/prefer-to-contain.ts +++ b/src/rules/prefer-to-contain.ts @@ -11,7 +11,6 @@ import { NotNegatableParsedModifier, ParsedEqualityMatcherCall, ParsedExpectMatcher, - assertNotNull, createRule, followTypeAssertionChain, hasOnlyOneArgument, @@ -58,9 +57,6 @@ type FixableIncludesCallExpression = KnownCallExpression<'includes'> & * @param {CallExpression} node * * @return {node is FixableIncludesCallExpression} - * - * @todo support `['includes']()` syntax (remove last property.type check to begin) - * @todo break out into `isMethodCall(node: TSESTree.Node, method: Name)` util-fn */ const isFixableIncludesCallExpression = ( node: TSESTree.Node, @@ -68,105 +64,27 @@ const isFixableIncludesCallExpression = ( node.type === AST_NODE_TYPES.CallExpression && node.callee.type === AST_NODE_TYPES.MemberExpression && isSupportedAccessor(node.callee.property, 'includes') && - // node.callee.property.type === AST_NODE_TYPES.Identifier && hasOnlyOneArgument(node); const buildToContainFuncExpectation = (negated: boolean) => negated ? `${ModifierName.not}.toContain` : 'toContain'; -/** - * Finds the first "accessor start" token (either a dot or opening square bracket) - * between the `object` & `property` of the given `member` expression. - * - * @param {TSESTree.MemberExpression} member - * @param {SourceCode} sourceCode - * - * @return {Token | null} - */ -const findAccessorStartToken = ( - member: TSESTree.MemberExpression, - sourceCode: TSESLint.SourceCode, -) => - sourceCode.getFirstTokenBetween( - member.object, - member.property, - token => token.value === '.' || token.value === '[', - ); - const getNegationFixes = ( - node: FixableIncludesCallExpression, modifier: NotNegatableParsedModifier, matcher: ParsedBooleanEqualityMatcherCall, - sourceCode: TSESLint.SourceCode, fixer: TSESLint.RuleFixer, - fileName: string, ) => { - const [containArg] = node.arguments; - const negationAccessorStart = findAccessorStartToken( - modifier.node, - sourceCode, - ); - const toContainFunc = buildToContainFuncExpectation( followTypeAssertionChain(matcher.arguments[0]).value, ); - assertNotNull(negationAccessorStart, fileName); - - const fixers = [ - fixer.remove(negationAccessorStart), - fixer.remove(modifier.node.property), + return [ + fixer.removeRange([ + modifier.node.property.range[0] - 1, + modifier.node.range[1], + ]), fixer.replaceText(matcher.node.property, toContainFunc), - fixer.replaceText(matcher.arguments[0], sourceCode.getText(containArg)), - ]; - - if (negationAccessorStart.value === '[') { - const negationAccessorStop = sourceCode.getTokenAfter( - modifier.node.property, - ); - - assertNotNull(negationAccessorStop, fileName); - - fixers.push(fixer.remove(negationAccessorStop)); - } - - return fixers; -}; - -const getCommonFixes = ( - node: FixableIncludesCallExpression, - sourceCode: TSESLint.SourceCode, - fileName: string, -): Array => { - const [containArg] = node.arguments; - const includesCallee = node.callee; - - const accessorStartToken = findAccessorStartToken(includesCallee, sourceCode); - - const closingParenthesis = sourceCode.getTokenAfter(containArg); - const openParenthesis = sourceCode.getTokenBefore(containArg); - - assertNotNull(accessorStartToken, fileName); - assertNotNull(closingParenthesis, fileName); - assertNotNull(openParenthesis, fileName); - - const fixers = [ - containArg, - includesCallee.property, - accessorStartToken, - closingParenthesis, - openParenthesis, ]; - - if (accessorStartToken.value === '[') { - const accessorStopToken = sourceCode.getTokenBefore(openParenthesis); - - assertNotNull(accessorStopToken, fileName); - - fixers.push(accessorStopToken); - } - - return fixers; }; // expect(array.includes()[not.]{toBe,toEqual}() @@ -214,40 +132,29 @@ export default createRule({ context.report({ fix(fixer) { const sourceCode = context.getSourceCode(); - const fileName = context.getFilename(); - const fixArr = getCommonFixes( - includesCall, - sourceCode, - fileName, - ).map(target => fixer.remove(target)); + const fixArr = [ + fixer.removeRange([ + includesCall.callee.property.range[0] - 1, + includesCall.range[1], + ]), + fixer.replaceText( + matcher.arguments[0], + sourceCode.getText(includesCall.arguments[0]), + ), + ]; if (modifier) { - return getNegationFixes( - includesCall, - modifier, - matcher, - sourceCode, - fixer, - fileName, - ).concat(fixArr); + return getNegationFixes(modifier, matcher, fixer).concat(fixArr); } const toContainFunc = buildToContainFuncExpectation( !followTypeAssertionChain(matcher.arguments[0]).value, ); - const [containArg] = includesCall.arguments; - fixArr.push( fixer.replaceText(matcher.node.property, toContainFunc), ); - fixArr.push( - fixer.replaceText( - matcher.arguments[0], - sourceCode.getText(containArg), - ), - ); return fixArr; }, diff --git a/src/rules/utils.ts b/src/rules/utils.ts index 2f4313c22..35635f60d 100644 --- a/src/rules/utils.ts +++ b/src/rules/utils.ts @@ -15,17 +15,6 @@ export const createRule = ESLintUtils.RuleCreator(name => { return `${REPO_URL}/blob/v${version}/docs/rules/${ruleName}.md`; }); -export function assertNotNull( - value: T | null, - fileName: string, -): asserts value is T { - if (value === null) { - throw new Error( - `Unexpected null when attempting to fix ${fileName} - please file a github issue at https://github.com/jest-community/eslint-plugin-jest`, - ); - } -} - export type MaybeTypeCast = | TSTypeCastExpression | Expression; From 33bf5cbd4648b41a288eee62812f63788cdb2a27 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Wed, 29 Dec 2021 10:03:09 +1300 Subject: [PATCH 3/6] refactor(prefer-to-contain): simplify fixer further --- src/rules/prefer-to-contain.ts | 48 +++++++++++----------------------- 1 file changed, 15 insertions(+), 33 deletions(-) diff --git a/src/rules/prefer-to-contain.ts b/src/rules/prefer-to-contain.ts index a88fcfb95..6ca12c99f 100644 --- a/src/rules/prefer-to-contain.ts +++ b/src/rules/prefer-to-contain.ts @@ -1,6 +1,5 @@ import { AST_NODE_TYPES, - TSESLint, TSESTree, } from '@typescript-eslint/experimental-utils'; import { @@ -8,7 +7,6 @@ import { KnownCallExpression, MaybeTypeCast, ModifierName, - NotNegatableParsedModifier, ParsedEqualityMatcherCall, ParsedExpectMatcher, createRule, @@ -66,27 +64,6 @@ const isFixableIncludesCallExpression = ( isSupportedAccessor(node.callee.property, 'includes') && hasOnlyOneArgument(node); -const buildToContainFuncExpectation = (negated: boolean) => - negated ? `${ModifierName.not}.toContain` : 'toContain'; - -const getNegationFixes = ( - modifier: NotNegatableParsedModifier, - matcher: ParsedBooleanEqualityMatcherCall, - fixer: TSESLint.RuleFixer, -) => { - const toContainFunc = buildToContainFuncExpectation( - followTypeAssertionChain(matcher.arguments[0]).value, - ); - - return [ - fixer.removeRange([ - modifier.node.property.range[0] - 1, - modifier.node.range[1], - ]), - fixer.replaceText(matcher.node.property, toContainFunc), - ]; -}; - // expect(array.includes()[not.]{toBe,toEqual}() export default createRule({ name: __filename, @@ -133,6 +110,8 @@ export default createRule({ fix(fixer) { const sourceCode = context.getSourceCode(); + const negated = modifier?.name === ModifierName.not; + const fixArr = [ fixer.removeRange([ includesCall.callee.property.range[0] - 1, @@ -142,20 +121,23 @@ export default createRule({ matcher.arguments[0], sourceCode.getText(includesCall.arguments[0]), ), + fixer.replaceText( + matcher.node.property, + followTypeAssertionChain(matcher.arguments[0]).value === negated + ? `${ModifierName.not}.toContain` + : 'toContain', + ), ]; - if (modifier) { - return getNegationFixes(modifier, matcher, fixer).concat(fixArr); + if (negated) { + fixArr.push( + fixer.removeRange([ + modifier.node.property.range[0] - 1, + modifier.node.range[1], + ]), + ); } - const toContainFunc = buildToContainFuncExpectation( - !followTypeAssertionChain(matcher.arguments[0]).value, - ); - - fixArr.push( - fixer.replaceText(matcher.node.property, toContainFunc), - ); - return fixArr; }, messageId: 'useToContain', From 213d30111958a08f7b0880218f6d7e6cc1e207a2 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Wed, 29 Dec 2021 10:26:05 +1300 Subject: [PATCH 4/6] refactor(prefer-to-contain): simplify fixer even further --- src/rules/__tests__/prefer-to-contain.test.ts | 11 +++++++- src/rules/prefer-to-contain.ts | 28 +++++++------------ 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/src/rules/__tests__/prefer-to-contain.test.ts b/src/rules/__tests__/prefer-to-contain.test.ts index 79dfa43e5..646c783d7 100644 --- a/src/rules/__tests__/prefer-to-contain.test.ts +++ b/src/rules/__tests__/prefer-to-contain.test.ts @@ -32,12 +32,16 @@ ruleTester.run('prefer-to-contain', rule, { output: 'expect(a).toContain(b);', errors: [{ messageId: 'useToContain', column: 23, line: 1 }], }, - // todo: support this, as it's counted by isSupportedAccessor { code: "expect(a['includes'](b)).toEqual(true);", output: 'expect(a).toContain(b);', errors: [{ messageId: 'useToContain', column: 26, line: 1 }], }, + { + code: "expect(a['includes'](b))['toEqual'](true);", + output: 'expect(a).toContain(b);', + errors: [{ messageId: 'useToContain', column: 26, line: 1 }], + }, { code: "expect(a['includes'](b)).toEqual(false);", output: 'expect(a).not.toContain(b);', @@ -53,6 +57,11 @@ ruleTester.run('prefer-to-contain', rule, { output: 'expect(a).toContain(b);', errors: [{ messageId: 'useToContain', column: 26, line: 1 }], }, + { + code: "expect(a['includes'](b))['not']['toEqual'](false);", + output: 'expect(a).toContain(b);', + errors: [{ messageId: 'useToContain', column: 26, line: 1 }], + }, { code: 'expect(a.includes(b)).toEqual(false);', output: 'expect(a).not.toContain(b);', diff --git a/src/rules/prefer-to-contain.ts b/src/rules/prefer-to-contain.ts index 6ca12c99f..427d2bd6f 100644 --- a/src/rules/prefer-to-contain.ts +++ b/src/rules/prefer-to-contain.ts @@ -91,6 +91,7 @@ export default createRule({ const { expect: { arguments: [includesCall], + range: [, expectCallEnd], }, matcher, modifier, @@ -110,9 +111,11 @@ export default createRule({ fix(fixer) { const sourceCode = context.getSourceCode(); - const negated = modifier?.name === ModifierName.not; + const addNotModifier = + followTypeAssertionChain(matcher.arguments[0]).value === + !!modifier; - const fixArr = [ + return [ fixer.removeRange([ includesCall.callee.property.range[0] - 1, includesCall.range[1], @@ -121,24 +124,13 @@ export default createRule({ matcher.arguments[0], sourceCode.getText(includesCall.arguments[0]), ), - fixer.replaceText( - matcher.node.property, - followTypeAssertionChain(matcher.arguments[0]).value === negated - ? `${ModifierName.not}.toContain` - : 'toContain', + fixer.replaceTextRange( + [expectCallEnd, matcher.node.range[1]], + addNotModifier + ? `.${ModifierName.not}.toContain` + : '.toContain', ), ]; - - if (negated) { - fixArr.push( - fixer.removeRange([ - modifier.node.property.range[0] - 1, - modifier.node.range[1], - ]), - ); - } - - return fixArr; }, messageId: 'useToContain', node: (modifier || matcher).node.property, From 1d6457c9d612bcf3af13c5555d4fc4456b6f44f9 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Wed, 29 Dec 2021 10:27:26 +1300 Subject: [PATCH 5/6] chore(prefer-to-contain): add comments --- src/rules/prefer-to-contain.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/rules/prefer-to-contain.ts b/src/rules/prefer-to-contain.ts index 427d2bd6f..a120e3f5f 100644 --- a/src/rules/prefer-to-contain.ts +++ b/src/rules/prefer-to-contain.ts @@ -111,19 +111,24 @@ export default createRule({ fix(fixer) { const sourceCode = context.getSourceCode(); + // we need to negate the expectation if the current expected + // value is itself negated by the "not" modifier const addNotModifier = followTypeAssertionChain(matcher.arguments[0]).value === !!modifier; return [ + // remove the "includes" call entirely fixer.removeRange([ includesCall.callee.property.range[0] - 1, includesCall.range[1], ]), + // replace the matcher argument with the value from the "includes" fixer.replaceText( matcher.arguments[0], sourceCode.getText(includesCall.arguments[0]), ), + // replace the current matcher with "toContain", adding "not" if needed fixer.replaceTextRange( [expectCallEnd, matcher.node.range[1]], addNotModifier From 6b2910722685f78a322ee1a31a0877c1fd56c4e0 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Wed, 29 Dec 2021 10:29:59 +1300 Subject: [PATCH 6/6] refactor(prefer-to-contain): swap order of fixers to read better --- src/rules/prefer-to-contain.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/rules/prefer-to-contain.ts b/src/rules/prefer-to-contain.ts index a120e3f5f..17a8dfddc 100644 --- a/src/rules/prefer-to-contain.ts +++ b/src/rules/prefer-to-contain.ts @@ -123,11 +123,6 @@ export default createRule({ includesCall.callee.property.range[0] - 1, includesCall.range[1], ]), - // replace the matcher argument with the value from the "includes" - fixer.replaceText( - matcher.arguments[0], - sourceCode.getText(includesCall.arguments[0]), - ), // replace the current matcher with "toContain", adding "not" if needed fixer.replaceTextRange( [expectCallEnd, matcher.node.range[1]], @@ -135,6 +130,11 @@ export default createRule({ ? `.${ModifierName.not}.toContain` : '.toContain', ), + // replace the matcher argument with the value from the "includes" + fixer.replaceText( + matcher.arguments[0], + sourceCode.getText(includesCall.arguments[0]), + ), ]; }, messageId: 'useToContain',