Repository: playwright-community/eslint-plugin-playwright (ts)

Issue:
no-useless-not is unaware of `enabled`/`visible` parameter
this reports "Unexpected usage of not.toBeVisible(). Use toBeHidden() instead", but using toBeHidden can't be used with a parameter.
```js
let visible = false;
// reports no-useless-not
await expect(page.locator('#selector')).not.toBeVisible({ visible });
```
same for `toBeEnabled`.

The lint was added in https://github.com/playwright-community/eslint-plugin-playwright/pull/81. Shortly thereafter, playwright 1.26 was released which added these parameters.

Pull request description:
Add support for `visible` and `enabled` options in `no-useless-not`
Fixes #260

Patch:
diff --git a/src/rules/no-useless-not.ts b/src/rules/no-useless-not.ts
index 0944cb8..d53fdf6 100644
--- a/src/rules/no-useless-not.ts
+++ b/src/rules/no-useless-not.ts
@@ -1,15 +1,45 @@
 import { Rule } from 'eslint';
-import { getStringValue } from '../utils/ast';
-import { getRangeOffset, replaceAccessorFixer } from '../utils/fixer';
-import { parseFnCall } from '../utils/parseFnCall';
+import * as ESTree from 'estree';
+import { getStringValue, isBooleanLiteral } from '../utils/ast';
+import {
+  getRangeOffset,
+  removePropertyFixer,
+  replaceAccessorFixer,
+} from '../utils/fixer';
+import { truthy } from '../utils/misc';
+import { type ParsedExpectFnCall, parseFnCall } from '../utils/parseFnCall';
 
-const matcherMap: Record<string, string> = {
-  toBeDisabled: 'toBeEnabled',
-  toBeEnabled: 'toBeDisabled',
-  toBeHidden: 'toBeVisible',
-  toBeVisible: 'toBeHidden',
+const matcherConfig: Record<string, { argName?: string; inverse: string }> = {
+  toBeDisabled: { inverse: 'toBeEnabled' },
+  toBeEnabled: {
+    argName: 'enabled',
+    inverse: 'toBeDisabled',
+  },
+  toBeHidden: { inverse: 'toBeVisible' },
+  toBeVisible: {
+    argName: 'visible',
+    inverse: 'toBeHidden',
+  },
 };
 
+function getOptions(call: ParsedExpectFnCall, name: string) {
+  const [arg] = call.matcherArgs;
+  if (arg?.type !== 'ObjectExpression') return;
+
+  const property = arg.properties.find(
+    (p): p is ESTree.Property =>
+      p.type === 'Property' &&
+      getStringValue(p.key) === name &&
+      isBooleanLiteral(p.value),
+  );
+
+  return {
+    arg,
+    property,
+    value: (property?.value as { value: boolean })?.value,
+  };
+}
+
 export default {
   create(context) {
     return {
@@ -17,34 +47,63 @@ export default {
         const call = parseFnCall(context, node);
         if (call?.type !== 'expect') return;
 
-        // As the name implies, this rule only implies if the not modifier is
-        // part of the matcher chain
+        // This rule only applies to specific matchers that have opposites
+        const config = matcherConfig[call.matcherName];
+        if (!config) return;
+
+        // If the matcher has an options argument, we need to check if it has
+        // a `visible` or `enabled` property that is a boolean literal.
+        const options = config.argName
+          ? getOptions(call, config.argName)
+          : undefined;
+
+        // If an argument is provided to the `visible` or `enabled` property, but
+        // we can't determine it's value, we can't safely remove the `not` modifier.
+        if (options?.arg && options.value === undefined) return;
+
         const notModifier = call.modifiers.find(
           (mod) => getStringValue(mod) === 'not',
         );
-        if (!notModifier) return;
 
-        // This rule only applies to specific matchers that have opposites
-        const matcherName = call.matcherName;
-        if (matcherName in matcherMap) {
-          const newMatcher = matcherMap[matcherName];
+        // If the matcher is not negated, or the matcher has no options, we can
+        // safely ignore it.
+        if (!notModifier && !options?.property) return;
+
+        // If the matcher is inverted, we need to remove the `not` modifier and
+        // replace the matcher with it's inverse.
+        const isInverted = !!notModifier !== (options?.value === false);
+        const newMatcherName = isInverted ? config.inverse : call.matcherName;
 
-          context.report({
-            data: { new: newMatcher, old: matcherName },
-            fix: (fixer) => [
-              fixer.removeRange([
-                notModifier.range![0] - getRangeOffset(notModifier),
-                notModifier.range![1] + 1,
-              ]),
-              replaceAccessorFixer(fixer, call.matcher, newMatcher),
-            ],
-            loc: {
-              end: call.matcher.loc!.end,
-              start: notModifier.loc!.start,
-            },
-            messageId: 'noUselessNot',
-          });
-        }
+        context.report({
+          data: {
+            new: newMatcherName,
+            old: call.matcherName,
+            property: config.argName ?? '',
+          },
+          fix: (fixer) => {
+            return [
+              // Remove the `not` modifier if it exists
+              notModifier &&
+                fixer.removeRange([
+                  notModifier.range![0] - getRangeOffset(notModifier),
+                  notModifier.range![1] + 1,
+                ]),
+              // Remove the `visible` or `enabled` property if it exists
+              options?.property && removePropertyFixer(fixer, options.property),
+              // Swap the matcher name if it's different
+              call.matcherName !== newMatcherName &&
+                replaceAccessorFixer(fixer, call.matcher, newMatcherName),
+            ].filter(truthy);
+          },
+          loc: notModifier
+            ? { end: call.matcher.loc!.end, start: notModifier.loc!.start }
+            : options!.property!.loc!,
+          messageId: notModifier
+            ? 'noUselessNot'
+            : isInverted
+            ? 'noUselessProperty'
+            : 'noUselessTruthy',
+        });
       },
     };
   },
@@ -58,6 +117,10 @@ export default {
     fixable: 'code',
     messages: {
       noUselessNot: 'Unexpected usage of not.{{old}}(). Use {{new}}() instead.',
+      noUselessProperty:
+        "Unexpected usage of '{{old}}({ {{property}}: false })'. Use '{{new}}()' instead.",
+      noUselessTruthy:
+        "Unexpected usage of '{{old}}({ {{property}}: true })'. Use '{{new}}()' instead.",
     },
     type: 'problem',
   },
diff --git a/src/utils/ast.ts b/src/utils/ast.ts
index e4ce05a..abcda59 100644
--- a/src/utils/ast.ts
+++ b/src/utils/ast.ts
@@ -27,7 +27,11 @@ export function isIdentifier(node: ESTree.Node, name?: string | RegExp) {
   );
 }
 
-function isLiteral<T>(node: ESTree.Node, type: string, value?: T) {
+function isLiteral<T>(
+  node: ESTree.Node,
+  type: string,
+  value?: T,
+): node is ESTree.Literal {
   return (
     node.type === 'Literal' &&
     (value === undefined
@@ -44,11 +48,17 @@ const isTemplateLiteral = (
   node.quasis.length === 1 && // bail out if not simple
   (value === undefined || node.quasis[0].value.raw === value);
 
-export function isStringLiteral(node: ESTree.Node, value?: string) {
+export function isStringLiteral(
+  node: ESTree.Node,
+  value?: string,
+): node is ESTree.Literal {
   return isLiteral(node, 'string', value);
 }
 
-export function isBooleanLiteral(node: ESTree.Node, value?: boolean) {
+export function isBooleanLiteral(
+  node: ESTree.Node,
+  value?: boolean,
+): node is ESTree.Literal {
   return isLiteral(node, 'boolean', value);
 }
 
diff --git a/src/utils/fixer.ts b/src/utils/fixer.ts
index e7705f9..1c79b8d 100644
--- a/src/utils/fixer.ts
+++ b/src/utils/fixer.ts
@@ -1,5 +1,6 @@
-import { Rule } from 'eslint';
+import { AST, Rule } from 'eslint';
 import ESTree from 'estree';
+import { getParent } from './ast';
 
 export const getRangeOffset = (node: ESTree.Node) =>
   node.type === 'Identifier' ? 0 : 1;
@@ -22,3 +23,29 @@ export function replaceAccessorFixer(
     text,
   );
 }
+
+/**
+ * Removes an object property, and if it's parent object contains no other keys,
+ * removes the object in it's entirety.
+ */
+export function removePropertyFixer(
+  fixer: Rule.RuleFixer,
+  property: ESTree.Property,
+) {
+  const parent = getParent(property);
+  if (parent?.type !== 'ObjectExpression') return;
+
+  // If the property is the only one in the object, remove the entire object.
+  if (parent.properties.length === 1) {
+    return fixer.remove(parent);
+  }
+
+  // If the property is the first in the object, remove the trailing comma,
+  // otherwise remove the property and the preceding comma.
+  const index = parent.properties.indexOf(property);
+  const range: AST.Range = index
+    ? [parent.properties[index - 1].range![1], property.range![1]]
+    : [property.range![0], parent.properties[1].range![0]];
+
+  return fixer.removeRange(range);
+}


Test patch:
diff --git a/src/rules/no-useless-not.test.ts b/src/rules/no-useless-not.test.ts
index 76a2f14..3b6cad9 100644
--- a/src/rules/no-useless-not.test.ts
+++ b/src/rules/no-useless-not.test.ts
@@ -16,6 +16,92 @@ runRuleTester('no-useless-not', rule, {
       ],
       output: 'expect(locator).toBeHidden()',
     },
+    {
+      code: 'expect(locator).not.toBeVisible({ visible: true })',
+      errors: [
+        {
+          column: 17,
+          data: { new: 'toBeHidden', old: 'toBeVisible' },
+          endColumn: 32,
+          line: 1,
+          messageId: 'noUselessNot',
+        },
+      ],
+      output: 'expect(locator).toBeHidden()',
+    },
+    {
+      code: 'expect(locator).toBeVisible({ visible: true })',
+      errors: [
+        {
+          column: 31,
+          data: {
+            new: 'toBeVisible',
+            old: 'toBeVisible',
+            property: 'visible',
+          },
+          endColumn: 44,
+          line: 1,
+          messageId: 'noUselessTruthy',
+        },
+      ],
+      output: 'expect(locator).toBeVisible()',
+    },
+    {
+      code: 'expect(locator).toBeVisible({ visible: false })',
+      errors: [
+        {
+          column: 31,
+          data: {
+            new: 'toBeHidden',
+            old: 'toBeVisible',
+            property: 'visible',
+          },
+          endColumn: 45,
+          line: 1,
+          messageId: 'noUselessProperty',
+        },
+      ],
+      output: 'expect(locator).toBeHidden()',
+    },
+    {
+      code: 'expect(locator).not.toBeVisible({ timeout: 1000, visible: true })',
+      errors: [
+        {
+          column: 17,
+          data: { new: 'toBeHidden', old: 'toBeVisible' },
+          endColumn: 32,
+          line: 1,
+          messageId: 'noUselessNot',
+        },
+      ],
+      output: 'expect(locator).toBeHidden({ timeout: 1000 })',
+    },
+    {
+      code: 'expect(locator).not.toBeVisible({ visible: true, timeout: 1000 })',
+      errors: [
+        {
+          column: 17,
+          data: { new: 'toBeHidden', old: 'toBeVisible' },
+          endColumn: 32,
+          line: 1,
+          messageId: 'noUselessNot',
+        },
+      ],
+      output: 'expect(locator).toBeHidden({ timeout: 1000 })',
+    },
+    {
+      code: 'expect(locator).not.toBeVisible({ visible: false })',
+      errors: [
+        {
+          column: 17,
+          data: { new: 'toBeVisible', old: 'toBeVisible' },
+          endColumn: 32,
+          line: 1,
+          messageId: 'noUselessNot',
+        },
+      ],
+      output: 'expect(locator).toBeVisible()',
+    },
     {
       code: 'expect(locator).not.toBeHidden()',
       errors: [
@@ -42,6 +128,66 @@ runRuleTester('no-useless-not', rule, {
       ],
       output: 'expect(locator).toBeDisabled()',
     },
+    {
+      code: 'expect(locator).toBeEnabled({ enabled: true })',
+      errors: [
+        {
+          column: 31,
+          data: {
+            new: 'toBeEnabled',
+            old: 'toBeEnabled',
+            property: 'enabled',
+          },
+          endColumn: 44,
+          line: 1,
+          messageId: 'noUselessTruthy',
+        },
+      ],
+      output: 'expect(locator).toBeEnabled()',
+    },
+    {
+      code: 'expect(locator).not.toBeEnabled({ enabled: true })',
+      errors: [
+        {
+          column: 17,
+          data: { new: 'toBeDisabled', old: 'toBeEnabled' },
+          endColumn: 32,
+          line: 1,
+          messageId: 'noUselessNot',
+        },
+      ],
+      output: 'expect(locator).toBeDisabled()',
+    },
+    {
+      code: 'expect(locator).toBeEnabled({ enabled: false })',
+      errors: [
+        {
+          column: 31,
+          data: {
+            new: 'toBeDisabled',
+            old: 'toBeEnabled',
+            property: 'enabled',
+          },
+          endColumn: 45,
+          line: 1,
+          messageId: 'noUselessProperty',
+        },
+      ],
+      output: 'expect(locator).toBeDisabled()',
+    },
+    {
+      code: 'expect(locator).not.toBeEnabled({ enabled: false })',
+      errors: [
+        {
+          column: 17,
+          data: { new: 'toBeEnabled', old: 'toBeEnabled' },
+          endColumn: 32,
+          line: 1,
+          messageId: 'noUselessNot',
+        },
+      ],
+      output: 'expect(locator).toBeEnabled()',
+    },
     {
       code: 'expect(locator).not.toBeDisabled()',
       errors: [
@@ -132,6 +278,11 @@ runRuleTester('no-useless-not', rule, {
     'expect.poll(() => locator).toBeDisabled()',
     'expect(locator)[`toBeEnabled`]()',
     'expect(locator)["toBeDisabled"]()',
+    // Ignores matchers with options
+    'expect(locator).toBeVisible({ visible })',
+    'expect(locator).not.toBeVisible({ visible })',
+    'expect(locator).toBeEnabled({ enabled })',
+    'expect(locator).not.toBeEnabled({ enabled })',
     // Incomplete call expression
     'expect(locator).toBeVisible',
     'expect(locator).toBeEnabled',
@@ -139,6 +290,7 @@ runRuleTester('no-useless-not', rule, {
     // Doesn't impact non-complimentary matchers
     "expect(locator).not.toHaveText('foo')",
     'expect(locator).not.toBeChecked()',
+    'expect(locator).not.toBeChecked({ checked })',
     'expect(locator).not.toBeChecked({ checked: false })',
     'expect(locator).not.toBeFocused()',
     // Global aliases