Skip to content

fix: do not increment a non-count option whose value is 1 - #551

Closed
MFA-G wants to merge 1 commit into
yargs:mainfrom
MFA-G:fix/count-increment-literal-one
Closed

MFA-G wants to merge 1 commit into
yargs:mainfrom
MFA-G:fix/count-increment-literal-one

Conversation

@MFA-G

@MFA-G MFA-G commented Sep 12, 2026

Copy link
Copy Markdown

Fixes #506.

The bug

An option that is not declared as a counter is incremented when it receives the literal value 1:

parser("-x 3 -x 1")   // => { _: [], x: 4 }     expected { _: [], x: [3, 1] }
parser("-x 1 -x 3")   // => { _: [], x: [1, 3] } (correct)

duplicate-arguments-array: false does not help either — it also yields 4.

Root cause

processValue replaces the value of a count argument with the result of increment(), which is the number 1:

if (checkAllAliases(key, flags.counts) && (isUndefined(value) || typeof value === "boolean")) {
  value = increment()
}

setKey then decides whether to increment the running count by comparing that value back:

if (value === increment()) {   // i.e. value === 1
  o[key] = increment(o[key])
}

So the check is really value === 1, and any option whose value happens to be 1 takes the counter branch. That explains why the result looks arbitrary: -x 3 -x 1 computes 3 + 1, while -x 1 -x 3 takes the branch on the first occurrence (where o.x is still undefined, so it stays 1) and then falls through normally for 3.

As @shadowspawn noted in the issue, this regressed in the TypeScript conversion: index.js used the increment function itself as a sentinel value (value === increment, a reference comparison) rather than its return value.

The fix

Restore the sentinel semantics with a dedicated symbol, so the condition identifies "this is a count argument" instead of "this value is 1":

const incrementMarker = Symbol("increment")
// processValue
value = incrementMarker
// setKey
if (value === incrementMarker) o[key] = increment(o[key])

A symbol is used rather than the function reference so it cannot collide with a user-supplied value. The marker never escapes the parser: setKey is the only consumer and it always replaces it with a number.

Behaviour

input before after
-x 3 -x 1 x: 4 x: [3, 1]
-x 1 -x 3 x: [1, 3] x: [1, 3]
-x 3 -x 1, duplicate-arguments-array: false x: 4 x: 1
-v -v -v with count: ["v"] v: 3 v: 3
-v 1 with count: ["v"] v: 1, _: [1] v: 1, _: [1]

Counter behaviour is unchanged, including the existing should increment regardless of arg value case.

Tests

Two regression tests added to the count describe block. Both fail on main and pass with the fix.

Validation

  • npm test — 364 passing (362 before, no existing test changed)
  • npm run test:typescript — 20 passing
  • npm run check (gts lint) — clean
  • npm run compile — clean

`processValue` replaces the value of a count argument with `increment()`,
which returns the number `1`. `setKey` then tested `value === increment()`
to decide whether to increment the running count, so any option receiving
the literal value `1` took the counter branch:

    parser('-x 3 -x 1') // => { _: [], x: 4 }

Use a dedicated symbol as the marker instead, so the check identifies a
count argument rather than the value 1. Reversing the order (`-x 1 -x 3`)
already worked, which is why the result looked arbitrary.

Fixes yargs#506
@shadowspawn

Copy link
Copy Markdown
Member

This looks like a drive-by AI contribution. The account created 28 Pull Requests on 28 repositories so far this month.

This may get looked at and used when the issue is prioritised.

The human maintainer does not have time to review all AI heavy PRs that are opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple arguments are incremented if they are equal to 1.

2 participants