From c29e5ac5beaeab96b5715ea221b9d23279af2ce9 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sat, 22 Sep 2018 14:34:00 +0200
Subject: [PATCH] =?UTF-8?q?cli:=20normalize=20`=5F`=20=E2=86=92=20`-`=20wh?=
 =?UTF-8?q?en=20parsing=20options?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This allows for option syntax similar to V8’s one, e.g.
`--no_warnings` has the same effect as `--no-warnings`.

PR-URL: https://github.com/nodejs/node/pull/23020
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
---
 doc/api/cli.md                                | 17 +++++---
 lib/internal/bootstrap/node.js                | 43 ++++++-------------
 src/node_options-inl.h                        | 19 +++-----
 src/node_options.cc                           | 12 +++---
 .../test-cli-node-options-disallowed.js       |  1 -
 test/parallel/test-cli-node-options.js        |  1 +
 test/parallel/test-pending-deprecation.js     |  8 ++++
 .../test-process-env-allowed-flags.js         |  2 +-
 8 files changed, 45 insertions(+), 58 deletions(-)

diff --git a/doc/api/cli.md b/doc/api/cli.md
index 3f5441b82e7ce8..9c6195aa4e2f9e 100644
--- a/doc/api/cli.md
+++ b/doc/api/cli.md
@@ -21,6 +21,18 @@ Execute without arguments to start the [REPL][].
 _For more info about `node inspect`, please see the [debugger][] documentation._
 
 ## Options
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/23020
+    description: Underscores instead of dashes are now allowed for
+                 Node.js options as well, in addition to V8 options.
+-->
+
+All options, including V8 options, allow words to be separated by both
+dashes (`-`) or underscores (`_`).
+
+For example, `--pending-deprecation` is equivalent to `--pending_deprecation`.
 
 ### `-`
 <!-- YAML
@@ -390,11 +402,6 @@ added: v0.1.3
 
 Print V8 command line options.
 
-V8 options allow words to be separated by both dashes (`-`) or
-underscores (`_`).
-
-For example, `--stack-trace-limit` is equivalent to `--stack_trace_limit`.
-
 ### `--v8-pool-size=num`
 <!-- YAML
 added: v5.10.0
diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
index 223930f6983cd8..97c4d70097a2b1 100644
--- a/lib/internal/bootstrap/node.js
+++ b/lib/internal/bootstrap/node.js
@@ -616,7 +616,7 @@
     // This builds process.allowedNodeEnvironmentFlags
     // from data in the config binding
 
-    const replaceDashesRegex = /-/g;
+    const replaceUnderscoresRegex = /_/g;
     const leadingDashesRegex = /^--?/;
     const trailingValuesRegex = /=.*$/;
 
@@ -628,20 +628,14 @@
     const get = () => {
       const {
         getOptions,
-        types: { kV8Option },
         envSettings: { kAllowedInEnvironment }
       } = internalBinding('options');
       const { options, aliases } = getOptions();
 
-      const allowedV8EnvironmentFlags = [];
       const allowedNodeEnvironmentFlags = [];
       for (const [name, info] of options) {
         if (info.envVarSettings === kAllowedInEnvironment) {
-          if (info.type === kV8Option) {
-            allowedV8EnvironmentFlags.push(name);
-          } else {
-            allowedNodeEnvironmentFlags.push(name);
-          }
+          allowedNodeEnvironmentFlags.push(name);
         }
       }
 
@@ -675,11 +669,9 @@
       // process.allowedNodeEnvironmentFlags.has() which lack leading dashes.
       // Avoid interference w/ user code by flattening `Set.prototype` into
       // each object.
-      const [nodeFlags, v8Flags] = [
-        allowedNodeEnvironmentFlags, allowedV8EnvironmentFlags
-      ].map((flags) => Object.defineProperties(
-        new Set(flags.map(trimLeadingDashes)),
-        Object.getOwnPropertyDescriptors(Set.prototype))
+      const nodeFlags = Object.defineProperties(
+        new Set(allowedNodeEnvironmentFlags.map(trimLeadingDashes)),
+        Object.getOwnPropertyDescriptors(Set.prototype)
       );
 
       class NodeEnvironmentFlagsSet extends Set {
@@ -703,29 +695,18 @@
         has(key) {
           // This will return `true` based on various possible
           // permutations of a flag, including present/missing leading
-          // dash(es) and/or underscores-for-dashes in the case of V8-specific
-          // flags.  Strips any values after `=`, inclusive.
+          // dash(es) and/or underscores-for-dashes.
+          // Strips any values after `=`, inclusive.
           // TODO(addaleax): It might be more flexible to run the option parser
           // on a dummy option set and see whether it rejects the argument or
           // not.
           if (typeof key === 'string') {
-            key = replace(key, trailingValuesRegex, '');
+            key = replace(key, replaceUnderscoresRegex, '-');
             if (test(leadingDashesRegex, key)) {
-              return has(this, key) ||
-                    has(v8Flags,
-                        replace(
-                          replace(
-                            key,
-                            leadingDashesRegex,
-                            ''
-                          ),
-                          replaceDashesRegex,
-                          '_'
-                        )
-                    );
+              key = replace(key, trailingValuesRegex, '');
+              return has(this, key);
             }
-            return has(nodeFlags, key) ||
-                  has(v8Flags, replace(key, replaceDashesRegex, '_'));
+            return has(nodeFlags, key);
           }
           return false;
         }
@@ -736,7 +717,7 @@
 
       return process.allowedNodeEnvironmentFlags = Object.freeze(
         new NodeEnvironmentFlagsSet(
-          allowedNodeEnvironmentFlags.concat(allowedV8EnvironmentFlags)
+          allowedNodeEnvironmentFlags
         ));
     };
 
diff --git a/src/node_options-inl.h b/src/node_options-inl.h
index 1d1eda54751a8f..277121036e519d 100644
--- a/src/node_options-inl.h
+++ b/src/node_options-inl.h
@@ -307,6 +307,12 @@ void OptionsParser<Options>::Parse(
     if (equals_index != std::string::npos)
       original_name += '=';
 
+    // Normalize by replacing `_` with `-` in options.
+    for (std::string::size_type i = 2; i < name.size(); ++i) {
+      if (name[i] == '_')
+        name[i] = '-';
+    }
+
     {
       auto it = aliases_.end();
       // Expand aliases:
@@ -341,19 +347,6 @@ void OptionsParser<Options>::Parse(
 
     auto it = options_.find(name);
 
-    if (it == options_.end()) {
-      // We would assume that this is a V8 option if neither we nor any child
-      // parser knows about it, so we convert - to _ for
-      // canonicalization (since V8 accepts both) and look up again in order
-      // to find a match.
-      // TODO(addaleax): Make the canonicalization unconditional, i.e. allow
-      // both - and _ in Node's own options as well.
-      std::string::size_type index = 2;  // Start after initial '--'.
-      while ((index = name.find('-', index + 1)) != std::string::npos)
-        name[index] = '_';
-      it = options_.find(name);
-    }
-
     if ((it == options_.end() ||
          it->second.env_setting == kDisallowedInEnvironment) &&
         required_env_settings == kAllowedInEnvironment) {
diff --git a/src/node_options.cc b/src/node_options.cc
index b14d18090b734a..2b95b3f811b4e7 100644
--- a/src/node_options.cc
+++ b/src/node_options.cc
@@ -102,8 +102,6 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             &EnvironmentOptions::experimental_worker,
             kAllowedInEnvironment);
   AddOption("--expose-internals", "", &EnvironmentOptions::expose_internals);
-  // TODO(addaleax): Remove this when adding -/_ canonicalization to the parser.
-  AddAlias("--expose_internals", "--expose-internals");
   AddOption("--loader",
             "(with --experimental-modules) use the specified file as a "
             "custom loader",
@@ -206,15 +204,15 @@ PerIsolateOptionsParser::PerIsolateOptionsParser() {
             kAllowedInEnvironment);
 
   // Explicitly add some V8 flags to mark them as allowed in NODE_OPTIONS.
-  AddOption("--abort_on_uncaught_exception",
+  AddOption("--abort-on-uncaught-exception",
             "aborting instead of exiting causes a core file to be generated "
             "for analysis",
             V8Option{},
             kAllowedInEnvironment);
-  AddOption("--max_old_space_size", "", V8Option{}, kAllowedInEnvironment);
-  AddOption("--perf_basic_prof", "", V8Option{}, kAllowedInEnvironment);
-  AddOption("--perf_prof", "", V8Option{}, kAllowedInEnvironment);
-  AddOption("--stack_trace_limit", "", V8Option{}, kAllowedInEnvironment);
+  AddOption("--max-old-space-size", "", V8Option{}, kAllowedInEnvironment);
+  AddOption("--perf-basic-prof", "", V8Option{}, kAllowedInEnvironment);
+  AddOption("--perf-prof", "", V8Option{}, kAllowedInEnvironment);
+  AddOption("--stack-trace-limit", "", V8Option{}, kAllowedInEnvironment);
 
   Insert(&EnvironmentOptionsParser::instance,
          &PerIsolateOptions::get_per_env_options);
diff --git a/test/parallel/test-cli-node-options-disallowed.js b/test/parallel/test-cli-node-options-disallowed.js
index 0351f83c52b50b..733e5cd7c7b977 100644
--- a/test/parallel/test-cli-node-options-disallowed.js
+++ b/test/parallel/test-cli-node-options-disallowed.js
@@ -29,7 +29,6 @@ disallow('--interactive');
 disallow('-i');
 disallow('--v8-options');
 disallow('--');
-disallow('--no_warnings'); // Node options don't allow '_' instead of '-'.
 
 function disallow(opt) {
   const env = Object.assign({}, process.env, { NODE_OPTIONS: opt });
diff --git a/test/parallel/test-cli-node-options.js b/test/parallel/test-cli-node-options.js
index c7096a3acb55f7..4ff63009a7a1e3 100644
--- a/test/parallel/test-cli-node-options.js
+++ b/test/parallel/test-cli-node-options.js
@@ -19,6 +19,7 @@ expect(`-r ${printA}`, 'A\nB\n');
 expect(`-r ${printA} -r ${printA}`, 'A\nB\n');
 expect('--no-deprecation', 'B\n');
 expect('--no-warnings', 'B\n');
+expect('--no_warnings', 'B\n');
 expect('--trace-warnings', 'B\n');
 expect('--redirect-warnings=_', 'B\n');
 expect('--trace-deprecation', 'B\n');
diff --git a/test/parallel/test-pending-deprecation.js b/test/parallel/test-pending-deprecation.js
index f8b4ec8e5b7b94..bb11d84bcc0aec 100644
--- a/test/parallel/test-pending-deprecation.js
+++ b/test/parallel/test-pending-deprecation.js
@@ -36,6 +36,14 @@ switch (process.argv[2]) {
       assert.strictEqual(code, 0, message('--pending-deprecation'));
     }));
 
+    // Test the --pending_deprecation command line switch.
+    fork(__filename, ['switch'], {
+      execArgv: ['--pending_deprecation'],
+      silent: true
+    }).on('exit', common.mustCall((code) => {
+      assert.strictEqual(code, 0, message('--pending_deprecation'));
+    }));
+
     // Test the NODE_PENDING_DEPRECATION environment var.
     fork(__filename, ['env'], {
       env: Object.assign({}, process.env, { NODE_PENDING_DEPRECATION: 1 }),
diff --git a/test/parallel/test-process-env-allowed-flags.js b/test/parallel/test-process-env-allowed-flags.js
index 7313736368ce22..089f139436ca57 100644
--- a/test/parallel/test-process-env-allowed-flags.js
+++ b/test/parallel/test-process-env-allowed-flags.js
@@ -19,10 +19,10 @@ require('../common');
   ].concat(process.config.variables.v8_enable_inspector ? [
     '--inspect-brk',
     'inspect-brk',
+    '--inspect_brk',
   ] : []);
 
   const badFlags = [
-    '--inspect_brk',
     'INSPECT-BRK',
     '--INSPECT-BRK',
     '--r',