-
-
Notifications
You must be signed in to change notification settings - Fork 34.3k
cli: fix negated boolean options not removing implied options #56313
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.
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
base: main
Are you sure you want to change the base?
Conversation
src/node_options-inl.h
Outdated
| 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before the fix, there was likely code intended to apply negations to implications as well, but options with the --no- prefix were never added to the implications_.
4801938 to
6425807
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56313 +/- ##
==========================================
+ Coverage 88.00% 88.52% +0.51%
==========================================
Files 704 704
Lines 208739 208744 +5
Branches 40196 40276 +80
==========================================
+ Hits 183706 184782 +1076
+ Misses 16992 15970 -1022
+ Partials 8041 7992 -49
🚀 New features to boost your workflow:
|
|
@islandryu Can you please rebase? Also, is this semver-major? It does change existing functionality |
6425807 to
4559f70
Compare
| default_field_map[value.name] = | ||
| *value.target_field | ||
| ->template Lookup<bool>(options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it so that it sets a default value instead of simply toggling a boolean.
When using negated options for boolean options that imply other options (e.g., --inspect-brk), the implied options are not removed as expected. This causes the implied option to remain active, leading to unexpected behavior.
shimaryuuheinoMac-mini:node shimaryuuhei$ node --inspect-brk --no-inspect-brk -e "console.log(\"foo\")" Debugger listening on ws://127.0.0.1:9229/67479127-5d17-4027-b514-70217b2fe102 For help, see: https://nodejs.org/en/docs/inspector fooWhen using --no-inspect-brk, only the effect of the --inspect option remains.
This change ensures that when a negated option (e.g., --no-...) is used, any implied options are also removed. After this fix, the above example will no longer activate the implied options.
Alternative
Currently, the negated options affected by this change are not documented in the official CLI documentation: https://nodejs.org/api/cli.html
An alternative approach could be to remove these undocumented negated options entirely, ensuring no unexpected behavior arises from their usage.