Skip to content

feat(linter/prefer-nullish-coalescing): add rule#16778

Merged
graphite-app[bot] merged 1 commit intomainfrom
c/12-12-feat_linter_prefer-nullish-coalescing_add_rule
Dec 14, 2025
Merged

feat(linter/prefer-nullish-coalescing): add rule#16778
graphite-app[bot] merged 1 commit intomainfrom
c/12-12-feat_linter_prefer-nullish-coalescing_add_rule

Conversation

@camc314
Copy link
Copy Markdown
Contributor

@camc314 camc314 commented Dec 12, 2025 •
edited
Loading

@github-actions github-actions bot added A-linter Area - Linter A-cli Area - CLI C-enhancement Category - New feature or request labels Dec 12, 2025
@camc314 Graphite App
Copy link
Copy Markdown
Contributor Author

camc314 commented Dec 12, 2025 •
edited
Loading

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@camc314 camc314 self-assigned this Dec 12, 2025
@camc314 camc314 force-pushed the c/12-12-feat_linter_prefer-nullish-coalescing_add_rule branch from 2ea797d to aaf0e91 Compare December 12, 2025 13:21
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Dec 12, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #16778 will not alter performance

Comparing c/12-12-feat_linter_prefer-nullish-coalescing_add_rule (6c250d6) with main (73da317)1

Summary

✅ 4 untouched
⏩ 41 skipped2

Footnotes

  1. No successful run was found on graphite-base/16778 (5835378) during the generation of this report, so main (73da317) was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

  2. 41 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@camc314 camc314 changed the base branch from main to graphite-base/16778 December 13, 2025 14:50
@camc314 camc314 force-pushed the c/12-12-feat_linter_prefer-nullish-coalescing_add_rule branch from aaf0e91 to 6c250d6 Compare December 13, 2025 14:50
@camc314 camc314 changed the base branch from graphite-base/16778 to renovate/oxlint December 13, 2025 14:50
@camc314 camc314 mentioned this pull request Dec 13, 2025
Merged
1 task
@graphite-app graphite-app bot changed the base branch from renovate/oxlint to graphite-base/16778 December 13, 2025 14:50
@camc314 camc314 marked this pull request as ready for review December 13, 2025 15:02
charliecreates[bot]
charliecreates bot reviewed Dec 13, 2025
View reviewed changes
Copy link
Copy Markdown

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The added fixture for prefer-nullish-coalescing is too narrow to protect against partial or incorrect implementations. There’s also a staging/consistency concern: the rule is marked pending with RUN_FUNCTIONS: Unknown, yet snapshots already include a diagnostic for it, which should be clarified or aligned. Finally, the ignorePrimitives config shape would benefit from explicit documentation that false is a no-op/default equivalent.

Additional notes (3)
  • Readability | apps/oxlint/fixtures/tsgolint/prefer-nullish-coalescing.ts:1-2
    The fixture intends to demonstrate a simplifiable ternary, but it uses nullableString !== null && nullableString !== undefined even though the type is string | null. Including an undefined check here is slightly inconsistent with the declared type and makes the example less crisp. A more canonical example would either include undefined in the type or use != null.

This matters because fixtures double as documentation and regression tests; having a minimal/accurate case reduces confusion when comparing against upstream rule behavior.

  • Readability | crates/oxc_linter/src/rules/typescript/prefer_nullish_coalescing.rs:31-48
    IgnorePrimitives::All(bool) allows false as a JSON value, which is redundant with the default and easy to misconfigure (users might assume "ignorePrimitives": false is meaningful/required). If you keep this shape for compatibility, consider making the semantics explicit in schema/docs (i.e., true means ignore all primitives; false is equivalent to omitting the option). Right now it’s only implied by Default returning All(false).

  • Maintainability | apps/oxlint/fixtures/tsgolint/.oxlintrc.json:31-35
    The rule is registered as pending, enabled in fixtures, and produces snapshot output, but the implementation file only defines configuration + metadata; there is no run/visitor logic shown. If this is intentionally a stub awaiting upstream work (blocked by the linked PR), enabling it in fixtures will make the rule appear supported even though its behavior is effectively delegated/absent.

Given the PR is blocked, it would be better to avoid turning this on in fixture configs until the rule actually runs, or clearly gate it behind the pending status in fixture generation/tests.

Summary of changes

Summary

This PR wires up a new TypeScript ESLint parity rule: typescript-eslint(prefer-nullish-coalescing).

What changed

  • Fixture config: Enabled "typescript/prefer-nullish-coalescing": "error" in apps/oxlint/fixtures/tsgolint/.oxlintrc.json.
  • New fixture: Added apps/oxlint/fixtures/tsgolint/prefer-nullish-coalescing.ts to trigger the rule.
  • Snapshots: Updated multiple oxlint snapshots to reflect +1 file and +1 error, and to include the new diagnostic output.
  • Linter wiring:
    • Registered the rule module in crates/oxc_linter/src/rules.rs and in the declare_all_lint_rules! list.
    • Added a generated RuleRunner impl entry in crates/oxc_linter/src/generated/rule_runner_impls.rs.
  • Rule skeleton: Added crates/oxc_linter/src/rules/typescript/prefer_nullish_coalescing.rs with configuration schema and Rule config (no detection logic shown in this diff).

@charliecreates charliecreates bot removed the request for review from CharlieHelps December 13, 2025 15:11
@camc314 camc314 added the 0-merge Merge with Graphite Merge Queue label Dec 13, 2025
@camc314 Graphite App
Copy link
Copy Markdown
Contributor Author

camc314 commented Dec 13, 2025 •
edited by graphite-app bot
Loading

Merge activity

  • Dec 13, 3:25 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Dec 13, 3:26 PM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.
  • Dec 14, 2:01 PM UTC: camc314 added this pull request to the Graphite merge queue.
  • Dec 14, 2:07 PM UTC: Merged by the Graphite merge queue.

@camc314 camc314 force-pushed the c/12-12-feat_linter_prefer-nullish-coalescing_add_rule branch from 6c250d6 to 9db66ec Compare December 13, 2025 15:25
@camc314 camc314 force-pushed the graphite-base/16778 branch from 5835378 to a3b9eff Compare December 13, 2025 15:25
@graphite-app graphite-app bot changed the base branch from graphite-base/16778 to main December 13, 2025 15:26
connorshea
connorshea approved these changes Dec 14, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@connorshea connorshea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits but LGTM.

Should the tsgolint version requirement be bumped to 0.9.0 as part of adding this rule?

Copilot AI review requested due to automatic review settings December 14, 2025 13:48
@camc314 camc314 force-pushed the c/12-12-feat_linter_prefer-nullish-coalescing_add_rule branch from 9db66ec to c9be8ef Compare December 14, 2025 13:48
@camc314 camc314 force-pushed the c/12-12-feat_linter_prefer-nullish-coalescing_add_rule branch from c9be8ef to d2d5763 Compare December 14, 2025 13:48
@camc314 camc314 mentioned this pull request Dec 14, 2025
Merged
@camc314
Copy link
Copy Markdown
Contributor Author

camc314 commented Dec 14, 2025

A few nits but LGTM.

Should the tsgolint version requirement be bumped to 0.9.0 as part of adding this rule?

good call out #16829

Copilot AI reviewed Dec 14, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds the prefer-nullish-coalescing TypeScript linting rule from typescript-eslint to the Oxc linter. The rule enforces using the nullish coalescing operator (??) instead of logical OR (||) or conditional expressions when checking for null or undefined values, helping prevent bugs with falsy values like 0, '', or false.

Key Changes:

  • Adds the prefer-nullish-coalescing rule implementation with comprehensive configuration options
  • Updates the oxlint-tsgolint dependency from 0.8.6 to 0.9.0 to include the rule logic
  • Adds test fixtures and snapshot tests to verify rule behavior

Reviewed changes

Copilot reviewed 8 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/oxc_linter/src/rules/typescript/prefer_nullish_coalescing.rs New rule implementation with configuration structs for customizing rule behavior
crates/oxc_linter/src/rules.rs Registers the new rule in the module declarations and rule list
crates/oxc_linter/src/generated/rule_runner_impls.rs Auto-generated runner implementation for the new rule
package.json Updates oxlint-tsgolint dependency to version 0.9.0
pnpm-lock.yaml Updates lock file with new dependency versions and integrity hashes
apps/oxlint/fixtures/tsgolint/prefer-nullish-coalescing.ts Adds test fixture demonstrating rule violation
apps/oxlint/fixtures/tsgolint/.oxlintrc.json Enables the rule in test configuration
apps/oxlint/src/snapshots/[email protected] Updates snapshot with expected diagnostic output
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

graphite-app[bot]
graphite-app bot reviewed Dec 14, 2025
View reviewed changes
@graphite-app graphite-app bot force-pushed the c/12-12-feat_linter_prefer-nullish-coalescing_add_rule branch from 10aef3b to 1bdaab1 Compare December 14, 2025 14:01
@graphite-app graphite-app bot merged commit 1bdaab1 into main Dec 14, 2025
20 checks passed
@graphite-app graphite-app bot deleted the c/12-12-feat_linter_prefer-nullish-coalescing_add_rule branch December 14, 2025 14:07
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Dec 14, 2025
@connorshea connorshea mentioned this pull request Dec 14, 2025
Merged
@Boshen Boshen mentioned this pull request Dec 15, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

Copilot code review Copilot Copilot left review comments

@charliecreates charliecreates[bot] charliecreates[bot] left review comments

@connorshea connorshea connorshea approved these changes

Assignees

@camc314 camc314

Labels

A-cli Area - CLI A-linter Area - Linter C-enhancement Category - New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@camc314 @connorshea