From 33668ee9d22e1988ba03e07fb547738bdb21dc0e Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Fri, 21 Oct 2022 13:26:36 -0700 Subject: [PATCH] fix: Ensure that glob patterns are matched correctly. (#16449) * fix: Ensure that glob patterns are matched correctly. Fixes #16413 Fixes #16299 * Fix cli tests * Upgrade config-array to fix globbing issues --- lib/eslint/eslint-helpers.js | 180 +++++++++---------- package.json | 5 +- tests/fixtures/example-app/app.js | 1 + tests/fixtures/example-app/eslint.config.js | 1 + tests/fixtures/example-app/subdir/util.js | 1 + tests/fixtures/example-app2/eslint.config.js | 3 + tests/fixtures/example-app2/subdir1/a.js | 0 tests/fixtures/example-app2/subdir2/b.js | 0 tests/lib/cli.js | 2 +- tests/lib/eslint/flat-eslint.js | 57 +++++- 10 files changed, 147 insertions(+), 103 deletions(-) create mode 100644 tests/fixtures/example-app/app.js create mode 100644 tests/fixtures/example-app/eslint.config.js create mode 100644 tests/fixtures/example-app/subdir/util.js create mode 100644 tests/fixtures/example-app2/eslint.config.js create mode 100644 tests/fixtures/example-app2/subdir1/a.js create mode 100644 tests/fixtures/example-app2/subdir2/b.js diff --git a/lib/eslint/eslint-helpers.js b/lib/eslint/eslint-helpers.js index 80e37f92310..ed4132cb621 100644 --- a/lib/eslint/eslint-helpers.js +++ b/lib/eslint/eslint-helpers.js @@ -17,6 +17,8 @@ const hash = require("../cli-engine/hash"); const minimatch = require("minimatch"); const util = require("util"); const fswalk = require("@nodelib/fs.walk"); +const globParent = require("glob-parent"); +const isPathInside = require("is-path-inside"); //----------------------------------------------------------------------------- // Fixup references @@ -111,7 +113,7 @@ function isGlobPattern(pattern) { * be ignored, so it is consistent with how ignoring works throughout * ESLint. * @param {Object} options The options for this function. - * @param {string} options.cwd The directory to search. + * @param {string} options.basePath The directory to search. * @param {Array} options.patterns An array of glob patterns * to match. * @param {FlatConfigArray} options.configs The config array to use for @@ -119,7 +121,7 @@ function isGlobPattern(pattern) { * @returns {Promise>} An array of matching file paths * or an empty array if there are no matches. */ -async function globSearch({ cwd, patterns, configs }) { +async function globSearch({ basePath, patterns, configs }) { if (patterns.length === 0) { return []; @@ -127,22 +129,22 @@ async function globSearch({ cwd, patterns, configs }) { const matchers = patterns.map(pattern => { const patternToUse = path.isAbsolute(pattern) - ? normalizeToPosix(path.relative(cwd, pattern)) + ? normalizeToPosix(path.relative(basePath, pattern)) : pattern; return new minimatch.Minimatch(patternToUse); }); - return (await doFsWalk(cwd, { + return (await doFsWalk(basePath, { deepFilter(entry) { - const relativePath = normalizeToPosix(path.relative(cwd, entry.path)); + const relativePath = normalizeToPosix(path.relative(basePath, entry.path)); const matchesPattern = matchers.some(matcher => matcher.match(relativePath, true)); return matchesPattern && !configs.isDirectoryIgnored(entry.path); }, entryFilter(entry) { - const relativePath = normalizeToPosix(path.relative(cwd, entry.path)); + const relativePath = normalizeToPosix(path.relative(basePath, entry.path)); // entries may be directories or files so filter out directories if (entry.dirent.isDirectory()) { @@ -157,19 +159,40 @@ async function globSearch({ cwd, patterns, configs }) { } +/** + * Performs multiple glob searches in parallel. + * @param {Object} options The options for this function. + * @param {Array<{patterns:Array,rawPatterns:Array}>} options.searches + * An array of glob patterns to match. + * @param {FlatConfigArray} options.configs The config array to use for + * determining what to ignore. + * @returns {Promise>} An array of matching file paths + * or an empty array if there are no matches. + */ +async function globMultiSearch({ searches, configs }) { + + const results = await Promise.all( + [...searches].map( + ([basePath, { patterns }]) => globSearch({ basePath, patterns, configs }) + ) + ); + + return [...new Set(results.flat())]; +} + /** * Determines if a given glob pattern will return any results. * Used primarily to help with useful error messages. * @param {Object} options The options for the function. - * @param {string} options.cwd The directory to search. + * @param {string} options.basePath The directory to search. * @param {string} options.pattern A glob pattern to match. * @returns {Promise} True if there is a glob match, false if not. */ -function globMatch({ cwd, pattern }) { +function globMatch({ basePath, pattern }) { let found = false; const patternToUse = path.isAbsolute(pattern) - ? normalizeToPosix(path.relative(cwd, pattern)) + ? normalizeToPosix(path.relative(basePath, pattern)) : pattern; const matcher = new Minimatch(patternToUse); @@ -177,7 +200,7 @@ function globMatch({ cwd, pattern }) { const fsWalkSettings = { deepFilter(entry) { - const relativePath = normalizeToPosix(path.relative(cwd, entry.path)); + const relativePath = normalizeToPosix(path.relative(basePath, entry.path)); return !found && matcher.match(relativePath, true); }, @@ -187,7 +210,7 @@ function globMatch({ cwd, pattern }) { return false; } - const relativePath = normalizeToPosix(path.relative(cwd, entry.path)); + const relativePath = normalizeToPosix(path.relative(basePath, entry.path)); if (matcher.match(relativePath)) { found = true; @@ -201,7 +224,7 @@ function globMatch({ cwd, pattern }) { return new Promise(resolve => { // using a stream so we can exit early because we just need one match - const globStream = fswalk.walkStream(cwd, fsWalkSettings); + const globStream = fswalk.walkStream(basePath, fsWalkSettings); globStream.on("data", () => { globStream.destroy(); @@ -242,8 +265,10 @@ async function findFiles({ }) { const results = []; - const globbyPatterns = []; const missingPatterns = []; + let globbyPatterns = []; + let rawPatterns = []; + const searches = new Map([[cwd, { patterns: globbyPatterns, rawPatterns: [] }]]); // check to see if we have explicit files and directories const filePaths = patterns.map(filePath => path.resolve(cwd, filePath)); @@ -271,69 +296,18 @@ async function findFiles({ // directories need extensions attached if (stat.isDirectory()) { - // filePatterns are all relative to cwd - const filePatterns = configs.files - .filter(filePattern => { - - // can only do this for strings, not functions - if (typeof filePattern !== "string") { - return false; - } - - // patterns starting with ** always apply - if (filePattern.startsWith("**")) { - return true; - } - - // patterns ending with * are not used for file search - if (filePattern.endsWith("*")) { - return false; - } - - // not sure how to handle negated patterns yet - if (filePattern.startsWith("!")) { - return false; - } - - // check if the pattern would be inside the config base path or not - const fullFilePattern = path.join(cwd, filePattern); - const patternRelativeToConfigBasePath = path.relative(configs.basePath, fullFilePattern); - - if (patternRelativeToConfigBasePath.startsWith("..")) { - return false; - } - - // check if the pattern matches - if (minimatch(filePath, path.dirname(fullFilePattern), { partial: true })) { - return true; - } - - // check if the pattern is inside the directory or not - const patternRelativeToFilePath = path.relative(filePath, fullFilePattern); - - if (patternRelativeToFilePath.startsWith("..")) { - return false; - } - - return true; - }) - .map(filePattern => { - if (filePattern.startsWith("**")) { - return path.join(pattern, filePattern); - } - - // adjust the path to be relative to the cwd - return path.relative( - cwd, - path.join(configs.basePath, filePattern) - ); - }) - .map(normalizeToPosix); - - if (filePatterns.length) { - globbyPatterns.push(...filePatterns); + // group everything in cwd together and split out others + if (isPathInside(filePath, cwd)) { + ({ patterns: globbyPatterns, rawPatterns } = searches.get(cwd)); + } else { + if (!searches.has(filePath)) { + searches.set(filePath, { patterns: [], rawPatterns: [] }); + } + ({ patterns: globbyPatterns, rawPatterns } = searches.get(filePath)); } + globbyPatterns.push(`${normalizeToPosix(filePath)}/**`); + rawPatterns.push(pattern); } return; @@ -341,39 +315,57 @@ async function findFiles({ // save patterns for later use based on whether globs are enabled if (globInputPaths && isGlobPattern(filePath)) { - globbyPatterns.push(pattern); + + const basePath = globParent(filePath); + + // group in cwd if possible and split out others + if (isPathInside(basePath, cwd)) { + ({ patterns: globbyPatterns, rawPatterns } = searches.get(cwd)); + } else { + if (!searches.has(basePath)) { + searches.set(basePath, { patterns: [], rawPatterns: [] }); + } + ({ patterns: globbyPatterns, rawPatterns } = searches.get(basePath)); + } + + globbyPatterns.push(filePath); + rawPatterns.push(pattern); } else { missingPatterns.push(pattern); } }); - // note: globbyPatterns can be an empty array - const globbyResults = await globSearch({ - cwd, - patterns: globbyPatterns, - configs, - shouldIgnore: true + const globbyResults = await globMultiSearch({ + searches, + configs }); // if there are no results, tell the user why if (!results.length && !globbyResults.length) { - // try globby without ignoring anything - for (const globbyPattern of globbyPatterns) { + for (const [basePath, { patterns: patternsToCheck, rawPatterns: patternsToReport }] of searches) { - // check if there are any matches at all - const patternHasMatch = await globMatch({ - cwd, - pattern: globbyPattern - }); + let index = 0; - if (patternHasMatch) { - throw new AllFilesIgnoredError(globbyPattern); - } + // try globby without ignoring anything + for (const patternToCheck of patternsToCheck) { + + // check if there are any matches at all + const patternHasMatch = await globMatch({ + basePath, + pattern: patternToCheck + }); + + if (patternHasMatch) { + throw new AllFilesIgnoredError(patternsToReport[index]); + } + + // otherwise no files were found + if (errorOnUnmatchedPattern) { + throw new NoFilesFoundError(patternsToReport[index], globInputPaths); + } - // otherwise no files were found - if (errorOnUnmatchedPattern) { - throw new NoFilesFoundError(globbyPattern, globInputPaths); + index++; } } diff --git a/package.json b/package.json index b8d324bc683..1d47d27d220 100644 --- a/package.json +++ b/package.json @@ -56,7 +56,7 @@ "bugs": "https://github.com/eslint/eslint/issues/", "dependencies": { "@eslint/eslintrc": "^1.3.3", - "@humanwhocodes/config-array": "^0.11.5", + "@humanwhocodes/config-array": "^0.11.6", "@humanwhocodes/module-importer": "^1.0.1", "@nodelib/fs.walk": "^1.2.8", "ajv": "^6.10.0", @@ -74,13 +74,14 @@ "fast-deep-equal": "^3.1.3", "file-entry-cache": "^6.0.1", "find-up": "^5.0.0", - "glob-parent": "^6.0.1", + "glob-parent": "^6.0.2", "globals": "^13.15.0", "grapheme-splitter": "^1.0.4", "ignore": "^5.2.0", "import-fresh": "^3.0.0", "imurmurhash": "^0.1.4", "is-glob": "^4.0.0", + "is-path-inside": "^3.0.3", "js-sdsl": "^4.1.4", "js-yaml": "^4.1.0", "json-stable-stringify-without-jsonify": "^1.0.1", diff --git a/tests/fixtures/example-app/app.js b/tests/fixtures/example-app/app.js new file mode 100644 index 00000000000..e5f581a97a9 --- /dev/null +++ b/tests/fixtures/example-app/app.js @@ -0,0 +1 @@ +console.log("Running"); diff --git a/tests/fixtures/example-app/eslint.config.js b/tests/fixtures/example-app/eslint.config.js new file mode 100644 index 00000000000..e0a30c5dfa3 --- /dev/null +++ b/tests/fixtures/example-app/eslint.config.js @@ -0,0 +1 @@ +module.exports = []; diff --git a/tests/fixtures/example-app/subdir/util.js b/tests/fixtures/example-app/subdir/util.js new file mode 100644 index 00000000000..f053ebf7976 --- /dev/null +++ b/tests/fixtures/example-app/subdir/util.js @@ -0,0 +1 @@ +module.exports = {}; diff --git a/tests/fixtures/example-app2/eslint.config.js b/tests/fixtures/example-app2/eslint.config.js new file mode 100644 index 00000000000..cfef56877df --- /dev/null +++ b/tests/fixtures/example-app2/eslint.config.js @@ -0,0 +1,3 @@ +module.exports = [{ + files: ["subdir*/**/*.js"] +}]; diff --git a/tests/fixtures/example-app2/subdir1/a.js b/tests/fixtures/example-app2/subdir1/a.js new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/fixtures/example-app2/subdir2/b.js b/tests/fixtures/example-app2/subdir2/b.js new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/lib/cli.js b/tests/lib/cli.js index 735141758a9..93b7680bd77 100644 --- a/tests/lib/cli.js +++ b/tests/lib/cli.js @@ -746,7 +746,7 @@ describe("cli", () => { : `--ignore-path ${getFixturePath(".eslintignore")}`; const filePath = getFixturePath("cli"); const expectedMessage = useFlatConfig - ? `All files matched by '${filePath.replace(/\\/gu, "/")}/**/*.js' are ignored.` + ? `All files matched by '${filePath.replace(/\\/gu, "/")}' are ignored.` : `All files matched by '${filePath}' are ignored.`; await stdAssert.rejects(async () => { diff --git a/tests/lib/eslint/flat-eslint.js b/tests/lib/eslint/flat-eslint.js index 9e2b694f9c8..f4d28039503 100644 --- a/tests/lib/eslint/flat-eslint.js +++ b/tests/lib/eslint/flat-eslint.js @@ -779,6 +779,49 @@ describe("FlatESLint", () => { assert.strictEqual(results[0].suppressedMessages.length, 0); }); + // https://github.com/eslint/eslint/issues/16413 + it("should find files and report zero messages when given a parent directory with a .js", async () => { + eslint = new FlatESLint({ + ignore: false, + cwd: getFixturePath("example-app/subdir") + }); + const results = await eslint.lintFiles(["../*.js"]); + + assert.strictEqual(results.length, 2); + assert.strictEqual(results[0].messages.length, 0); + assert.strictEqual(results[0].suppressedMessages.length, 0); + assert.strictEqual(results[1].messages.length, 0); + assert.strictEqual(results[1].suppressedMessages.length, 0); + }); + + // https://github.com/eslint/eslint/issues/16299 + it("should only find files in the subdir1 directory when given a directory name", async () => { + eslint = new FlatESLint({ + ignore: false, + cwd: getFixturePath("example-app2") + }); + const results = await eslint.lintFiles(["subdir1"]); + + assert.strictEqual(results.length, 1); + assert.strictEqual(results[0].messages.length, 0); + assert.strictEqual(results[0].filePath, getFixturePath("example-app2/subdir1/a.js")); + assert.strictEqual(results[0].suppressedMessages.length, 0); + }); + + // https://github.com/eslint/eslint/issues/16275 + it("should throw an error for a missing pattern when combined with a found pattern", async () => { + eslint = new FlatESLint({ + ignore: false, + cwd: getFixturePath("example-app2") + }); + const results = await eslint.lintFiles(["subdir1", "doesnotexist/*.js"]); + + assert.strictEqual(results.length, 1); + assert.strictEqual(results[0].messages.length, 0); + assert.strictEqual(results[0].filePath, getFixturePath("example-app2/subdir1/a.js")); + assert.strictEqual(results[0].suppressedMessages.length, 0); + }); + // https://github.com/eslint/eslint/issues/16260 describe("Globbing based on configs", () => { it("should report zero messages when given a directory with a .js and config file specifying a subdirectory", async () => { @@ -811,8 +854,10 @@ describe("FlatESLint", () => { assert.strictEqual(results.length, 2); assert.strictEqual(results[0].messages.length, 1); + assert.strictEqual(results[0].filePath, getFixturePath("shallow-glob/subdir/subsubdir/broken.js")); assert(results[0].messages[0].fatal, "Fatal error expected."); assert.strictEqual(results[0].suppressedMessages.length, 0); + assert.strictEqual(results[1].filePath, getFixturePath("shallow-glob/subdir/subsubdir/plain.jsx")); assert.strictEqual(results[1].messages.length, 0); assert.strictEqual(results[1].suppressedMessages.length, 0); }); @@ -963,7 +1008,7 @@ describe("FlatESLint", () => { await assert.rejects(async () => { await eslint.lintFiles(["node_modules"]); - }, /All files matched by 'node_modules\/\*\*\/\*.js' are ignored\./u); + }, /All files matched by 'node_modules' are ignored\./u); }); // https://github.com/eslint/eslint/issues/5547 @@ -975,7 +1020,7 @@ describe("FlatESLint", () => { await assert.rejects(async () => { await eslint.lintFiles(["node_modules"]); - }, /All files matched by 'node_modules\/\*\*\/\*\.js' are ignored\./u); + }, /All files matched by 'node_modules' are ignored\./u); }); it("should throw an error when all given files are ignored", async () => { @@ -985,7 +1030,7 @@ describe("FlatESLint", () => { await assert.rejects(async () => { await eslint.lintFiles(["tests/fixtures/cli-engine/"]); - }, /All files matched by 'tests\/fixtures\/cli-engine\/\*\*\/\*\.js' are ignored\./u); + }, /All files matched by 'tests\/fixtures\/cli-engine\/' are ignored\./u); }); it("should throw an error when all given files are ignored even with a `./` prefix", async () => { @@ -995,7 +1040,7 @@ describe("FlatESLint", () => { await assert.rejects(async () => { await eslint.lintFiles(["./tests/fixtures/cli-engine/"]); - }, /All files matched by 'tests\/fixtures\/cli-engine\/\*\*\/\*\.js' are ignored\./u); + }, /All files matched by '\.\/tests\/fixtures\/cli-engine\/' are ignored\./u); }); // https://github.com/eslint/eslint/issues/3788 @@ -1032,7 +1077,7 @@ describe("FlatESLint", () => { await assert.rejects(async () => { await eslint.lintFiles(["./tests/fixtures/cli-engine/"]); - }, /All files matched by 'tests\/fixtures\/cli-engine\/\*\*\/\*\.js' are ignored\./u); + }, /All files matched by '\.\/tests\/fixtures\/cli-engine\/' are ignored\./u); }); it("should throw an error when all given files are ignored via ignore-pattern", async () => { @@ -2650,7 +2695,7 @@ describe("FlatESLint", () => { it("should throw if the directory exists and is empty", async () => { await assert.rejects(async () => { await eslint.lintFiles(["empty"]); - }, /No files matching 'empty\/\*\*\/\*\.js' were found\./u); + }, /No files matching 'empty' were found\./u); }); it("one glob pattern", async () => {