Strange spans for exported classes with decorators

[overlookmotel]

@dec export class A {}

This produces strange spans:

• ExportNamedDeclaration: start 5, end 22
• Class: start 0, end 22
• Decorator: start 0, end 4

Class is a child of ExportNamedDeclaration but the child has a span which starts before the parent. This seems odd.

I assume we should consider this to be incorrect?

Playground

(ignore that Program start span isn't 0 in Playground. That's an artefact of ESTree conversion - it's 0 in Rust AST, as it should be).

[overlookmotel]

@leaysgur FYI, looks like this is related to a bunch of TS-ESTree test fails. But TS-ESTree actually does something different again:

• ExportNamedDeclaration: start 5, end 22
• ClassDeclaration: start 12, end 22
• Decorator: start 0, end 4

So in TS-ESLint, Decorator has a span outside of its parent, and it's grandparent.

AST explorer

I'd say that's a bug in TS-ESTree too! Probably best we decide what we think is correct first and then raise a bug report with them.

[leaysgur]

I've found that TS-ESLint seems to be aware of this: typescript-eslint/typescript-eslint#8769 and refs estree/estree#315 .

[overlookmotel]

Ah ha. Thanks for digging up that discussion. OK, it seems everyone has a different view on what's "right".

But from estree/estree#315 (comment), it sounds like Babel won the argument and TS-ESLint intends to align with Babel's behavior in future.

Babel gives:

• ExportNamedDeclaration: start 0, end 22
• ClassDeclaration: start 0, end 22
• Decorator: start 0, end 4

With that approach, no child exceeds the span of its parent.

AST Explorer - Babel

To clarify the various approaches people are taking:

// Oxc
@dec export class A {}
     ^^^^^^^^^^^^^^^^^ ExportNamedDeclaration
^^^^^^^^^^^^^^^^^^^^^^ Class
^^^^                   Decorator

// TS-ESLint
@dec export class A {}
     ^^^^^^^^^^^^^^^^^ ExportNamedDeclaration
            ^^^^^^^^^^ Class
^^^^                   Decorator

// Babel
@dec export class A {}
^^^^^^^^^^^^^^^^^^^^^^ ExportNamedDeclaration
^^^^^^^^^^^^^^^^^^^^^^ Class
^^^^                   Decorator

Personally, I like Babel's approach. I thought it was a hard rule that a child node cannot have a span which exceeds its parent's span. I don't think there are any other places in AST where that rule is broken, and it feels weird to break it. It may also be a source of bugs.

The only way to maintain the rule is to do what Babel does - extend the span of both Class and ExportNamedDeclaration back to cover decorators. Unfortunately this makes the span of Class include the export keyword, but we already do that, and in my view the alternative is worse.

@Boshen Could you please make a judgement on this?

[Boshen]

Both are reasonable. Let's align ts-eslint. I'll fix the parser.

The parser had to adjust for export @dec class A {} or x = @dec class A {}

[overlookmotel]

#10438 makes spans in our AST follow TS-ESLint:

@dec export class C {}
     ^^^^^^^^^^^^^^^^^ ExportNamedDeclaration
            ^^^^^^^^^^ Class
^^^^                   Decorator

Also:

@dec export default class {}
     ^^^^^^^^^^^^^^^^^^^^^^^ ExportDefaultDeclaration
                    ^^^^^^^^ Class
^^^^                         Decorator