Skip to content

Rename transform-relevant subtree facts with what they actually should track #1277

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.

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

Merged
merged 1 commit into from
Jun 24, 2025

Conversation

weswigham
Copy link
Member

Following up my comments in #1270, this PR adjusts the terminology used in the subtree facts enum to make plain it's about optimizing transformations. The old names are left in as aliases for reference when porting code, but you should really use (one of) what they alias. This renaming already called out some nodes which set ContainsESXXXX flags but didn't have corresponding transform logic, causing us to needlessly perform traversals on ASTs containing those node kinds (namely, bigints and import.meta were big offenders that triggered a no-op es2020 transform, and an async modifier would guarantee a function hits both the es2018 and es2017 transforms, even though the containing function set the more specific individual transform flag already).

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR renames and refines the subtree facts used in the transformation logic to more accurately reflect the AST transformations being performed. Key changes include:

  • Renaming of old ES* flags (e.g., SubtreeContainsESNext, SubtreeContainsES2019) to new, more descriptive flags (e.g., SubtreeContainsUsing, SubtreeContainsMissingCatchClauseVariable).
  • Updating transformation logic in various transformers and AST node computations to use the updated flags.
  • Adjustments to ensure that nodes like bigint and import.meta are treated correctly with respect to transformation downleveling.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
internal/transformers/estransforms/using.go Updated the flag check from SubtreeContainsESNext to SubtreeContainsUsing.
internal/transformers/estransforms/optionalcatch.go Updated the flag check from SubtreeContainsES2019 to SubtreeContainsMissingCatchClauseVariable.
internal/ast/subtreefacts.go Refactored enum values and introduced alias definitions to reflect new flag groupings.
internal/ast/ast.go Modified AST node methods to propagate the new flag values and behavior across various node types.
Comments suppressed due to low confidence (2)

internal/ast/ast.go:2931

  • Ensure that test cases are updated to cover the scenario when a catch clause has a missing variable declaration, as the transformation now uses SubtreeContainsMissingCatchClauseVariable instead of the previous alias.
		res |= SubtreeContainsMissingCatchClauseVariable

internal/ast/ast.go:5525

  • Verify that the removal of any downlevel flag for BigIntLiteral is intentional and that downstream transformation logic correctly handles bigint literals as they are now explicitly excluded from transformation.
	return SubtreeFactsNone // `bigint` is not downleveled in any way

SubtreeContainsES2016
SubtreeContainsESDecorators
SubtreeContainsUsing
SubtreeContainsClassStaticBlocks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this list exhaustive all the way down to ES6?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so. I double checked what our transforms cover against the list of accepted proposals from each year. The only thing I'm still a bit undecided on split-wise is if I really want to split class static blocks out from class fields - they're combined into one transform (along with the legacy class fields transform) in strada. Some decorator things compile to class static blocks, though, so it has the potential to save some node traversal on classes-which-use-decorators-but-not-fields by being split out. They're also separate tc39 proposals, even though they got accepted in the same year... eh. I'll finalize that one way or another when I put up the PR for it - this split is fine for now. Class fields and async transforms are probably going to be the last two I post, given their size - honestly I forgot core async wasn't es6 and so was still in the wheelhouse of needing transformation logic when I started working on these.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just surprised that it's such a short list.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, some end up in the module transforms (like import attributes) and some just don't get downlevel support due to complexity (like bigint), but most new es features end up just being lib updates and not new syntax. 🤷‍♂️

@weswigham weswigham added this pull request to the merge queue Jun 24, 2025
Merged via the queue into microsoft:main with commit 5795354 Jun 24, 2025
22 checks passed
@weswigham weswigham deleted the subtree-fact-rename-split branch June 24, 2025 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants