Skip to content

feat(no-thenable): add no-thenable rule - #1531

Open
bartlomieju wants to merge 1 commit into
mainfrom
oxlint-port-no-thenable
Open

feat(no-thenable): add no-thenable rule#1531
bartlomieju wants to merge 1 commit into
mainfrom
oxlint-port-no-thenable

Conversation

@bartlomieju

Copy link
Copy Markdown
Member

Ports the oxlint unicorn/no-thenable rule to deno_lint as a native Rust
rule. It flags declaring an object or class member named then, which makes
the value accidentally "thenable" and can break when the value is used in an
await expression.

The rule detects the following cases:

  • Object literals with a then property — shorthand, key/value, method,
    getter, or setter, whether the key is the identifier then, the string
    "then", a `then` template, or a computed key that is an identifier
    resolving to a const/let/var initialized to "then".
  • Classes with a then member — method, property, getter, setter, and their
    static variants (private #then members are intentionally not flagged).
  • Object.defineProperty(foo, "then", ...) and
    Reflect.defineProperty(foo, "then", ...).
  • Object.fromEntries([["then", ...]]).
  • Assignments to a then member, e.g. foo.then = 1, foo["then"] += 1.
  • Exports named then, both via declarations (export const then = 1,
    export function then() {}, export class then {}, and destructuring
    patterns) and via specifiers (export { then }, export { x as then }).

The diagnostic message and help text, as well as the complete pass/fail
fixture arrays, are ported from the oxc reference:
https://github.com/oxc-project/oxc/blob/main/crates/oxc_linter/src/rules/unicorn/no_thenable.rs

Tagged RECOMMENDED (on by default) — flagging for maintainer sign-off.

@bartlomieju

Copy link
Copy Markdown
Member Author

Verdict: needs changes — the computed-key resolution is scope-blind and can false-positive. Otherwise a thorough, correct port. Not merging (new rule + this issue).

The literal/string/template/then-member/export/defineProperty/fromEntries detection all looks correct and comprehensive. The one problem is in resolving computed keys.

Correctness — scope-blindness (false positive)
collect_then_vars builds HashMap<String, SourceRange> keyed by the variable name only (binding.id.sym().to_string()), and check_key_expr looks up ident.sym() — no SyntaxContext/binding identity. So any const/let/var <name> = "then" anywhere in the file makes every computed key [<name>] resolve to "then", even when that identifier is a different binding (shadowed local or a parameter). Concrete false positive:

const then = "then";          // collected: "then" -> range
function f(then: string) {    // parameter shadows the outer const
  return { [then]: 1 };       // `then` here is the PARAM (unknown), but is flagged
}

oxc avoids this because it resolves the computed-key identifier to its declaration via the semantic model. Here the name-based map conflates distinct bindings.

Fix: key then_vars by Id (Atom + SyntaxContext) — collect binding.id.inner.to_id() and look up ident.inner.to_id(). That's exactly what the sibling PR #1525 (prefer-set-size) does for the same "resolve a const binding" pattern, so it'd also make this consistent. Add a shadowing test (param/local named the same as an outer const x = "then").

Secondary (same root cause): let x = "then"; x = "foo"; ({ [x]: 1 }) is still flagged because only the declarator init is inspected. Id-keying won't fix reassignment, but that's a narrower/rarer case and matches oxc's declaration-based resolution.

Otherwise correct

  • contains_then handles ident/string/template(cooked) keys; private #then intentionally excluded (not thenable-exposing); export declarations + specifiers + destructuring patterns all covered.

Perf: an initial full-AST collect_then_vars pass plus the handler traversal — two passes, O(nodes) each; fine.

Housekeeping: RECOMMENDED sign-off (you flagged it); mergeable: UNKNOWN → rebase; docs .md not in diff.

Blocker: switch the computed-key map from name-keyed to Id-keyed (as in #1525) + a shadowing regression test. The rest is solid.

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.

1 participant