[eslint] Don't crash on multiple @babel/parser copies

[nicolo-ribaudo]

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

This PR replaces object ref comparisons with string comparisons.

[nicolo-ribaudo]

This throws without the fix (it's the example from #12985 (comment))

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5601f32:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

[babel-bot]

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

[JLHwung]

Can we check whether we have different copies of @babel/parser when loading covertTokens? Then we can use the slower string match only when we know a fast token match check is not feasible.

[nicolo-ribaudo]

When the objects would be equal, the strings are the same (I don't mean that they are ===, I mean that they are exactly the same string created in the same place at the same time).

v8 internally compares them by pointer, so it's as fast as comparing the objects:

1. The === implementation: https://github.com/v8/v8/blob/dc712da548c7fb433caed56af9a021d964952728/src/objects/objects.cc#L816
2. The equality implementation for strings: https://github.com/v8/v8/blob/dc712da548c7fb433caed56af9a021d964952728/src/objects/string-inl.h#L373

If we want to improve perf, a good thing to do might be replacing the if-else with a switch (however, I didn't test it but I think it hardly makes any difference since it's preceded by a full JS parse and followed by ESLint's traversal phase).

EDIT: Since the strings are interned (because they are short and directly present in the source code), also the false path in the second link doesn't use the slower equality function.

[JLHwung]

Did you mean https://github.com/v8/v8/blob/dc712da548c7fb433caed56af9a021d964952728/src/objects/objects.cc#L821 ? The token is a plain object while token.label is a string, so comparing token is faster than comparing strings because the former essentially compare the memory address.

I agree that they are probably neglectable compared to parse/estraverse. So I am good on this PR.

[kaicataldo]

One question about increasing test coverage but otherwise LGTM!

[kaicataldo]

Do we want to try to figure out a way to test this? Would it work to just manually set process.env.BABEL_8_BREAKING in our tests?

[nicolo-ribaudo]

Yeah, it's tested by the "Test Babel 8 breaking changes" job (even if it doesn't show up in codecov).
When running locally, you can use BABEL_8_BREAKING=true make test.

[nicolo-ribaudo]

Did you mean v8/v8@dc712da/src/objects/objects.cc#L821 ? The token is a plain object while token.label is a string, so comparing token is faster than comparing strings because the former essentially compare the memory address.

Reading the v8 code (but it's the first time I do it!), I think that when comparing interned strings it also compares just the memory address (the second of my links), so it should be equally fast (it does three checks: pointer + first is interned + second is interned, but it still doesn't need to iterate the string).