fix: graceful fallback in cvt_smloc for invalid SMLoc

[YevheniiKotyrlo]

Important

Updated diagnosis (2026-05-08) — the rationale below ("Hermes can hand us an invalid SMLoc for synthesized nodes") is wrong. Hermes does not produce null SMLocs in this path; the original assert!(loc.is_valid(), …) at convert.rs:120 is correct. The apparent null SMLocs were a side effect of an MSVC empty-base-optimization layout bug in the bundled llvh simple_ilist. The real structural fix is at #18 (build-time patch, open) and facebook/hermes#2012 — MERGED 2026-05-11 at facebook/hermes@768bab2a (upstream architectural fix). This PR's graceful fallback is at most defense in depth — no longer required for correctness now that the EBO mismatch is fixed at the source and #18 ships the same diff as a build-time patch. Disposition is the maintainer's call: merge as defense, or close superseded by #18 (and #2012 upstream). Full corrected analysis in this comment. Original PR body preserved below.

---

Summary

cvt_smloc aborts the process via assert!(loc.is_valid(), …) whenever Hermes hands fft an SMLoc with a null pointer. This panics on Windows for at least one synthesized node when bundling a React Native source tree (fast-flow-transform-win32-x64-msvc@0.0.3, via vxrn's Vite plugin adapter inside onejs/one's native bundler). Reproducible across machines on Windows; not reproducible on Linux Docker.

Filed and reproduced in detail at #16.

Fix

Replace the assert! with an early-return that uses the established SourceLoc::invalid() sentinel:

     pub fn cvt_smloc(&mut self, loc: SMLoc) -> ast::SourceLoc {
-        assert!(
-            loc.is_valid(),
-            "All source locations from Hermes parser must be valid"
-        );
+        // Hermes can hand us an invalid (null-pointer) SMLoc for synthesized
+        // nodes (error recovery, EOF tokens, some Flow constructs). Mirror
+        // generated_cvt's existing fallback for `range.end` rather than
+        // aborting the process.
+        if !loc.is_valid() {
+            return ast::SourceLoc::invalid();
+        }

Why this is structural rather than a band-aid:

• SourceLoc::invalid() ({ line: 0, col: 0 }, defined in crates/fft_support/src/source_manager.rs) is already the established sentinel in this codebase. It's used as the initial range.end value in generated_cvt.rs before it's filled in, as the fallback in parse_with_flags when there's no first-error coordinate, and at crates/fft_ast/src/node_child.rs:64-65. This patch just lets cvt_smloc return the same sentinel for null-pointer input.
• It removes an asymmetry that already existed inside generated_cvt.rs: range.start is passed to cvt_smloc unconditionally, while range.end is guarded by is_empty(). With this patch, both ends uniformly fall back to invalid() when source location is missing.
• No behavior change for valid input — the early-return only affects the previously-panicking path. The cache path, find_line path, and make_source_loc path all stay byte-identical.
• The downstream consumers (gen_js, sourcemap emission, error reporting) already accept invalid() as a no-position marker. Worst case for an affected node: the line/col in error messages becomes 0:0 rather than aborting the bundler — far better than the current behavior.

Why now (not earlier)

The same assert! exists upstream in Juno (hermes/unsupported/juno/crates/juno/src/hparser/convert.rs) — fft inherited it on the 2026-03-05 rename commit. For Juno (a CLI tool with controlled input) the assert is reasonable. For fft (a library invoked by bundler adapters with arbitrary user input), the same assert is a denial-of-service surface. Worth softening here even if Juno keeps it stricter.

Verification

• Confirmed hparser::tests::test1 (parse("function foo(p1) { var x = (10 + p1); }")) hits the existing panic on Windows on upstream/main — see Rust panic in cvt_smloc on Windows: "All source locations from Hermes parser must be valid" #16 for repro detail.
• After this patch, the lib test no longer panics in cvt_smloc. (It then hits a separate Windows-only access violation downstream — STATUS_ACCESS_VIOLATION 0xc0000005 — which the panic was previously short-circuiting. Different bug; outside this PR's scope.)
• Rebuilt the napi binding on Windows 11 (Rust 1.95 + VS 2022 BuildTools + CMake 4.3.2 + the facebook/hermes submodule) and dropped it into the original consuming project (OneJS Vite-native bundler with vite.config.ts: native.bundler: 'vite'). The cvt_smloc panic is gone from the bundling pipeline. First request emits a complete ~609 KB UMD bundle.

Test plan

• Windows host repro: cvt_smloc panic gone (verified via lib test + napi runtime path)
• cargo fmt --all --check clean (workspace)
• cargo test --workspace — clean on Linux CI (no null SMLoc → no early-return code path → behavior byte-identical for valid input). On Windows, the lib test hits a pre-existing access violation that this PR does not address (the same Windows-only downstream bug noted above, present on upstream/main once you remove the cvt_smloc panic).
• pnpm check / pnpm test / pnpm e2e — Linux CI runs these; structural soundness on the patch is high (single-function early-return, no API change, no new code paths for valid SMLoc).

Notes

This PR is intentionally minimal — single-function, ~5 lines. Larger questions (why Hermes produces null SMLoc only on Windows; whether the C++ source manager has a path-encoding bug) need a debug-build binding and a native debugger, and feel like they belong to a separate investigation rather than blocking this fix.

Closes #16

[YevheniiKotyrlo]

Update on the diagnosis: while investigating the downstream STATUS_ACCESS_VIOLATION that this PR's note marks as out-of-scope, I found that the original assert!(loc.is_valid(), ...) at crates/fft/src/hparser/convert.rs:120 is actually correct — Hermes does not produce null SMLocs in this code path. The apparent null SMLoc cases this PR softens are a side effect of an MSVC layout bug in the bundled llvh simple_ilist: missing __declspec(empty_bases) shifts the embedded Sentinel member from offset 0 to offset 8, which makes the Rust FFI iterator in crates/hermes/src/parser/node.rs walk past the end of NodeLists and dereference the sentinel as if it were a real Node. The 8-byte offset on dereference is what cvt_smloc then reads as a "null" loc.as_ptr() — it isn't really null, the iterator is just one node past where it should have stopped.

Repro and full diagnosis with stack-level traces is in the new PRs:

• Upstream fix at facebook/hermes: facebook/hermes#2012 (single line: __declspec(empty_bases) on simple_ilist).
• Stop-gap for fft until the Hermes pin can be bumped: jbroma/fast-flow-transform#18 (idempotent build-time patch on external/llvh/include/llvh/ADT/simple_ilist.h).

With #18 applied, both the original repro from #16 (hparser::tests::test1 panicking on Windows) and the downstream STATUS_ACCESS_VIOLATION go away; the OneJS Vite-native bundler that motivated #16 generates a full ~5 MB React Native bundle in ~1.2 s where it previously segfaulted on the first request.

I'll leave the disposition of this PR to your judgement — happy with either path:

1. Close fix: graceful fallback in cvt_smloc for invalid SMLoc #17 in favor of fix: enable EBO for vendored llvh simple_ilist on MSVC via build-time LLVM_DECLARE_EMPTY_BASES patch #18 — the original assert is structurally correct and the cvt_smloc graceful fallback is no longer needed once fix: enable EBO for vendored llvh simple_ilist on MSVC via build-time LLVM_DECLARE_EMPTY_BASES patch #18 lands.
2. Keep fix: graceful fallback in cvt_smloc for invalid SMLoc #17 as a defensive layer alongside fix: enable EBO for vendored llvh simple_ilist on MSVC via build-time LLVM_DECLARE_EMPTY_BASES patch #18 — the early return is harmless on valid SMLoc input and could in principle defend against a future Hermes change that does start producing null SMLocs.

Either way, #18 is the structural root-cause fix and the one that actually unblocks Windows users.

[YevheniiKotyrlo]

FYI — the upstream architectural fix landed: facebook/hermes#2012 was merged on 2026-05-11 at facebook/hermes@768bab2a. The MSVC EBO layout bug that was the actual root cause of this PR's symptom is fixed at the source.

This further reduces the defensive value of this PR — the original assert!(loc.is_valid(), ...) at crates/fft/src/hparser/convert.rs:120 is correct because Hermes does not produce null SMLocs on this code path; the apparent nulls were layout-corrupted reads from the EBO mismatch, which is now fixed.

Disposition still your call:

• Close as superseded by #18 (build-time stop-gap) and facebook/hermes#2012 (upstream fix).
• Merge alongside #18 as belt-and-suspenders — defensive layer for any future similar regression.
• Leave open until #18 is reviewed.

No rush from my end.