JS: Hoist function declarations to the top of a block statement #18661
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This seems to have reordered the TRAP lines but without semantic change.
'buildFunctionBody' no longer needs to handle hoisting, because hoisting now happens when visiting the block statement that is the body of the function. Note that curly-brace functions contain a block statement as their body, not a list of statements.
The extra successor edge was due to visiting hoisted function declaration IDs multiple times, which has now been fixed.
There was a problem hiding this comment.
PR Overview
This PR introduces block-level function declaration hoisting in JavaScript code to align with non-standard but common engine behavior, while removing the prior function-level hoisting method. It also bumps the extractor version to track the extractor changes.
- Updated the JavaScript extractor version in Main.java
- Removed function-level hoisting logic for IFunction bodies and replaced it with block-level hoisting logic
- Added comments referencing non-standard engine-specific behavior
Changes
| File | Description |
|---|---|
| Main.java | Updated the extractor version from 2025-01-21 to 2025-02-03. |
| CFGExtractor.java | Removed function-level hoisting method and introduced block-level hoisting logic. |
Copilot reviewed 72 out of 72 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
javascript/extractor/src/com/semmle/js/extractor/CFGExtractor.java:589
- Removing the function-level hoisting method may be a breaking change if external code relies on it. Consider providing an alternative or ensuring it's no longer needed.
- public static List<Identifier> of(IFunction fn) { ... }
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
javascript/extractor/src/com/semmle/js/extractor/CFGExtractor.java
Outdated
Show resolved
Hide resolved
…java Co-authored-by: Copilot <[email protected]>
erik-krogh
approved these changes
Feb 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes the issue reported in #18652
We did not hoist function declarations inside block statements, as this is not standard behaviour, but it is non-standard behaviour implemented by most engines.
The specification discusses this in B.3.2 Block-Level Function Declarations Web Legacy Compatibility Semantics though I must admit I haven't read the compatibility semantics in detail.
This PR changes it to hoist function declarations to the top of a block statement in all cases.