Skip to content

feat(prefer-set-size): add prefer-set-size rule - #1525

Open
bartlomieju wants to merge 1 commit into
mainfrom
oxlint-port-prefer-set-size
Open

feat(prefer-set-size): add prefer-set-size rule#1525
bartlomieju wants to merge 1 commit into
mainfrom
oxlint-port-prefer-set-size

Conversation

@bartlomieju

Copy link
Copy Markdown
Member

This ports the prefer-set-size rule from oxlint's unicorn plugin as a native
Rust rule.

The rule flags a .length member access on an array produced by converting a
Set to an array, and recommends using Set#size instead. It catches both
spread conversions ([...set].length) and Array.from(set).length, where the
Set is either a direct new Set(...) or a const-declared variable
initialized to new Set(...).

It deliberately does not flag: optional chaining (?.length), computed access
([length] / ["length"]), non-length properties, new NotSet(...), bare
Set(...) without new, let/var-declared or destructured set variables,
Array.from(set, mapFn) (a second map-fn argument), Array?.from /
Array.from?.(), and NotArray.from.

The pass/fail fixtures are ported verbatim from the oxc reference rule:
https://github.com/oxc-project/oxc/blob/main/crates/oxc_linter/src/rules/unicorn/prefer_set_size.rs
(the autofix-only fixtures are dropped since deno_lint has no autofixer here).

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

@bartlomieju

Copy link
Copy Markdown
Member Author

Verdict: LGTM — correct and well-tested. Recommend merge after the RECOMMENDED sign-off you flagged + rebase. Not auto-merging (new rule).

Careful two-pass port (collect const Set vars → check .length). I verified the tricky parts:

Correctness — verified

  • Soundness via const-only: only const x = new Set(...) with a simple Ident binding is collected, so the variable can't be reassigned to a non-Set. let/var/destructured bindings are (correctly) not flagged. Right call.
  • Hygiene: set_vars is keyed by Id (Atom + SyntaxContext), so a const set in one scope won't false-match a same-named binding in another. Good.
  • Exclusions all hold: ?.length (parent is OptChainExpr), computed [length]/["length"], non-length props, new NotSet(), bare Set() w/o new, Array.from(set, mapFn) (is_array_from_call requires args.len() == 1), Array?.from/Array.from?.() (not a plain Expr::Member/Expr::Call), and multi-element arrays ([...set, x].length) — each is correctly rejected and covered by valid tests.
  • get_set_node requires a single spread element for the array form ([set].length with a non-spread element is not flagged), and unwrap_parens is applied at both the object and set-node levels, so [...(set)].length still resolves.

Perf: two full traversals (collector + handler), each O(nodes), with O(1) HashSet lookups — fine for a per-file pass.

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

No code changes requested — the const-only soundness and the exclusion set are exactly right. Just tagging + 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