[Fix] exportMap: export map cache is tainted by unreliable parse results

[michaelfaith]

This change addresses an issue observed with the v9 upgrade, where the ExportMap Cache is being tainted with a bad export map, if the parse doesn't yield a visitorKeys (which can happen if an incompatible parser is used (e.g. og babel eslint)). If one run of the no-cycle rule uses an incompatible parser and taints the cache and a subsequent test is run with a compatible parser, then the bad export map will be found in the cache and used instead of attempting to proceed with the parse.

I don't think this is likely to occur in the wild, but in the event that it does, this should help. One thing to note, though: if someone is using an incompatible parser, then this change will have a performance impact, since it will attempt to create an export map every time, until it yields one that has visitorKeys attached.

I also updated the getExports test to use a valid parserPath, rather than a mocked one, so the tests are acting on a valid parserPath. Otherwise the export map won't be cached following this change and the test for it caching would fail.

Additional context: #2996 (comment)

[michaelfaith]

The test failures are from eslint 2 and 3, which I'm guessing doesn't yield a visitorKeys from the parse, causing the cache to be bypassed (failing the test that the export map is cached).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.72%. Comparing base (5c9757c) to head (8117601).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3062      +/-   ##
==========================================
- Coverage   92.21%   90.72%   -1.50%     
==========================================
  Files          82       82              
  Lines        3557     3557              
  Branches     1244     1244              
==========================================
- Hits         3280     3227      -53     
- Misses        277      330      +53     

Flag	Coverage Δ
	90.72% <100.00%> (-1.50%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[michaelfaith]

Another idea for how to approach this, to limit the impact to just the rules that need it:
we could add an additional optional param to ExportMapBuilder.get that takes a skip cache function, which takes an export map and returns a boolean for whether that instance of the map should be cached. no-cycle could then pass in (exportMap) => !exportMap.visitorKeys, which has the same effect, but limits it to only that rule.

[G-Rath]

Confirming that this looks to address some of the test failures in #2996 - definitely the ones from no-cycle and probably others.

The test failures are from eslint 2 and 3

Could you just also check what version of ESLint is being used? as this seems to only take effect when using flat config and/or ESLint v9 which neither of those versions will support, and it feels like it could be more of a footgun to have rules control the cache

[michaelfaith]

Could you just also check what version of ESLint is being used? as this seems to only take effect when using flat config and/or ESLint v9 which neither of those versions will support, and it feels like it could be more of a footgun to have rules control the cache

Yeah, I like that too.

[ljharb]

This approach seems fine to me. If a version check is the cleanest way to handle this, that's fine, but it'd be much nicer to feature-test instead.

[michaelfaith]

This approach seems fine to me. If a version check is the cleanest way to handle this, that's fine, but it'd be much nicer to feature-test instead.

Updated the tests to only run the caching test if we determine babel-eslint has visitor-keys.

cc @G-Rath you may want to pull this into your PR, to resolve the v2/v3 test errors.