Standarize handling of string array params

docs/modules/ROOT/pages/api-core.adoc

@@ -119,9 +119,9 @@ If any errors are found, the command will exit with a non-zero exit code and pri
 * `--contract <CONTRACT>` - The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated.
 * `--reference <REFERENCE_CONTRACT>` - Can only be used when the `--contract` option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the `@custom:oz-upgrades-from` annotation if it is defined in the contract that is being validated.
 * `--requireReference` - Can only be used when the `--contract` option is also provided. Not compatible with `--unsafeSkipStorageCheck`. If specified, requires either the `--reference` option to be provided or the contract to have a `@custom:oz-upgrades-from` annotation.
-* `--referenceBuildInfoDirs` - Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix `<dirName>:` before the contract name or fully qualified name in the `--reference` option or `@custom:oz-upgrades-from` annotation, where `<dirName>` is the directory short name. Each directory short name must be unique, including compared to the main build info directory.
-* `--exclude "<GLOB_PATTERN>"` - Exclude validations for contracts in source file paths that match the given glob pattern. For example, `--exclude "contracts/mocks/\**/*.sol"`. Does not apply to reference contracts.
-* `--unsafeAllow "<VALIDATION_ERRORS>"` - Selectively disable one or more validation errors. Comma-separated list with one or more of the following: `state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage`
+* `--referenceBuildInfoDirs "<BUILD_INFO_DIR>[,<BUILD_INFO_DIR>...]"` - Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix `<dirName>:` before the contract name or fully qualified name in the `--reference` option or `@custom:oz-upgrades-from` annotation, where `<dirName>` is the directory short name. Each directory short name must be unique, including compared to the main build info directory. If passing in multiple directories, separate them with commas or call the option multiple times, once for each directory.
+* `--exclude "<GLOB_PATTERN>[,<GLOB_PATTERN>...]"` - Exclude validations for contracts in source file paths that match the given glob pattern. For example, `--exclude "contracts/mocks/\**/*.sol"`. Does not apply to reference contracts. If passing in multiple patterns, separate them with commas or call the option multiple times, once for each pattern.
+* `--unsafeAllow "<VALIDATION_ERROR>[,<VALIDATION_ERROR>...]"` - Selectively disable one or more validation errors. Comma-separated list with one or more of the following: `state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage`
 * `--unsafeAllowRenames` - Configure storage layout check to allow variable renaming.
 * `--unsafeSkipStorageCheck` - Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.
 

packages/core/src/cli/cli.test.ts

@@ -460,10 +460,10 @@ async function setupMultipleBuildInfoDirsTest(t: ExecutionContext<unknown>) {
   const v2BranchDir = path.join(temp, 'build-info-v2-branch');
   await fs.mkdir(v2BranchDir);
   const v2BranchBuildInfoOk = await artifacts.getBuildInfo(
-    `contracts/test/cli/ValidateBuildInfoV2_Branch_Ok.sol:MyContract`
+    `contracts/test/cli/ValidateBuildInfoV2_Branch_Ok.sol:MyContract`,
   );
   const v2BranchBuildInfoBad = await artifacts.getBuildInfo(
-    `contracts/test/cli/ValidateBuildInfoV2_Branch_Bad.sol:MyContract`
+    `contracts/test/cli/ValidateBuildInfoV2_Branch_Bad.sol:MyContract`,
   );
   await fs.writeFile(path.join(v2BranchDir, 'ok.json'), JSON.stringify(v2BranchBuildInfoOk));
   await fs.writeFile(path.join(v2BranchDir, 'bad.json'), JSON.stringify(v2BranchBuildInfoBad));
@@ -640,4 +640,3 @@ test('validate - excludes one contract from layout comparisions', async t => {
   const expectation: string[] = [`Stdout: ${(error as any).stdout}`, `Stderr: ${(error as any).stderr}`];
   t.snapshot(expectation.join('\n'));
 });
-

packages/core/src/cli/cli.test.ts.md

@@ -19,9 +19,9 @@ Generated by [AVA](https://avajs.dev).
       --contract <CONTRACT>  The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated.␊
       --reference <REFERENCE_CONTRACT>  Can only be used when the --contract option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the @custom:oz-upgrades-from annotation if it is defined in the contract that is being validated.␊
       --requireReference  Can only be used when the --contract option is also provided. Not compatible with --unsafeSkipStorageCheck. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.␊
-      --referenceBuildInfoDirs  Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix '<dirName>:' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where <dirName> is the directory short name. Each directory short name must be unique, including compared to the main build info directory.␊
-      --exclude "<GLOB_PATTERN>"  Exclude validations for contracts in source file paths that match the given glob pattern. For example, --exclude "contracts/mocks/**/*.sol". Does not apply to reference contracts.␊
-      --unsafeAllow "<VALIDATION_ERRORS>"  Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage␊
+      --referenceBuildInfoDirs "<BUILD_INFO_DIR>[,<BUILD_INFO_DIR>...]"  Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix '<dirName>:' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where <dirName> is the directory short name. Each directory short name must be unique, including compared to the main build info directory. If passing in multiple directories, separate them with commas or call the option multiple times, once for each directory.␊
+      --exclude "<GLOB_PATTERN>[,<GLOB_PATTERN>...]"  Exclude validations for contracts in source file paths that match the given glob patterns. For example, --exclude "contracts/mocks/**/*.sol". Does not apply to reference contracts. If passing in multiple patterns, separate them with commas or call the option multiple times, once for each pattern.␊
+      --unsafeAllow "<VALIDATION_ERROR>[,<VALIDATION_ERROR>...]"  Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage␊
       --unsafeAllowRenames  Configure storage layout check to allow variable renaming.␊
       --unsafeSkipStorageCheck  Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.␊
     `
@@ -41,9 +41,9 @@ Generated by [AVA](https://avajs.dev).
       --contract <CONTRACT>  The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated.␊
       --reference <REFERENCE_CONTRACT>  Can only be used when the --contract option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the @custom:oz-upgrades-from annotation if it is defined in the contract that is being validated.␊
       --requireReference  Can only be used when the --contract option is also provided. Not compatible with --unsafeSkipStorageCheck. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.␊
-      --referenceBuildInfoDirs  Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix '<dirName>:' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where <dirName> is the directory short name. Each directory short name must be unique, including compared to the main build info directory.␊
-      --exclude "<GLOB_PATTERN>"  Exclude validations for contracts in source file paths that match the given glob pattern. For example, --exclude "contracts/mocks/**/*.sol". Does not apply to reference contracts.␊
-      --unsafeAllow "<VALIDATION_ERRORS>"  Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage␊
+      --referenceBuildInfoDirs "<BUILD_INFO_DIR>[,<BUILD_INFO_DIR>...]"  Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix '<dirName>:' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where <dirName> is the directory short name. Each directory short name must be unique, including compared to the main build info directory. If passing in multiple directories, separate them with commas or call the option multiple times, once for each directory.␊
+      --exclude "<GLOB_PATTERN>[,<GLOB_PATTERN>...]"  Exclude validations for contracts in source file paths that match the given glob patterns. For example, --exclude "contracts/mocks/**/*.sol". Does not apply to reference contracts. If passing in multiple patterns, separate them with commas or call the option multiple times, once for each pattern.␊
+      --unsafeAllow "<VALIDATION_ERROR>[,<VALIDATION_ERROR>...]"  Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage␊
       --unsafeAllowRenames  Configure storage layout check to allow variable renaming.␊
       --unsafeSkipStorageCheck  Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.␊
     `

packages/core/src/cli/validate.test.ts

@@ -110,3 +110,25 @@ test('withDefaults - invalid unsafeAllow', t => {
     )}`,
   });
 });
+
+test('withDefaults - empty unsafeAllow', t => {
+  const parsedArgs = minimist(['validate', 'build-info.json', '--unsafeAllow']);
+  t.throws(() => withDefaults(parsedArgs), {
+    message: `Invalid option: --unsafeAllow cannot be empty`,
+  });
+});
+
+test('withDefaults - unsafeAllow multiple times', t => {
+  const parsedArgs = minimist([
+    'validate',
+    'build-info.json',
+    '--unsafeAllow',
+    'selfdestruct',
+    '--unsafeAllow',
+    'delegatecall',
+    '--unsafeAllow',
+    'constructor',
+  ]);
+  const opts = withDefaults(parsedArgs);
+  t.deepEqual(opts.unsafeAllow, ['selfdestruct', 'delegatecall', 'constructor']);
+});

packages/core/src/cli/validate.ts

@@ -16,9 +16,9 @@ Options:
   --contract <CONTRACT>  The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated.
   --reference <REFERENCE_CONTRACT>  Can only be used when the --contract option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the @custom:oz-upgrades-from annotation if it is defined in the contract that is being validated.
   --requireReference  Can only be used when the --contract option is also provided. Not compatible with --unsafeSkipStorageCheck. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.
-  --referenceBuildInfoDirs  Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix '<dirName>:' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where <dirName> is the directory short name. Each directory short name must be unique, including compared to the main build info directory.
-  --exclude "<GLOB_PATTERN>"  Exclude validations for contracts in source file paths that match the given glob pattern. For example, --exclude "contracts/mocks/**/*.sol". Does not apply to reference contracts.
-  --unsafeAllow "<VALIDATION_ERRORS>"  Selectively disable one or more validation errors. Comma-separated list with one or more of the following: ${errorKinds.join(
+  --referenceBuildInfoDirs "<BUILD_INFO_DIR>[,<BUILD_INFO_DIR>...]"  Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix '<dirName>:' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where <dirName> is the directory short name. Each directory short name must be unique, including compared to the main build info directory. If passing in multiple directories, separate them with commas or call the option multiple times, once for each directory.
+  --exclude "<GLOB_PATTERN>[,<GLOB_PATTERN>...]"  Exclude validations for contracts in source file paths that match the given glob patterns. For example, --exclude "contracts/mocks/**/*.sol". Does not apply to reference contracts. If passing in multiple patterns, separate them with commas or call the option multiple times, once for each pattern.
+  --unsafeAllow "<VALIDATION_ERROR>[,<VALIDATION_ERROR>...]"  Selectively disable one or more validation errors. Comma-separated list with one or more of the following: ${errorKinds.join(
     ', ',
   )}
   --unsafeAllowRenames  Configure storage layout check to allow variable renaming.
@@ -113,7 +113,7 @@ export function getFunctionArgs(parsedArgs: minimist.ParsedArgs, extraArgs: stri
 function getAndValidateString(parsedArgs: minimist.ParsedArgs, option: string): string | undefined {
   const value = parsedArgs[option];
   if (value !== undefined) {
-    assertNonEmptyOption(value, option);
+    assertNonEmptyString(value, option);
   }
   return value;
 }
@@ -124,15 +124,15 @@ function getAndValidateStringArray(parsedArgs: minimist.ParsedArgs, option: stri
     if (Array.isArray(value)) {
       return value;
     } else {
-      assertNonEmptyOption(value, option);
-      return value.split(/,+/);
+      assertNonEmptyString(value, option);
+      return value.split(/[\s,]+/);
     }
   }
   return value;
 }
 
-function assertNonEmptyOption(value: string, option: string) {
-  if (value.trim().length === 0) {
+function assertNonEmptyString(value: string, option: string) {
+  if (typeof value !== 'string' || value.trim().length === 0) {
     throw new Error(`Invalid option: --${option} cannot be empty`);
   }
 }
@@ -161,18 +161,13 @@ function validateOptions(parsedArgs: minimist.ParsedArgs) {
   }
 }
 
-function getUnsafeAllowKinds(unsafeAllow: string | undefined): ValidationError['kind'][] {
+function getUnsafeAllowKinds(parseArgs: minimist.ParsedArgs): ValidationError['kind'][] {
   type errorKindsType = (typeof errorKinds)[number];
 
-  if (unsafeAllow === undefined) {
-    return [];
-  }
-
-  const unsafeAllowTokens: string[] = unsafeAllow.split(/[\s,]+/);
+  const unsafeAllowTokens: string[] = getAndValidateStringArray(parseArgs, 'unsafeAllow') ?? [];
   if (unsafeAllowTokens.some(token => !errorKinds.includes(token as errorKindsType))) {
-    // This includes empty strings
     throw new Error(
-      `Invalid option: --unsafeAllow "${unsafeAllow}". Supported values for the --unsafeAllow option are: ${errorKinds.join(
+      `Invalid option: --unsafeAllow "${parseArgs['unsafeAllow']}". Supported values for the --unsafeAllow option are: ${errorKinds.join(
         ', ',
       )}`,
     );
@@ -188,7 +183,7 @@ export function withDefaults(parsedArgs: minimist.ParsedArgs): Required<Validate
     unsafeSkipStorageCheck: parsedArgs['unsafeSkipStorageCheck'],
     unsafeAllowCustomTypes: parsedArgs['unsafeAllowCustomTypes'],
     unsafeAllowLinkedLibraries: parsedArgs['unsafeAllowLinkedLibraries'],
-    unsafeAllow: getUnsafeAllowKinds(parsedArgs['unsafeAllow']),
+    unsafeAllow: getUnsafeAllowKinds(parsedArgs),
     requireReference: parsedArgs['requireReference'],
   };