Skip to content

feat(no-unnecessary-await): add no-unnecessary-await rule - #1523

Open
bartlomieju wants to merge 2 commits into
mainfrom
oxlint-port-no-unnecessary-await
Open

feat(no-unnecessary-await): add no-unnecessary-await rule#1523
bartlomieju wants to merge 2 commits into
mainfrom
oxlint-port-no-unnecessary-await

Conversation

@bartlomieju

Copy link
Copy Markdown
Member

Ports the no-unnecessary-await rule from oxlint's unicorn plugin as a
native deno_lint rule. It flags await applied to an expression that is
definitely not a thenable/promise, since awaiting such values has no useful
effect.

The following argument expressions are reported: array, arrow function,
nested await, binary expression, class expression, function expression,
JSX element/fragment, literals (boolean, null, numeric, bigint, regex,
string), template literal, unary expression, and update expression. Sequence
expressions are inspected by their final element, and parenthesized
expressions are unwrapped, both matching the upstream behavior.

The rule is diagnostic-only; oxlint's autofix is intentionally not ported,
in line with the diagnostic-only convention for rules in this repository.

Reference implementation:
https://github.com/oxc-project/oxc/blob/main/crates/oxc_linter/src/rules/unicorn/no_unnecessary_await.rs

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

@bartlomieju

Copy link
Copy Markdown
Member Author

Verdict: needs changes — false positive on logical expressions due to an oxc→swc AST-dialect difference. Not merging (new rule + bug).

Correctness — blocking
not_promise matches Expr::Bin(_) unconditionally. That's correct in oxc, where LogicalExpression (&&/||/??) is a separate node from BinaryExpression. But this rule runs on the swc AST (deno_ast), where &&/||/?? are folded into BinExpr as BinaryOp::LogicalAnd | LogicalOr | NullishCoalescing (confirmed by existing rules like no_constant_condition and no_dupe_else_if). So this flags logical expressions, which can evaluate to a promise:

  • await (a || b) → e.g. await (cachedPromise || fetchFresh()) — legitimately awaits a promise, but is reported as unnecessary. False positive.
  • Same for await (a && b) and await (a ?? b).

The existing valid tests await a || b / await a && b / await a ?? b only pass because of precedence (they parse as (await a) || b, so the await arg is just a) — they do not exercise the parenthesized await (a || b) case, which is where the bug lives.

Fix — exclude logical operators:

Expr::Bin(bin) => !matches!(
    bin.op(),
    BinaryOp::LogicalOr | BinaryOp::LogicalAnd | BinaryOp::NullishCoalescing
),

and add valid-case tests: await (a || b), await (a && b), await (a ?? b).

Otherwise correct

  • Array/Arrow/Await/Class/Fn/JSX/Lit/Tpl/Unary/Update never yield promises. Seq → last element, Paren → unwrap, Cond/Ident/Call/New/Member → not flagged (conservative). All good, matching the intent once the logical case is fixed.

Perf: trivial (per await_expr).

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

Blocker: the Expr::Bin match must exclude logical ops (swc dialect), plus the three regression tests. Then tagging sign-off + rebase.

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