@stas-vilchik Thank you for submitting this PR!

🔔 @RReverser - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

For compatibility with existing code and better typecheck, I'd prefer this to be forked into separate interface which could only appear in export.

@RReverser I'm not sure what could be the best way to do that. If I created a new interface with nullable id:

export interface ExportDefaultFunctionDeclaration extends BaseFunction, BaseDeclaration {
  type: "FunctionDeclaration";
  id: Identifier | null;
  body: BlockStatement;
}

it would share the same type as the normal FunctionDeclaration. Later if I wanted to create an eslint rule using this typings, I would write something like:

const rule: Rule.RuleModule = {
  create(context: Rule.RuleContext) {
    return {
      FunctionDeclaration(node: Node) {
        (node as FunctionDeclaration).id; // nullable!
      },

In this case I'd need to remember (or even know) that I can't safely cast node to FunctionDeclaration, instead I should cast it to FunctionDeclaration | ExportDefaultFunctionDeclaration. I think this approach is quite error-prone.

Not sure what is RuleModule in your snippet, but if it's kind of a visitor, it should be redefined to accept the broader FunctionDeclaration variant with nullable id, while the non-nullable one would still be used for statement level.

The only way to do that would be to know what the type of the parent node is, and that is not part of the type of a node.

Even if you do add another interface, say, NonNullableFunctionDeclaration, the moment you | it with FunctionDeclaration inside the definition of Declaration, it'd make FunctionDeclaration widen to be the exact same type as NonNullableFunctionDeclaration.

By the way, while we're at it, I just noticed that ClassDeclaration also has non-nullable id in the definition but it also is supposed to be nullable if it's the export default, like export default class {}.

Possible idea to do something close to what you want: change Statement to use a new type, BlockDeclaration instead of Declaration, that would have only the non-nullable declarations. Or remove Declaration entirely from Statement and change BlockStatement to explicitly accept BlockDeclaration.

@RReverser my bad, I should have mentioned that it was an example of eslint rule. Its visitor relies on the node type and returns a generic Node. Therefore as a custom rule author, I'd need to remember that the FunctionDeclaration visitor doesn't simply return a estree.FunctionDeclaration, but something different.

@Kovensky thanks for the review comments and for the ClassDeclaration case. I'm not sure to understand your idea though.

Anyway I think it'd better if the estree.FunctionDeclaration represented a node with the type FunctionDeclaration with all possible variants.

I surely understand the complexities of updating this in a clean way, but want to find a way that would break as little code out there as possible.

@RReverser I understand, I just don't know how to achieve that.

A definition author has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

The TypeScript syntax tree suffers the same problem and it's a big source of bugs.

I think the fix as-is is honestly the right one - attempting to split out the tree differently doesn't really work because it's really a matrix (export, default, named, async, etc) rather than a directable graph.

@RReverser without a concrete better PR I think this one should be merged. Any objections?

Yeah, I think at this point trying to split out turns out to be too complicated to be worth it. We can always try to improve AST in future. @stas-vilchik sorry for the delay on this one!

🌟 🎈 🎉 🏆 🎂 ✨ ⭐️

Congratulations on your first DefinitelyTyped contribution!

🌟 🎈 🎉 🏆 🎂 ✨ ⭐️

Great, thanks!