From 3c7a7d9ee42d242a0f25f28f777daf9243d47be3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Sun, 13 Jun 2021 12:46:35 +0200 Subject: [PATCH] src: allow to negate boolean CLI flags This change allows all boolean flags to be negated using the `--no-` prefix. Flags that are `true` by default (for example `--deprecation`) are still documented as negations. With this change, it becomes possible to easily flip the default value of a boolean flag and to override the value of a flag passed in the NODE_OPTIONS environment variable. `process.allowedNodeEnvironmentFlags` contains both the negated and non-negated versions of boolean flags. Co-authored-by: Anna Henningsen PR-URL: https://github.com/nodejs/node/pull/39023 Reviewed-By: Franziska Hinkelmann Reviewed-By: Michael Dawson Reviewed-By: Ruben Bridgewater --- lib/internal/bootstrap/pre_execution.js | 2 +- lib/internal/main/print_help.js | 26 +++++++++++-- lib/internal/options.js | 9 ++++- lib/internal/process/per_thread.js | 7 +++- src/env.cc | 2 +- src/env.h | 1 + src/node.cc | 6 +-- src/node_options-inl.h | 36 ++++++++++++++--- src/node_options.cc | 29 ++++++++------ src/node_options.h | 12 +++--- test/parallel/test-cli-options-negation.js | 39 +++++++++++++++++++ ...rocess-env-allowed-flags-are-documented.js | 14 ++++++- 12 files changed, 150 insertions(+), 33 deletions(-) create mode 100644 test/parallel/test-cli-options-negation.js diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index 62ba7da371d39e..83ccfe90c11065 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -140,7 +140,7 @@ function setupWarningHandler() { const { onWarning } = require('internal/process/warning'); - if (!getOptionValue('--no-warnings') && + if (getOptionValue('--warnings') && process.env.NODE_NO_WARNINGS !== '1') { process.on('warning', onWarning); } diff --git a/lib/internal/main/print_help.js b/lib/internal/main/print_help.js index 029701307980b1..6aa422c657b2d0 100644 --- a/lib/internal/main/print_help.js +++ b/lib/internal/main/print_help.js @@ -8,6 +8,8 @@ const { MathMax, ObjectKeys, RegExp, + StringPrototypeLocaleCompare, + StringPrototypeSlice, StringPrototypeTrimLeft, StringPrototypeRepeat, StringPrototypeReplace, @@ -110,12 +112,30 @@ function format( let text = ''; let maxFirstColumnUsed = 0; + const sortedOptions = ArrayPrototypeSort( + [...options.entries()], + ({ 0: name1, 1: option1 }, { 0: name2, 1: option2 }) => { + if (option1.defaultIsTrue) { + name1 = `--no-${StringPrototypeSlice(name1, 2)}`; + } + if (option2.defaultIsTrue) { + name2 = `--no-${StringPrototypeSlice(name2, 2)}`; + } + return StringPrototypeLocaleCompare(name1, name2); + }, + ); + for (const { - 0: name, 1: { helpText, type, value } - } of ArrayPrototypeSort([...options.entries()])) { + 0: name, 1: { helpText, type, value, defaultIsTrue } + } of sortedOptions) { if (!helpText) continue; let displayName = name; + + if (defaultIsTrue) { + displayName = `--no-${StringPrototypeSlice(displayName, 2)}`; + } + const argDescription = getArgDescription(type); if (argDescription) displayName += `=${argDescription}`; @@ -138,7 +158,7 @@ function format( } let displayHelpText = helpText; - if (value === true) { + if (value === !defaultIsTrue) { // Mark boolean options we currently have enabled. // In particular, it indicates whether --use-openssl-ca // or --use-bundled-ca is the (current) default. diff --git a/lib/internal/options.js b/lib/internal/options.js index 1c97aaee97742d..aa9c52e6988d65 100644 --- a/lib/internal/options.js +++ b/lib/internal/options.js @@ -25,8 +25,13 @@ function getAliasesFromBinding() { return aliasesMap; } -function getOptionValue(option) { - return getOptionsFromBinding().get(option)?.value; +function getOptionValue(optionName) { + const options = getOptionsFromBinding(); + if (optionName.startsWith('--no-')) { + const option = options.get('--' + optionName.slice(5)); + return option && !option.value; + } + return options.get(optionName)?.value; } function getAllowUnauthorized() { diff --git a/lib/internal/process/per_thread.js b/lib/internal/process/per_thread.js index 8f35051041c9a3..f1d11911a4444a 100644 --- a/lib/internal/process/per_thread.js +++ b/lib/internal/process/per_thread.js @@ -259,7 +259,8 @@ const trailingValuesRegex = /=.*$/; // from data in the config binding. function buildAllowedFlags() { const { - envSettings: { kAllowedInEnvironment } + envSettings: { kAllowedInEnvironment }, + types: { kBoolean }, } = internalBinding('options'); const { options, aliases } = require('internal/options'); @@ -267,6 +268,10 @@ function buildAllowedFlags() { for (const { 0: name, 1: info } of options) { if (info.envVarSettings === kAllowedInEnvironment) { ArrayPrototypePush(allowedNodeEnvironmentFlags, name); + if (info.type === kBoolean) { + const negatedName = `--no-${name.slice(2)}`; + ArrayPrototypePush(allowedNodeEnvironmentFlags, negatedName); + } } } diff --git a/src/env.cc b/src/env.cc index 9f6172de82bd92..1cc7da1ce15f43 100644 --- a/src/env.cc +++ b/src/env.cc @@ -447,7 +447,7 @@ void Environment::InitializeMainContext(Local context, CreateProperties(); } - if (options_->no_force_async_hooks_checks) { + if (!options_->force_async_hooks_checks) { async_hooks_.no_force_checks(); } diff --git a/src/env.h b/src/env.h index 1dae1f710f8377..7b136f70fbad1e 100644 --- a/src/env.h +++ b/src/env.h @@ -211,6 +211,7 @@ constexpr size_t kFsStatsBufferLength = V(crypto_rsa_pss_string, "rsa-pss") \ V(cwd_string, "cwd") \ V(data_string, "data") \ + V(default_is_true_string, "defaultIsTrue") \ V(deserialize_info_string, "deserializeInfo") \ V(dest_string, "dest") \ V(destroyed_string, "destroyed") \ diff --git a/src/node.cc b/src/node.cc index 75ad5689fca209..b60be116b6139b 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1132,9 +1132,9 @@ int Start(int argc, char** argv) { Isolate::CreateParams params; const std::vector* indices = nullptr; const EnvSerializeInfo* env_info = nullptr; - bool force_no_snapshot = - per_process::cli_options->per_isolate->no_node_snapshot; - if (!force_no_snapshot) { + bool use_node_snapshot = + per_process::cli_options->per_isolate->node_snapshot; + if (use_node_snapshot) { v8::StartupData* blob = NodeMainInstance::GetEmbeddedSnapshotBlob(); if (blob != nullptr) { params.snapshot_blob = blob; diff --git a/src/node_options-inl.h b/src/node_options-inl.h index 4e1a12296bc77e..7facb22afc3c9b 100644 --- a/src/node_options-inl.h +++ b/src/node_options-inl.h @@ -23,12 +23,14 @@ template void OptionsParser::AddOption(const char* name, const char* help_text, bool Options::* field, - OptionEnvvarSettings env_setting) { + OptionEnvvarSettings env_setting, + bool default_is_true) { options_.emplace(name, OptionInfo{kBoolean, std::make_shared>(field), env_setting, - help_text}); + help_text, + default_is_true}); } template @@ -186,7 +188,8 @@ auto OptionsParser::Convert( return OptionInfo{original.type, Convert(original.field, get_child), original.env_setting, - original.help_text}; + original.help_text, + original.default_is_true}; } template @@ -225,6 +228,10 @@ inline std::string RequiresArgumentErr(const std::string& arg) { return arg + " requires an argument"; } +inline std::string NegationImpliesBooleanError(const std::string& arg) { + return arg + " is an invalid negation because it is not a boolean option"; +} + // We store some of the basic information around a single Parse call inside // this struct, to separate storage of command line arguments and their // handling. In particular, this makes it easier to introduce 'synthetic' @@ -325,6 +332,13 @@ void OptionsParser::Parse( name[i] = '-'; } + // Convert --no-foo to --foo and keep in mind that we're negating. + bool is_negation = false; + if (name.find("--no-") == 0) { + name.erase(2, 3); // remove no- + is_negation = true; + } + { auto it = aliases_.end(); // Expand aliases: @@ -367,7 +381,12 @@ void OptionsParser::Parse( } { - auto implications = implications_.equal_range(name); + std::string implied_name = name; + if (is_negation) { + // Implications for negated options are defined with "--no-". + implied_name.insert(2, "no-"); + } + auto implications = implications_.equal_range(implied_name); for (auto it = implications.first; it != implications.second; ++it) { if (it->second.type == kV8Option) { v8_args->push_back(it->second.name); @@ -384,6 +403,13 @@ void OptionsParser::Parse( } const OptionInfo& info = it->second; + + // Some V8 options can be negated and they are validated by V8 later. + if (is_negation && info.type != kBoolean && info.type != kV8Option) { + errors->push_back(NegationImpliesBooleanError(arg)); + break; + } + std::string value; if (info.type != kBoolean && info.type != kNoOp && info.type != kV8Option) { if (equals_index != std::string::npos) { @@ -412,7 +438,7 @@ void OptionsParser::Parse( switch (info.type) { case kBoolean: - *Lookup(info.field, options) = true; + *Lookup(info.field, options) = !is_negation; break; case kInteger: *Lookup(info.field, options) = std::atoll(value.c_str()); diff --git a/src/node_options.cc b/src/node_options.cc index bf18d77d7d104b..1e3659cd00c843 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -391,18 +391,21 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { kAllowedInEnvironment); AddAlias("--es-module-specifier-resolution", "--experimental-specifier-resolution"); - AddOption("--no-deprecation", + AddOption("--deprecation", "silence deprecation warnings", - &EnvironmentOptions::no_deprecation, - kAllowedInEnvironment); - AddOption("--no-force-async-hooks-checks", + &EnvironmentOptions::deprecation, + kAllowedInEnvironment, + true); + AddOption("--force-async-hooks-checks", "disable checks for async_hooks", - &EnvironmentOptions::no_force_async_hooks_checks, - kAllowedInEnvironment); - AddOption("--no-warnings", + &EnvironmentOptions::force_async_hooks_checks, + kAllowedInEnvironment, + true); + AddOption("--warnings", "silence all process warnings", - &EnvironmentOptions::no_warnings, - kAllowedInEnvironment); + &EnvironmentOptions::warnings, + kAllowedInEnvironment, + true); AddOption("--force-context-aware", "disable loading non-context-aware addons", &EnvironmentOptions::force_context_aware, @@ -594,9 +597,9 @@ PerIsolateOptionsParser::PerIsolateOptionsParser( "track heap object allocations for heap snapshots", &PerIsolateOptions::track_heap_objects, kAllowedInEnvironment); - AddOption("--no-node-snapshot", + AddOption("--node-snapshot", "", // It's a debug-only option. - &PerIsolateOptions::no_node_snapshot, + &PerIsolateOptions::node_snapshot, kAllowedInEnvironment); // Explicitly add some V8 flags to mark them as allowed in NODE_OPTIONS. @@ -1014,6 +1017,10 @@ void GetOptions(const FunctionCallbackInfo& args) { env->type_string(), Integer::New(isolate, static_cast(option_info.type))) .FromMaybe(false) || + !info->Set(context, + env->default_is_true_string(), + Boolean::New(isolate, option_info.default_is_true)) + .FromMaybe(false) || info->Set(context, env->value_string(), value).IsNothing() || options->Set(context, name, info).IsEmpty()) { return; diff --git a/src/node_options.h b/src/node_options.h index a91dbd259784e0..d737c4f55aee36 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -119,9 +119,9 @@ class EnvironmentOptions : public Options { int64_t heap_snapshot_near_heap_limit = 0; std::string heap_snapshot_signal; uint64_t max_http_header_size = 16 * 1024; - bool no_deprecation = false; - bool no_force_async_hooks_checks = false; - bool no_warnings = false; + bool deprecation = true; + bool force_async_hooks_checks = true; + bool warnings = true; bool force_context_aware = false; bool pending_deprecation = false; bool preserve_symlinks = false; @@ -193,7 +193,7 @@ class PerIsolateOptions : public Options { public: std::shared_ptr per_env { new EnvironmentOptions() }; bool track_heap_objects = false; - bool no_node_snapshot = false; + bool node_snapshot = true; bool report_uncaught_exception = false; bool report_on_signal = false; bool experimental_top_level_await = true; @@ -301,7 +301,8 @@ class OptionsParser { void AddOption(const char* name, const char* help_text, bool Options::* field, - OptionEnvvarSettings env_setting = kDisallowedInEnvironment); + OptionEnvvarSettings env_setting = kDisallowedInEnvironment, + bool default_is_true = false); void AddOption(const char* name, const char* help_text, uint64_t Options::* field, @@ -424,6 +425,7 @@ class OptionsParser { std::shared_ptr field; OptionEnvvarSettings env_setting; std::string help_text; + bool default_is_true = false; }; // An implied option is composed of the information on where to store a diff --git a/test/parallel/test-cli-options-negation.js b/test/parallel/test-cli-options-negation.js new file mode 100644 index 00000000000000..bfbee635ab1a2e --- /dev/null +++ b/test/parallel/test-cli-options-negation.js @@ -0,0 +1,39 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const { spawnSync } = require('child_process'); + +// --warnings is on by default. +assertHasWarning(spawnWithFlags([])); + +// --warnings can be passed alone. +assertHasWarning(spawnWithFlags(['--warnings'])); + +// --no-warnings can be passed alone. +assertHasNoWarning(spawnWithFlags(['--no-warnings'])); + +// Last flag takes precedence. +assertHasWarning(spawnWithFlags(['--no-warnings', '--warnings'])); + +// Non-boolean flags cannot be negated. +assert(spawnWithFlags(['--no-max-http-header-size']).stderr.toString().includes( + '--no-max-http-header-size is an invalid negation because it is not ' + + 'a boolean option', +)); + +// Inexistant flags cannot be negated. +assert(spawnWithFlags(['--no-i-dont-exist']).stderr.toString().includes( + 'bad option: --no-i-dont-exist', +)); + +function spawnWithFlags(flags) { + return spawnSync(process.execPath, [...flags, '-e', 'new Buffer(0)']); +} + +function assertHasWarning(proc) { + assert(proc.stderr.toString().includes('Buffer() is deprecated')); +} + +function assertHasNoWarning(proc) { + assert(!proc.stderr.toString().includes('Buffer() is deprecated')); +} diff --git a/test/parallel/test-process-env-allowed-flags-are-documented.js b/test/parallel/test-process-env-allowed-flags-are-documented.js index 493017130167ab..64626b71f01902 100644 --- a/test/parallel/test-process-env-allowed-flags-are-documented.js +++ b/test/parallel/test-process-env-allowed-flags-are-documented.js @@ -31,7 +31,8 @@ assert.deepStrictEqual(v8OptionsLines, [...v8OptionsLines].sort()); const documented = new Set(); for (const line of [...nodeOptionsLines, ...v8OptionsLines]) { for (const match of line.matchAll(/`(-[^`]+)`/g)) { - const option = match[1]; + // Remove negation from the option's name. + const option = match[1].replace('--no-', '--'); assert(!documented.has(option), `Option '${option}' was documented more than once as an ` + `allowed option for NODE_OPTIONS in ${cliMd}.`); @@ -86,12 +87,23 @@ const undocumented = difference(process.allowedNodeEnvironmentFlags, documented); // Remove intentionally undocumented options. assert(undocumented.delete('--debug-arraybuffer-allocations')); +assert(undocumented.delete('--no-debug-arraybuffer-allocations')); assert(undocumented.delete('--es-module-specifier-resolution')); assert(undocumented.delete('--experimental-report')); assert(undocumented.delete('--experimental-worker')); +assert(undocumented.delete('--node-snapshot')); assert(undocumented.delete('--no-node-snapshot')); assert(undocumented.delete('--loader')); assert(undocumented.delete('--verify-base-objects')); +assert(undocumented.delete('--no-verify-base-objects')); + +// Remove negated versions of the flags. +for (const flag of undocumented) { + if (flag.startsWith('--no-')) { + assert(documented.has(`--${flag.slice(5)}`), flag); + undocumented.delete(flag); + } +} assert.strictEqual(undocumented.size, 0, 'The following options are not documented as allowed in ' +