From 61f02a2cac4b9c12bba8da670e5ccdd772a363e2 Mon Sep 17 00:00:00 2001 From: michael faith Date: Sun, 15 Sep 2024 11:12:26 -0500 Subject: [PATCH] [Fix] `exportMap`: export map cache is tainted by unreliable parse results This change addresses and 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)) for one run of the no-cycle rule. If 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 also updated the `getExports` test to use a valid parserPath, rather than a mocked one, so that the tests are acting on a valid parserPath. Otherwise the export map won't be cached following this change. --- CHANGELOG.md | 2 ++ src/exportMap/builder.js | 6 +++++- tests/src/core/getExports.js | 24 ++++++++++++++++++------ 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 33b4eb4f4..da66f7e87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange - `ExportMap` / flat config: include `languageOptions` in context ([#3052], thanks [@michaelfaith]) - [`no-named-as-default`]: Allow using an identifier if the export is both a named and a default export ([#3032], thanks [@akwodkiewicz]) - [`export`]: False positive for exported overloaded functions in TS ([#3065], thanks [@liuxingbaoyu]) +- `exportMap`: export map cache is tainted by unreliable parse results ([#3062], thanks [@michaelfaith]) ### Changed - [Docs] [`no-relative-packages`]: fix typo ([#3066], thanks [@joshuaobrien]) @@ -1146,6 +1147,7 @@ for info on changes for earlier releases. [#3068]: https://github.com/import-js/eslint-plugin-import/pull/3068 [#3066]: https://github.com/import-js/eslint-plugin-import/pull/3066 [#3065]: https://github.com/import-js/eslint-plugin-import/pull/3065 +[#3062]: https://github.com/import-js/eslint-plugin-import/pull/3062 [#3052]: https://github.com/import-js/eslint-plugin-import/pull/3052 [#3043]: https://github.com/import-js/eslint-plugin-import/pull/3043 [#3036]: https://github.com/import-js/eslint-plugin-import/pull/3036 diff --git a/src/exportMap/builder.js b/src/exportMap/builder.js index 5348dba37..f7b9006ef 100644 --- a/src/exportMap/builder.js +++ b/src/exportMap/builder.js @@ -92,7 +92,11 @@ export default class ExportMapBuilder { exportMap.mtime = stats.mtime; - exportCache.set(cacheKey, exportMap); + // If the visitor keys were not populated, then we shouldn't save anything to the cache, + // since the parse results may not be reliable. + if (exportMap.visitorKeys) { + exportCache.set(cacheKey, exportMap); + } return exportMap; } diff --git a/tests/src/core/getExports.js b/tests/src/core/getExports.js index 76003410d..f11a26131 100644 --- a/tests/src/core/getExports.js +++ b/tests/src/core/getExports.js @@ -1,15 +1,18 @@ import { expect } from 'chai'; +import fs from 'fs'; import semver from 'semver'; import sinon from 'sinon'; import eslintPkg from 'eslint/package.json'; +import { test as testUnambiguous } from 'eslint-module-utils/unambiguous'; import typescriptPkg from 'typescript/package.json'; import * as tsConfigLoader from 'tsconfig-paths/lib/tsconfig-loader'; -import ExportMapBuilder from '../../../src/exportMap/builder'; - -import * as fs from 'fs'; +import ExportMapBuilder from '../../../src/exportMap/builder'; import { getFilename } from '../utils'; -import { test as testUnambiguous } from 'eslint-module-utils/unambiguous'; + +const babelPath = require.resolve('babel-eslint'); +const hypotheticalLocation = babelPath.replace('index.js', 'visitor-keys.js'); +const isVisitorKeysSupported = fs.existsSync(hypotheticalLocation); describe('ExportMap', function () { const fakeContext = Object.assign( @@ -21,7 +24,7 @@ describe('ExportMap', function () { }, { settings: {}, - parserPath: 'babel-eslint', + parserPath: require.resolve('babel-eslint'), }, ); @@ -36,11 +39,20 @@ describe('ExportMap', function () { }); - it('returns a cached copy on subsequent requests', function () { + (isVisitorKeysSupported ? it : it.skip)('returns a cached copy on subsequent requests', function () { expect(ExportMapBuilder.get('./named-exports', fakeContext)) .to.exist.and.equal(ExportMapBuilder.get('./named-exports', fakeContext)); }); + it('does not return a cached copy if the parse does not yield a visitor keys', function () { + const mockContext = { + ...fakeContext, + parserPath: 'not-real', + }; + expect(ExportMapBuilder.get('./named-exports', mockContext)) + .to.exist.and.not.equal(ExportMapBuilder.get('./named-exports', mockContext)); + }); + it('does not return a cached copy after modification', (done) => { const firstAccess = ExportMapBuilder.get('./mutator', fakeContext); expect(firstAccess).to.exist;