8000 AST standardisation: remove `undefined`/optional from node properties unless it really makes sense · Issue #5020 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
bradzacher opened this issue May 20, 2022 · 5 comments · Fixed by #6274
Closed

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

bradzacher opened this issue May 20, 2022 · 5 comments · Fixed by #6274
Labels
accepting prs Go ahead, send a pull request that resolves this issue AST PRs and Issues about the AST structure breaking change This change will require a new major version to be released enhancement New feature or request
Milestone
6.0.0

Comments

@bradzacher
Copy link
Member

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.
@bradzacher bradzacher added enhancement New feature or request breaking change This change will require a new major version to be released AST PRs and Issues about the AST structure labels May 20, 2022
@bradzacher bradzacher added this to the 6.0.0 milestone May 20, 2022
@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Jul 29, 2022
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Aug 9, 2022
Merged
3 tasks
This was referenced Dec 24, 2022
Closed
Merged
@JoshuaKGoldberg
Copy link
Member
JoshuaKGoldberg commented Dec 24, 2022 •
edited
Loading

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:

What do you think?

@bradzacher
Copy link
Member Author

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
Copy link
Member

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

@bradzacher
Copy link
Member Author

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.

@bradzacher bradzacher linked a pull request Feb 22, 2023 that will close this issue
feat(typescript-estree): remove optionality from AST boolean properties #6274
Merged
3 tasks
@JoshuaKGoldberg
Copy link
Member

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue AST PRs and Issues about the AST structure breaking change This change will require a new major version to be released enhancement New feature or request
Projects
None yet
Milestone
6.0.0
2 participants
@JoshuaKGoldberg @bradzacher
0