Skip to content

Commit

Permalink
src: allow to negate boolean CLI flags
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
targos committed Sep 4, 2021
1 parent 14fc4dd commit f793380
Show file tree
Hide file tree
Showing 12 changed files with 152 additions and 34 deletions.
2 changes: 1 addition & 1 deletion lib/internal/bootstrap/pre_execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
26 changes: 23 additions & 3 deletions lib/internal/main/print_help.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ const {
MathMax,
ObjectKeys,
RegExp,
StringPrototypeLocaleCompare,
StringPrototypeSlice,
StringPrototypeTrimLeft,
StringPrototypeRepeat,
StringPrototypeReplace,
Expand Down Expand Up @@ -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}`;
Expand All @@ -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.
Expand Down
9 changes: 7 additions & 2 deletions lib/internal/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
10 changes: 8 additions & 2 deletions lib/internal/process/per_thread.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

const {
ArrayIsArray,
ArrayPrototypePush,
BigUint64Array,
Float64Array,
NumberMAX_SAFE_INTEGER,
Expand Down Expand Up @@ -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);
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
1 change: 1 addition & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -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") \
Expand Down
6 changes: 3 additions & 3 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
36 changes: 31 additions & 5 deletions src/node_options-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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);
Expand All @@ -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) {
Expand Down Expand Up @@ -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());
Expand Down
29 changes: 18 additions & 11 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down
12 changes: 7 additions & 5 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
39 changes: 39 additions & 0 deletions test/parallel/test-cli-options-negation.js
Original file line number Diff line number Diff line change
@@ -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'));
}
14 changes: 13 additions & 1 deletion test/parallel/test-process-env-allowed-flags-are-documented.js
Original file line number Diff line number Diff line change
Expand Up @@ -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}.`);
Expand Down Expand Up @@ -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 ' +
Expand Down

0 comments on commit f793380

Please sign in to comment.