Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
69 changes: 64 additions & 5 deletions eslint-factory/src/rules/no-unsafe-catch-error-property.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -342,17 +342,76 @@ try {
});
});

it("valid: typeof err === 'object' guard suppresses all warnings in the catch block", () => {
it("valid: typeof err === 'object' with non-null guard suppresses warnings in the catch block", () => {
cjsRuleTester.run("no-unsafe-catch-error-property", noUnsafeCatchErrorPropertyRule, {
valid: [
`try { f(); } catch (err) { if (typeof err === 'object') { console.log(err.status); } }`,
`try { f(); } catch (err) { if (typeof err === 'object' && err !== null) { console.log(err.status); } }`,
`try { f(); } catch (err) { if ('object' === typeof err) { console.log(err.status); } }`,
`try { f(); } catch (err) { if ('object' === typeof err && null !== err) { console.log(err.status); } }`,
`try { f(); } catch (err) { if (typeof err === 'object' && err != null) { console.log(err.status); } }`,
`try { f(); } catch (err) { if (err && typeof err === 'object') { console.log(err.status); } }`,
`try { f(); } catch (err) { if (!err) return; if (typeof err === 'object') { console.log(err.status); } }`,
],
invalid: [],
});
});

it("invalid: bare typeof err === 'object' guard is insufficient", () => {
cjsRuleTester.run("no-unsafe-catch-error-property", noUnsafeCatchErrorPropertyRule, {
valid: [],
invalid: [
{
code: `try { f(); } catch (err) { if (typeof err === 'object') { console.log(err.status); } }`,
errors: [
{
messageId: "unsafeProperty",
data: { prop: "status", errorVar: "err" },
suggestions: [
{
messageId: "wrapWithInstanceof",
data: { errorVar: "err", prop: "status" },
output: `try { f(); } catch (err) { if (typeof err === 'object') { console.log((err instanceof Error ? err.status : undefined)); } }`,
},
],
},
],
},
{
code: `try { f(); } catch (err) { if ('object' === typeof err) { console.log(err.status); } }`,
errors: [
{
messageId: "unsafeProperty",
data: { prop: "status", errorVar: "err" },
suggestions: [
{
messageId: "wrapWithInstanceof",
data: { errorVar: "err", prop: "status" },
output: `try { f(); } catch (err) { if ('object' === typeof err) { console.log((err instanceof Error ? err.status : undefined)); } }`,
},
],
},
],
},
{
// Standalone err !== null in a separate if (without return) does not count as companion guard
code: `try { f(); } catch (err) { if (err !== null) { } if (typeof err === 'object') { console.log(err.status); } }`,
errors: [
{
messageId: "unsafeProperty",
data: { prop: "status", errorVar: "err" },
suggestions: [
{
messageId: "wrapWithInstanceof",
data: { errorVar: "err", prop: "status" },
output: `try { f(); } catch (err) { if (err !== null) { } if (typeof err === 'object') { console.log((err instanceof Error ? err.status : undefined)); } }`,
},
],
},
],
},
],
});
});

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 new invalid test block covers bare typeof but is missing a regression test for the BinaryExpression false negative: a standalone err !== null in a separate if should not guard a subsequent typeof check.

💡 Suggested test to add
it("invalid: err !== null in a separate if does not guard a subsequent typeof check", () => {
  cjsRuleTester.run("no-unsafe-catch-error-property", noUnsafeCatchErrorPropertyRule, {
    valid: [],
    invalid: [
      {
        code: `try { f(); } catch (err) { if (err !== null) { } if (typeof err === 'object') { console.log(err.status); } }`,
        errors: [
          {
            messageId: "unsafeProperty",
            data: { prop: "status", errorVar: "err" },
          },
        ],
      },
    ],
  });
});

Without this, the fix to the BinaryExpression handler cannot be verified and the false negative can silently regress.

@copilot please address this.


it("invalid: err.status without guard is flagged", () => {
cjsRuleTester.run("no-unsafe-catch-error-property", noUnsafeCatchErrorPropertyRule, {
valid: [],
Expand Down Expand Up @@ -425,9 +484,9 @@ try {
});
});

it("valid: typeof err === 'object' guard suppresses .status access (mirrors real call sites)", () => {
it("valid: typeof err === 'object' with truthy err guard suppresses .status access", () => {
cjsRuleTester.run("no-unsafe-catch-error-property", noUnsafeCatchErrorPropertyRule, {
valid: [`try { f(); } catch (err) { if (typeof err === 'object' && err.status === 404) { } }`],
valid: [`try { f(); } catch (err) { if (err && typeof err === 'object' && err.status === 404) { } }`],
invalid: [],
});
});
Expand Down
90 changes: 69 additions & 21 deletions eslint-factory/src/rules/no-unsafe-catch-error-property.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,34 @@ const UNSAFE_PROPERTIES = new Set(["message", "stack", "code", "status", "cause"
interface CatchFrame {
varName: string;
hasGuard: boolean;
hasNonNullGuard: boolean;
unsafeNodes: Array<{ node: TSESTree.MemberExpression; prop: string }>;
}

function isTypeofObjectCheck(node: TSESTree.Expression, varName: string): boolean {

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.

[/codebase-design] isTypeofObjectCheck and isNonNullGuardCheck are copy-pasted verbatim into both rule files, creating a maintenance trap: any future change to guard semantics must be applied in two places.

💡 Suggested extraction

Extract to a shared module, e.g. eslint-factory/src/rules/guard-helpers.ts:

// guard-helpers.ts
import { AST_NODE_TYPES, TSESTree } from "`@typescript-eslint/utils`";

export function isTypeofObjectCheck(node: TSESTree.Expression, varName: string): boolean { ... }
export function isNonNullGuardCheck(node: TSESTree.Expression, varName: string): boolean { ... }

Then import in each rule:

import { isTypeofObjectCheck, isNonNullGuardCheck } from "./guard-helpers";

This also enables testing the helpers in isolation, which would directly expose edge cases like the false negative discussed in the BinaryExpression handler.

@copilot please address this.

if (node.type !== AST_NODE_TYPES.BinaryExpression || node.operator !== "===") return false;
const { left, right } = node;
return (
(left.type === AST_NODE_TYPES.UnaryExpression && left.operator === "typeof" && left.argument.type === AST_NODE_TYPES.Identifier && left.argument.name === varName && right.type === AST_NODE_TYPES.Literal && right.value === "object") ||
(right.type === AST_NODE_TYPES.UnaryExpression && right.operator === "typeof" && right.argument.type === AST_NODE_TYPES.Identifier && right.argument.name === varName && left.type === AST_NODE_TYPES.Literal && left.value === "object")
);
}

function isNonNullGuardCheck(node: TSESTree.Expression, varName: string): boolean {
if (node.type === AST_NODE_TYPES.Identifier) {
return node.name === varName;
}

if (node.type !== AST_NODE_TYPES.BinaryExpression || (node.operator !== "!==" && node.operator !== "!=")) {
return false;
}

const isVarRef = (value: TSESTree.Expression): value is TSESTree.Identifier => value.type === AST_NODE_TYPES.Identifier && value.name === varName;
const isNullLiteral = (value: TSESTree.Expression): value is TSESTree.Literal => value.type === AST_NODE_TYPES.Literal && value.value === null;

return (isVarRef(node.left) && isNullLiteral(node.right)) || (isVarRef(node.right) && isNullLiteral(node.left));
}

export const noUnsafeCatchErrorPropertyRule = createRule({
name: "no-unsafe-catch-error-property",
meta: {
Expand All @@ -36,11 +61,11 @@ export const noUnsafeCatchErrorPropertyRule = createRule({
// Only handle simple identifier bindings; skip bare catch {} and destructuring patterns.
// Push a sentinel frame so CatchClause:exit always has a matching pop.
if (!param || param.type !== AST_NODE_TYPES.Identifier) {
catchStack.push({ varName: "", hasGuard: true, unsafeNodes: [] });
catchStack.push({ varName: "", hasGuard: true, hasNonNullGuard: true, unsafeNodes: [] });
return;
}

catchStack.push({ varName: param.name, hasGuard: false, unsafeNodes: [] });
catchStack.push({ varName: param.name, hasGuard: false, hasNonNullGuard: false, unsafeNodes: [] });
},

"CatchClause:exit"() {
Expand Down Expand Up @@ -90,7 +115,7 @@ export const noUnsafeCatchErrorPropertyRule = createRule({
},

// Detect catchVar instanceof Error — also accepted as a safe guard
// Detect typeof catchVar === 'object' — also accepted as a safe guard
// Detect typeof catchVar === 'object' with a non-null companion guard
BinaryExpression(node) {
if (catchStack.length === 0) return;
const top = catchStack[catchStack.length - 1];
Expand All @@ -101,25 +126,48 @@ export const noUnsafeCatchErrorPropertyRule = createRule({
return;
}

// typeof varName === 'object' or 'object' === typeof varName
if (node.operator === "===") {
const { left, right } = node;
const isTypeofObject =
(left.type === AST_NODE_TYPES.UnaryExpression &&
left.operator === "typeof" &&
left.argument.type === AST_NODE_TYPES.Identifier &&
left.argument.name === top.varName &&
right.type === AST_NODE_TYPES.Literal &&
right.value === "object") ||
(right.type === AST_NODE_TYPES.UnaryExpression &&
right.operator === "typeof" &&
right.argument.type === AST_NODE_TYPES.Identifier &&
right.argument.name === top.varName &&
left.type === AST_NODE_TYPES.Literal &&
left.value === "object");
if (isTypeofObject) {
top.hasGuard = true;
if (isTypeofObjectCheck(node, top.varName) && top.hasNonNullGuard) {
top.hasGuard = true;

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.

Potential false negative: BinaryExpression cross-block null-check. The hasNonNullGuard flag is set for any err !== null expression seen anywhere in the catch block. This means the following will incorrectly be considered safe:

} catch (err) {
  if (err !== null) {
    doSomethingElse();   // null check scoped only here
  }
  // err might be null again here
  if (typeof err === 'object') {
    console.log(err.status);  // false negative: hasNonNullGuard=true from above
  }
}

The LogicalExpression handler correctly handles in-expression conjuncts. For sequential statements, only the !err return pattern (via IfStatement) is truly scoped. The BinaryExpression path treats any null-check anywhere in the block as sufficient.

This is an edge case but worth documenting as a known limitation (e.g., a comment), or tightening by only allowing BinaryExpression-based null checks that appear as the direct test of an IfStatement (not inside another block).

@copilot please address this.

}
},

LogicalExpression(node) {
if (catchStack.length === 0) return;
const top = catchStack[catchStack.length - 1];
if (!top || top.hasGuard || !top.varName || node.operator !== "&&") return;

const conjuncts: TSESTree.Expression[] = [];
const collectConjuncts = (expr: TSESTree.Expression): void => {
if (expr.type === AST_NODE_TYPES.LogicalExpression && expr.operator === "&&") {
collectConjuncts(expr.left);
collectConjuncts(expr.right);
return;
}
conjuncts.push(expr);
};
collectConjuncts(node);

const hasTypeofObject = conjuncts.some(expr => isTypeofObjectCheck(expr, top.varName));
const hasNonNullGuard = conjuncts.some(expr => isNonNullGuardCheck(expr, top.varName));
if (hasTypeofObject && hasNonNullGuard) {
top.hasGuard = true;
top.hasNonNullGuard = true;
}
},

IfStatement(node) {

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.

[/diagnosing-bugs] The IfStatement visitor fires for any depth in the catch block, including if (!err) return; nested inside another branch. A deeply-nested early exit may not actually guard the top-level catch body, creating a potential false negative.

💡 Example & mitigation
catch (err) {
  if (someCondition) {
    if (!err) return;  // only runs when someCondition is truthy
  }
  // err could still be falsy here — but hasNonNullGuard is now set!
  if (typeof err === 'object') {
    console.log(err.status);  // incorrectly allowed
  }
}

Consider restricting hasNonNullGuard = true to IfStatement nodes whose parent is the CatchClause's BlockStatement directly (depth-1 check). This trades off some false-positive tolerance for correctness on the false-negative side.

@copilot please address this.

if (catchStack.length === 0) return;
const top = catchStack[catchStack.length - 1];
if (!top || top.hasGuard || !top.varName) return;

if (
node.test.type === AST_NODE_TYPES.UnaryExpression &&
node.test.operator === "!" &&
node.test.argument.type === AST_NODE_TYPES.Identifier &&
node.test.argument.name === top.varName &&
node.consequent.type === AST_NODE_TYPES.ReturnStatement

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.

IfStatement handler misses block-body early return. node.consequent.type === AST_NODE_TYPES.ReturnStatement only matches bare if (!err) return;, but NOT the equally common if (!err) { return; } (a BlockStatement whose first/only statement is ReturnStatement).

Example that will NOT be recognized as a non-null guard today:

} catch (err) {
  if (!err) { return; }             // consequent is BlockStatement — missed!
  if (typeof err === 'object') {
    console.log(err.status);        // false negative: rule stays silent
  }
}

Consider also matching:

node.consequent.type === AST_NODE_TYPES.BlockStatement &&
node.consequent.body.length === 1 &&
node.consequent.body[0].type === AST_NODE_TYPES.ReturnStatement

The same gap exists in no-unsafe-promise-catch-error-property.ts.

@copilot please address this.

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.

[/diagnosing-bugs] The early-exit check only recognises return; a throw statement is equally valid as a non-null escape hatch but won't set hasNonNullGuard.

💡 Suggested fix
const consequent = node.consequent;
const isEarlyExit =
  consequent.type === AST_NODE_TYPES.ReturnStatement ||
  consequent.type === AST_NODE_TYPES.ThrowStatement;
if (isEarlyExit) {
  top.hasNonNullGuard = true;
}

The same change is needed in no-unsafe-promise-catch-error-property.ts.

@copilot please address this.

) {
top.hasNonNullGuard = true;
}
},

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -240,13 +240,40 @@ describe("no-unsafe-promise-catch-error-property", () => {
});
});

it("valid: typeof err === 'object' guard suppresses warnings in .catch() callback", () => {
it("valid: typeof err === 'object' with non-null guard suppresses warnings in .catch() callback", () => {
cjsRuleTester.run("no-unsafe-promise-catch-error-property", noUnsafePromiseCatchErrorPropertyRule, {
valid: [`promise.catch(err => { if (typeof err === 'object') { console.log(err.status); } });`, `promise.catch(err => { if ('object' === typeof err) { console.log(err.status); } });`],
valid: [
`promise.catch(err => { if (typeof err === 'object' && err !== null) { console.log(err.status); } });`,
`promise.catch(err => { if ('object' === typeof err && null !== err) { console.log(err.status); } });`,
`promise.catch(err => { if (typeof err === 'object' && err != null) { console.log(err.status); } });`,
`promise.catch(err => { if (err && typeof err === 'object') { console.log(err.status); } });`,
`promise.catch(err => { if (!err) return; if (typeof err === 'object') { console.log(err.status); } });`,
],
invalid: [],
});
});

it("invalid: bare typeof err === 'object' guard is insufficient in .catch() callback", () => {
cjsRuleTester.run("no-unsafe-promise-catch-error-property", noUnsafePromiseCatchErrorPropertyRule, {
valid: [],
invalid: [
{
code: `promise.catch(err => { if (typeof err === 'object') { console.log(err.status); } });`,
errors: [{ messageId: "unsafeProperty", data: { prop: "status", errorVar: "err" } }],
},
{
code: `promise.catch(err => { if ('object' === typeof err) { console.log(err.status); } });`,
errors: [{ messageId: "unsafeProperty", data: { prop: "status", errorVar: "err" } }],
},
{
// Standalone err !== null in a separate if (without return) does not count as companion guard
code: `promise.catch(err => { if (err !== null) { } if (typeof err === 'object') { console.log(err.status); } });`,
errors: [{ messageId: "unsafeProperty", data: { prop: "status", errorVar: "err" } }],
},
],
});
});

it("invalid: err.status without guard is flagged in .catch() callback", () => {
cjsRuleTester.run("no-unsafe-promise-catch-error-property", noUnsafePromiseCatchErrorPropertyRule, {
valid: [],
Expand Down
92 changes: 70 additions & 22 deletions eslint-factory/src/rules/no-unsafe-promise-catch-error-property.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,34 @@ const UNSAFE_PROPERTIES = new Set(["message", "stack", "code", "status", "cause"
interface CatchFrame {
varName: string;
hasGuard: boolean;
hasNonNullGuard: boolean;
unsafeNodes: Array<{ node: TSESTree.MemberExpression; prop: string }>;
}

function isTypeofObjectCheck(node: TSESTree.Expression, varName: string): boolean {
if (node.type !== AST_NODE_TYPES.BinaryExpression || node.operator !== "===") return false;
const { left, right } = node;
return (
(left.type === AST_NODE_TYPES.UnaryExpression && left.operator === "typeof" && left.argument.type === AST_NODE_TYPES.Identifier && left.argument.name === varName && right.type === AST_NODE_TYPES.Literal && right.value === "object") ||
(right.type === AST_NODE_TYPES.UnaryExpression && right.operator === "typeof" && right.argument.type === AST_NODE_TYPES.Identifier && right.argument.name === varName && left.type === AST_NODE_TYPES.Literal && left.value === "object")
);
}

function isNonNullGuardCheck(node: TSESTree.Expression, varName: string): boolean {
if (node.type === AST_NODE_TYPES.Identifier) {
return node.name === varName;
}

if (node.type !== AST_NODE_TYPES.BinaryExpression || (node.operator !== "!==" && node.operator !== "!=")) {
return false;
}

const isVarRef = (value: TSESTree.Expression): value is TSESTree.Identifier => value.type === AST_NODE_TYPES.Identifier && value.name === varName;
const isNullLiteral = (value: TSESTree.Expression): value is TSESTree.Literal => value.type === AST_NODE_TYPES.Literal && value.value === null;

return (isVarRef(node.left) && isNullLiteral(node.right)) || (isVarRef(node.right) && isNullLiteral(node.left));
}

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.

Code duplication — extract shared helpers. isTypeofObjectCheck and isNonNullGuardCheck are copied verbatim from no-unsafe-catch-error-property.ts. Any future logic change will need to be applied in both files, making divergence likely.

Consider extracting them to a shared utility, e.g. eslint-factory/src/rules/guard-helpers.ts, then importing from both rules.

@copilot please address this.


function isCatchCallback(node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression): boolean {
const parent = node.parent;
if (!parent || parent.type !== AST_NODE_TYPES.CallExpression) return false;
Expand Down Expand Up @@ -45,14 +70,14 @@ export const noUnsafePromiseCatchErrorPropertyRule = createRule({
const params = node.params;
// Only handle simple identifier bindings; skip no-param and destructuring callbacks.
if (params.length === 1 && params[0].type === AST_NODE_TYPES.Identifier) {
stack.push({ varName: params[0].name, hasGuard: false, unsafeNodes: [] });
stack.push({ varName: params[0].name, hasGuard: false, hasNonNullGuard: false, unsafeNodes: [] });
} else {
stack.push({ varName: "", hasGuard: true, unsafeNodes: [] });
stack.push({ varName: "", hasGuard: true, hasNonNullGuard: true, unsafeNodes: [] });
}
} else {
// Sentinel: prevents the outer .catch() frame from collecting accesses
// to a shadowed parameter name inside this nested function.
stack.push({ varName: "", hasGuard: true, unsafeNodes: [] });
stack.push({ varName: "", hasGuard: true, hasNonNullGuard: true, unsafeNodes: [] });
}
}

Expand Down Expand Up @@ -101,7 +126,7 @@ export const noUnsafePromiseCatchErrorPropertyRule = createRule({
},

// Detect catchVar instanceof Error — also accepted as a safe guard
// Detect typeof catchVar === 'object' — also accepted as a safe guard
// Detect typeof catchVar === 'object' with a non-null companion guard
BinaryExpression(node) {
if (stack.length === 0) return;
const top = stack[stack.length - 1];
Expand All @@ -112,25 +137,48 @@ export const noUnsafePromiseCatchErrorPropertyRule = createRule({
return;
}

// typeof varName === 'object' or 'object' === typeof varName
if (node.operator === "===") {
const { left, right } = node;
const isTypeofObject =
(left.type === AST_NODE_TYPES.UnaryExpression &&
left.operator === "typeof" &&
left.argument.type === AST_NODE_TYPES.Identifier &&
left.argument.name === top.varName &&
right.type === AST_NODE_TYPES.Literal &&
right.value === "object") ||
(right.type === AST_NODE_TYPES.UnaryExpression &&
right.operator === "typeof" &&
right.argument.type === AST_NODE_TYPES.Identifier &&
right.argument.name === top.varName &&
left.type === AST_NODE_TYPES.Literal &&
left.value === "object");
if (isTypeofObject) {
top.hasGuard = true;
if (isTypeofObjectCheck(node, top.varName) && top.hasNonNullGuard) {
top.hasGuard = true;
}
},

LogicalExpression(node) {
if (stack.length === 0) return;
const top = stack[stack.length - 1];
if (!top || top.hasGuard || !top.varName || node.operator !== "&&") return;

const conjuncts: TSESTree.Expression[] = [];
const collectConjuncts = (expr: TSESTree.Expression): void => {
if (expr.type === AST_NODE_TYPES.LogicalExpression && expr.operator === "&&") {
collectConjuncts(expr.left);
collectConjuncts(expr.right);
return;
}
conjuncts.push(expr);
};
collectConjuncts(node);

const hasTypeofObject = conjuncts.some(expr => isTypeofObjectCheck(expr, top.varName));
const hasNonNullGuard = conjuncts.some(expr => isNonNullGuardCheck(expr, top.varName));
if (hasTypeofObject && hasNonNullGuard) {
top.hasGuard = true;
top.hasNonNullGuard = true;
}
},

IfStatement(node) {
if (stack.length === 0) return;
const top = stack[stack.length - 1];
if (!top || top.hasGuard || !top.varName) return;

if (
node.test.type === AST_NODE_TYPES.UnaryExpression &&
node.test.operator === "!" &&
node.test.argument.type === AST_NODE_TYPES.Identifier &&
node.test.argument.name === top.varName &&
node.consequent.type === AST_NODE_TYPES.ReturnStatement
) {
top.hasNonNullGuard = true;
}
},

Expand Down
Loading