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: #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]>

Author: addaleax

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

lib/internal/bootstrap/node.js

@@ -616,7 +616,7 @@
 // This builds process.allowedNodeEnvironmentFlags
 // from data in the config binding
 
-constreplaceDashesRegex=/-/g;
+constreplaceUnderscoresRegex=/_/g;
 constleadingDashesRegex=/^--?/;
 consttrailingValuesRegex=/=.*$/;
 
@@ -628,20 +628,14 @@
 constget=()=>{
 const{
  getOptions,
-types: { kV8Option },
 envSettings: { kAllowedInEnvironment }
 }=internalBinding('options');
 const{ options, aliases }=getOptions();
 
-constallowedV8EnvironmentFlags=[];
 constallowedNodeEnvironmentFlags=[];
 for(const[name,info]ofoptions){
 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(
-newSet(flags.map(trimLeadingDashes)),
-Object.getOwnPropertyDescriptors(Set.prototype))
+constnodeFlags=Object.defineProperties(
+newSet(allowedNodeEnvironmentFlags.map(trimLeadingDashes)),
+Object.getOwnPropertyDescriptors(Set.prototype)
 );
 
 classNodeEnvironmentFlagsSetextendsSet{
@@ -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(typeofkey==='string'){
-key=replace(key,trailingValuesRegex,'');
+key=replace(key,replaceUnderscoresRegex,'-');
 if(test(leadingDashesRegex,key)){
-returnhas(this,key)||
-has(v8Flags,
-replace(
-replace(
-key,
-leadingDashesRegex,
-''
-),
-replaceDashesRegex,
-'_'
-)
-);
+key=replace(key,trailingValuesRegex,'');
+returnhas(this,key);
 }
-returnhas(nodeFlags,key)||
-has(v8Flags,replace(key,replaceDashesRegex,'_'));
+returnhas(nodeFlags,key);
 }
 returnfalse;
 }
@@ -736,7 +717,7 @@
 
 returnprocess.allowedNodeEnvironmentFlags=Object.freeze(
 newNodeEnvironmentFlagsSet(
-allowedNodeEnvironmentFlags.concat(allowedV8EnvironmentFlags)
+allowedNodeEnvironmentFlags
 ));
 };
 

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

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

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 '-'.
 
 functiondisallow(opt){
 constenv=Object.assign({},process.env,{NODE_OPTIONS: opt});

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');

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}),

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',
 ] : []);
 
 constbadFlags=[
-'--inspect_brk',
 'INSPECT-BRK',
 '--INSPECT-BRK',
 '--r',