-
Notifications
You must be signed in to change notification settings - Fork 436
eslint-factory: require null-safe companion for typeof err === "object" catch guards
#42568
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.
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
Changes from all commits
09cef2e
676847e
5522de3
f9bf045
4c04d11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/codebase-design] 💡 Suggested extractionExtract to a shared module, e.g. // 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 @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: { | ||
|
|
@@ -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"() { | ||
|
|
@@ -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]; | ||
|
|
@@ -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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential false negative: } 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 This is an edge case but worth documenting as a known limitation (e.g., a comment), or tightening by only allowing @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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnosing-bugs] The 💡 Example & mitigationcatch (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 @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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.ReturnStatementThe same gap exists in @copilot please address this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnosing-bugs] The early-exit check only recognises 💡 Suggested fixconst 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 @copilot please address this. |
||
| ) { | ||
| top.hasNonNullGuard = true; | ||
| } | ||
| }, | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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)); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code duplication — extract shared helpers. Consider extracting them to a shared utility, e.g. @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; | ||
|
|
@@ -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: [] }); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -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]; | ||
|
|
@@ -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; | ||
| } | ||
| }, | ||
|
|
||
|
|
||
There was a problem hiding this comment.
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
typeofbut is missing a regression test for theBinaryExpressionfalse negative: a standaloneerr !== nullin a separateifshould not guard a subsequenttypeofcheck.💡 Suggested test to add
Without this, the fix to the
BinaryExpressionhandler cannot be verified and the false negative can silently regress.@copilot please address this.