fix(es/react-compiler): Recover spans from loc for compiler-generated nodes - #11979
fix(es/react-compiler): Recover spans from loc for compiler-generated nodes#11979残月 (Chastrlove) wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: d6e90d0 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd6383f4cb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| return DUMMY_SP; | ||
| }; | ||
| match (loc.start.index, loc.end.index) { | ||
| (Some(start), Some(end)) => Span::new(BytePos(start + 1), BytePos(end + 1)), |
There was a problem hiding this comment.
Recovering loc for every loc-only node also gives compiler-generated identifiers real span.lo values. After conversion, transform still calls apply_renames on the entire program, and that pass is keyed only by span.lo even though compiled functions already contain the compiler's renamed identifiers. When a component has a BindingRenameInfo and a generated temp/cache identifier inherits the loc of an original reference, this new span makes the rename pass rewrite the synthetic name to the user binding name, corrupting the compiled function. Please keep these recovered source-map spans from participating in the rename pass, or skip compiled regions, before enabling them globally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — addressed in d6e90d0. The reverse conversion now records the span.lo of every identifier whose span was recovered from loc (recovered_ident_spans), and transform removes those positions from the rename plan before apply_renames runs. Recovered spans therefore only feed source mapping; a generated identifier that inherits the loc of an original reference can no longer be rewritten to a user binding name. Identifiers in passthrough-preserved nodes keep start/end and still participate in renaming as before.
| return DUMMY_SP; | ||
| }; | ||
| match (loc.start.index, loc.end.index) { | ||
| (Some(start), Some(end)) => Span::new(BytePos(start + 1), BytePos(end + 1)), |
There was a problem hiding this comment.
This conversion treats Babel loc.index as if every transformed file starts at BytePos(1). In the Compiler::process_js_file/shared SourceMap path, later SourceFiles have a non-1 start_pos, but the React Compiler bridge only passes &fm.src into convert_program, so recovered loc-only nodes in those files get byte positions in an earlier file or outside the current file range. With source maps enabled, compiled React output for any file after the first can map to the wrong source instead of the file being transformed; the recovered span needs to be rebased with the current SourceFile start position (or kept dummy when that context is unavailable).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in d6e90d0. Instead of assuming BytePos(1), the rebase offset is now derived from the root program node, which carries both the absolute start (set by the forward conversion from span.lo) and the file-relative 0-based loc.start.index — their difference is exactly the current SourceFile base. For the common per-file transform this evaluates to 1, and for a shared SourceMap it anchors recovered spans into the correct file range. Note the forward conversion's position() clamps offsets to the source text length, so file-relative loc line/columns for start_pos != 1 files have a pre-existing inaccuracy that affects diagnostics as well; that seems worth fixing separately from this PR.
Merging this PR will not alter performance
Comparing 
Footnotes
|
… nodes Nodes regenerated by the React Compiler codegen carry only a Babel-style loc (with 0-based index offsets) on their BaseNode; start/end are left unset. span_from_base therefore returned DUMMY_SP for every node in a compiled function body, which: - erased all source mappings for compiled components (breakpoints in devtools no longer bind anywhere inside a compiled function), and - broke the span-keyed preserved-AST lookup, silently dropping preserved TS metadata such as JSX type arguments (see the updated fixture). Fall back to loc.start.index / loc.end.index (converted back to 1-based BytePos) when start/end are absent.
…e loc offsets Recovered spans exist only for source mapping. Record the positions of identifiers whose spans were recovered from loc and exclude them from the span.lo-keyed rename plan, so a generated identifier that inherits the loc of an original reference can never be rewritten to a user binding name. Derive the loc rebase offset from the root node (absolute start minus file-relative loc index) instead of assuming BytePos(1), so recovered spans stay inside the current SourceFile when it does not start at 1.
42b6d06 to
d6e90d0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6e90d073b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| if let (Some(start), Some(index)) = ( | ||
| program.base.start, | ||
| program.base.loc.as_ref().and_then(|loc| loc.start.index), | ||
| ) { | ||
| self.loc_base.set(start.saturating_sub(index).max(1)); |
There was a problem hiding this comment.
Fresh evidence in this revision: this new base is derived from program.base.loc, but the forward converter’s position() still clamps absolute BytePos values against the current fm.src.len(), so a shared SourceMap file after the first gets a root loc.index of the file length instead of 0. In that common process_js_file path, loc-only compiler-generated nodes are shifted by start - len (or collapse to the file start) rather than the current SourceFile start, so recovered spans/source maps and span-keyed preserved TS metadata can point outside the file or to the wrong node; use the actual source-file start position instead of trusting the root loc index.
Useful? React with 👍 / 👎.
|
I’m a bit concerned that this may not be the right layer to fix this. As I understand it, However, once we start reading positions from My preference would be to avoid adding UTF-8/UTF-16 complexity here if possible. Ideally, this should be fixed on the React Compiler side, so that generated nodes always carry valid |
3 participants
Description:
Function bodies compiled by the React Compiler currently lose all source mappings, so devtools breakpoints cannot bind anywhere inside a compiled component (clicking a line number in Chrome DevTools does nothing). This reproduces with plain
@swc/core1.15.43 (jsc.transform.reactCompiler: { target: "18" }) as well as through rspack 2.1'sbuiltin:swc-loader.Root cause:
codegen_reactive_functionin the React Compiler crates emits regenerated AST nodes whoseBaseNodecarries only a Babel-styleloc(with 0-basedindexoffsets);start/endare left unset (base_node_with_loc). The reverse bridge'sspan_from_baseonly readsstart/end, so every node in a compiled function body came back asDUMMY_SPand the codegen had nothing to map.This also broke the span-keyed
preserved_astlookup: preserved TS metadata such as JSX type arguments was silently dropped from compiled output because thespan.lokeys were all dummy. The existingast-roundtrip/jsx.tsxfixture snapshot had captured that data loss (<UI.Panel<T>became<UI.Panel); this PR updates the snapshot to the now-correct output that round-trips the type argument.Fix: when
start/endare absent, recover the span fromloc.start.index/loc.end.index, converting the 0-based Babel indices back to SWC's 1-basedBytePos(the forward conversion inconvert_ast.rssetsindex = BytePos - 1, so the coordinate spaces line up).Measured on a 31-line TSX component with hooks (emitting with
build_source_map):The remaining unmapped lines are nested-closure bodies and JSX tails whose nodes are emitted without
locupstream (BaseNode::typed) — improving that is follow-up work in the React Compiler codegen, but this change restores mappings for the bulk of compiled function bodies and makes breakpoints usable again.BREAKING CHANGE:
None.
Related issue (if exists):
None filed; discovered while debugging missing breakpoints with rspack 2.1 +
reactCompiler.