Conversation
Tracks locations for reactive scope dependencies, both on the deps and portions of the path. The immediate need for this is a non-public experiment where we're exploring type-directed compilation, and sometimes look up the types of expressions by location. We need to preserve locations accurately for that to work, including the locations of the deps. ## Test Plan Locations for dependencies are not easy to test: i manually spot-checked the new fixture to ensure that the deps look right. This is fine as best-effort since it doesn't impact any of our core compilation logic, i may fix forward if there are issues and will think about how to test.
Greptile SummaryThis PR adds
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 6616fc6 |
| printIdentifier(dependency.identifier) + | ||
| printType(dependency.identifier.type); | ||
| return `${identifier}${dependency.path.map(token => `${token.optional ? '?.' : '.'}${token.property}`).join('')}`; | ||
| return `${identifier}${dependency.path.map(token => `${token.optional ? '?.' : '.'}${token.property}`).join('')}_${printSourceLocation(dependency.loc)}`; |
There was a problem hiding this comment.
Location suffix changes debug IR output format
printDependency now appends _${printSourceLocation(dependency.loc)} (e.g. _3:5:3:12) to every dependency string. This function is used in printReactiveScopeSummary (which feeds into printReactiveFunction, exported publicly and used by the snap test runner) and in debug logging in CollectHoistablePropertyLoads.
Is it intentional for this location suffix to appear in the debug IR output? If so, the underscore separator could be ambiguous with identifiers that contain underscores. Consider using a more distinct delimiter (e.g., @ or wrapping in brackets like [3:5:3:12]).
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction.ts
Line: 118:118
Comment:
**Location suffix changes debug IR output format**
`printDependency` now appends `_${printSourceLocation(dependency.loc)}` (e.g. `_3:5:3:12`) to every dependency string. This function is used in `printReactiveScopeSummary` (which feeds into `printReactiveFunction`, exported publicly and used by the snap test runner) and in debug logging in `CollectHoistablePropertyLoads`.
Is it intentional for this location suffix to appear in the debug IR output? If so, the underscore separator could be ambiguous with identifiers that contain underscores. Consider using a more distinct delimiter (e.g., `@` or wrapping in brackets like `[3:5:3:12]`).
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
When Given the PR description mentions this is "best effort" and the locations are used for a non-public type-directed compilation experiment, this is likely acceptable — but worth being aware of if location accuracy becomes important for merged nodes. Prompt To Fix With AIThis is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts
Line: 385:387
Comment:
**`loc` not updated when merging existing nodes**
When `makeOrMergeProperty` finds an existing child node, `accessType` is merged via `merge()` but `loc` is left as-is from the first encounter. The same pattern applies in `#getOrCreateRoot` (line 94-100). This means the location on a merged node is determined by whichever dependency was processed first, which could be arbitrary depending on traversal order.
Given the PR description mentions this is "best effort" and the locations are used for a non-public type-directed compilation experiment, this is likely acceptable — but worth being aware of if location accuracy becomes important for merged nodes.
How can I resolve this? If you propose a fix, please make it concise. |
|
Upstream PR was closed or merged. Code is synced via branch mirror. |
Mirror of facebook/react#35794
Original author: josephsavona
Tracks locations for reactive scope dependencies, both on the deps and portions of the path. The immediate need for this is a non-public experiment where we're exploring type-directed compilation, and sometimes look up the types of expressions by location. We need to preserve locations accurately for that to work, including the locations of the deps.
Test Plan
Locations for dependencies are not easy to test: i manually spot-checked the new fixture to ensure that the deps look right. This is fine as best-effort since it doesn't impact any of our core compilation logic, i may fix forward if there are issues and will think about how to test.