⚠️ No Changeset found

Latest commit: f8ee699

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Binary Sizes

Commit: dd86413

Merging this PR will not alter performance

✅ 219 untouched benchmarks

_{Comparing kdy1/flow-hermes-stripping-parity (f8ee699) with main (918b6ac)¹}

PR Review: feat(parser): complete Hermes Flow stripping parity

Overall this is a solid, well-structured PR that adds substantial Flow syntax support (components, hooks, pattern matching, decorators) and backs it up with a thorough strip correctness test. A few observations:

Code Quality

Good:

• Clean separation of concerns: syntax flags in syntax.rs, parsing in stmt.rs/typescript.rs, test harness in flow_strip_hermes.rs.
• The known-fail-strip.txt + stale detection pattern is a nice way to track incremental progress.
• Consistent use of #[cfg(feature = "flow")] / #[cfg(not(feature = "flow"))] paired stubs keeps the non-flow build clean.
• The build_flow_syntax helper with fallback enablement by directory prefix is a pragmatic approach for the Hermes corpus.

Suggestions:

• flow_strip_hermes.rs and flow_hermes.rs both define HermesOptions, fixture_root(), corpus_root(), read_expected_errors(), and build_flow_syntax() with essentially identical logic. Consider extracting these into a shared test utility module to reduce duplication and prevent the two copies from drifting apart over time.

Potential Bugs / Issues

1. is_flow_match_keyword lookahead semantics (stmt.rs:1054-1060): The function calls self.input().next() to peek ahead, but next() on an Input typically advances the cursor. If this is actually advancing the token stream as a side effect of a check, then calling is_flow_match_keyword() in expr.rs:2634 before the Class / arrow checks could silently consume a token when the condition is false (i.e., when match is followed by ( but the spacing condition fails). Please verify that self.input().next() here is a non-consuming peek or that the token stream state is properly restored on the false path.

2. flow_match_make_temp_ident naming collisions (stmt.rs:1061-1063): The generated identifier __match_subject_{span.lo.0} is deterministic per source position, which is good for a single match expression. However, if the lowered output is later fed through another parser pass (e.g., minifier or bundler), identifiers starting with __ could collide with other synthetic names. This is low-risk but worth noting.

3. __flow_ leakage check (flow_strip_hermes.rs:202-205): The test asserts !output.code.contains("__flow_") but the match lowering generates __match_subject_ prefixed identifiers. If in the future any Flow-specific synthetic names are introduced with a different prefix, this check won't catch them. Consider also asserting !output.code.contains("__match_subject_") if the intent is that lowered output should be free of all synthetic names (or document that __match_subject_ is expected to survive stripping).

4. Error code reuse (stmt.rs, typescript.rs): Several Flow-specific error conditions emit SyntaxError::TS1003 or SyntaxError::TS1014. While this may be intentional (reusing existing error codes), it could be confusing for users seeing TypeScript error codes in Flow parsing errors. If Flow-specific error codes are planned for the future, a tracking comment would help.

Performance Considerations

• The flow_match_lower_pattern function clones value: Box<Expr> for each property/element in object/array patterns (e.g., value.clone() at multiple sites). For deeply nested patterns with many properties, this produces O(n) clones of the subject expression subtree per nesting level. For typical use this is fine, but for pathological cases (very large match patterns), it could be expensive. A future optimization could use a shared reference or temp variable per nesting level.

• The is_flow_match_keyword function is called on every expression and statement parse. The early flow_pattern_matching() flag check ensures it short-circuits quickly when the feature is disabled, which is good.

Test Coverage

• The flow_strip_hermes.rs test is thorough: it validates no __flow_ leakage, ES reparse success, known-fail management, and stale known-fail detection.
• The known-fail list is minimal (2 async_await cases), which is a good sign for coverage completeness.
• One gap: there don't appear to be dedicated unit/fixture tests for the new parsing logic (component/hook declarations, match expressions/statements) beyond the Hermes corpus. If any of these constructs have edge cases not covered by the Hermes fixtures, they won't be caught. Consider adding targeted fixture tests for:
  • Nested match expressions
  • Match with all pattern types combined
  • Component/hook with complex generic parameters
  • Error recovery paths (e.g., match with no cases, component with missing renders)

Minor Nits

• stmt.rs:902-903: In flow_match_parse_array_pattern, eating a comma and immediately continue-ing means consecutive commas silently produce no element (elision). This may be intentional for alignment with JS array destructuring, but it differs from Flow's match semantics where each arm should have an explicit pattern. Worth a comment if intentional.
• The duplicated decorator-handling blocks in module_item.rs (lines 348-357 and 397-407) are identical — consider extracting a small helper.

Security

No concerns — this is purely parsing/AST lowering with no I/O or external input beyond source files.

Verdict: This is high-quality work that significantly advances Flow support. The main actionable items are (1) verifying the is_flow_match_keyword lookahead doesn't consume tokens on the false path, (2) deduplicating the test helpers between the two test files, and (3) considering targeted fixture tests for the new syntax. The rest are minor suggestions for future improvement.