Expose .index on Position to internally track nodes location

[tolmasky]

Per the discussion in #14130, I am opening this PR to discuss the addition of an index property to the Position class. For context, we decided to use an indexes WeakMap in the above PR to avoid adding new members in a patch release, and figured we'd discuss whether to add this property for a minor release. In this current PR, I've kept it as an enumerable: false property, mainly to avoid having to generate all new fixtures, but I'd be happy to make it a normal property and do that too.

Also potentially relevant bug: #14173 - Although this bug can be done with either approach, it would certainly be more elegant to add more instances of loc.index than indexes.get(loc).

There may exist some performance argument for switching over to .index as well, but from what I've seen so far it doesn't seem to be too substantial.

Q	A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?	Yes
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

[nicolo-ribaudo]

Yeah, let's expose .index. It significantly simplifies our internal implementation since we don't need to interact with the WeakMap anymore.

Also, I like that it consolidates all the location-related nodes data in .loc, rather than being scattered across three properties (well, we still have .start and .end, but @babel/parser consumers can now use just .loc).

[nicolo-ribaudo]

In this current PR, I've kept it as an enumerable: false property, mainly to avoid having to generate all new fixtures, but I'd be happy to make it a normal property and do that too.

Let's make it enumerable, even if it is a (one-time) annoying change that affects all our fixtures.

[tolmasky]

Cool, I just changed it to a normal property. This will make a bunch of tests fail of course, but I'll regenerate them and push that.

[nicolo-ribaudo]

You might have to regenerate them twice, the second time specifying the BABEL_8_BREAKING=true env flag.

[tolmasky]

Sounds good, it might end up touching every fixture because of that newline change from the last PR too.

[existentialism]

💯

[joux3]

Just a small FYI: I wondered why prettier performance on their latest development branch had taken a hit. JS profiler showed the culprit to be the Position constructor. With the changes in this PR the performance problem is gone. I've attached parse times of 2000-line JS file when prettier runs 30 times in a row.

With indexes WeakMap

parse: 141.369ms
parse: 72.971ms
parse: 62.54ms
parse: 39.797ms
parse: 37.258ms
parse: 53.959ms
parse: 39.975ms
parse: 43.777ms
parse: 61.212ms
parse: 39.372ms
parse: 42.96ms
parse: 37.395ms
parse: 43.549ms
parse: 40.546ms
parse: 84.72ms
parse: 54.479ms
parse: 37.793ms
parse: 34.357ms
parse: 35.037ms
parse: 37.151ms
parse: 39.981ms
parse: 38.672ms
parse: 41.188ms
parse: 39.386ms
parse: 37.607ms
parse: 37.418ms
parse: 38.515ms
parse: 39.945ms
parse: 56.595ms
parse: 36.949ms

With index as a property on Position

parse: 125.497ms
parse: 54.491ms
parse: 38.092ms
parse: 30.726ms
parse: 29.803ms
parse: 27.034ms
parse: 32.897ms
parse: 33.431ms
parse: 28.893ms
parse: 27.017ms
parse: 25.962ms
parse: 24.177ms
parse: 24.763ms
parse: 22.585ms
parse: 26.872ms
parse: 22.765ms
parse: 24.774ms
parse: 30.861ms
parse: 35.862ms
parse: 25.584ms
parse: 25.947ms
parse: 25.213ms
parse: 21.999ms
parse: 22.546ms
parse: 20.826ms
parse: 21.315ms
parse: 21.283ms
parse: 27.481ms
parse: 22.744ms
parse: 21.553ms

[tolmasky]

Just a heads up so that it doesn't slip in with all the test changes, in order to get estree tests passing, I made it so that the estree plugin takes Positions and makes their index property non-enumerable. I specifically don't delete the property just in case it might be being used somewhere else (since we don't copy Positions when assigning them to nodes -- this is equivalent to the previous behavior and not anything new). The reason I didn't just update the estree tests instead, is because 1) if I understand correctly their point is to match the spec that we don't control, and 2) even when I do do that, we still fail the tests the compare our results with espree (in parseAndAssertSame). Here are the relevant changes: f8b85e4

[tolmasky]

Hit comment a bit too soon, wanted to end the above thought by saying that I'm happy to change the strategy with estree if someone proposes something else, and wanted to also explain that this issue didn't come up until I made index a public (and not undefined) property, as when it is either "private" or undefined, it doesn't get compared in the diffing algorithm.

[nicolo-ribaudo]

Yeah I noticed that you made it non-enumerable in ESTree; I don't particularly care about what it is since technically we shouldn't have a property (because ESTree doesn't have it), but removing it from the AST would require a full traversal. I'm fine with just "partially hiding" it as you did.

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51042/