[compiler] Fix VariableDeclarator source location

[nathanmarks]

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 #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 id and end of the init can be used to accurately reconstruct it when generating the AST.
• Add source locations for object/array patterns for destructuring assignment source location support

[josephsavona]

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.

Re the whitespace changes in fixtures: we've seen this pretty consistently as we change source locations. The source location info influences whether Babel's code generator adds a newline or not, and prettier preserves single empty lines between blocks of code. It's fine!

[nathanmarks]

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?

[josephsavona]

Hmm let me look into how onerous that would be

[nathanmarks]

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]

Sounds good!

[nathanmarks]

@josephsavona see fc833f6 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

[nathanmarks]

@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>.

[nathanmarks]

friendly bump here again, I'd love to push on with this!

[josephsavona]

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.

[nathanmarks]

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.

[nathanmarks]

@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!

[nathanmarks]

@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!

[mofeiZ]

Thanks for the fix, this looks great!

[sebmarkbage]

Had to revert since this broke main and we're publishing security patch.

[nathanmarks]

@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?

[nathanmarks]

put it up again here for whenever we're ready #35348