Fix TypeScript never-narrowing in require-async-entrypoint-catch for setup/js lint path#43440
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
require-async-entrypoint-catch for setup/js lint path
🤖 PR Triage — Run §28725134784
Notes: Small targeted fix (2 files, +31/-12). Adds a test covering the corrected narrowing. Undraft to enable CI and expedite batch review with #43404.
|
|
Thanks for this clean fix to The change is tight and well-reasoned — replacing the ad-hoc The new invalid-case test for the multi-declarator
|
There was a problem hiding this comment.
Pull request overview
Fixes a TypeScript control-flow narrowing regression in the require-async-entrypoint-catch ESLint rule that caused eslint-factory to fail to build (blocking lint:setup-js) by aligning scope-definition typing/narrowing with @typescript-eslint’s official definition types.
Changes:
- Replaced ad-hoc
TSESLint.Scope.Definition & { ... }intersections withTSESLint.Scope.Definitions.*types and narrowed viaTSESLint.Scope.DefinitionType.*. - Switched variable entrypoint declaration lookup to the typed
definition.parentpath to avoidnever-narrowing and preserve behavior. - Added a regression test covering a module-scope async entrypoint declared in a multi-declarator
const.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/require-async-entrypoint-catch.ts | Updates scope-definition typing and declaration access to avoid TS never narrowing while keeping async entrypoint detection behavior. |
| eslint-factory/src/rules/require-async-entrypoint-catch.test.ts | Adds a targeted invalid-case regression test for multi-declarator module-scope async entrypoints. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43440 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — leaving 3 comments, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Test completeness (
/tdd): The new regression test only exercises the invalid (firing) path. Adding a paired valid case would confirm thedefinition.parentfix doesn't over-flag the handled form. - Type consistency (
/codebase-design):FunctionDeclarationDefinitioncarries a redundant{ node: TSESTree.FunctionDeclaration }intersection thatVariableDefinitioncorrectly drops; worth checking if it's still needed. - Test naming (
/tdd): Minor — the new test name describes the scenario but not the expected outcome, departing slightly from the spec-style naming used in the rest of the file.
Positive Highlights
- ✅ Root cause squarely addressed: replacing ad-hoc string-literal type guards with
TSESLint.Scope.DefinitionType.*enum values is the right fix, not just a workaround. - ✅
definition.parentswap is clean and makes the intent obvious — the typed accessor removes the need to manually re-check.type === AST_NODE_TYPES.VariableDeclaration. - ✅ Regression test added — the PR does not leave the fix unvalidated.
- ✅ Small, focused diff: 6 lines added in the source, 25 in the test — easy to review and revert if needed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 48 AIC · ⌖ 6.07 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| it("invalid: bare call to async function in multi-declarator module-scope const is flagged", () => { | ||
| cjsRuleTester.run("require-async-entrypoint-catch", requireAsyncEntrypointCatchRule, { | ||
| valid: [], |
There was a problem hiding this comment.
[/tdd] The new test block only covers the invalid case — valid: [] leaves the mirror path untested. Adding a valid case (multi-declarator const with .catch()) would confirm the rule does not over-flag and that definition.parent works for both branches.
💡 Suggested addition
valid: [
{
code: `const helper = () => 1, main = async () => { return 42; };
main().catch(err => { console.error(err); process.exitCode = 1; });`,
},
],Without this, the test only checks that the rule fires, not that it correctly accepts the handled form after the definition.parent fix.
@copilot please address this.
| type: "Variable"; | ||
| node: TSESTree.VariableDeclarator; | ||
| }; | ||
| type FunctionDeclarationDefinition = TSESLint.Scope.Definitions.FunctionNameDefinition & { node: TSESTree.FunctionDeclaration }; |
There was a problem hiding this comment.
[/codebase-design] FunctionDeclarationDefinition still intersects with an explicit { node: TSESTree.FunctionDeclaration } override while VariableDefinition uses the library type as-is. If TSESLint.Scope.Definitions.FunctionNameDefinition already types node as TSESTree.FunctionDeclaration, the extra intersection is redundant and will silently diverge if the upstream type ever changes.
💡 Investigate and simplify
Check whether FunctionNameDefinition.node is already TSESTree.FunctionDeclaration in the installed @typescript-eslint/utils version. If so, simplify to:
type FunctionDeclarationDefinition = TSESLint.Scope.Definitions.FunctionNameDefinition;This mirrors how VariableDefinition is defined on line 8 and removes a maintenance hazard.
@copilot please address this.
| }); | ||
| }); | ||
|
|
||
| it("invalid: bare call to async function in multi-declarator module-scope const is flagged", () => { |
There was a problem hiding this comment.
[/tdd] The test name describes the fix trigger (multi-declarator const) but not the expected outcome. Renaming it to follow the Arrange/Act/Assert convention makes it self-documenting and easier to diagnose in CI output.
💡 Suggested rename
it("invalid: bare call to async entrypoint in multi-declarator const reports requireCatch", () => {The pattern [valid|invalid]: <condition> [reports|does not report] <messageId> matches the spec-style naming used throughout the rest of the file.
@copilot please address this.
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 55.5 AIC · ⌖ 6.03 AIC · ⊞ 4.9K
| type: "Variable"; | ||
| node: TSESTree.VariableDeclarator; | ||
| }; | ||
| type FunctionDeclarationDefinition = TSESLint.Scope.Definitions.FunctionNameDefinition & { node: TSESTree.FunctionDeclaration }; |
There was a problem hiding this comment.
Good call using TSESLint.Scope.Definitions.FunctionNameDefinition directly. One note: the intersection & { node: TSESTree.FunctionDeclaration } is still needed here since FunctionNameDefinition.node is typed more broadly (TSESTree.FunctionDeclaration | TSESTree.TSDeclareFunction in some versions). If the upstream type already narrows to FunctionDeclaration, this intersection becomes a no-op — either way, no harm. But it is worth confirming with the installed version that the constraint is redundant rather than load-bearing.
Test Quality Sentinel ReportTest Quality Score: 90/100 - Excellent Analyzed 1 test(s): 1 design, 0 implementation, 0 violation(s). Metrics:
Test: Verdict: PASSED. 0% implementation tests (threshold: 30%). The new test covers a previously-broken edge case (multi-declarator const statements) and validates both error detection and the auto-fix suggestion output. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
/souschef |
|
✅ PR Sous Chef finished PR sous-chef nudges. processed=5; skipped_checks_running=0; skipped_last_comment_from_sous_chef=0; skipped_cooldown=3; nudged=2; branch_update_attempts=0; formatter_pushes=0; merge_main_scheduled=0 |
There was a problem hiding this comment.
Non-blocking observations
The TypeScript narrowing fix is mechanically correct — replacing ad-hoc string literal intersection types with canonical TSESLint.Scope.Definitions.* types is the right approach, and switching definition.node.parent → definition.parent correctly resolves the never-collapse bug. The targeted test for the multi-declarator case is a good addition.
Findings (3 non-blocking)
-
Missing ESM test coverage (line 416, test file) — the new multi-declarator test only runs in CJS mode, leaving the
ExportNamedDeclarationbranch ofisModuleScopeVariableDeclarationunverified for this scenario. -
Removed declaration type guard (line 59, source) — the
declaration.type !== AST_NODE_TYPES.VariableDeclarationcheck was dropped. While the type alias now guarantees this statically, the downstreamisModuleScopeVariableDeclarationaccessesnode.parent.typewithout guarding against a null/undefined parent. A defensive guard there would add robustness. -
Redundant type intersection (line 7, source) —
FunctionNameDefinition & { node: TSESTree.FunctionDeclaration }duplicates a field already present in the upstream type. Either remove the intersection or add a comment explaining the intent.
🔎 Code quality review by PR Code Quality Reviewer · 159.2 AIC · ⌖ 8.38 AIC · ⊞ 5.4K
Comment /review to run again
| }); | ||
|
|
||
| it("invalid: bare call to async function in multi-declarator module-scope const is flagged", () => { | ||
| cjsRuleTester.run("require-async-entrypoint-catch", requireAsyncEntrypointCatchRule, { |
There was a problem hiding this comment.
Missing ESM regression coverage: the multi-declarator fix is only tested in CJS mode, leaving the ExportNamedDeclaration branch of isModuleScopeVariableDeclaration unexercised for this new case.
💡 Add an ESM test variant
isModuleScopeVariableDeclaration has two branches:
return node.parent.type === AST_NODE_TYPES.Program ||
(node.parent.type === AST_NODE_TYPES.ExportNamedDeclaration &&
node.parent.parent.type === AST_NODE_TYPES.Program);The new test only exercises the first (parent.type === Program) via CJS. A regression in the ExportNamedDeclaration path for multi-declarator export const helper = ..., main = async () => {} would not be caught. Add an ESM variant using esmRuleTester:
it("invalid: bare call in multi-declarator exported const is flagged (ESM)", () => {
esmRuleTester.run("require-async-entrypoint-catch", requireAsyncEntrypointCatchRule, {
valid: [],
invalid: [
{
code: `export const helper = () => 1, main = async () => { return 42; };\nmain();`,
errors: [{ messageId: "requireCatch", data: { name: "main" } }],
},
],
});
});| const declaration = definition.node.parent; | ||
| if (!declaration || declaration.type !== AST_NODE_TYPES.VariableDeclaration || !isModuleScopeVariableDeclaration(declaration)) return false; | ||
| const declaration = definition.parent; | ||
| if (!declaration || !isModuleScopeVariableDeclaration(declaration)) return false; |
There was a problem hiding this comment.
Silent implicit type change in definition.parent: switching from definition.node.parent to definition.parent relies on the upstream VariableDefinition.parent type always being a VariableDeclaration, but this invariant is not verified and the type is not checked at runtime.
💡 Why this matters
The old code had an explicit guard:
if (!declaration || declaration.type !== AST_NODE_TYPES.VariableDeclaration || ...)The new code drops the declaration.type !== AST_NODE_TYPES.VariableDeclaration check entirely:
const declaration = definition.parent;
if (!declaration || !isModuleScopeVariableDeclaration(declaration)) return false;TSESLint.Scope.Definitions.VariableDefinition.parent is typed as TSESTree.VariableDeclaration in the library, so TypeScript won't complain — but isModuleScopeVariableDeclaration calls node.parent.type without any null check on node.parent:
function isModuleScopeVariableDeclaration(node: TSESTree.VariableDeclaration): boolean {
return node.parent.type === AST_NODE_TYPES.Program || ...
}If node.parent is ever null or undefined (e.g., detached AST nodes in edge-case rule scenarios), this throws at runtime instead of returning false. The removed declaration.type check was redundant given the new type, but the node.parent null guard was providing defensive depth that's now removed one layer up. At minimum, isModuleScopeVariableDeclaration itself should guard against a missing parent.
| type: "Variable"; | ||
| node: TSESTree.VariableDeclarator; | ||
| }; | ||
| type FunctionDeclarationDefinition = TSESLint.Scope.Definitions.FunctionNameDefinition & { node: TSESTree.FunctionDeclaration }; |
There was a problem hiding this comment.
Redundant node intersection on FunctionDeclarationDefinition: FunctionNameDefinition already types node as TSESTree.FunctionDeclaration, so the intersection & { node: TSESTree.FunctionDeclaration } is a no-op and creates confusion about what value the intersection actually adds.
💡 Simplify the type alias
TSESLint.Scope.Definitions.FunctionNameDefinition already declares node: TSESTree.FunctionDeclaration. Intersecting it with { node: TSESTree.FunctionDeclaration } doesn't narrow anything further — it duplicates the same field. Simplify to:
type FunctionDeclarationDefinition = TSESLint.Scope.Definitions.FunctionNameDefinition;If the purpose of keeping the intersection is to be explicit or guard against future upstream type drift, add a comment explaining the intent. Otherwise the redundancy is misleading — a reader will wonder what additional narrowing is happening here.
|
|
||
| function isVariableDefinition(definition: TSESLint.Scope.Definition): definition is VariableDefinition { | ||
| return definition.type === "Variable" && definition.node.type === AST_NODE_TYPES.VariableDeclarator; | ||
| return definition.type === TSESLint.Scope.DefinitionType.Variable && definition.node.type === AST_NODE_TYPES.VariableDeclarator; |
There was a problem hiding this comment.
Dead runtime check in isVariableDefinition: after switching to TSESLint.Scope.Definitions.VariableDefinition, definition.node is already typed as TSESTree.VariableDeclarator, so definition.node.type === AST_NODE_TYPES.VariableDeclarator is always true when the first condition passes — this check is unreachable dead code.
💡 Remove or explain the redundant check
Before the type alias change, the explicit runtime check provided a meaningful runtime narrowing guard. Now that VariableDefinition.node is typed as TSESTree.VariableDeclarator by the upstream library, it cannot be anything else. Simplify to:
function isVariableDefinition(definition: TSESLint.Scope.Definition): definition is VariableDefinition {
return definition.type === TSESLint.Scope.DefinitionType.Variable;
}The type-guard cast to VariableDefinition will already ensure callers see the narrowed type correctly.
| }); | ||
| }); | ||
|
|
||
| it("invalid: bare call to async function in multi-declarator module-scope const is flagged", () => { |
There was a problem hiding this comment.
New test missing valid-case for non-async sibling declarator: the test only asserts main() is flagged, but doesn't verify that helper() — the non-async sibling in the same const statement — is not incorrectly flagged.
💡 Add a valid case to fully exercise the per-declarator discrimination
Without a valid entry for helper(), the test doesn't confirm that isAsyncVariableEntrypoint correctly ignores the sync sibling. Add:
valid: [
{
code: `const helper = () => 1, main = async () => { return 42; };
helper();`,
},
],This ensures the rule doesn't accidentally flag all declarators in a multi-declarator const rather than only the async ones.
|
@copilot run pr-finisher skill |
eslint-factorywas failing to build becauserequire-async-entrypoint-catchrelied on definition narrowing that now collapses toneverunder current TypeScript control-flow. This blockedlint:setup-jsbefore ESLint could run onactions/setup/js.Rule typing/narrowing cleanup
Definitionintersections with@typescript-eslintscope definition types:TSESLint.Scope.Definitions.FunctionNameDefinitionTSESLint.Scope.Definitions.VariableDefinitionTSESLint.Scope.DefinitionType.*, eliminatingneveraccess paths.AST declaration access fix
definition.node.parentto typeddefinition.parent.Targeted regression coverage
const, ensuring the corrected declaration path remains enforced.