Skip to content

feat(linter): implement n/handle-callback-err rule#19616

Open
baevm wants to merge 7 commits intooxc-project:mainfrom
baevm:n/handle_callback_error
Open

feat(linter): implement n/handle-callback-err rule#19616
baevm wants to merge 7 commits intooxc-project:mainfrom
baevm:n/handle_callback_error

Conversation

@baevm
Copy link
Contributor

@baevm baevm commented Feb 22, 2026

this PR implements n/handle-callback-err rule, passes all tests from original rule

issue #493

@baevm baevm requested a review from camc314 as a code owner February 22, 2026 09:50
Copilot AI review requested due to automatic review settings February 22, 2026 09:50
@github-actions github-actions bot added A-linter Area - Linter C-enhancement Category - New feature or request labels Feb 22, 2026
Copy link
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 implements the n/handle-callback-err rule from eslint-plugin-n, which enforces proper error handling in Node.js callback functions. The rule checks whether the first parameter of a function (typically named "err" or "error") is referenced within the function body, helping developers avoid forgetting to handle errors in callback patterns.

Changes:

  • Implements the HandleCallbackErr rule with configurable pattern matching (plain string or regex) for identifying error parameters
  • Adds comprehensive test coverage covering various scenarios including nested callbacks, arrow functions, and custom error parameter names
  • Integrates the rule into the linter infrastructure through generated enum and runner files

Reviewed changes

Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/oxc_linter/src/rules/node/handle_callback_err.rs New rule implementation with pattern matching, configuration parsing, and logic to detect unhandled error parameters
crates/oxc_linter/src/snapshots/node_handle_callback_err.snap Snapshot tests showing expected diagnostic output for failing test cases
crates/oxc_linter/src/rules.rs Adds the new rule module to the node rules collection
crates/oxc_linter/src/generated/rules_enum.rs Generated code integrating the rule into the RuleEnum system
crates/oxc_linter/src/generated/rule_runner_impls.rs Generated code specifying which AST node types the rule should run on

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 22, 2026

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 51 skipped benchmarks1


Comparing baevm:n/handle_callback_error (9ed0f41) with main (1710f56)

Open in CodSpeed

Footnotes

  1. 51 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.

@baevm
Copy link
Contributor Author

baevm commented Feb 22, 2026

CI spell check is failing for test case with regex option, not sure how to fix/ignore it

image

@camc314
Copy link
Contributor

camc314 commented Feb 22, 2026

@baevm you should be able to do // spellchecker:disable-line ?

@baevm
Copy link
Contributor Author

baevm commented Feb 22, 2026

@baevm you should be able to do // spellchecker:disable-line ?

ty! fixed it

@baevm baevm force-pushed the n/handle_callback_error branch 3 times, most recently from d716c47 to 3eae25e Compare February 25, 2026 08:52
graphite-app bot pushed a commit that referenced this pull request Feb 26, 2026
#19731)

Previously, we did not correctly handle config docs for rules that only take one primitive argument. As a result, some docs were missing the config section entirely.

This fixes the problem by handling that case when generating docs. It fixes the docs for `typescript/class-literal-property-style`, `node/handle-callback-err` (not merged yet), and `react/no-will-update-set-state`.

I have checked this change against the existing docs site, and no changes to the docs seem to be problematic here, only a small handful of rules were impacted.

We discovered this problem in #19616

The change to the docs generation itself was generated by Claude Code, I tested the change manually and verified it worked, and also added the two extra rules to the docs snapshot.
graphite-app bot pushed a commit that referenced this pull request Feb 26, 2026
#19731)

Previously, we did not correctly handle config docs for rules that only take one primitive argument. As a result, some docs were missing the config section entirely.

This fixes the problem by handling that case when generating docs. It fixes the docs for `typescript/class-literal-property-style`, `node/handle-callback-err` (not merged yet), and `react/no-will-update-set-state`.

I have checked this change against the existing docs site, and no changes to the docs seem to be problematic here, only a small handful of rules were impacted.

We discovered this problem in #19616

The change to the docs generation itself was generated by Claude Code, I tested the change manually and verified it worked, and also added the two extra rules to the docs snapshot.
baevm and others added 5 commits February 26, 2026 18:13
@baevm baevm force-pushed the n/handle_callback_error branch from 3eae25e to eb9e8db Compare February 26, 2026 13:13
@baevm baevm requested a review from connorshea February 27, 2026 14:42
@camc314 camc314 self-assigned this Feb 27, 2026

#[derive(Debug, Default, Clone, JsonSchema, Deserialize)]
pub struct HandleCallbackErr(
#[serde(deserialize_with = "deserialize_error_pattern")] Box<ErrorPattern>,
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to format this as:

Suggested change
#[serde(deserialize_with = "deserialize_error_pattern")] Box<ErrorPattern>,
#[serde(deserialize_with = "deserialize_error_pattern")]
Box<ErrorPattern>,

Copy link
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.

From a docs/config perspective this looks fine to me now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants