Skip to content

Fix TypeScript never-narrowing in require-async-entrypoint-catch for setup/js lint path #43440

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
Jul 5, 2026
Merged

Fix TypeScript never-narrowing in require-async-entrypoint-catch for setup/js lint path #43440

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
25c8c01
Initial plan
Copilot Jul 4, 2026
3324abd
Fix require-async-entrypoint-catch TypeScript narrowing
Copilot Jul 4, 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
25 changes: 25 additions & 0 deletions eslint-factory/src/rules/require-async-entrypoint-catch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,4 +411,29 @@ main().then(() => process.exit(0)).catch(err => { console.error(err); process.ex
],
});
});

it("invalid: bare call to async function in multi-declarator module-scope const is flagged", () => {

@github-actions github-actions Bot Jul 5, 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 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.

@github-actions github-actions Bot Jul 5, 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.

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.

cjsRuleTester.run("require-async-entrypoint-catch", requireAsyncEntrypointCatchRule, {

@github-actions github-actions Bot Jul 5, 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.

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" } }],
      },
    ],
  });
});

valid: [],

@github-actions github-actions Bot Jul 5, 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 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.

invalid: [
{
code: `const helper = () => 1, main = async () => { return 42; };
main();`,
errors: [
{
messageId: "requireCatch",
data: { name: "main" },
suggestions: [
{
messageId: "addCatch",
output: `const helper = () => 1, main = async () => { return 42; };
main().catch(err => { console.error(err); process.exitCode = 1; });`,
},
],
},
],
},
],
});
});
});
18 changes: 6 additions & 12 deletions eslint-factory/src/rules/require-async-entrypoint-catch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,8 @@ const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh

type AsyncFuncNode = TSESTree.FunctionDeclaration | TSESTree.FunctionExpression | TSESTree.ArrowFunctionExpression;
type SourceCodeScope = TSESLint.Scope.Scope;
type FunctionDeclarationDefinition = TSESLint.Scope.Definition & {
type: "FunctionName";
node: TSESTree.FunctionDeclaration;
};
type VariableDefinition = TSESLint.Scope.Definition & {
type: "Variable";
node: TSESTree.VariableDeclarator;
};
type FunctionDeclarationDefinition = TSESLint.Scope.Definitions.FunctionNameDefinition & { node: TSESTree.FunctionDeclaration };

@github-actions github-actions Bot Jul 5, 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.

[/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.

@github-actions github-actions Bot Jul 5, 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.

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.

@github-actions github-actions Bot Jul 5, 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.

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.

type VariableDefinition = TSESLint.Scope.Definitions.VariableDefinition;

function isAsyncFuncNode(node: TSESTree.Node): node is AsyncFuncNode {
return node.type === AST_NODE_TYPES.FunctionDeclaration || node.type === AST_NODE_TYPES.FunctionExpression || node.type === AST_NODE_TYPES.ArrowFunctionExpression;
Expand Down Expand Up @@ -49,20 +43,20 @@ function getRootCallIdentifier(node: TSESTree.CallExpression): TSESTree.Identifi
}

function isFunctionDeclarationDefinition(definition: TSESLint.Scope.Definition): definition is FunctionDeclarationDefinition {
return definition.type === "FunctionName" && definition.node.type === AST_NODE_TYPES.FunctionDeclaration;
return definition.type === TSESLint.Scope.DefinitionType.FunctionName && definition.node.type === AST_NODE_TYPES.FunctionDeclaration;
}

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;

@github-actions github-actions Bot Jul 5, 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.

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.

}

function isModuleScopeVariableDeclaration(node: TSESTree.VariableDeclaration): boolean {
return node.parent.type === AST_NODE_TYPES.Program || (node.parent.type === AST_NODE_TYPES.ExportNamedDeclaration && node.parent.parent.type === AST_NODE_TYPES.Program);
}

function isAsyncVariableEntrypoint(definition: VariableDefinition): boolean {
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;

@github-actions github-actions Bot Jul 5, 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.

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.

const init = definition.node.init;
return (init?.type === AST_NODE_TYPES.FunctionExpression || init?.type === AST_NODE_TYPES.ArrowFunctionExpression) && init.async;
}
Expand Down
Loading