Repository navigation
Add Glob module and portable FileSystem.glob - #9438
jpowersdev wants to merge 9 commits into
Conversation
🦋 Changeset detectedLatest commit: 49f9ed6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 33 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Review complete: three confirmed blocking issues reported. Validated with targeted Node reproductions of the matcher and derived walker; no dependency installation or repository changes retained.
Reviewed at 5fcd726: 3 blocking, 0 nits.
Disagree? Reply in the finding's thread, or a maintainer can comment /janitor to re-run or dismiss this review.
| } else if (char === "?") { | ||
| magic = true | ||
| source += "[^/]" | ||
| } else if (char === "[") { |
There was a problem hiding this comment.
Each * becomes another greedy regex quantifier, so matching has exponential backtracking for ordinary valid patterns. I reproduced match(compile("*a".repeat(14) + "b", false), "a".repeat(28)) taking about 1.4 seconds; increasing to 16 exceeded a 10-second check. These are short patterns and valid filename lengths, and the same regex runs synchronously inside the filesystem walk, so a user-supplied pattern can block the runtime. Please use bounded/non-backtracking segment matching (or otherwise prevent this combinatorial search), with an adversarial matching regression test.
There was a problem hiding this comment.
Addressed in b91e486.
Replaced segment regex backtracking with bounded token matching and added an adversarial regression test; the extracted matcher completes the reproduction promptly.
| } | ||
| out.push(segment) | ||
| } |
There was a problem hiding this comment.
Dropping . segments before tracking their position relative to ** violates the new documented contract that . or .. after ** match nothing. For example, **/. is compiled as **, and a walk over a root containing only the file a.ts returns ["a.ts"]; it should return no matches under that contract (and must not treat a file as having a . child). Preserve/reject these segments after a globstar rather than normalizing them away, and cover both the matcher and walker.
There was a problem hiding this comment.
Addressed in b91e486.
Compilation now rejects dot and dot-dot segments after a globstar before normalization; matcher and shared walker regressions were added.
| i++ | ||
| } else if (char === "{") { | ||
| stack.push({ open: i, commas: [] }) | ||
| } else if (char === ",") { | ||
| stack[stack.length - 1]?.commas.push(i) | ||
| } else if (char === "}") { | ||
| const group = stack.pop() |
There was a problem hiding this comment.
Brace expansion scans inside bracket classes, where braces and commas are literal class members. Consequently [{a,b}] expands to [a] and [b], and rejects the single-character paths {, ,, and } even though they are members of the specified class. This also affects FileSystem.glob for filenames containing these characters. Make the brace scanner respect bracket-class boundaries (including escaped/initial closing brackets), and add a combined class/brace regression test.
There was a problem hiding this comment.
Addressed in b91e486.
Brace scanning now skips bracket classes, including initial closing brackets, and combined class/brace regressions were added. Confirmed literal brace and comma members match.
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
…in classes, and UNC patterns
…eral, and reject . after **
There was a problem hiding this comment.
Review complete. All three earlier blocking threads still apply, and one additional failing regression case was confirmed. Validation used the current dependency-free compiler/matcher extracted into a temporary TypeScript harness; the harness has been removed.
Reviewed at eca5178: 1 blocking, 0 nits.
Disagree? Reply in the finding's thread, or a maintainer can comment /janitor to re-run or dismiss this review.
| { pattern: "**/..", matches: [], rejects: ["..", "a/.."] }, | ||
| { pattern: "/abs/*.ts", matches: ["/abs/a.ts"], rejects: ["abs/a.ts"] }, | ||
| { pattern: "C:/x/*", matches: ["C:/x/a"], rejects: ["x/a"] }, | ||
| { pattern: "//server/share/*", matches: [], rejects: ["//server/share/a", "/server/share/a"] }, |
There was a problem hiding this comment.
This new UNC case fails with the current implementation: match(compile("//server/share/*", false), "/server/share/a") returns true (and so does matching //server/share/a). normalizeSegments collapses the second leading empty segment in both patterns and paths, making this an ordinary absolute pattern rather than rejecting it. Please implement the intended UNC rejection or reconcile the expectation with the intended semantics; as written the new matcher suite cannot pass.
There was a problem hiding this comment.
Addressed in b91e486.
Patterns and paths now reject UNC forms explicitly. Confirmed the original reproduction returns false.
There was a problem hiding this comment.
All four earlier blocking findings are addressed. No new merge-blocking issues found. Validated the updated matcher with the existing Glob test cases and prior reproductions using a dependency-free extracted-source harness; the full Vitest and filesystem suites were not run.
Reviewed at b91e486: no new findings.
| } else if (char === "?") { | ||
| magic = true | ||
| source += "[^/]" | ||
| } else if (char === "[") { |
There was a problem hiding this comment.
Addressed in b91e486.
Replaced segment regex backtracking with bounded token matching and added an adversarial regression test; the extracted matcher completes the reproduction promptly.
| } | ||
| out.push(segment) | ||
| } |
There was a problem hiding this comment.
Addressed in b91e486.
Compilation now rejects dot and dot-dot segments after a globstar before normalization; matcher and shared walker regressions were added.
| i++ | ||
| } else if (char === "{") { | ||
| stack.push({ open: i, commas: [] }) | ||
| } else if (char === ",") { | ||
| stack[stack.length - 1]?.commas.push(i) | ||
| } else if (char === "}") { | ||
| const group = stack.pop() |
There was a problem hiding this comment.
Addressed in b91e486.
Brace scanning now skips bracket classes, including initial closing brackets, and combined class/brace regressions were added. Confirmed literal brace and comma members match.
| { pattern: "**/..", matches: [], rejects: ["..", "a/.."] }, | ||
| { pattern: "/abs/*.ts", matches: ["/abs/a.ts"], rejects: ["abs/a.ts"] }, | ||
| { pattern: "C:/x/*", matches: ["C:/x/a"], rejects: ["x/a"] }, | ||
| { pattern: "//server/share/*", matches: [], rejects: ["//server/share/a", "/server/share/a"] }, |
There was a problem hiding this comment.
Addressed in b91e486.
Patterns and paths now reject UNC forms explicitly. Confirmed the original reproduction returns false.
Dismissed by janitor: no blocking finding stands at b91e486.
…pansion Replaces the regex segment and class matchers with code point tokens and range lists, and uses one class parser for both brace expansion and segment compilation. `x/.` now requires `x` to be a directory, internal helpers are no longer exported, `Glob.isGlob` is removed, and the Bun positional write workaround is reverted so it can land separately.
|
@jpowersdev Heads-up before independent review: the compiler simplification in fe4cff2 was rebased on top of your fixes (4c48a81 and b91e486), not force-pushed over them. It retains your UNC path rejection and triple-slash absolute-path distinction, while replacing per-class regexes and the separate class scanner with code-point tokens and one shared class parser for brace expansion and segment compilation. This keeps class parsing consistent and bounds wildcard matching without regex backtracking. I have now pushed tests-only coverage for UNC paths and directory |
There was a problem hiding this comment.
Reviewed the updated implementation and marked all four earlier threads addressed. Reported two blocking error-propagation issues, confirmed with targeted runtime reproductions. Full targeted test execution was unavailable because frozen dependency installation failed with a registry 403.
Reviewed at 7e5d56e: 2 blocking, 0 nits.
Disagree? Reply in the finding's thread, or a maintainer can comment /janitor to re-run or dismiss this review.
| const exists = (relative: string): Effect.Effect<boolean> => | ||
| Effect.map(Effect.exit(fs.stat(toFsPath(relative))), (exit) => exit._tag === "Success") |
There was a problem hiding this comment.
The synthetic-entry existence probe turns every failed Exit into false, including unexpected filesystem errors and defects. I verified that globbing . with stat failing with Unknown, or dying with "boom", succeeds with [] instead of propagating the failure. This also makes the newly added probing-error and probing-defect tests in FileSystem.test.ts fail. Recover only the intended missing/unreadable cases and preserve other errors and causes.
There was a problem hiding this comment.
Addressed in 49f9ed6.
Addressed by matchEffect with selective recovery; targeted checks confirm unexpected stat errors and defects propagate.
| return type === "Unknown" | ||
| ? Effect.map( | ||
| Effect.exit(fs.readLink(toFsPath(relative))), | ||
| (exit) => ({ relative, accepted, next: exit._tag === "Success" ? followed : next }) |
There was a problem hiding this comment.
Every failed readLink Exit is treated as evidence that the entry is not a symlink, including unexpected errors and defects. With an untyped listing, a directory whose readLink fails with Busy still produces a successful glob result; Effect.die is swallowed as well. Besides failing the new readLink error/defect tests, this can traverse a symlink when link detection fails unexpectedly, undermining globstar cycle prevention. Distinguish the expected not-a-link errors from unexpected failures and propagate the latter, including defects.
There was a problem hiding this comment.
Addressed in 49f9ed6.
Addressed by selective readLink recovery; targeted checks confirm Busy failures and defects propagate.
There was a problem hiding this comment.
All six earlier findings are addressed. Targeted Node checks passed for bounded wildcard matching, globstar dot rejection, class/brace parsing, UNC rejection, and propagation of filesystem errors and defects. No new blocking findings; diff whitespace checks passed.
Reviewed at 49f9ed6: no new findings.
| const exists = (relative: string): Effect.Effect<boolean> => | ||
| Effect.map(Effect.exit(fs.stat(toFsPath(relative))), (exit) => exit._tag === "Success") |
There was a problem hiding this comment.
Addressed in 49f9ed6.
Addressed by matchEffect with selective recovery; targeted checks confirm unexpected stat errors and defects propagate.
| return type === "Unknown" | ||
| ? Effect.map( | ||
| Effect.exit(fs.readLink(toFsPath(relative))), | ||
| (exit) => ({ relative, accepted, next: exit._tag === "Success" ? followed : next }) |
There was a problem hiding this comment.
Addressed in 49f9ed6.
Addressed by selective readLink recovery; targeted checks confirm Busy failures and defects propagate.
Dismissed by janitor: no blocking finding stands at 49f9ed6.
Problem
FileSystem.globgave different results on each runtime because it wrapped each platform's native glob. On one fixture:fs.globexpandGlobroot**/*.ts.hidden/secret.ts.*.env,.hidden[].env,.hiddensrc/**srcsrcsrcThere was also no way to match a path against a pattern without touching a file system.
Changes
effect/Globmodule (@stability experimental):Glob.make(pattern, { dot }),Glob.match, andGlob.escape. Supports*,?, bracket classes (negation, ranges, POSIX classes), nested{a,b},**, trailing/for directories, and\escapes.FileSystem.makederivesglobfromreadDirectory,stat, andreadLinkwhen it is omitted, and the Node and Deno file systems now use that walk instead of their native glob.FileSystem.globgains adotoption and its JSDoc documents the contract.FileSystem.makeaccepts an optionalreadDirectoryEntries(returning the newFileSystem.DirectoryEntry), which Node (readdirwithwithFileTypes) and Deno (Deno.readDir) provide so the walk takes entry types from the listing instead of astatper entry.testGlob), which also run on Node without typed listings.Notable decisions
/-separated and relative toroot(absolute for absolute patterns), visited depth-first with entries sorted by name. Wildcards skip dotfiles unless the segment starts with.ordot: true;dir/**includesdir;**does not traverse symbolic links but a link named by another segment is followed;excludeprunes excluded directories; missing or unreadable directories are empty.x/.is the same asx/;.or..after**and UNC patterns or paths (//server/share) match nothing. Braces and commas inside[...]are class members. Extended glob syntax and leading!negation are not interpreted, but ordinary wildcards still apply within them (for example,?(a)matchesx(a)). Malformed syntax matches literally.\is now always an escape, Deno no longer returns absolute paths or matches dotfiles under**, and Node-only extglobs no longer expand.FileSystem.makegrows by about 2.25 KB; importing only theFileSystemservice ormakeNoopis unchanged (tree-shaken).Benchmarks
Median times on macOS, Node 24, warm cache, 10 runs each; all three return identical file sets.
fs.glob(previous)glob@13**/*.tsinpackages/, ~8k files*/src/**/*.tsinpackages/effect/src/*.ts**/package.jsoninnode_modules/.pnpm, ~56k filesFile systems built with
FileSystem.makewithoutreadDirectoryEntriesfall back to astatper candidate entry and are slower (about 2x native on large trees).Notes
pnpm codegencurrently fails onmain(Effect.ts: missing top-level module JSDoc, since it starts with a triple-slash reference), so theGlobbarrel entry inindex.tswas added by hand in the generated format.C:/) are handled and covered by matcher tests only.glob, and consider a typed listing on theFileSystemservice itself.Closes EFF-2375