src: allow to negate boolean CLI flags

targos · danielleadams · commit 3c7a7d9ee42d · 2021-06-21T18:30:42.000-04:00
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 <anna@addaleax.net> PR-URL: #39023 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
diff --git 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
@@ -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
@@ -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
@@ -259,14 +259,19 @@ 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');
 
 const allowedNodeEnvironmentFlags = [];
 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
@@ -447,7 +447,7 @@ void Environment::InitializeMainContext(Local<Context> 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
@@ -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
@@ -1132,9 +1132,9 @@ int Start(int argc, char** argv){
 Isolate::CreateParams params;
 const std::vector<size_t>* 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
@@ -23,12 +23,14 @@ template <typename Options>
 void OptionsParser<Options>::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<SimpleOptionField<bool>>(field),
 env_setting,
- help_text});
+ help_text,
+ default_is_true});
 }
 
 template <typename Options>
@@ -186,7 +188,8 @@ auto OptionsParser<Options>::Convert(
 return OptionInfo{original.type,
 Convert(original.field, get_child),
 original.env_setting,
- original.help_text};
+ original.help_text,
+ original.default_is_true};
 }
 
 template <typename Options>
@@ -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<Options>::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<Options>::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<Options>::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<Options>::Parse(
 
 switch (info.type){
 case kBoolean:
- *Lookup<bool>(info.field, options) = true;
+ *Lookup<bool>(info.field, options) = !is_negation;
 break;
 case kInteger:
 *Lookup<int64_t>(info.field, options) = std::atoll(value.c_str());
diff --git 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<Value>& args){
 env->type_string(),
 Integer::New(isolate, static_cast<int>(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
@@ -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<EnvironmentOptions> 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<BaseOptionField> 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
@@ -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
@@ -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 ' +

Original file line number	Diff line number	Diff line change
`@@ -140,7 +140,7 @@ function setupWarningHandler(){`
`140`	`140`	`const{`
`141`	`141`	`onWarning`
`142`	`142`	`}=require('internal/process/warning');`
`143`		`-if(!getOptionValue('--no-warnings')&&`
	`143`	`+if(getOptionValue('--warnings')&&`
`144`	`144`	`process.env.NODE_NO_WARNINGS!=='1'){`
`145`	`145`	`process.on('warning',onWarning);`
`146`	`146`	`}`
-Original file line number
+Diff line change
  MathMax,
  ObjectKeys,
  RegExp,
 + StringPrototypeLocaleCompare,
 + StringPrototypeSlice,
  StringPrototypeTrimLeft,
  StringPrototypeRepeat,
  StringPrototypeReplace,
 lettext='';
 letmaxFirstColumnUsed=0;
 +constsortedOptions=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)}`;
 +}
 +returnStringPrototypeLocaleCompare(name1,name2);
 +},
 +);
++
 for(const{
 -0: name,1: { helpText, type, value }
 -}ofArrayPrototypeSort([...options.entries()])){
 +0: name,1: { helpText, type, value, defaultIsTrue}
 +}ofsortedOptions){
 if(!helpText)continue;
 letdisplayName=name;
++
 +if(defaultIsTrue){
 +displayName=`--no-${StringPrototypeSlice(displayName,2)}`;
 +}
++
 constargDescription=getArgDescription(type);
 if(argDescription)
 displayName+=`=${argDescription}`;
+}
 letdisplayHelpText=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.
Original file line number	Diff line number	Diff line change
`@@ -25,8 +25,13 @@ function getAliasesFromBinding(){`
`25`	`25`	`returnaliasesMap;`
`26`	`26`	`}`
`27`	`27`
`28`		`-functiongetOptionValue(option){`
`29`		`-returngetOptionsFromBinding().get(option)?.value;`
	`28`	`+functiongetOptionValue(optionName){`
	`29`	`+constoptions=getOptionsFromBinding();`
	`30`	`+if(optionName.startsWith('--no-')){`
	`31`	`+constoption=options.get('--'+optionName.slice(5));`
	`32`	`+returnoption&&!option.value;`
	`33`	`+}`
	`34`	`+returnoptions.get(optionName)?.value;`
`30`	`35`	`}`
`31`	`36`
`32`	`37`	`functiongetAllowUnauthorized(){`
-Original file line number
+Diff line change
 // from data in the config binding.
 functionbuildAllowedFlags(){
 const{
 -envSettings: { kAllowedInEnvironment }
 +envSettings: { kAllowedInEnvironment },
 +types: { kBoolean },
 }=internalBinding('options');
 const{ options, aliases }=require('internal/options');
 constallowedNodeEnvironmentFlags=[];
 for(const{0: name,1: info}ofoptions){
 if(info.envVarSettings===kAllowedInEnvironment){
 ArrayPrototypePush(allowedNodeEnvironmentFlags,name);
 +if(info.type===kBoolean){
 +constnegatedName=`--no-${name.slice(2)}`;
 +ArrayPrototypePush(allowedNodeEnvironmentFlags,negatedName);
 +}
+}
+}
Original file line number	Diff line number	Diff line change
`@@ -447,7 +447,7 @@ void Environment::InitializeMainContext(Local<Context> context,`
`447`	`447`	`CreateProperties();`
`448`	`448`	`}`
`449`	`449`
`450`		`-if (options_->no_force_async_hooks_checks){`
	`450`	`+if (!options_->force_async_hooks_checks){`
`451`	`451`	`async_hooks_.no_force_checks();`
`452`	`452`	`}`
`453`	`453`