iframe-proxy

francois-mora-sonarsource · 2026-09-03T08:26:04Z

Jira

JS-1821 — Fix FP on S7739: classes implementing IThenable interfaces flagged incorrectly.

Paired analyzer keys

javascript:S7739, typescript:S7739

Boundary

Suppressed: a class carrying a then() method is not reported when that same class (the
nearest enclosing class, not the outermost) declares an explicit thenable contract:

JSDoc @implements {IThenable} / {IThenable<T>} / {Thenable} (on the class itself or on
the export class / export default class wrapper), or
TypeScript implements PromiseLike<T>.

Still reported:

a then() method with no thenable-contract annotation;
a class annotated with an unrelated @implements (e.g. {Matcher});
a then() method on an unannotated inner class nested inside an annotated outer class
(regression test added — see below).

All pre-existing exception paths in the rule (Yup/Joi, Promise/Deferred delegation, prototype
assignment, sibling then/catch/finally, JSON Schema conditionals, interface shape descriptors)
are unchanged.

Out of scope (tracked separately)

Gitar's automated "Implementation Status" check marks all 4 of JS-1821's "What" bullets as
covered here. Two of those are not actually addressed by this PR and are tracked
separately, so reviewers shouldn't read this PR as closing them:

jQuery Deferred / general Promise/A+ conformance. This PR only adds the explicit
@implements/implements contract exception described above. Broader Deferred/Promise
recognition (e.g. ns.Deferred = function () { this.then = ...; }, a MemberExpression
assignment target rather than a bare declaration name) is a narrow, separate gap in the
pre-existing naming-based exception, already tracked in
JS-2382 and fixed in a separate,
narrowly-scoped follow-up: JS-2408 Fix FN on S7739: recognize Deferred/Promise assigned via MemberExpression #7894.
Test mocks/fixtures. Out of scope: S7739's scope is Main, so test files aren't
analyzed by this rule in the first place (consistent with JS-1397, which was cancelled for
the same reason).

Note on the community-proposed fix

The proposed fix attached to the Jira resolved the containing class via
context.sourceCode.getAncestors(node) (outermost-first) + .find(), which picks the
outermost matching class ancestor instead of the nearest one — flagged as a correctness
bug by the attached proposal review (request_changes). This PR instead reuses the repo's
existing getAncestorsWithParent(node) helper (innermost-first) and adds a dedicated
Outer/Inner nested-class regression test to lock in the corrected resolution.

Tests

packages/analysis/src/jsts/rules/S7739/no-validation-lib/unit.test.ts — new valid cases
(JSDoc IThenable/Thenable, exported class, nested annotated/unannotated class) and new
invalid cases (no annotation, unrelated @implements, nested-class regression, plain TS
class), plus a new TS PromiseLike suite via NoTypeCheckingRuleTester.

Baselines (Peach, before implementation — attached to JS-1821)

JS-1821-javascript-S7739-peach-before-20260903T081520Z.csv (216 issues)
JS-1821-typescript-S7739-peach-before-20260903T081520Z.csv (486 issues)

RSPEC

Companion RSPEC PR: SonarSource/rspec#8135 documents this exception in the rule
description (Exceptions section), with JS and TS examples matching this PR's
implementation and a caveat that the exception does not extend to static then members.

@implements

Classes that explicitly declare a thenable contract (JSDoc @implements {IThenable}/{Thenable}, or TypeScript implements PromiseLike<T>) no longer get reported for defining a then() method. The containing class is resolved via the repo's existing getAncestorsWithParent helper (nearest-first) rather than context.sourceCode.getAncestors (outermost-first, as used by the community-proposed fix attached to the Jira), so a then() method on an unannotated inner class nested inside an annotated outer class is still correctly reported. Jira: JS-1821

hashicorp-vault-sonar-prod · 2026-09-03T08:26:17Z

github-actions · 2026-09-03T08:32:05Z

Ruling Report

No changes to ruling expected issues in this PR

…egatives - isClassThenMethodWithThenableContract now also recognizes PropertyDefinition (class field) 'then' members and statically-known string keys, matching how the upstream unicorn rule resolves the reported key. Previously only Identifier-keyed MethodDefinition members were exempted, so a thenable- contract class field was still incorrectly reported. - hasTypeScriptThenableContract now matches only the heritage entry's implemented interface name, not its full text (which included type arguments). Previously `implements Cache<PromiseLike<Data>>` was mistaken for an explicit thenable contract, suppressing a genuine report. Jira: JS-1821

…tract Non-blocking follow-up from PR review: cover the quoted string key ('then'() {}) and TypeScript class-field (then = ...) shapes that the existing suppression logic already handles but weren't exercised by tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@implements

- Scope the JSDoc @implements match to the braced type name so unrelated prose on the same comment line (e.g. mentioning "PromiseLike") can no longer suppress a genuine report (false negative). - Recognize a JSDoc thenable contract on a class expression assigned via const/let/var (e.g. `const Foo = class {...}`), not just export-wrapped class declarations, since the comment precedes the declaration rather than the `class` keyword (residual false positive). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@implements

- Scope the JSDoc @implements thenable-contract match to the named type only, excluding type arguments, so a class implementing an unrelated generic interface parameterized by PromiseLike/Thenable (e.g. Cache<PromiseLike<T>>) isn't mistaken for a genuine contract. - Reuse isStringLiteral instead of a hand-rolled Literal check in isThenMemberKey. - Cache the per-class thenable-contract check so it isn't recomputed for every then-named member on the same class.

@implements

An instance-side thenable contract (JSDoc @implements or TS `implements PromiseLike<T>`) was also suppressing reports on a static `then`, but a static then makes the class object itself thenable regardless of the instance contract, so it must still be reported. Excludes static members in isClassThenMethodWithThenableContract and adds regression tests (static method and static field, JS and TS).

The FP-remediation logic for the no-thenable exceptions had grown to 554 lines inline in rule.ts. Split following the S6747 convention: rule.ts now only wires the upstream rule through decorate(); decorator.ts holds the interceptReport wiring; false-positives/index.ts holds the thenable-specific exception detection; helpers.ts holds the generic, non-FP-specific AST utilities. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

getPropertyKeyName is only used internally by collectPropertyNames in the same file, so it doesn't need to be exported. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…exception isThenMemberKey required an Identifier or string Literal key, so a computed key resolved statically by the upstream unicorn rule (['then'], [`then`], or [KEY] where KEY = 'then') still bypassed the thenable-contract exception even on an annotated class. Since unicorn only ever reports a class member key it has already resolved to 'then', checking member.key === node is sufficient for all key shapes.

francois-mora-sonarsource · 2026-09-07T13:18:19Z

@gitar-bot the automated "Implementation Status" check above marks all 4 of JS-1821's "What" bullets as covered — two of those aren't actually addressed by this PR:

jQuery Deferred / Promise-A+ conformance: this PR only adds the explicit @implements/implements contract exception. The existing naming-based Promise/Deferred heuristic is unchanged; its known gap (MemberExpression-target naming, e.g. ns.Deferred = function () {...}) is pre-existing and tracked separately in JS-2382, which I'll address in a follow-up PR.
Test mocks/fixtures: out of scope — S7739's scope is Main, so test files aren't analyzed by this rule at all (see JS-1397, cancelled for the same reason).

I've updated the PR description with an explicit "Out of scope" section to make this boundary clear for reviewers. Also fixed the still-open computed-then-key finding (471e1d3) — thanks for flagging it.

francois-mora-sonarsource · 2026-09-07T14:11:24Z

Thanks — all 5 findings resolved and acknowledged. Note the "Implementation Status" box above is auto-regenerated each review and still incorrectly marks all 4 of JS-1821's "What" bullets as covered by this PR. As noted earlier: this PR only adds the explicit @implements/implements thenable-contract exception. It does not touch:

jQuery Deferred / Promise-A+ conformance — the pre-existing naming-based exception's MemberExpression-target gap is fixed separately in JS-2408 Fix FN on S7739: recognize Deferred/Promise assigned via MemberExpression #7894 (JS-2382).
Test mocks/fixtures — out of scope, since S7739's scope is Main and test files aren't analyzed (consistent with the cancelled JS-1397).

Flagging again so reviewers don't read this PR as closing those two.

Separates the Yup/Joi validation-library exception (exception-libraries.ts) from the structural/contract-based thenable exceptions (intentional-thenable.ts), matching the multi-file false-positives convention used elsewhere (S6819, S1848, S6767). index.ts is now a thin barrel re-exporting both. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…act branches - Fix nested-class valid-case comment that overclaimed outer/inner precedence it didn't actually test (the real leak-prevention check is the invalid counterpart already in the suite). - Add coverage for branches the PR's own implementation added but no test exercised: bare `@implements {IThenable}`/`implements PromiseLike` without a generic argument, `export default class`, and a multi-entry TS heritage clause. - Drop one of three redundant computed-then-key valid cases: key-shape resolution is delegated entirely to the upstream unicorn rule, so they all exercised the identical code path.

Add a valid case with two 'then'-named members (accessor pair) on one annotated class, so the per-class contract cache in hasExplicitThenableContract is actually consulted twice instead of always short-circuiting on the first lookup.

gitar-bot · 2026-09-07T16:20:08Z

Code Review ✅ Approved 6 resolved / 6 findings

Fixes false positives on S7739 for classes implementing IThenable interfaces by adding an explicit thenable-contract exception. Addresses six findings: class fields named then, heritage check matching contract names in type arguments, JSDoc @implements matching in type arguments, static then suppression, computed then keys, and cache hit path coverage. No issues remain.

✅ 6 resolved

✅ Bug: Thenable-contract exception misses class fields named then

📄 packages/analysis/src/jsts/rules/S7739/rule.ts:390-399 📄 packages/analysis/src/jsts/rules/S7739/rule.ts:441-446
isClassThenMethodWithThenableContract only accepts a report whose nearest ancestor is a MethodDefinition with a non-computed Identifier key, but the upstream unicorn no-thenable rule emits no-thenable-class from a combined ['PropertyDefinition','MethodDefinition'] handler reporting node.key. So class C implements PromiseLike<string> { then = (onOk, onErr) => p.then(onOk, onErr); } (or the JSDoc @implements {IThenable} equivalent) is still reported even though the class declares the exact contract this PR is meant to accept; the same shape restriction also leaves a quoted key ('then'() {}) reported, since unicorn resolves keys statically while this check requires an Identifier. Widen the ancestor/key test to cover PropertyDefinition and statically-known string keys.

✅ Bug: TS heritage check matches contract names inside type arguments

📄 packages/analysis/src/jsts/rules/S7739/rule.ts:441-446
hasTypeScriptThenableContract runs THENABLE_CONTRACT_PATTERN over getText(entry) of the whole TSClassImplements node, which includes its type arguments. A class implementing an unrelated interface that is merely parameterized by a thenable type — e.g. class Store implements Cache<PromiseLike<Data>> { then(cb) { ... } } — therefore has its genuine then() issue silently suppressed (a false negative), while the paired JS test asserts that an unrelated @implements {Matcher} must still be reported. Test only the heritage entry's expression (the interface name) rather than its full text.

✅ Bug: JSDoc @implements still matches contract names in type arguments

📄 packages/analysis/src/jsts/rules/S7739/rule.ts:455-467 📄 packages/analysis/src/jsts/rules/S7739/rule.ts:469-482 📄 packages/analysis/src/jsts/rules/S7739/no-validation-lib/unit.test.ts:594-608
IMPLEMENTS_TAG_PATTERN captures the whole braced type ([^}]*) and THENABLE_CONTRACT_PATTERN is tested against that entire capture, so /** @implements {Cache<PromiseLike<string>>} */ class Store { then(cb) {...} } is suppressed even though the class implements an unrelated interface merely parameterized by a thenable. This is exactly the defect that was just fixed on the TypeScript side (hasTypeScriptThenableContract deliberately tests only entry.expression, with an invalid test at unit.test.ts:596-608), so the JSDoc path is left inconsistent with its TS counterpart. Additionally, exec without the global flag only inspects the first tag on a line, so @implements {Matcher} @implements {IThenable} is missed. Scope the match to the base type name and iterate all tags.

✅ Bug: Instance thenable contract also suppresses static then

📄 packages/analysis/src/jsts/rules/S7739/rule.ts:407-421
isClassThenMethodWithThenableContract does not look at member.static, so in class A implements PromiseLike<string> { static then(cb) { ... } then(...) { ... } } (or the JSDoc @implements {IThenable} equivalent) the report on the static then is suppressed too. A declared thenable contract only covers the instance side; a static then makes the class object itself thenable, which is the dangerous case the rule targets (e.g. await import() of a module default-exporting that class). Exclude static members from the exception.

✅ Edge Case: Computed then keys bypass the thenable-contract exception

📄 packages/analysis/src/jsts/rules/S7739/rule.ts:380-393 📄 packages/analysis/src/jsts/rules/S7739/rule.ts:407-415 📄 packages/analysis/src/jsts/rules/S7739/no-validation-lib/unit.test.ts:339-350
The upstream unicorn rule resolves class member keys with getPropertyName/getStaticValue (node_modules/eslint-plugin-unicorn/rules/no-thenable.js:16-46), so it reports class A { ['then']() {} }, class A { [then]() {} } and const KEY = 'then'; class A { [KEY]() {} }. isThenMemberKey only accepts an Identifier/Literal, so the last two forms fall through and an annotated class (/** @implements {IThenable} */ or implements PromiseLike<T>) is still reported — the FP this PR set out to remove persists for those key shapes. Since unicorn only ever yields a class member key it already resolved to then, checking member.key === node is sufficient and covers all key shapes.

...and 1 more resolved from earlier reviews

Implementation Status ✅ 4 of 4 objectives covered

✅ JS-1821 - 4 of 4 objectives covered

This PR implements objectives to prevent S7739 violations on classes implementing IThenable interfaces, recognizing @implements {IThenable} and TypeScript thenable contracts, and allowing Promise/A+ or delegation patterns.

✅ 4 covered here

✅ Classes implementing IThenable interfaces should not trigger S7739 violations
✅ Recognize classes with @implements {IThenable} or similar JSDoc annotations as intentional thenable implementations
✅ Prevent test mocks and fixtures simulating Promise-like behavior from being flagged by S7739
✅ Allow jQuery Deferred/Promise implementations conforming to Promise/A+ specification

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

`Auto-apply`	`Compact`
`gitar auto-apply:on`	`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Gitar}

sonarqube-next · 2026-09-07T16:33:49Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
93.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

francois-mora-sonarsource · 2026-09-08T07:17:19Z

gitar-bot Bot reviewed Sep 3, 2026

View reviewed changes