Skip to content

fix(eslint-factory): extend no-caught-error-interpolation to cover EventEmitter .on('error') handlers #49768

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Aug 2, 2026
+134 −2
Merged

fix(eslint-factory): extend no-caught-error-interpolation to cover EventEmitter .on('error') handlers #49768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03bfcb8
Initial plan
Copilot Aug 2, 2026
4fe3434
fix(eslint-factory): extend no-caught-error-interpolation to cover Ev…
Copilot Aug 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 106 additions & 0 deletions eslint-factory/src/rules/no-caught-error-interpolation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,4 +329,110 @@ describe("no-caught-error-interpolation", () => {
],
});
});

it("valid: non-'error' EventEmitter event name is not flagged", () => {

@github-actions github-actions Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The test for the named/hoisted function reference passes only because the rule never sees the inner template literal (it's inside a separate function declaration, not inside the .on callback). Consider adding a case where a FunctionExpression (not arrow) is used inline to confirm both callback forms are covered:

💡 Suggested test for inline FunctionExpression 
it("invalid: inline function expression in .on('error', ...) is flagged", () => {
  cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, {
    valid: [],
    invalid: [
      {
        code: `emitter.on('error', function(err) { log(\`error: \${err}\`); });`,
        errors: [{ messageId: "bareErrorInterpolation", data: { errorVar: "err" } }],
      },
    ],
  });
});

The implementation handles FunctionExpression via the union type, but no test exercises it.

@copilot please address this.

cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, {
valid: [`emitter.on('data', chunk => { log(\`got: \${chunk}\`); });`, `emitter.once('close', code => { log(\`exited: \${code}\`); });`, `emitter.addListener('message', msg => { log(\`msg: \${msg}\`); });`],
invalid: [],
});
});

it("valid: named/hoisted listener function passed to .on('error', ...) is not flagged (inline-only restriction)", () => {
cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, {
valid: [
// Named function reference — not an inline callback, so out of scope per the inline restriction
`function onError(err) { log(\`\${err}\`); } emitter.on('error', onError);`,
],
invalid: [],
});
});

it("invalid: bare .on('error', ...) listener variable is flagged", () => {
cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, {
valid: [],
invalid: [
{
code: `emitter.on('error', err => { log(\`event error: \${err}\`); });`,
Comment on lines +350 to +355
errors: [
{
messageId: "bareErrorInterpolation",
data: { errorVar: "err" },
suggestions: [
{
messageId: "useStringFallback",
data: { errorVar: "err" },
output: `emitter.on('error', err => { log(\`event error: \${String(err)}\`); });`,
},
],
},
],
},
{
// Mirrors the grounded live occurrence from mcp_server_core.cjs:1052
code: `process.stdin.on("error", err => server.debug(\`stdin error: \${err}\`));`,
errors: [
{
messageId: "bareErrorInterpolation",
data: { errorVar: "err" },
suggestions: [
{
messageId: "useStringFallback",
data: { errorVar: "err" },
output: `process.stdin.on("error", err => server.debug(\`stdin error: \${String(err)}\`));`,
},
],
},
],
},
],
});
});

it("invalid: bare .once('error', ...) listener variable is flagged", () => {
cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, {
valid: [],
invalid: [
{
code: `emitter.once('error', err => { log(\`once error: \${err}\`); });`,
errors: [
{
messageId: "bareErrorInterpolation",
data: { errorVar: "err" },
suggestions: [
{
messageId: "useStringFallback",
data: { errorVar: "err" },
output: `emitter.once('error', err => { log(\`once error: \${String(err)}\`); });`,
},
],
},
],
},
],
});
});

it("invalid: bare .addListener('error', ...) listener variable is flagged", () => {
cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, {
valid: [],
invalid: [
{
code: `emitter.addListener('error', err => { log(\`add error: \${err}\`); });`,
errors: [
{
messageId: "bareErrorInterpolation",
data: { errorVar: "err" },
suggestions: [
{
messageId: "useStringFallback",
data: { errorVar: "err" },
output: `emitter.addListener('error', err => { log(\`add error: \${String(err)}\`); });`,
},
],
},
],
},
],
});
});
});
30 changes: 28 additions & 2 deletions eslint-factory/src/rules/no-caught-error-interpolation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,24 @@ function isInlineRejectionHandler(node: TSESTree.ArrowFunctionExpression | TSEST
return false;
}

/**
* Returns true when the function node is an inline listener passed as the
* second argument to an EventEmitter-style `.on('error', fn)`,
* `.once('error', fn)`, or `.addListener('error', fn)` call.
*/
function isInlineEventErrorHandler(node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression): boolean {
const parent = node.parent;
if (!parent || parent.type !== AST_NODE_TYPES.CallExpression) return false;
const callee = parent.callee;
if (callee.type !== AST_NODE_TYPES.MemberExpression || callee.computed) return false;
const prop = callee.property;
if (prop.type !== AST_NODE_TYPES.Identifier) return false;

@github-actions github-actions Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This heuristic matches any object's .on/.once/.addListener('error', fn) call, not just Node EventEmitters, which will produce false positives on non-Error-passing APIs.

💡 Why this matters and how to narrow it

isInlineEventErrorHandler only checks the method name and that the first argument is the string literal "error" — it has no way to distinguish a real Node EventEmitter from jQuery-style .on('error', fn) (jQuery passes a jQuery.Event, not an Error), RxJS-like observables, or arbitrary user-defined pub/sub classes with an on method. Unlike .catch/.then, which are effectively unambiguous promise-shaped names, on/once/addListener are extremely common generic method names across unrelated APIs.

Concretely, this will now force String(err)/getErrorMessage(err) wrapping on code like:

$(elem).on('error', (event) => log(`img failed: ${event}`)); // event is not an Error

Consider either:

  1. Restricting to call sites where the receiver is a known EventEmitter-typed variable (requires type information via ESLintUtils.RuleCreator + parserServices), or
  2. At minimum documenting this as a known limitation/tradeoff in the rule's docs.description, and adding a test case demonstrating the accepted false-positive behavior so it's an intentional, reviewed tradeoff rather than an unverified assumption.

if (prop.name !== "on" && prop.name !== "once" && prop.name !== "addListener") return false;
if (parent.arguments[1] !== node) return false;
const eventArg = parent.arguments[0];
return eventArg?.type === AST_NODE_TYPES.Literal && eventArg.value === "error";
}

/**
* Returns true when `node` is a bare `Identifier` expression — no member
* access, no call, no unary/binary operation, no nullish coercion. Used to
Expand All @@ -44,13 +62,20 @@ function isCaughtErrorVariableDef(def: TSESLint.Scope.Definition): boolean {
}

// Inline rejection handler parameter (.catch(err => ...) / .then(_, err => ...))
// or inline EventEmitter 'error' event listener (.on('error', err => ...) etc.)
// def.node is the function node for Parameter definitions
if (def.type === "Parameter") {
const fn = def.node as TSESTree.Node;
if (fn.type !== AST_NODE_TYPES.ArrowFunctionExpression && fn.type !== AST_NODE_TYPES.FunctionExpression) {
return false;
}
return isInlineRejectionHandler(fn as TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression);
const fnNode = fn as TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression;
if (isInlineRejectionHandler(fnNode)) return true;
if (isInlineEventErrorHandler(fnNode)) {
// Only the first parameter receives the error object from an 'error' event
return fnNode.params[0] === def.name;

@github-actions github-actions Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fnNode.params[0] === def.name silently fails to flag destructured error parameters, e.g. .on('error', ({message}) => ...), leaving that case untested and its behavior unverified.

💡 Why this matters

When the first parameter is a destructuring pattern (ObjectPattern/ArrayPattern), def.name refers to the Identifier bound inside the pattern, not the pattern node itself, so it never strictly equals fnNode.params[0]. The function returns false in that branch, meaning destructured error bindings from an .on('error', ...) handler are never flagged — a silent false negative rather than a crash, but it's an edge case with no test coverage, so there's no evidence this fallthrough is the intended behavior versus an overlooked gap.

Add a test case such as:

emitter.on('error', ({message}) => log(`err: ${message}`));

to confirm/document whether destructured members should be in scope for this rule, and to guard against regressions.

}
return false;

@github-actions github-actions Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The fnNode.params[0] === def.name guard (lines 75–78) suppresses false-positives on extra parameters, but there is no test verifying this branch holds.

💡 Suggested test

Add a valid case to confirm that extra params in an error handler are not flagged:

it("valid: second param in .on('error', ...) is not flagged", () => {
  cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, {
    valid: [
      `emitter.on('error', (err, context) => { log(\`ctx: \${context}\`); });`,
    ],
    invalid: [],
  });
});

Without this, a future refactor could silently drop the guard and start over-flagging context.

@copilot please address this.

}

return false;
Expand All @@ -67,7 +92,8 @@ export const noCaughtErrorInterpolationRule = createRule({
"For Error objects this produces the redundant 'Error: message' prefix; for non-Error throws (plain objects, strings, etc.) " +
"it silently produces '[object Object]' or another useless string. " +
"Use getErrorMessage(err) for consistent, safe formatting, or String(err) when getErrorMessage is unavailable. " +
"Detected scopes: try/catch bindings, .catch(fn) inline callbacks, and .then(onFulfilled, onRejected) inline rejection handlers.",
"Detected scopes: try/catch bindings, .catch(fn) inline callbacks, .then(onFulfilled, onRejected) inline rejection handlers, " +
"and inline EventEmitter 'error' event listeners (.on('error', fn) / .once('error', fn) / .addListener('error', fn)).",
},
schema: [],
messages: {
Expand Down
Loading