Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: normalize _ → - when parsing options #23020

Closed
wants to merge 4 commits into from
Closed

cli: normalize _ → - when parsing options #23020

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
671173e
cli: normalize `_` → `-` when parsing options
addaleax Sep 22, 2018
841cf93
fixup! cli: normalize `_` → `-` when parsing options
addaleax Sep 22, 2018
a389c08
fixup! cli: normalize `_` → `-` when parsing options
addaleax Sep 22, 2018
7ca87b0
fixup! cli: normalize `_` → `-` when parsing options
addaleax Sep 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/REPLACEME
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REPLACEME -> 23020?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, done!

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
Expand Down Expand Up @@ -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
Expand Down
43 changes: 12 additions & 31 deletions lib/internal/bootstrap/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,7 @@
// This builds process.allowedNodeEnvironmentFlags
// from data in the config binding

const replaceDashesRegex = /-/g;
const replaceUnderscoresRegex = /_/g;
const leadingDashesRegex = /^--?/;
const trailingValuesRegex = /=.*$/;

Expand All @@ -725,20 +725,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);
}
}

Expand Down Expand Up @@ -772,11 +766,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 {
Expand All @@ -800,29 +792,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;
}
Expand All @@ -833,7 +814,7 @@

return process.allowedNodeEnvironmentFlags = Object.freeze(
new NodeEnvironmentFlagsSet(
allowedNodeEnvironmentFlags.concat(allowedV8EnvironmentFlags)
allowedNodeEnvironmentFlags
));
};

Expand Down
20 changes: 7 additions & 13 deletions src/node_options-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,13 @@ void OptionsParser<Options>::Parse(
if (equals_index != std::string::npos)
original_name += '=';

{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO should be:

for(auto& i: name) {
  if (i == '_') i = '-';
}

So there's no need for the scope, and no use of while when there's an iterator.
ES.71

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have as a rule here to avoid using non-const references., and the rule you linked doesn’t seem to apply here (this is not a for loop to begin with). If you want, this can be

for (std::string::size_type i = 0; i < name.size(); ++i) {
  if (name[i] == '_') 
    name[i] = '-';
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like the for variant better. It's simple, and probably has comparable performance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.S. There ES.72 - Prefer a for-statement to a while-statement when there is an obvious loop variable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which for variant? The no-non-const-references rule has a good reason, which is that it should be obvious what is being modified (and in your code that is name and not just i itself).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one you suggested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The no-non-const-references rule has a good reason

I have no doubt rules have good reasons. Personally I'm assuming anything that is not const is going to be changed, but I'd be happy to discuss this in the context of updating the c++ style guide.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The no-non-const-references rule

Where can I find a reference to that?

// Normalize by replacing `_` with `-` in options.
std::string::size_type index = 2; // Start after initial '--'.
while ((index = name.find('_', index + 1)) != std::string::npos)
name[index] = '-';
}

{
auto it = aliases_.end();
// Expand aliases:
Expand Down Expand Up @@ -341,19 +348,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) {
Expand Down
12 changes: 5 additions & 7 deletions src/node_options.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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);
Expand Down
1 change: 0 additions & 1 deletion test/parallel/test-cli-node-options-disallowed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-cli-node-options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
8 changes: 8 additions & 0 deletions test/parallel/test-pending-deprecation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }),
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-process-env-allowed-flags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down