facebook
[compiler] Fix VariableDeclarator source location
What
Fixes source locations for VariableDeclarator in the generated AST. Fixes a number of the errors in the snapshot I added yesterday in the source loc validator PR https://github.com/facebook/react/pull/35109
I'm not entirely sure why, but a side effect of the fix has resulted in a ton of snaps needing updating, with some empty lines no longer present in the generated output. I broke the change up into 2 separate commits. The first commit has the core change and the update to the missing source locations test expectation, and the second commit has the rest of the snapshot updates.
How
- Add location for variable declarators in ast codegen.
- We don't actually have the location preserved in HIR, since when we lower the declarations we pass through the location for the VariableDeclaration. Since VariableDeclarator is just a container for each of the assignments, the start of the
idand end of theinitcan be used to accurately reconstruct it when generating the AST.
- We don't actually have the location preserved in HIR, since when we lower the declarations we pass through the location for the VariableDeclaration. Since VariableDeclarator is just a container for each of the assignments, the start of the
- Add source locations for object/array patterns for destructuring assignment source location support
Awesome! One question to see if we can get even more coverage for variable declarations w/o an initial value - see comment - otherwise this looks good to go.
I'm glad you asked.... I ran into some stuff looking through this. These declarations are also missing source locations for the VariableDeclarations. I wasn't tracking that before, but if we're wanting to have as much completeness as possible (rather than just surgically fixing things I care about the most for to fix my issues) we might want to address this.
Basically, the declarations hoisted to reactive scopes don't have anywhere for us to track these locations. We only track the location for the identifier. I can fix this by having ReactiveScopeDeclaration track the location for the declaration through the pipeline. What do you think?
I have a commit where I gave it a go. I can push it to another branch when I'm at my laptop for you to have a gander.
@josephsavona see https://github.com/facebook/react/commit/fc833f6a52a244105422821400314c4135accab6 for the changes to the pipeline to address the VariableDeclaration location preservation.
note: the changes to BuildHIR.ts (id.node.loc -> stmt.node.loc) make this consistent with StoreLocal. The id loc is already preserved (pretty sure via place.identifier).
--
separately, I pushed a few commits to this PR just adding the change you had requested (had to rebase as well), along with a number of changes to the test fixture + validator, including checking source location for VariableDeclaration & Identifier nodes so that we don't accidentally regress those as we implement other fixes. I think there's a couple of cases where you could accidentally clobber something else trying to find an appropriate place to pass something through, so thought it was worthwhile to add. And I think a more full picture can help guide more "correct" decisions.
If you'd like to see it all in action, I can add the commit to this branch (or on a separate tmp branch) with the pipeline changes from the commit I linked above, and in the test fixture we would expect to see the new VariableDeclaration errors removed without any other regressions
@josephsavona just following up here. Let me know if you want to take more time to think about that commit/approach before doing anything else here, or if you want we could also just merge this PR as-is and continue discussing that after, or <insert some option I didn't think of>.
Nice, this looks great. I think we'll have to wait to merge until we can re-enable CI (currently disabled out an abundance of caution given the npm vulnerability), but makes sense to ship after CI is restored.
Yup. No worries!
Let me know what you want me to do about that other commit. I can open a PR with it to discuss further as a next step.
@josephsavona
Sorry for the delay, I was on holiday. I just rebased on latest main and had to update a couple of snaps. Good to merge? need you to whack that button!
@josephsavona sorry to keep bugging you -- just a reminder to get this merged
cc @poteto if joseph is busy/away and you're ok to merge it!
Had to revert since this broke main and we're publishing security patch.
@sebmarkbage it's because it causes a formatting change in lots of snaps, so when i rebase it needs to get merged pretty quick not to cause breakages. I'll put another PR up?
put it up again here for whenever we're ready https://github.com/facebook/react/pull/35348