AST standardisation: remove undefined/optional from node properties unless it really makes sense

[bradzacher]

Across the AST we have a lot of cases where we've made properties optional.
I think there were various reasons for why it was done this way (not that I can find the comments any more).

From the perspective of an AST consumer, undefined can create some very weird states. Two examples of this:

1. There are a number of boolean properties that are optional. On the surface this makes it look like the property with 3 meaningful states (true, false, or undefined), even though in reality it only ever actually represents 2 states.
   Usually in these cases the parser emits true or undefined (and never false), and in some rarer cases it emits true, and then uses false or undefined interchangeably depending on some internal, obfuscated criteria.
2. There are a few cases where an array property is optional. In these cases usually the parser will never emit an empty array, and will instead emit undefined.
   • From a memory-usage perspective, this could be justified (an empty array is really a heavy "empty" marker). But if that is our goal then these cases should be typed so that it is clear that there are never 0 elements (eg by defining [Foo, ...Foo[]])

We should standardise the AST to remove undefined/optional from all places, unless there is a very good reason for it to be optional.

[JoshuaKGoldberg]

For the ?: boolean properties, instead of going : boolean, we could alternately go ?: true... It's conceptually a bit trickier and requires more runtime-complex ...(conditional && { property: true }) kind of expressions in convert.ts. But has the benefit of smaller runtime AST size. Which feels preferable to me?

Sent two draft PRs for comparison:

• feat(typescript-estree): remove false from optional AST boolean properties #6273
• feat(typescript-estree): remove optionality from AST boolean properties #6274

What do you think?

[bradzacher]

In regards to making the AST smaller - does it hugely matter? Booleans are the smallest sized value in any language (usually at most 1 byte), so we're not talking a whole lot of space allocated (1 + n where n is the length of the key).

There's a whole thing about "hidden classes" with objects that JS runtimes do to reduce memory usage and improve property access when objects have a shared shape - so I think using optional properties could actually have the inverse effect because the shape changes - leading to multiple "hidden classes"? We wouldn't know one way or the other without in-depth testing.

Personally I'd prefer to have a property there always because consistency and then you can reasonably do duck-typing checks like if ('property' in node) { /* quack */ } (which TS understands and can narrow off of), which you can't do if the property is optional.

[JoshuaKGoldberg]

Makes sense, thanks! Agreed. I'll go with #6274 and close out #6273.

[bradzacher]

For posterity this TS change microsoft/TypeScript#51682 suggest I wasn't too far off the mark with regards to optionality in the objects. Having a standard shape should help V8 optimise things at the (allbeit small) memory impact of additional "useless" values.

[JoshuaKGoldberg]

#6274 was merged into the v6 branch. This will be available in the v6 release. 🚀