Remove span field from BindingPattern

[overlookmotel]

#3855 added a span field to BindingPattern. There's good reason for doing this - simplifying codegen-ing GetSpan impls. But it's not ideal as it's duplicate information (BindingPatternKind already has a span) and requires keeping the 2 in sync if you change BindingPatternKind in a transform.

Once GetSpan codegen is merged, see if can find another way, and remove this field again.

[overlookmotel]

@rzvxa Rather than hard-coding logic into codegen for this, could we add a #[span] attr to the kind field which codegen would recognise and implement GetSpan::span for BindingPattern accordingly?

i.e. remove the span field from BindingPattern and instead do:

pub struct BindingPattern<'a> {
    #[span] // Get `span` from `kind`
    pub kind: BindingPatternKind<'a>,
    pub type_annotation: Option<Box<'a, TSTypeAnnotation<'a>>>,
    pub optional: bool,
}

I think it's ideal if the codegen contains as little "special case" logic as possible - better in my view if the AST types themselves are the single source of truth, and the codegen just acts on this information.

#3882 (if it's merged) adds ability to introduce extra attributes on types for passing information to the codegen - just add span to the helper attrs of visited_node_derive.

[rzvxa]

So what would it look like? something like this:

#[visited_node]
#[derive(Debug, Hash)]
#[cfg_attr(feature = "serialize", derive(Serialize, Tsify))]
#[cfg_attr(feature = "serialize", serde(rename_all = "camelCase"))]
pub struct BindingPattern<'a> {
    // serde(flatten) the attributes because estree has no `BindingPattern`
    #[cfg_attr(feature = "serialize", serde(flatten))]
    #[cfg_attr(
        feature = "serialize",
        tsify(type = "(BindingIdentifier | ObjectPattern | ArrayPattern | AssignmentPattern)")
    )]
    #[span]
    pub kind: BindingPatternKind<'a>,
    pub type_annotation: Option<Box<'a, TSTypeAnnotation<'a>>>,
    pub optional: bool,
}

[overlookmotel]

So what would it look like? something like this:

Yes, exactly like that.

[overlookmotel]

Orthogonal to this, but we can also add serde and tsify to helper attrs for visited_node_derive and remove a bunch of #[cfg_attr(feature = "serialize", ...)] boilerplate:

#[visited_node]
#[derive(Debug, Hash)]
#[cfg_attr(feature = "serialize", derive(Serialize, Tsify))]
#[serde(rename_all = "camelCase")]
pub struct BindingPattern<'a> {
    // serde(flatten) the attributes because estree has no `BindingPattern`
    #[serde(flatten)]
    #[tsify(type = "(BindingIdentifier | ObjectPattern | ArrayPattern | AssignmentPattern)")]
    #[span]
    pub kind: BindingPatternKind<'a>,
    pub type_annotation: Option<Box<'a, TSTypeAnnotation<'a>>>,
    pub optional: bool,
}

[rzvxa]

Then I think it is time we rename the visited_node attribute to something more general like ast_node, and use it for all of our markings, Instead of compiling a bunch of small noop macros, we just build one and mark everything using that.

#[ast_node(
    visitable,
    scope(ScopeFlags::Top),
    strict_if(self.source_type.is_strict() || self.directives.iter().any(Directive::is_use_strict))
)]
#[derive(Debug)]
#[cfg_attr(feature = "serialize", derive(Serialize, Tsify))]
#[cfg_attr(feature = "serialize", serde(tag = "type", rename_all = "camelCase"))]
pub struct Program<'a> {
    #[cfg_attr(feature = "serialize", serde(flatten))]
    pub span: Span,
    pub source_type: SourceType,
    pub directives: Vec<'a, Directive<'a>>,
    pub hashbang: Option<Hashbang<'a>>,
    pub body: Vec<'a, Statement<'a>>,
    pub scope_id: Cell<Option<ScopeId>>,
}

I mean if we want to have some nodes that are visitable and some that aren't, This is the way to go.
Without this, we can't have something like a span hint for JSXClosingFragment which isn't visited_node at the moment.

[overlookmotel]

Yes, I tend to agree with you.

As as style nit, I'd prefer if we bust out scope-related stuff in #[ast_node] to it's own attr:

// Before
#[ast_node(
    visitable,
    scope(ScopeFlags::Top),
    strict_if(self.source_type.is_strict() || self.directives.iter().any(Directive::is_use_strict))
)]

// After
#[ast_node(visit)]
#[scope(
    flags(ScopeFlags::Top)),
    strict_if(self.source_type.is_strict() || self.directives.iter().any(Directive::is_use_strict)),
)]

NB: scope here doesn't need to be an attr macro. It can just be added as a helper attribute to the visited_node_derive macro (which will become ast_node_derive).

I have avoided using the name ast_node for attr because it is reminiscent of SWC!

[overlookmotel]

Closed by #3899.