fix(semantic): correctly visit `IfStmt` `test` when building cfg by camc314 · Pull Request #9864 · oxc-project/oxc

camc314 · 2025-03-18T10:17:43Z

This PR fixes the construction of the cfg within oxc_semantic, by marking the test of IfStatement as unconditionally visited.

Given the following:

if (foo) {
  bar();
} else {
  baz();
}

Would produce the following graph:

graph TD
    0["bb0"]
    1["bb1 IfStatement"]
    2["bb2 Condition(IdentifierReference(foo))"]
    3["bb3 BlockStatement ExpressionStatement"]
    4["bb4 BlockStatement ExpressionStatement"]
    5["bb5"]

    1 -- "Error(Implicit)" --> 0
    2 -- "Error(Implicit)" --> 0
    3 -- "Error(Implicit)" --> 0
    4 -- "Error(Implicit)" --> 0
    5 -- "Error(Implicit)" --> 0
    1 -- "Normal" --> 2
    1 -- "Normal" --> 4
    3 -- "Normal" --> 5
    2 -- "Jump" --> 3
    4 -- "Normal" --> 5

After this change, it produces the following graph:

graph TD
    0["bb0"]
    1["bb1 IfStatement"]
    2["bb2 Condition(IdentifierReference(foo))"]
    3["bb3 BlockStatement ExpressionStatement"]
    4["bb4 BlockStatement ExpressionStatement"]
    5["bb5"]

    1 -- "Error(Implicit)" --> 0
    2 -- "Error(Implicit)" --> 0
    3 -- "Error(Implicit)" --> 0
    4 -- "Error(Implicit)" --> 0
    5 -- "Error(Implicit)" --> 0
    1 -- "Normal" --> 2
    3 -- "Normal" --> 5
    2 -- "Jump" --> 3
    2 -- "Jump" --> 4
    4 -- "Normal" --> 5

Rather than jumping from the if statment (bb1\nIfStatement) directly to the consequent (bb4 BlockStatement ExpressionStatement), it now unconditionally visits the test of the if statement. This can be seen in the after graph as the jump to bb4 BlockStatement ExpressionStatement" comes from bb2 Condition(IdentifierReference(foo)) rather than from bb1 IfStatement.

As a result of this change, rules_of_hooks, does not have false positives when a hook is in a position such as:

if (useMe) {

As the cfg would previously think that useMe was called conditionally when in fact it was not

fixes #9795
related #9807

camc314 · 2025-03-18T10:18:03Z

fix(semantic): correctly visit IfStmt test when building cfg #9864 👈 (View in Graphite)
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.

codspeed-hq · 2025-03-18T10:24:54Z

CodSpeed Performance Report

Merging #9864 will not alter performance

_{Comparing c/03-18-fix_semantic_correctly_visit_ifstmt_test_when_building_cfg (3d4c5f3) with main (544a090)}

Summary

✅ 33 untouched benchmarks

camc314 · 2025-03-18T11:18:26Z

cc @hulin32 hopefully that description makes sense

hulin32 · 2025-03-18T22:27:05Z

@camc314 thx a lot for your explanation.

This part I understood. Need to look into it to get more ideas.

Boshen · 2025-03-19T01:06:55Z

Merge activity

Mar 18, 9:06 PM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Mar 18, 9:06 PM EDT: A user added this pull request to the Graphite merge queue.
Mar 18, 9:14 PM EDT: A user merged this pull request with the Graphite merge queue.

This PR fixes the construction of the cfg within `oxc_semantic`, by marking the `test` of `IfStatement` as unconditionally visited. Given the following: ```ts if (foo) { bar(); } else { baz(); } ``` Would produce the following graph: ```mermaid graph TD 0["bb0"] 1["bb1 IfStatement"] 2["bb2 Condition(IdentifierReference(foo))"] 3["bb3 BlockStatement ExpressionStatement"] 4["bb4 BlockStatement ExpressionStatement"] 5["bb5"] 1 -- "Error(Implicit)" --> 0 2 -- "Error(Implicit)" --> 0 3 -- "Error(Implicit)" --> 0 4 -- "Error(Implicit)" --> 0 5 -- "Error(Implicit)" --> 0 1 -- "Normal" --> 2 1 -- "Normal" --> 4 3 -- "Normal" --> 5 2 -- "Jump" --> 3 4 -- "Normal" --> 5 ``` After this change, it produces the following graph: ```mermaid graph TD 0["bb0"] 1["bb1 IfStatement"] 2["bb2 Condition(IdentifierReference(foo))"] 3["bb3 BlockStatement ExpressionStatement"] 4["bb4 BlockStatement ExpressionStatement"] 5["bb5"] 1 -- "Error(Implicit)" --> 0 2 -- "Error(Implicit)" --> 0 3 -- "Error(Implicit)" --> 0 4 -- "Error(Implicit)" --> 0 5 -- "Error(Implicit)" --> 0 1 -- "Normal" --> 2 3 -- "Normal" --> 5 2 -- "Jump" --> 3 2 -- "Jump" --> 4 4 -- "Normal" --> 5 ``` Rather than jumping from the if statment (`bb1\nIfStatement`) directly to the consequent (`bb4 BlockStatement ExpressionStatement`), it now unconditionally visits the `test` of the if statement. This can be seen in the after graph as the jump to `bb4 BlockStatement ExpressionStatement"` comes **from** `bb2 Condition(IdentifierReference(foo))` rather than from `bb1 IfStatement`. As a result of this change, `rules_of_hooks`, does not have false positives when a hook is in a position such as: ```ts if (useMe) { ``` As the cfg would previously think that `useMe` was called conditionally when in fact it was not fixes #9795 related #9807

## [0.61.0] - 2025-03-20 - c631291 parser: [**BREAKING**] Parse `TSImportAttributes` as `ObjectExpression` (#9902) (Boshen) - eef7eb6 minifier: [**BREAKING**] Rename `CompressOptions::all_true`/`all_false` to `smallest`/`safest` (#9866) (sapphi-red) ### Features - 38ad787 data_structures: Add `assert_unchecked!` macro (#9885) (overlookmotel) - dcd356e minifier: Support `keep_names` option (#9867) (sapphi-red) - 6565fc4 napi: Feature gate allocator (#9921) (Boshen) - 2cedfe4 napi: Add codeframe to napi error (#9893) (Boshen) - a9a47a6 parser: Add regex cargo feature to oxc_parser (#9879) (Toshit) - d4a83ba parser: Report duplicate modifier `Accessibility modifier already seen.` (#9890) (Boshen) - 59c8f71 parser,codegen: Handle lone surrogate in string literal (#9918) (Boshen) ### Bug Fixes - 28a2ed3 estree/ast: Fix `IdentifierName` and `IdentifierReference` (#9863) (hi-ogawa) - 3d4c5f3 semantic: Correctly visit `IfStmt` `test` when building cfg (#9864) (camc314) - 1774225 transformer/using: Incorrect scope ids for bindings (#9871) (camc314)- 68018e1 Ast changes (Boshen) ### Performance - 5f97f28 ast/estree: Speed up raw deser for `JSXElement` (#9895) (overlookmotel) - b272893 mangler, minifier: Initialize a Vec with a specific value using `Vec::from_iter_in` combined with `repeat_with` (#9908) (Dunqing) - f7d078c semantic: Use `reserve_exact` instead of `reserve` to save memory in pre-reserve (#9910) (Dunqing) ### Documentation - 590a258 napi/parser: Add stackblitz link for wasm build (Boshen) ### Refactor - 62e2859 ast/ast_builder: Use `self.vec_from_iter` instead of `Vec::from_iter_in` for consistency (#9909) (Dunqing) - b2f3d23 isolated_declarations: Remove unused `self` params (#9868) (overlookmotel) - 961b95d napi: Move common code to `oxc_napi` (#9875) (Boshen) - 233c1fc napi/playground: Add JSON.parse wrapper (#9880) (Hiroshi Ogawa) - dbe61c5 transformer/module-runner-transform: Remove redundant converison (#9912) (Dunqing) - ecdfe2e transformer/using: Move work to `exit_static_block` (#9713) (camc314) ### Testing - 9314147 data_structures: Enable doc tests for `oxc_data_structures` crate (#9884) (overlookmotel) - 040e993 napi: Refactor NAPI parser benchmarks (#9911) (overlookmotel) - e637e2e napi/parser: Tweak vitest config (#9878) (Hiroshi Ogawa) Co-authored-by: Boshen <[email protected]>

github-actions bot added A-linter Area - Linter A-semantic Area - Semantic C-bug Category - Bug labels Mar 18, 2025

camc314 marked this pull request as ready for review March 18, 2025 10:18

camc314 requested review from Boshen and Dunqing as code owners March 18, 2025 10:18

camc314 marked this pull request as draft March 18, 2025 10:18

camc314 force-pushed the c/03-18-fix_semantic_correctly_visit_ifstmt_test_when_building_cfg branch from 618998a to 45c87a6 Compare March 18, 2025 11:18

camc314 marked this pull request as ready for review March 18, 2025 11:18

camc314 force-pushed the c/03-18-fix_semantic_correctly_visit_ifstmt_test_when_building_cfg branch from 45c87a6 to ee6bb75 Compare March 18, 2025 11:30

Boshen added the 0-merge Merge with Graphite Merge Queue label Mar 19, 2025

graphite-app bot force-pushed the c/03-18-fix_semantic_correctly_visit_ifstmt_test_when_building_cfg branch from ee6bb75 to 3d4c5f3 Compare March 19, 2025 01:07

graphite-app bot merged commit 3d4c5f3 into main Mar 19, 2025
24 checks passed

graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Mar 19, 2025

graphite-app bot deleted the c/03-18-fix_semantic_correctly_visit_ifstmt_test_when_building_cfg branch March 19, 2025 01:14

This was referenced Mar 20, 2025

release(oxlint): v0.16.1 #9903

Merged

release(crates): v0.61.0 #9923

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

fix(semantic): correctly visit `IfStmt` `test` when building cfg#9864

fix(semantic): correctly visit `IfStmt` `test` when building cfg#9864
graphite-app[bot] merged 1 commit intomainfrom
c/03-18-fix_semantic_correctly_visit_ifstmt_test_when_building_cfg

camc314 commented Mar 18, 2025 •

edited

Loading

Uh oh!

camc314 commented Mar 18, 2025

Uh oh!

codspeed-hq bot commented Mar 18, 2025 •

edited

Loading

Uh oh!

camc314 commented Mar 18, 2025

Uh oh!

hulin32 commented Mar 18, 2025

Uh oh!

Boshen commented Mar 19, 2025 •

edited by graphite-app bot

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Comments

Conversation

camc314 commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camc314 commented Mar 18, 2025

How to use the Graphite Merge Queue

Uh oh!

codspeed-hq bot commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #9864 will not alter performance

Summary

Uh oh!

camc314 commented Mar 18, 2025

Uh oh!

hulin32 commented Mar 18, 2025

Uh oh!

Boshen commented Mar 19, 2025 • edited by graphite-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

camc314 commented Mar 18, 2025 •

edited

Loading

codspeed-hq bot commented Mar 18, 2025 •

edited

Loading

Boshen commented Mar 19, 2025 •

edited by graphite-app bot

Loading