Is there a preferred way to tackle this? I think this is currently an unreachable line.

#10388 - either it's reachable or it's not.

If it's not actually reachable then I'd say use type assertion(s) to tell TypeScript why it's wrong.

There might be opportunities to suggest improvements to AST types too. Example: thegetMembers(node.parent.parent) later on has the provided type asTSESTree.Node. But maybe somewhere in there a type could be made more specific? IsTSESTree.ExportDefaultDeclaration's.parent always aTSESTree.BlockStatement | TSESTree.Program, rather than the catch-allTSESTree.Node?

Interesting! I didn't consider proposing changes/improvements to the AST types. I've found#9560, and I think it would be a good option if I could be sure the changes are correct.

To my understanding, TypeScript's AST doesn't even have an actual AST node forExportDefaultDeclaration orExportNamedDeclaration as they're expressed asmodifiers.

I've looked into@babel/parser,@swc/types, and@oxc-project/types (also the more technical1,2), but I didn't find anything that makes me confident this is valid in every case. I did verify@typescript-eslint itself is type-checked successfully with these assumptions.

The stricter parent types for the AST I'd like to propose:

• ExportDefaultDeclaration andExportNamedDeclaration should have the stricterparent type ofTSESTree.BlockStatement | TSESTree.Program | TSESTree. TSModuleBlock (I'm not sure ifTSESTree.BlockStatement is an option).

• FunctionDeclaration should have the stricterparent type ofTSESTree.BlockStatement | TSESTree.ExportDefaultDeclaration | TSESTree.ExportNamedDeclaration | TSESTree.Program.

  TypeScript itself doesn't have a specificparent forFunctionDeclaration (it falls back to the genericts.Node), although they do have stricter parent types for some other nodes.

How did we verify the correctness of the proposals in#6225? Also, would this type of change be considered a breaking change? I noticed that#9560 was released in v8, but it wasn't marked as abreaking change.

I'm a bit hesitant to change the AST's types, as it can potentially lead to bugs, and I would love to hear opinions on this.

I updated the PR with these changes, so it's easier to discuss them.

The stricter parent types for the AST I'd like to propose:

I'd say those proposals should be a separate issue. I'm generally in favor of stricter parent types in general but we should discuss.

So I think how to handle that in this PR is up to you. Maybe either:

• Link to the issue with anas
• Draft this PR and mark it blocked till that's resolved

How did we verify the correctness of the proposals in#6225?

Other thanyarn build andyarn typecheck, pretty much just manual knowledge and testing.

breaking change?

I don't think improving the types of the AST to better reflect reality is breaking. In that sense it's a "feature".

Changing theruntime value would IMO be a breaking change.

That sounds good; I'm not sure why I mixed both on the same PR; thanks! 👍

I've opened#10682, drafted this PR, and marked it as blocked.

If the issue is accepted, I'll create a new PR with the stricter parent node changes and eventually re-open this PR for review with only the relevant changes.

Thiscurrently only gets called for nodes whose parents aren'tExportDefaultDeclaration, and so this can never benull. If this code gets refactored, it may change, and this can throw.

I can change this to a chain-expression, though this will show up as a partially covered line by codecov.

Similar to the other comment, my fallback would be to use a! with a disable comment explaining why the types are wrong. If possible, fixing up the types to be correct would be even better.

You might find the existing uses of theMakeRequired utils type interesting. Some places in code use for node types in similar situations. E.g.no-confusing-void-expression has handling for return statements that must have anargument. Maybe there's a similar flow here for functions that must have anid?

I'm not worried about refactors. Our test coverage is so high that the cost of unnecessary safety precautions in code is IMO higher than the cost of the brittleness.

I'm confident the only possibility ofnode.id beingnull (forFunctionDeclaration orTSDeclareFunction) is in cases where the parent node is anExportDefaultDeclaration (also according to JavaScript's spec):

I think that updating the types to be stricter here would be more challenging. I've resorted to using a! with a disable comment and explaining why this is necessary.

#10388 - either it's reachable or it's not.

If it's not actually reachable then I'd say use type assertion(s) to tell TypeScript why it's wrong.

There might be opportunities to suggest improvements to AST types too. Example: thegetMembers(node.parent.parent) later on has the provided type asTSESTree.Node. But maybe somewhere in there a type could be made more specific? IsTSESTree.ExportDefaultDeclaration's.parent always aTSESTree.BlockStatement | TSESTree.Program, rather than the catch-allTSESTree.Node?

Similar to the other comment, my fallback would be to use a! with a disable comment explaining why the types are wrong. If possible, fixing up the types to be correct would be even better.

You might find the existing uses of theMakeRequired utils type interesting. Some places in code use for node types in similar situations. E.g.no-confusing-void-expression has handling for return statements that must have anargument. Maybe there's a similar flow here for functions that must have anid?

I'm not worried about refactors. Our test coverage is so high that the cost of unnecessary safety precautions in code is IMO higher than the cost of the brittleness.

This test is unrelated to the PR, but it adds coverage to part of the code that is currently uncovered (it seemed small enough to be part of the PR, even if it's unrelated).

Please let me know if I should revert this part.

I think this may be useful as a shared utility function for different rules. Re-using this function (pretty much as is) simplifies tackling#4586 (which I'll be happy to push a PR for), for example.

Adding that this attempts to emulatechecker.isImplementationOfOverload() for rules which aren't type-aware.