JS-1423 Fix S7778 false positives for custom class methods with single argument#6555
Conversation
Tests cover the scenario where consecutive calls to custom class methods named push(), add(), or remove() are incorrectly flagged as combinable, even though the custom methods only accept a single argument. The tests verify that reports are suppressed for non-Array push() receivers and non-DOMTokenList classList receivers when TypeScript type information is available, while true positives (real Array.push, DOM classList, and importScripts) are still reported. Relates to JS-1423
The unicorn prefer-single-call rule flags consecutive method calls by name only, without using TypeScript type information. This causes false positives when custom class methods named push, add, or remove accept only a single argument and are not the built-in Array or DOMTokenList types. Implemented decorator.ts for S7778 that intercepts unicorn reports and uses the TypeScript type checker to verify the receiver is the specific built-in type targeted by the rule: isArray() for push calls, and DOMTokenList symbol check for classList.add/remove calls. importScripts is always reported. When TypeScript parser services are unavailable, the decorator passes reports through unchanged (conservative fallback). Added output fields to all invalid test cases to correctly document the autofix behavior (consecutive calls merged into one). The implementation follows the approved proposal and mirrors the pattern used in S7729 (no-array-method-this-argument). Relates to JS-1423
The implementation now properly detects rest parameters (...args) in method signatures using the dotDotDotToken property. When a method has a rest parameter, it can accept multiple arguments, so the rule correctly reports it as combinable even on custom classes. This fixes the false negative where methods like push(...items: any[]) were incorrectly being suppressed. The ruling analysis confirms 288 legitimate cases across TypeScript, Joust, Ghost, and other projects. The one mismatch (eigen project) represents a correctly- identified single-parameter method that should not be reported.
Ticket: JS-1423 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
s7778-methods-accepting-only-a-single-argument-incorrectly-flagged-as-combinable-sonnet
Replaced the logical AND check `declaration && declaration.dotDotDotToken` with an optional chain expression `declaration?.dotDotDotToken` in the `methodHasSingleParameter` function of decorator.ts. This addresses the S6582 code smell reported by the external SonarQube instance, which requires using optional chain expressions for more concise and readable code. No functional change — the behavior is identical since `declaration?.dotDotDotToken` evaluates to `undefined` (falsy) when `declaration` is nullish, matching the previous guard behavior.
sonar-nigel
bot
requested review from
edward-gonzales-sonarsource,
francois-mora-sonarsource,
gabriel-vivas-sonarsource,
vdiez and
zglicz
|
Instead of checking call signature arity, use the TypeScript type checker to verify the receiver is one of the built-in types the upstream rule targets: use the existing |
Comment: Instead of checking call signature arity, use the TypeScript type checker to verify the receiver is one of the built-in types the upstream rule targets: use the existing `isArray` helper to guard `push` calls, and check that the receiver type is `DOMTokenList` for `classList.add`/`classList.remove`. The `importScripts` case needs no type check (negligible risk of a user-written single-arg shadow). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Done. Simplified the decorator to use pure type-checking as suggested:
Removed the |
|
Missing upstream FP assertion (4E) The test file imports This acts as a sentinel: if the upstream unicorn rule is later improved to understand custom class types and the FP disappears naturally, the test will fail and signal that the decorator is no longer needed. Example structure: import { rules } from '../external/unicorn.js';
const upstreamRule = rules['prefer-single-call'];
// upstream rule still flags the FP pattern
upstreamRuleTester.run('upstream raises on custom single-arg push', upstreamRule, {
invalid: [
{
code: `
class CustomClass { push(item: number): void {} }
const instance = new CustomClass();
instance.push(1);
instance.push(2);
`,
errors: 1,
},
],
valid: [],
}); |
Comment: **Missing upstream FP assertion (4E)**
The test file imports `rule` from `./index.js` throughout, which is the already-decorated rule. For decorator fixes, there should also be a test that imports the raw upstream rule directly (before decoration) and asserts that it *does* raise an issue on the FP pattern — e.g., a custom class with a single-arg `push` method.
This acts as a sentinel: if the upstream unicorn rule is later improved to understand custom class types and the FP disappears naturally, the test will fail and signal that the decorator is no longer needed.
Example structure:
```ts
import { rules } from '../external/unicorn.js';
const upstreamRule = rules['prefer-single-call'];
// upstream rule still flags the FP pattern
upstreamRuleTester.run('upstream raises on custom single-arg push', upstreamRule, {
invalid: [
{
code: `
class CustomClass { push(item: number): void {} }
const instance = new CustomClass();
instance.push(1);
instance.push(2);
`,
errors: 1,
},
],
valid: [],
});
```
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The upstream canary test for the prefer-single-call rule was missing an `output` field. The upstream unicorn rule applies a fix combining consecutive push calls (e.g. `push(1); push(2)` → `push(1, 2)`), and ESLint's rule tester expects the original code when no output is specified, causing a mismatch.
|
Good call. Added an upstream canary |
s7778-methods-accepting-only-a-single-argument-incorrectly-flagged-as-combinable-sonnet
|
Done — the upstream canary test was added in the previous commit and is already in the branch. See |
|
decorator.ts line 67 — unsafe type assertion In const identifier = tsNode as TSESTree.Identifier;
const methodName = identifier.name; // crashes if tsNode is not an IdentifierThis is safe in practice because Please add a defensive check before the assertion: if (tsNode.type !== 'Identifier') {
context.report(descriptor);
return;
}This eliminates the crash risk without changing any current behavior. |
Comment: **decorator.ts line 67 — unsafe type assertion**
In `reportExempting`, after confirming `tsNode.parent?.type === 'MemberExpression'`, the code casts `tsNode` directly to `TSESTree.Identifier`:
```typescript
const identifier = tsNode as TSESTree.Identifier;
const methodName = identifier.name; // crashes if tsNode is not an Identifier
```
This is safe in practice because `prefer-single-call` targets named method calls and only reports `Identifier` property nodes — computed access like `foo['push']` would not trigger the upstream rule. However, there is no explicit guard, so if a future unicorn version ever reports a non-Identifier node the decorator would throw at `identifier.name` rather than silently passing through.
Please add a defensive check before the assertion:
```typescript
if (tsNode.type !== 'Identifier') {
context.report(descriptor);
return;
}
```
This eliminates the crash risk without changing any current behavior.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Good catch. Added the defensive |
Remove unnecessary type assertion in decorator.ts. After the `tsNode.type !== 'Identifier'` type guard, TypeScript already narrows `tsNode` from `TSESTree.Node` to `TSESTree.Identifier`, making the explicit `as TSESTree.Identifier` cast on the former line 72 redundant. Removed the intermediate `identifier` variable and access `tsNode.name` directly. No functional change — behavior is identical.
Add unit tests to cover previously untested code paths in S7778's decorator.ts: the defensive branches for descriptors without a node property (lines 38-39), nodes with non-CallExpression/MemberExpression parents (lines 56-57), and non-Identifier nodes with a MemberExpression parent when TypeScript services are available (lines 68-69). This improves code coverage to meet the quality gate threshold. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thank you for the review and LGTM! |
zglicz
left a comment
zglicz
left a comment
There was a problem hiding this comment.
Lets try a bit more generic approach.
| if (methodName === 'push') { | ||
| // Report only if receiver is an Array | ||
| if (isArray(receiver, services)) { | ||
| context.report(descriptor); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // add or remove: report only if the receiver is a DOMTokenList | ||
| const receiverType = getTypeFromTreeNode(receiver, services); | ||
| if (receiverType.symbol?.name === 'DOMTokenList') { | ||
| context.report(descriptor); | ||
| } |
There was a problem hiding this comment.
Could we check the typechecker that this method exists and accepts multiple arguments? That would be a more robust fix than just patching a few methods?
I'm not sure, but I imagine, that once we have the call expression, we can ask for the signatures, and hopefully there is a variadic one that would confirm that there is or isn't a method that takes multiple arguments.
There was a problem hiding this comment.
Great suggestion — this is indeed more robust. Implemented it.
The decorator now uses getTypeFromTreeNode(callee).getCallSignatures() to check whether any TypeScript call signature accepts more than one argument (rest parameter or multiple parameters). If yes, the report is kept; if all signatures are single-arg, the report is suppressed as a false positive.
This replaces the hard-coded isArray / DOMTokenList name checks with a general, type-aware guard:
function methodAcceptsMultipleArguments(
callee: TSESTree.MemberExpression,
services: RequiredParserServices,
): boolean {
const calleeType = getTypeFromTreeNode(callee as unknown as estree.Node, services);
return calleeType.getCallSignatures().some(sig => {
const params = sig.parameters;
if (params.length === 0) return false;
const lastParam = params[params.length - 1];
const decl = lastParam.valueDeclaration;
return (decl !== undefined && ts.isParameter(decl) && !!decl.dotDotDotToken) || params.length > 1;
});
}The existing test cases for Array.push and DOMTokenList.add/remove continue to pass (their signatures are variadic). The CustomClass { push(item: T) } case is still suppressed (single non-rest param). As a side effect, a custom class with push(...items: any[]) is now correctly reported (its calls can genuinely be combined) — the test was updated accordingly.
…ecorator.ts:87 Comment: Could we check the typechecker that this method exists and accepts multiple arguments? That would be a more robust fix than just patching a few methods? I'm not sure, but I imagine, that once we have the call expression, we can ask for the signatures, and hopefully there is a variadic one that would confirm that there is or isn't a method that takes multiple arguments. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixed two code smells reported by the external SonarQube instance in decorator.ts's methodAcceptsMultipleArguments function: - S121: Added braces around the single-statement `if (params.length === 0)` body to comply with the "Expected { after 'if' condition" rule. - S7755: Replaced `params[params.length - 1]` with `params.at(-1)!` to prefer the more readable `.at()` array method. The non-null assertion is safe because params.length > 0 is guaranteed by the guard above.
Remove forbidden non-null assertion in `methodAcceptsMultipleArguments` function of decorator.ts. Replaced `params.at(-1)!` with a null-safe `params.at(-1)` call guarded by an explicit `undefined` check. The `params.length === 0` guard is subsumed by the new `lastParam === undefined` check, so it has been removed. Behavior is identical — empty parameter lists still return false, and non-empty lists proceed to check for rest parameters or multiple parameters.
Comment: Lets try a bit more generic approach.
|
Done — refactored
|
Add unit tests to cover previously untested code paths. This improves code coverage to meet the quality gate threshold.
s7778-methods-accepting-only-a-single-argument-incorrectly-flagged-as-combinable-sonnet
![sonar-review-alpha[bot]](https://avatars.githubusercontent.com/in/3025063?s=60&v=4){kind=small}
There was a problem hiding this comment.
Conclusion: Solid fix. The decorator correctly gates on TypeScript type info to suppress false positives on single-argument custom class methods, and the conservative pass-through when type info is unavailable is the right call. The ruling snapshots confirm a substantial reduction in false positives, and the test suite covers all meaningful branches including the upstream canary test (which will self-invalidate if unicorn ever gains native TS awareness).
s7778-methods-accepting-only-a-single-argument-incorrectly-flagged-as-combinable-sonnet
|
|
Thank you for the thorough review and positive conclusion! |
|
Thank you for the review! |
2 participants
Fixes false positives in rule S7778 (prefer-single-call) where custom class methods named
push,add, orremoveaccepting only a single argument were incorrectly flagged as combinable.Problem
The unicorn prefer-single-call rule flags consecutive method calls by name only, without using TypeScript type information. This caused false positives when custom class methods named
push,add, orremoveaccepted only a single argument and were not the built-inArrayorDOMTokenListtypes.Changes
decorator.tsfor S7778 that intercepts unicorn reports and uses the TypeScript type checker to verify the receiver is the specific built-in type targeted by the rule:isArray()check forpushcallsDOMTokenListsymbol check forclassList.add/removecallsimportScriptsis always reported (no type check needed)The implementation mirrors the pattern used in S7729 (no-array-method-this-argument).
Relates to JS-1423