refactor(ast_tools/formatter): compile list of no following nodes as TypeIds

[overlookmotel]

Follow-on after #12864. Pure refactor - does not alter generated code.

There's a couple of things going on in this PR.

Firstly, perf:

• Prefer comparing TypeIds over string comparison, because it's cheaper.
• Compile a single Vec of all types which have no following node at the start (including those listed in AST_NODE_WITHOUT_FOLLOWING_NODE_LIST), rather than checking 2 Vecs each time in generate_struct_impls.

Secondly, the code previously was relying on the fact that almost all of Statements variants have the same name as the types those variants contain e.g.:

pub enum Statement<'a> {
    BlockStatement(Box<'a, BlockStatement<'a>>),
    // ...
}

But that the names match is a bit of a co-incidence. This PR makes it so we don't rely on that co-incidence, and instead makes the "no following node" list from the actual types of the variants.

This change reveals a side-effect of the previous behavior which may or may not be intentional. The only variants of Statement where the name of the variant and name of the type of the variant don't match are Function and Class.

To maintain the same output as before, I've added an exclude list AST_NODE_WITH_FOLLOWING_NODE_LIST containing these 2 types.

Presumably the reason why these 2 need an exclusion is because Function and Class can be either a Statement or an Expression. I don't know if this may be problematic and might need logic elsewhere to handle them differently, depending on the context?

Note: TypeDef::innermost_type is to get the TypeId of the inner type of a variant (e.g. BlockStatement), rather than the type which is the actual enum variant (e.g. Box<BlockStatement>).

@Dunqing I don't know the formatter at all, so these changes may be unhelpful. Feel free to close or modify this PR if so.

[overlookmotel]

• refactor(ast_tools/formatter): compile list of no following nodes as TypeIds #12930 👈 (View in Graphite)
• refactor(ast_tools/formatter): shorten code #12929
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[Dunqing]

Thanks for looking into this and improving it! I haven't started to polish this because those generator files are still constantly changing, so putting effort into it might be wasted at some point. Anyway, your changes are very good, which could let me make full use of existing APIs to achieve my desired functionality.

Presumably the reason why these 2 need an exclusion is because Function and Class can be either a Statement or an Expression. I don't know if this may be problematic and might need logic elsewhere to handle them differently, depending on the context?

Oh, this is a potential problem that I wasn't aware of before. Now, whether we have or do not have a following node depends on whether printing comments relies on. Currently, the class printing implementation isn't complete, so I am not sure whether Class or Function should be excluded or not. I will revisit this once the Class implementation is complete.

[Dunqing]

Merge activity

• Aug 9, 2:03 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Aug 9, 2:03 PM UTC: Dunqing added this pull request to the Graphite merge queue.
• Aug 9, 2:09 PM UTC: Merged by the Graphite merge queue.

[overlookmotel]

Oh, this is a potential problem that I wasn't aware of before. Now, whether we have or do not have a following node depends on whether printing comments relies on. Currently, the class printing implementation isn't complete, so I am not sure whether Class or Function should be excluded or not. I will revisit this once the Class implementation is complete.

In my opinion we should have separate AST types for FunctionExpression and FunctionDeclaration, like ESTree and Babel (ditto for classes). Maybe if a single Function type causes problems for formatter, it'll be the spur to finally make that change!

[Dunqing]

In my opinion we should have separate AST types for FunctionExpression and FunctionDeclaration, like ESTree and Babel (ditto for classes). Maybe if a single Function type causes problems for formatter, it'll be the spur to finally make that change!

Yes!! We've discussed this many times, please see #4240 (comment). This is what we really want to change, but it hasn't happened yet. Hope it is not too far.