feat: no-depends-on-boolean-flag rule

.github/workflows/test.yml

@@ -10,6 +10,8 @@ jobs:
   linux-unit-tests:
     needs: yarn-lockfile-check
     uses: salesforcecli/github-workflows/.github/workflows/unitTestsLinux.yml@main
+    with:
+      skipTsDepCheck: true
   windows-unit-tests:
     needs: yarn-lockfile-check
     uses: salesforcecli/github-workflows/.github/workflows/unitTestsWindows.yml@main

README.md

@@ -72,6 +72,7 @@ module.exports = {
 | [no-builtin-flags](docs/rules/no-builtin-flags.md)                                     | Handling for sfdxCommand's flags.builtin                                                                                         | ✈️      |      |         | 🔧 |    |
 | [no-classes-in-command-return-type](docs/rules/no-classes-in-command-return-type.md)   | The return type of the run method should not contain a class.                                                                    | ✈️ ✅    |      |         | 🔧 |    |
 | [no-default-and-depends-on-flags](docs/rules/no-default-and-depends-on-flags.md)       | Do not allow creation of a flag with default value and dependsOn                                                                 | ✈️ ✅    |      |         |    |    |
+| [no-depends-on-boolean-flag](docs/rules/no-depends-on-boolean-flag.md)                 | Do not allow flags to depend on boolean flags                                                                                    |         | ✈️ ✅ |         |    |    |
 | [no-deprecated-properties](docs/rules/no-deprecated-properties.md)                     | Removes non-existent properties left over from SfdxCommand                                                                       | ✈️      |      |         | 🔧 |    |
 | [no-duplicate-short-characters](docs/rules/no-duplicate-short-characters.md)           | Prevent duplicate use of short characters or conflicts between aliases and flags                                                 | ✈️ ✅    |      |         |    |    |
 | [no-execcmd-double-quotes](docs/rules/no-execcmd-double-quotes.md)                     | Do not use double quotes in NUT examples.  They will not work on windows                                                         |         |      | 📚 ✈️ ✅ | 🔧 |    |

docs/rules/no-depends-on-boolean-flag.md

@@ -0,0 +1,81 @@
+# Do not allow flags to depend on boolean flags (`sf-plugin/no-depends-on-boolean-flag`)
+
+⚠️ This rule _warns_ in the following configs: ✈️ `migration`, ✅ `recommended`.
+
+<!-- end auto-generated rule header -->
+
+Flags depending on other boolean flags via `dependsOn` can cause unexpected behaviors:
+
+```ts
+// src/commands/data/query.ts
+
+export class DataSoqlQueryCommand extends SfCommand<unknown> {
+  public static readonly flags = {
+    query: Flags.string({
+      char: 'q',
+      summary: messages.getMessage('flags.query.summary'),
+    }),
+    bulk: Flags.boolean({
+      char: 'b',
+      default: false,
+      summary: messages.getMessage('flags.bulk.summary'),
+    }),
+    wait: Flags.duration({
+      unit: 'minutes',
+      char: 'w',
+      summary: messages.getMessage('flags.wait.summary'),
+      dependsOn: ['bulk'],
+    }) 
+  }
+}
+```
+
+This code is supposed to only allow `--wait` to be used when `--bulk` was provided. 
+
+However, because `--bulk` has a default value of `false`, oclif's flag parser will allow `--wait` even without passing in `--bulk` because the `wait.dependsOn` check only ensures that `bulk` has a value, so the following execution would be allowed:
+
+```
+sf data query -q 'select name,id from account limit 1' --wait 10
+```
+
+But even if `--bulk` didn't have a default value, it could still allow a wrong combination if it had `allowNo: true`: 
+
+```ts
+bulk: Flags.boolean({
+  char: 'b',
+  default: false,
+  summary: messages.getMessage('flags.bulk.summary'),
+  allowNo: true // Support reversible boolean flag with `--no-` prefix (e.g. `--no-bulk`).
+}),
+```
+
+The following example would still run because `--no-bulk` sets `bulk` value to `false`:
+
+```
+sf data query -q 'select name,id from account limit 1' --wait 10 --no-bulk
+```
+
+If the desired behavior is to only allow a flag when another boolean flag was provided you should use oclif's relationships feature to verify the boolean flag value is `true`:
+
+```ts
+bulk: Flags.boolean({
+  char: 'b',
+  default: false,
+  summary: messages.getMessage('flags.bulk.summary'),
+  allowNo: true // Support reversible boolean flag with `--no-` prefix (e.g. `--no-bulk`).
+}),
+wait: Flags.duration({
+  unit: 'minutes',
+  char: 'w',
+  summary: messages.getMessage('flags.wait.summary'),
+  relationships: [
+    {
+      type: 'some',
+      // eslint-disable-next-line @typescript-eslint/require-await
+      flags: [{ name: 'bulk', when: async (flags): Promise<boolean> => Promise.resolve(flags['bulk'] === true) }],
+    },
+  ]
+}) 
+```
+
+See: https://oclif.io/docs/flags

src/index.ts

@@ -49,6 +49,7 @@ import { esmMessageImport } from './rules/esm-message-import';
 import { noDefaultDependsOnFlags } from './rules/no-default-depends-on-flags';
 import { onlyExtendSfCommand } from './rules/only-extend-sfCommand';
 import { spreadBaseFlags } from './rules/spread-base-flags';
+import { noDependsOnBooleanFlags } from './rules/no-depends-on-boolean-flags';
 
 const library = {
   plugins: ['sf-plugin'],
@@ -90,6 +91,7 @@ const recommended = {
     'sf-plugin/no-default-and-depends-on-flags': 'error',
     'sf-plugin/only-extend-SfCommand': 'warn',
     'sf-plugin/spread-base-flags': 'warn',
+    'sf-plugin/no-depends-on-boolean-flag': 'warn',
   },
 };
 
@@ -124,6 +126,7 @@ export = {
     'no-h-short-char': dashH,
     'no-default-and-depends-on-flags': noDefaultDependsOnFlags,
     'only-extend-SfCommand': onlyExtendSfCommand,
+    'no-depends-on-boolean-flag': noDependsOnBooleanFlags,
     'spread-base-flags': spreadBaseFlags,
     'no-duplicate-short-characters': noDuplicateShortCharacters,
     'run-matches-class-type': runMatchesClassType,

src/rules/no-depends-on-boolean-flags.ts

@@ -0,0 +1,69 @@
+/*
+ * Copyright (c) 2024, salesforce.com, inc.
+ * All rights reserved.
+ * Licensed under the BSD 3-Clause license.
+ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
+ */
+import { AST_NODE_TYPES } from '@typescript-eslint/utils';
+import { RuleCreator } from '@typescript-eslint/utils/eslint-utils';
+import { ancestorsContainsSfCommand, isInCommandDirectory } from '../shared/commands';
+import { isFlag } from '../shared/flags';
+
+export const noDependsOnBooleanFlags = RuleCreator.withoutDocs({
+  meta: {
+    docs: {
+      description: 'Do not allow flags to depend on boolean flags',
+      recommended: 'recommended',
+    },
+    messages: {
+      message: 'Cannot create a flag that `dependsOn` a boolean flag',
+    },
+    type: 'problem',
+    schema: [],
+  },
+  defaultOptions: [],
+  create(context) {
+    return isInCommandDirectory(context)
+      ? {
+          Property(node): void {
+            if (
+              isFlag(node) &&
+              ancestorsContainsSfCommand(context) &&
+              node.value?.type === AST_NODE_TYPES.CallExpression &&
+              node.value.arguments?.[0]?.type === AST_NODE_TYPES.ObjectExpression
+            ) {
+              const dependsOnFlagsProp = node.value.arguments[0].properties.find(
+                (p) => p.type === 'Property' && p.key.type === 'Identifier' && p.key.name === 'dependsOn'
+              );
+
+              if (dependsOnFlagsProp) {
+                // @ts-ignore we know `dependsOn` exists/is an array
+                const dependedOnFlags = dependsOnFlagsProp.value.elements.map((l) => l.value);
+
+                for (const flag of dependedOnFlags) {
+                  if (node.parent.type === 'ObjectExpression') {
+                    const possibleBoolFlag = node.parent.properties.find(
+                      (f) =>
+                        f.type === AST_NODE_TYPES.Property &&
+                        f.key.type == AST_NODE_TYPES.Identifier &&
+                        f.key.name === flag &&
+                        f.value.type == AST_NODE_TYPES.CallExpression &&
+                        f.value.callee.type == AST_NODE_TYPES.MemberExpression &&
+                        f.value.callee.property.type == AST_NODE_TYPES.Identifier &&
+                        f.value.callee.property.name === 'boolean'
+                    );
+                    if (possibleBoolFlag) {
+                      context.report({
+                        node: node,
+                        messageId: 'message',
+                      });
+                    }
+                  }
+                }
+              }
+            }
+          },
+        }
+      : {};
+  },
+});

src/rules/only-extend-sfCommand.ts

@@ -15,6 +15,7 @@ export const onlyExtendSfCommand = RuleCreator.withoutDocs({
     docs: {
       description: 'Only allow commands that directly extend SfCommand',
       recommended: 'recommended',
+      url: 'https://github.com/salesforcecli/eslint-plugin-sf-plugin/blob/main/docs/rules/no-depends-on-boolean-flag.md',
     },
     messages: {
       message: 'In order to inherit default flags correctly, extend from SfCommand directly',

test/rules/migration/no-this-flags.test.ts

@@ -59,6 +59,7 @@ export default class EnvCreateScratch extends SfCommand<ScratchCreateResponse> {
             {
               messageId: 'useFlags',
               output: `
+import {SfCommand} from '@salesforce/sf-plugins-core';
 export default class EnvCreateScratch extends SfCommand<ScratchCreateResponse> {
   public static flags = {
     foo: flags.string({ char: 'f', description: 'foo flag' }),
@@ -75,6 +76,7 @@ export default class EnvCreateScratch extends SfCommand<ScratchCreateResponse> {
             {
               messageId: 'instanceProp',
               output: `
+import {SfCommand} from '@salesforce/sf-plugins-core';
 export default class EnvCreateScratch extends SfCommand<ScratchCreateResponse> {
   public static flags = {
     foo: flags.string({ char: 'f', description: 'foo flag' }),

test/rules/no-depends-on-boolean-flags.test.ts

@@ -0,0 +1,67 @@
+/*
+ * Copyright (c) 2024, salesforce.com, inc.
+ * All rights reserved.
+ * Licensed under the BSD 3-Clause license.
+ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
+ */
+import path from 'path';
+import { RuleTester } from '@typescript-eslint/rule-tester';
+
+import { noDependsOnBooleanFlags } from '../../src/rules/no-depends-on-boolean-flags';
+
+const ruleTester = new RuleTester({
+  parser: '@typescript-eslint/parser',
+});
+
+ruleTester.run('noDependsOnBooleanFlags', noDependsOnBooleanFlags, {
+  valid: [
+    {
+      name: 'dependsOn non-boolean flag',
+      filename: path.normalize('src/commands/foo.ts'),
+      code: `
+import {SfCommand} from '@salesforce/sf-plugins-core';
+export default class DataQuery extends SfCommand<void> {
+  public static readonly flags = {
+    query: Flags.string({
+      char: 'q',
+      summary: messages.getMessage('flags.query.summary'),
+      exactlyOne: ['query', 'file'],
+    }),
+    test: Flags.string({
+      summary: 'test flag',
+      dependsOn: ['query']
+    }),
+  }
+}
+`
+    }
+  ],
+  invalid: [
+    {
+      name: 'dependsOn boolean flag',
+      filename: path.normalize('src/commands/foo.ts'),
+      errors: [{ messageId: 'message' }],
+      code: `
+import {SfCommand} from '@salesforce/sf-plugins-core';
+export default class DataQuery extends SfCommand<void> {
+  public static readonly flags = {
+    ...orgFlags,
+    bulk: Flags.boolean({
+      char: 'b',
+      default: false,
+      summary: messages.getMessage('flags.bulk.summary'),
+      exclusive: ['use-tooling-api'],
+    }),
+    wait: Flags.duration({
+      unit: 'minutes',
+      char: 'w',
+      summary: messages.getMessage('flags.wait.summary'),
+      dependsOn: ['bulk'],
+      exclusive: ['async'],
+    }),
+  }
+}
+`,
+    }
+  ],
+});