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 <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>

Author: targos

Diff for: lib/internal/bootstrap/pre_execution.js

@@ -128,7 +128,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 for: 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 for: 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 for: lib/internal/process/per_thread.js

@@ -6,6 +6,7 @@
 
 const {
   ArrayIsArray,
+  ArrayPrototypePush,
   BigUint64Array,
   Float64Array,
   NumberMAX_SAFE_INTEGER,
@@ -249,14 +250,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 [name, info] of options) {
     if (info.envVarSettings === kAllowedInEnvironment) {
-      allowedNodeEnvironmentFlags.push(name);
+      ArrayPrototypePush(allowedNodeEnvironmentFlags, name);
+      if (info.type === kBoolean) {
+        const negatedName = `--no-${name.slice(2)}`;
+        ArrayPrototypePush(allowedNodeEnvironmentFlags, negatedName);
+      }
     }
   }
 

Diff for: src/env.cc

@@ -425,7 +425,7 @@ Environment::Environment(IsolateData* isolate_data,
   // By default, always abort when --abort-on-uncaught-exception was passed.
   should_abort_on_uncaught_toggle_[0] = 1;
 
-  if (options_->no_force_async_hooks_checks) {
+  if (!options_->force_async_hooks_checks) {
     async_hooks_.no_force_checks();
   }
 

Diff for: src/env.h

@@ -207,6 +207,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 for: src/node.cc

@@ -1082,9 +1082,9 @@ int Start(int argc, char** argv) {
     const std::vector<size_t>* indexes = nullptr;
     std::vector<intptr_t> external_references;
 
-    bool force_no_snapshot =
-        per_process::cli_options->per_isolate->no_node_snapshot;
-    if (!force_no_snapshot) {
+    bool use_no_snapshot =
+        per_process::cli_options->per_isolate->node_snapshot;
+    if (use_no_snapshot) {
       v8::StartupData* blob = NodeMainInstance::GetEmbeddedSnapshotBlob();
       if (blob != nullptr) {
         // TODO(joyeecheung): collect external references and set it in

Diff for: 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 for: src/node_options.cc

@@ -377,18 +377,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,
@@ -579,9 +582,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.
@@ -991,6 +994,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 for: src/node_options.h

@@ -120,9 +120,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;
@@ -194,7 +194,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;
@@ -302,7 +302,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,
@@ -425,6 +426,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 for: 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 for: 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}.`);
@@ -87,12 +88,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 ' +