cli: normalize _ → - when parsing options

This allows for option syntax similar to V8’s one, e.g. `--no_warnings` has the same effect as `--no-warnings`. PR-URL: nodejs#23020 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
vaibhavb · Sep 25, 2018 · 0227635 · 0227635
1 parent 5605cec
commit 0227635
Show file tree

Hide file tree

Showing 8 changed files with 45 additions and 58 deletions.
diff --git 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
@@ -742,7 +742,7 @@
     // This builds process.allowedNodeEnvironmentFlags
     // from data in the config binding
 
-    const replaceDashesRegex = /-/g;
+    const replaceUnderscoresRegex = /_/g;
     const leadingDashesRegex = /^--?/;
     const trailingValuesRegex = /=.*$/;
 
@@ -754,20 +754,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);
         }
       }
 
@@ -801,11 +795,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 {
@@ -829,29 +821,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;
         }
@@ -862,7 +843,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
@@ -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
@@ -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",
@@ -204,15 +202,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
@@ -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
@@ -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
@@ -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
@@ -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',