From 219a27dde51bf4c04af2d905f71d1cec021ee98e Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Tue, 2 Apr 2024 16:20:48 -0700 Subject: [PATCH] fix(ext/node): Support returning tokens and option defaults in `node:util.parseArgs` (#23192) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #23179. Fixes #22454. Enables passing `{tokens: true}` to `parseArgs` and setting default values for options. With this PR, the observable framework works with deno out of the box (no unstable flags needed). The existing code was basically copied straight from node, so this PR mostly just updates that (out of date) vendored code. Also fixes some issues with error exports (before this PR, in certain error cases we were attempting to construct error classes that weren't actually in scope). The last change (in the second commit) adds a small hack so that we actually exercise the `test-parse-args.js` node_compat test, previously it was reported as passing though it should have failed. That test now passes. --------- Co-authored-by: Bartek Iwańczuk --- ext/node/polyfills/internal/errors.ts | 8 + .../internal/util/parse_args/parse_args.js | 363 +++++++++++------- .../internal/util/parse_args/utils.js | 13 + ext/node/polyfills/internal/validators.mjs | 48 ++- tests/node_compat/test.ts | 14 +- 5 files changed, 304 insertions(+), 142 deletions(-) diff --git a/ext/node/polyfills/internal/errors.ts b/ext/node/polyfills/internal/errors.ts index 5a6446ae8f7bd..c3aeff8b209d8 100644 --- a/ext/node/polyfills/internal/errors.ts +++ b/ext/node/polyfills/internal/errors.ts @@ -17,6 +17,8 @@ * ERR_INVALID_PACKAGE_CONFIG // package.json stuff, probably useless */ +import { primordials } from "ext:core/mod.js"; +const { JSONStringify } = primordials; import { format, inspect } from "ext:deno_node/internal/util/inspect.mjs"; import { codes } from "ext:deno_node/internal/error_codes.ts"; import { @@ -2629,6 +2631,11 @@ codes.ERR_OUT_OF_RANGE = ERR_OUT_OF_RANGE; codes.ERR_SOCKET_BAD_PORT = ERR_SOCKET_BAD_PORT; codes.ERR_BUFFER_OUT_OF_BOUNDS = ERR_BUFFER_OUT_OF_BOUNDS; codes.ERR_UNKNOWN_ENCODING = ERR_UNKNOWN_ENCODING; +codes.ERR_PARSE_ARGS_INVALID_OPTION_VALUE = ERR_PARSE_ARGS_INVALID_OPTION_VALUE; +codes.ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL = + ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL; +codes.ERR_PARSE_ARGS_UNKNOWN_OPTION = ERR_PARSE_ARGS_UNKNOWN_OPTION; + // TODO(kt3k): assign all error classes here. /** @@ -2846,6 +2853,7 @@ export default { ERR_OUT_OF_RANGE, ERR_PACKAGE_IMPORT_NOT_DEFINED, ERR_PACKAGE_PATH_NOT_EXPORTED, + ERR_PARSE_ARGS_INVALID_OPTION_VALUE, ERR_QUICCLIENTSESSION_FAILED, ERR_QUICCLIENTSESSION_FAILED_SETSOCKET, ERR_QUICSESSION_DESTROYED, diff --git a/ext/node/polyfills/internal/util/parse_args/parse_args.js b/ext/node/polyfills/internal/util/parse_args/parse_args.js index 8abe8a9f8ffd6..742f2a1f0dd74 100644 --- a/ext/node/polyfills/internal/util/parse_args/parse_args.js +++ b/ext/node/polyfills/internal/util/parse_args/parse_args.js @@ -5,6 +5,7 @@ import { primordials } from "ext:core/mod.js"; const { ArrayPrototypeForEach, ArrayPrototypeIncludes, + ArrayPrototypeMap, ArrayPrototypePushApply, ArrayPrototypeShift, ArrayPrototypeSlice, @@ -15,13 +16,16 @@ const { StringPrototypeCharAt, StringPrototypeIndexOf, StringPrototypeSlice, + StringPrototypeStartsWith, } = primordials; import { validateArray, validateBoolean, + validateBooleanArray, validateObject, validateString, + validateStringArray, validateUnion, } from "ext:deno_node/internal/validators.mjs"; @@ -36,6 +40,7 @@ import { isShortOptionGroup, objectGetOwn, optionsGetOwn, + useDefaultValueOption, } from "ext:deno_node/internal/util/parse_args/utils.js"; import { codes } from "ext:deno_node/internal/error_codes.ts"; @@ -68,20 +73,16 @@ function getMainArgs() { /** * In strict mode, throw for possible usage errors like --foo --bar - * - * @param {string} longOption - long option name e.g. 'foo' - * @param {string|undefined} optionValue - value from user args - * @param {string} shortOrLong - option used, with dashes e.g. `-l` or `--long` - * @param {boolean} strict - show errors, from parseArgs({ strict }) + * @param {object} token - from tokens as available from parseArgs */ -function checkOptionLikeValue(longOption, optionValue, shortOrLong, strict) { - if (strict && isOptionLikeValue(optionValue)) { +function checkOptionLikeValue(token) { + if (!token.inlineValue && isOptionLikeValue(token.value)) { // Only show short example if user used short option. - const example = (shortOrLong.length === 2) - ? `'--${longOption}=-XYZ' or '${shortOrLong}-XYZ'` - : `'--${longOption}=-XYZ'`; - const errorMessage = `Option '${shortOrLong}' argument is ambiguous. -Did you forget to specify the option argument for '${shortOrLong}'? + const example = StringPrototypeStartsWith(token.rawName, "--") + ? `'${token.rawName}=-XYZ'` + : `'--${token.name}=-XYZ' or '${token.rawName}-XYZ'`; + const errorMessage = `Option '${token.rawName}' argument is ambiguous. +Did you forget to specify the option argument for '${token.rawName}'? To specify an option argument starting with a dash use ${example}.`; throw new ERR_PARSE_ARGS_INVALID_OPTION_VALUE(errorMessage); } @@ -89,39 +90,27 @@ To specify an option argument starting with a dash use ${example}.`; /** * In strict mode, throw for usage errors. - * - * @param {string} longOption - long option name e.g. 'foo' - * @param {string|undefined} optionValue - value from user args - * @param {object} options - option configs, from parseArgs({ options }) - * @param {string} shortOrLong - option used, with dashes e.g. `-l` or `--long` - * @param {boolean} strict - show errors, from parseArgs({ strict }) - * @param {boolean} allowPositionals - from parseArgs({ allowPositionals }) + * @param {object} config - from config passed to parseArgs + * @param {object} token - from tokens as available from parseArgs */ -function checkOptionUsage( - longOption, - optionValue, - options, - shortOrLong, - strict, - allowPositionals, -) { - // Strict and options are used from local context. - if (!strict) return; - - if (!ObjectHasOwn(options, longOption)) { - throw new ERR_PARSE_ARGS_UNKNOWN_OPTION(shortOrLong, allowPositionals); +function checkOptionUsage(config, token) { + if (!ObjectHasOwn(config.options, token.name)) { + throw new ERR_PARSE_ARGS_UNKNOWN_OPTION( + token.rawName, + config.allowPositionals, + ); } - const short = optionsGetOwn(options, longOption, "short"); - const shortAndLong = short ? `-${short}, --${longOption}` : `--${longOption}`; - const type = optionsGetOwn(options, longOption, "type"); - if (type === "string" && typeof optionValue !== "string") { + const short = optionsGetOwn(config.options, token.name, "short"); + const shortAndLong = `${short ? `-${short}, ` : ""}--${token.name}`; + const type = optionsGetOwn(config.options, token.name, "type"); + if (type === "string" && typeof token.value !== "string") { throw new ERR_PARSE_ARGS_INVALID_OPTION_VALUE( `Option '${shortAndLong} ' argument missing`, ); } // (Idiomatic test for undefined||null, expecting undefined.) - if (type === "boolean" && optionValue != null) { + if (type === "boolean" && token.value != null) { throw new ERR_PARSE_ARGS_INVALID_OPTION_VALUE( `Option '${shortAndLong}' does not take an argument`, ); @@ -130,7 +119,6 @@ function checkOptionUsage( /** * Store the option value in `values`. - * * @param {string} longOption - long option name e.g. 'foo' * @param {string|undefined} optionValue - value from user args * @param {object} options - option configs, from parseArgs({ options }) @@ -160,71 +148,56 @@ function storeOption(longOption, optionValue, options, values) { } } -export const parseArgs = (config = { __proto__: null }) => { - const args = objectGetOwn(config, "args") ?? getMainArgs(); - const strict = objectGetOwn(config, "strict") ?? true; - const allowPositionals = objectGetOwn(config, "allowPositionals") ?? !strict; - const options = objectGetOwn(config, "options") ?? { __proto__: null }; - - // Validate input configuration. - validateArray(args, "args"); - validateBoolean(strict, "strict"); - validateBoolean(allowPositionals, "allowPositionals"); - validateObject(options, "options"); - ArrayPrototypeForEach( - ObjectEntries(options), - ({ 0: longOption, 1: optionConfig }) => { - validateObject(optionConfig, `options.${longOption}`); - - // type is required - validateUnion( - objectGetOwn(optionConfig, "type"), - `options.${longOption}.type`, - ["string", "boolean"], - ); - - if (ObjectHasOwn(optionConfig, "short")) { - const shortOption = optionConfig.short; - validateString(shortOption, `options.${longOption}.short`); - if (shortOption.length !== 1) { - throw new ERR_INVALID_ARG_VALUE( - `options.${longOption}.short`, - shortOption, - "must be a single character", - ); - } - } +/** + * Store the default option value in `values`. + * @param {string} longOption - long option name e.g. 'foo' + * @param {string + * | boolean + * | string[] + * | boolean[]} optionValue - default value from option config + * @param {object} values - option values returned in `values` by parseArgs + */ +function storeDefaultOption(longOption, optionValue, values) { + if (longOption === "__proto__") { + return; // No. Just no. + } - if (ObjectHasOwn(optionConfig, "multiple")) { - validateBoolean( - optionConfig.multiple, - `options.${longOption}.multiple`, - ); - } - }, - ); + values[longOption] = optionValue; +} - const result = { - values: { __proto__: null }, - positionals: [], - }; +/** + * Process args and turn into identified tokens: + * - option (along with value, if any) + * - positional + * - option-terminator + * @param {string[]} args - from parseArgs({ args }) or mainArgs + * @param {object} options - option configs, from parseArgs({ options }) + */ +function argsToTokens(args, options) { + const tokens = []; + let index = -1; + let groupCount = 0; const remainingArgs = ArrayPrototypeSlice(args); while (remainingArgs.length > 0) { const arg = ArrayPrototypeShift(remainingArgs); const nextArg = remainingArgs[0]; + if (groupCount > 0) { + groupCount--; + } else { + index++; + } // Check if `arg` is an options terminator. // Guideline 10 in https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html if (arg === "--") { - if (!allowPositionals && remainingArgs.length > 0) { - throw new ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL(nextArg); - } - // Everything after a bare '--' is considered a positional argument. + ArrayPrototypePush(tokens, { kind: "option-terminator", index }); ArrayPrototypePushApply( - result.positionals, - remainingArgs, + tokens, + ArrayPrototypeMap(remainingArgs, (arg) => { + return { kind: "positional", index: ++index, value: arg }; + }), ); break; // Finished processing args, leave while loop. } @@ -233,24 +206,28 @@ export const parseArgs = (config = { __proto__: null }) => { // e.g. '-f' const shortOption = StringPrototypeCharAt(arg, 1); const longOption = findLongOptionForShort(shortOption, options); - let optionValue; + let value; + let inlineValue; if ( optionsGetOwn(options, longOption, "type") === "string" && isOptionValue(nextArg) ) { // e.g. '-f', 'bar' - optionValue = ArrayPrototypeShift(remainingArgs); - checkOptionLikeValue(longOption, optionValue, arg, strict); + value = ArrayPrototypeShift(remainingArgs); + inlineValue = false; } - checkOptionUsage( - longOption, - optionValue, - options, - arg, - strict, - allowPositionals, + ArrayPrototypePush( + tokens, + { + kind: "option", + name: longOption, + rawName: arg, + index, + value, + inlineValue, + }, ); - storeOption(longOption, optionValue, options, result.values); + if (value != null) ++index; continue; } @@ -274,6 +251,7 @@ export const parseArgs = (config = { __proto__: null }) => { } } ArrayPrototypeUnshiftApply(remainingArgs, expanded); + groupCount = expanded.length; continue; } @@ -281,67 +259,178 @@ export const parseArgs = (config = { __proto__: null }) => { // e.g. -fFILE const shortOption = StringPrototypeCharAt(arg, 1); const longOption = findLongOptionForShort(shortOption, options); - const optionValue = StringPrototypeSlice(arg, 2); - checkOptionUsage( - longOption, - optionValue, - options, - `-${shortOption}`, - strict, - allowPositionals, + const value = StringPrototypeSlice(arg, 2); + ArrayPrototypePush( + tokens, + { + kind: "option", + name: longOption, + rawName: `-${shortOption}`, + index, + value, + inlineValue: true, + }, ); - storeOption(longOption, optionValue, options, result.values); continue; } if (isLoneLongOption(arg)) { // e.g. '--foo' const longOption = StringPrototypeSlice(arg, 2); - let optionValue; + let value; + let inlineValue; if ( optionsGetOwn(options, longOption, "type") === "string" && isOptionValue(nextArg) ) { // e.g. '--foo', 'bar' - optionValue = ArrayPrototypeShift(remainingArgs); - checkOptionLikeValue(longOption, optionValue, arg, strict); + value = ArrayPrototypeShift(remainingArgs); + inlineValue = false; } - checkOptionUsage( - longOption, - optionValue, - options, - arg, - strict, - allowPositionals, + ArrayPrototypePush( + tokens, + { + kind: "option", + name: longOption, + rawName: arg, + index, + value, + inlineValue, + }, ); - storeOption(longOption, optionValue, options, result.values); + if (value != null) ++index; continue; } if (isLongOptionAndValue(arg)) { // e.g. --foo=bar - const index = StringPrototypeIndexOf(arg, "="); - const longOption = StringPrototypeSlice(arg, 2, index); - const optionValue = StringPrototypeSlice(arg, index + 1); - checkOptionUsage( - longOption, - optionValue, - options, - `--${longOption}`, - strict, - allowPositionals, + const equalIndex = StringPrototypeIndexOf(arg, "="); + const longOption = StringPrototypeSlice(arg, 2, equalIndex); + const value = StringPrototypeSlice(arg, equalIndex + 1); + ArrayPrototypePush( + tokens, + { + kind: "option", + name: longOption, + rawName: `--${longOption}`, + index, + value, + inlineValue: true, + }, ); - storeOption(longOption, optionValue, options, result.values); continue; } - // Anything left is a positional - if (!allowPositionals) { - throw new ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL(arg); - } + ArrayPrototypePush(tokens, { kind: "positional", index, value: arg }); + } + return tokens; +} + +export const parseArgs = (config = { __proto__: null }) => { + const args = objectGetOwn(config, "args") ?? getMainArgs(); + const strict = objectGetOwn(config, "strict") ?? true; + const allowPositionals = objectGetOwn(config, "allowPositionals") ?? !strict; + const returnTokens = objectGetOwn(config, "tokens") ?? false; + const options = objectGetOwn(config, "options") ?? { __proto__: null }; + // Bundle these up for passing to strict-mode checks. + const parseConfig = { args, strict, options, allowPositionals }; + + // Validate input configuration. + validateArray(args, "args"); + validateBoolean(strict, "strict"); + validateBoolean(allowPositionals, "allowPositionals"); + validateBoolean(returnTokens, "tokens"); + validateObject(options, "options"); + ArrayPrototypeForEach( + ObjectEntries(options), + ({ 0: longOption, 1: optionConfig }) => { + validateObject(optionConfig, `options.${longOption}`); + + // type is required + const optionType = objectGetOwn(optionConfig, "type"); + validateUnion(optionType, `options.${longOption}.type`, [ + "string", + "boolean", + ]); + + if (ObjectHasOwn(optionConfig, "short")) { + const shortOption = optionConfig.short; + validateString(shortOption, `options.${longOption}.short`); + if (shortOption.length !== 1) { + throw new ERR_INVALID_ARG_VALUE( + `options.${longOption}.short`, + shortOption, + "must be a single character", + ); + } + } + + const multipleOption = objectGetOwn(optionConfig, "multiple"); + if (ObjectHasOwn(optionConfig, "multiple")) { + validateBoolean(multipleOption, `options.${longOption}.multiple`); + } + + const defaultValue = objectGetOwn(optionConfig, "default"); + if (defaultValue !== undefined) { + let validator; + switch (optionType) { + case "string": + validator = multipleOption ? validateStringArray : validateString; + break; - ArrayPrototypePush(result.positionals, arg); + case "boolean": + validator = multipleOption ? validateBooleanArray : validateBoolean; + break; + } + validator(defaultValue, `options.${longOption}.default`); + } + }, + ); + + // Phase 1: identify tokens + const tokens = argsToTokens(args, options); + + // Phase 2: process tokens into parsed option values and positionals + const result = { + values: { __proto__: null }, + positionals: [], + }; + if (returnTokens) { + result.tokens = tokens; } + ArrayPrototypeForEach(tokens, (token) => { + if (token.kind === "option") { + if (strict) { + checkOptionUsage(parseConfig, token); + checkOptionLikeValue(token); + } + storeOption(token.name, token.value, options, result.values); + } else if (token.kind === "positional") { + if (!allowPositionals) { + throw new ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL(token.value); + } + ArrayPrototypePush(result.positionals, token.value); + } + }); + + // Phase 3: fill in default values for missing args + ArrayPrototypeForEach( + ObjectEntries(options), + ({ 0: longOption, 1: optionConfig }) => { + const mustSetDefault = useDefaultValueOption( + longOption, + optionConfig, + result.values, + ); + if (mustSetDefault) { + storeDefaultOption( + longOption, + objectGetOwn(optionConfig, "default"), + result.values, + ); + } + }, + ); return result; }; diff --git a/ext/node/polyfills/internal/util/parse_args/utils.js b/ext/node/polyfills/internal/util/parse_args/utils.js index a51a460988ee7..1ceed0b9e83dc 100644 --- a/ext/node/polyfills/internal/util/parse_args/utils.js +++ b/ext/node/polyfills/internal/util/parse_args/utils.js @@ -173,6 +173,18 @@ function findLongOptionForShort(shortOption, options) { return longOptionEntry?.[0] ?? shortOption; } +/** + * Check if the given option includes a default value + * and that option has not been set by the input args. + * @param {string} longOption - long option name e.g. 'foo' + * @param {object} optionConfig - the option configuration properties + * @param {object} values - option values returned in `values` by parseArgs + */ +function useDefaultValueOption(longOption, optionConfig, values) { + return objectGetOwn(optionConfig, "default") !== undefined && + values[longOption] === undefined; +} + export { findLongOptionForShort, isLoneLongOption, @@ -184,4 +196,5 @@ export { isShortOptionGroup, objectGetOwn, optionsGetOwn, + useDefaultValueOption, }; diff --git a/ext/node/polyfills/internal/validators.mjs b/ext/node/polyfills/internal/validators.mjs index 89527a84cd686..d4cd955462c7c 100644 --- a/ext/node/polyfills/internal/validators.mjs +++ b/ext/node/polyfills/internal/validators.mjs @@ -288,9 +288,51 @@ const validateArray = hideStackFrames( }, ); +/** + * @callback validateStringArray + * @param {*} value + * @param {string} name + * @returns {asserts value is string[]} + */ + +/** @type {validateStringArray} */ +const validateStringArray = hideStackFrames((value, name) => { + validateArray(value, name); + for (let i = 0; i < value.length; ++i) { + // Don't use validateString here for performance reasons, as + // we would generate intermediate strings for the name. + if (typeof value[i] !== "string") { + throw new codes.ERR_INVALID_ARG_TYPE(`${name}[${i}]`, "string", value[i]); + } + } +}); + +/** + * @callback validateBooleanArray + * @param {*} value + * @param {string} name + * @returns {asserts value is boolean[]} + */ + +/** @type {validateBooleanArray} */ +const validateBooleanArray = hideStackFrames((value, name) => { + validateArray(value, name); + for (let i = 0; i < value.length; ++i) { + // Don't use validateBoolean here for performance reasons, as + // we would generate intermediate strings for the name. + if (value[i] !== true && value[i] !== false) { + throw new codes.ERR_INVALID_ARG_TYPE( + `${name}[${i}]`, + "boolean", + value[i], + ); + } + } +}); + function validateUnion(value, name, union) { if (!ArrayPrototypeIncludes(union, value)) { - throw new ERR_INVALID_ARG_TYPE( + throw new codes.ERR_INVALID_ARG_TYPE( name, `('${ArrayPrototypeJoin(union, "|")}')`, value, @@ -305,6 +347,7 @@ export default { validateAbortSignal, validateArray, validateBoolean, + validateBooleanArray, validateBuffer, validateFunction, validateInt32, @@ -314,6 +357,7 @@ export default { validateOneOf, validatePort, validateString, + validateStringArray, validateUint32, validateUnion, }; @@ -324,6 +368,7 @@ export { validateAbortSignal, validateArray, validateBoolean, + validateBooleanArray, validateBuffer, validateFunction, validateInt32, @@ -333,6 +378,7 @@ export { validateOneOf, validatePort, validateString, + validateStringArray, validateUint32, validateUnion, }; diff --git a/tests/node_compat/test.ts b/tests/node_compat/test.ts index 04a85f1135eec..2f690e94363aa 100644 --- a/tests/node_compat/test.ts +++ b/tests/node_compat/test.ts @@ -76,18 +76,24 @@ async function runTest(t: Deno.TestContext, path: string): Promise { // contain actual JS objects, not strings :)). envVars["NODE_TEST_KNOWN_GLOBALS"] = "0"; } - + // TODO(nathanwhit): once we match node's behavior on executing + // `node:test` tests when we run a file, we can remove this + const usesNodeTest = testSource.includes("node:test"); const args = [ - "run", + usesNodeTest ? "test" : "run", "-A", "--quiet", //"--unsafely-ignore-certificate-errors", "--unstable-unsafe-proto", "--unstable-bare-node-builtins", "--v8-flags=" + v8Flags.join(), - "runner.ts", - testCase, ]; + if (usesNodeTest) { + // deno test typechecks by default + we want to pass script args + args.push("--no-check", "runner.ts", "--", testCase); + } else { + args.push("runner.ts", testCase); + } // Pipe stdout in order to output each test result as Deno.test output // That way the tests will respect the `--quiet` option when provided