[glob] Make `listFileSystemFiles` check the context of the file system. by lrhn · Pull Request #2564 · dart-lang/tools

lrhn · 2026-08-28T14:42:51Z

Fixes #1998

Makes the ListTree class use the fileSystem.path path context instead of the platform context, p.context of package:path.
Calls to ListTree checks that the glob's path context and the file system's path context are the same style.

github-actions · 2026-08-28T14:43:37Z

Package publishing

If you have publishing permissions, you can use the links below to publish the changes after merging this PR.

Package	Version	Status	Publish tag (post-merge)
package:glob ⭐	2.2.1-wip	WIP (no publish necessary)
package:test_reflective_loader	0.6.0	ready to publish	`test_reflective_loader-v0.6.0`

24 already published.
16 WIP (no publish necessary).

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

github-actions · 2026-08-28T14:44:27Z

PR Health

Unused Dependencies ✔️

Package	Status
glob	✔️ All dependencies utilized correctly.

For details on how to fix these, see dependency_validator.

This check can be disabled by tagging the PR with skip-unused-dependencies-check.

Breaking changes ✔️

Package	Change	Current Version	New Version	Needed Version	Looking good?
glob	None	2.2.0	2.2.1-wip	2.2.1-wip	✔️

This check can be disabled by tagging the PR with skip-breaking-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package	Leaked API symbol	Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

Changelog Entry ✔️

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

License Headers ✔️

// Copyright (c) 2026, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/html/test/differential_test.dart
pkgs/html/test/dom_parsing_test.dart
pkgs/html/test/mxss_test.dart
pkgs/html/test/noah_ark_clause_limit_test.dart

This check can be disabled by tagging the PR with skip-license-check.

Coverage

⚠️

File	Coverage
pkgs/glob/lib/glob.dart	💚 98 % ⬆️ 0 %
pkgs/glob/lib/src/list_tree.dart	💔 95 % ⬇️ 1 %

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

gemini-code-assist

Code Review

This pull request updates the glob package to use the file system's path context instead of the platform's path context when listing files. The review feedback identifies a critical bug where text.replaceAll is called as a no-op on an immutable string, and p.isAbsolute is incorrectly used instead of fileSystem.path.isAbsolute. Additionally, the reviewer points out that the new tests are false positives because they do not fully consume the stream or populate the mock file system, and suggests cleaner string formatting for error messages to improve readability.

gemini-code-assist · 2026-08-28T14:45:25Z

+        if (fileSystem.path.style == p.windows.style)
+          text.replaceAll('/', r'\');
        if (p.isAbsolute(text)) {


The call to text.replaceAll is a no-op because strings in Dart are immutable. The returned string is discarded, leaving text unchanged. It must be assigned back to text.

Additionally, p.isAbsolute(text) on line 94 uses the platform-specific path context p instead of the file system's path context fileSystem.path.isAbsolute(text). When the file system style differs from the platform style (e.g., Windows style on a Posix platform), this check will return incorrect results.

Suggested change

if (fileSystem.path.style == p.windows.style)

text.replaceAll('/', r'\');

if (p.isAbsolute(text)) {

if (fileSystem.path.style == p.windows.style) {

text = text.replaceAll('/', r'\');

}

if (fileSystem.path.isAbsolute(text)) {

References

In Dart, a raw string literal can end with a backslash (e.g., r'') without causing an unterminated string literal error. The backslash is treated as a literal character, not an escape for the closing quote.

gemini-code-assist · 2026-08-28T14:45:25Z

+    test("does not fail if the fileSystem and glob contexts match", () {
+      // Shouldn't fail even if neither is the platform context.
+      Glob('*', context: p.posix)
+          .listFileSystem(MemoryFileSystem(style: FileSystemStyle.posix));
+      Glob('*', context: p.windows)
+          .listFileSystem(MemoryFileSystem(style: FileSystemStyle.windows));
    });


This test passes, but it is currently a false positive. Because listFileSystem returns a lazy Stream, and the test never listens to or awaits the stream (e.g., by calling .toList()), the actual path listing code is never executed.

If you listen to the stream, the test will fail on platforms with a different path style because _ListTreeNode.list still uses the platform-specific p context (like p.relative, p.join) instead of fileSystem.path.

To make this test robust and catch actual runtime failures, you should populate the memory file system with a file and fully consume the stream using await ...toList().

test("does not fail if the fileSystem and glob contexts match", () async { // Shouldn't fail even if neither is the platform context. var posixFs = MemoryFileSystem(style: FileSystemStyle.posix); posixFs.file('/foo').createSync(recursive: true); await Glob('/*', context: p.posix) .listFileSystem(posixFs) .toList(); var windowsFs = MemoryFileSystem(style: FileSystemStyle.windows); windowsFs.file('C:\\foo').createSync(recursive: true); await Glob('C:\\*', context: p.windows) .listFileSystem(windowsFs) .toList(); });

gemini-code-assist · 2026-08-28T14:45:25Z

+    test("does not fail if the fileSystem and glob contexts match", () {
+      // Shouldn't fail even if neither is the platform context.
+      Glob('*', context: p.posix)
+          .listFileSystemSync(MemoryFileSystem(style: FileSystemStyle.posix));
+      Glob('*', context: p.windows)
+          .listFileSystemSync(MemoryFileSystem(style: FileSystemStyle.windows));
    });


Similar to the async test, this synchronous test passes only because the MemoryFileSystem is completely empty. When the file system is empty, listSync returns early without executing the path manipulation code (which still incorrectly uses the platform-specific p context).

To ensure this test is actually validating cross-platform compatibility, populate the file system with a file so that the path traversal and matching logic are executed.

test("does not fail if the fileSystem and glob contexts match", () { // Shouldn't fail even if neither is the platform context. var posixFs = MemoryFileSystem(style: FileSystemStyle.posix); posixFs.file('/foo').createSync(recursive: true); Glob('/*', context: p.posix).listFileSystemSync(posixFs); var windowsFs = MemoryFileSystem(style: FileSystemStyle.windows); windowsFs.file('C:\\foo').createSync(recursive: true); Glob('C:\\*', context: p.windows).listFileSystemSync(windowsFs); });

gemini-code-assist · 2026-08-28T14:45:25Z

+      throw ArgumentError.value(
+          fileSystem,
+          'fileSystem',
+          '''Can't list using glob \"$this\"; it matches '''
+              '${context.style} paths, but the file system uses '
+              '${fileSystem.path.style} paths.');


The string literal concatenation here uses an inconsistent mix of triple-quotes ''' and single quotes ', and unnecessarily escapes the double quotes \" inside the triple-quotes.

To adhere to the repository style guide's philosophy of "Optimize for readability", we should use a consistent and cleaner string quoting style. Since the string contains single quotes (Can't), we can use double quotes for the first segment and single quotes for the rest, avoiding any triple-quotes or unnecessary escaping.

Suggested change

throw ArgumentError.value(

fileSystem,

'fileSystem',

'''Can't list using glob \"$this\"; it matches '''

'${context.style} paths, but the file system uses '

'${fileSystem.path.style} paths.');

throw ArgumentError.value(

fileSystem,

'fileSystem',

"Can't list using glob \"$this\"; it matches "

'${context.style} paths, but the file system uses '

'${fileSystem.path.style} paths.');

References

Optimize for readability - code is read more often than it is written. ^(link)

gemini-code-assist · 2026-08-28T14:45:25Z

+      throw ArgumentError.value(
+          fileSystem,
+          'fileSystem',
+          '''Can't list using glob \"$this\"; it matches '''
+              '${context.style} paths, but the file system uses '
+              '${fileSystem.path.style} paths.');


The string literal concatenation here uses an inconsistent mix of triple-quotes ''' and single quotes ', and unnecessarily escapes the double quotes \" inside the triple-quotes.

To adhere to the repository style guide's philosophy of "Optimize for readability", we should use a consistent and cleaner string quoting style. Since the string contains single quotes (Can't), we can use double quotes for the first segment and single quotes for the rest, avoiding any triple-quotes or unnecessary escaping.

Suggested change

throw ArgumentError.value(

fileSystem,

'fileSystem',

'''Can't list using glob \"$this\"; it matches '''

'${context.style} paths, but the file system uses '

'${fileSystem.path.style} paths.');

throw ArgumentError.value(

fileSystem,

'fileSystem',

"Can't list using glob \"$this\"; it matches "

'${context.style} paths, but the file system uses '

'${fileSystem.path.style} paths.');

References

Optimize for readability - code is read more often than it is written. ^(link)

natebosch · 2026-08-29T00:00:57Z

+        if (fileSystem.path.style == p.windows.style)
+          text.replaceAll('/', r'\');


Suggested change

if (fileSystem.path.style == p.windows.style)

text.replaceAll('/', r'\');

This was always a no-op, and always unnecessary. This replacement wouldn't impact p.isAbsolute result.

Varadraj75 · 2026-09-15T20:13:03Z

This fixes the guard and the split, but the node listing methods still use the platform context: p.join and p.relative in list, listSync and both _validateIntermediateChildren, plus toPosixPath(p.context, ...) in _matches and p.isAbsolute in the factory. They already receive the FileSystem, so they can use fileSystem.path the same way the split now does. Otherwise a windows-context glob over a windows-style MemoryFileSystem on a posix host passes the new check and then joins with /.

Also text.replaceAll('/', r'\') still discards its result, so the line is a no-op. Did you mean text = text.replaceAll(...)?

Happy to send a follow-up for the node methods once this lands, or fold it in here if you prefer.

Make listFileSystemFiles check the context of the file system.

4f8d6c9

lrhn requested a review from a team as a code owner August 28, 2026 14:42

github-actions Bot added the package:glob label Aug 28, 2026

gemini-code-assist Bot reviewed Aug 28, 2026

View reviewed changes

Merge branch 'main' into path-context-glob

ea515d7

natebosch approved these changes Aug 29, 2026

View reviewed changes

Navigation Menu

Repository navigation

[glob] Make `listFileSystemFiles` check the context of the file system. - #2564

[glob] Make `listFileSystemFiles` check the context of the file system.#2564
lrhn wants to merge 2 commits into
mainfrom
path-context-glob

lrhn commented Aug 28, 2026

Uh oh!

github-actions Bot commented Aug 28, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 28, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Aug 28, 2026

Uh oh!

gemini-code-assist Bot Aug 28, 2026

Uh oh!

gemini-code-assist Bot Aug 28, 2026

Uh oh!

gemini-code-assist Bot Aug 28, 2026

Uh oh!

gemini-code-assist Bot Aug 28, 2026

Uh oh!

natebosch Aug 29, 2026

Uh oh!

Varadraj75 commented Sep 15, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		if (fileSystem.path.style == p.windows.style)
		text.replaceAll('/', r'\');

Conversation

lrhn commented Aug 28, 2026

Uh oh!

github-actions Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Package publishing

Uh oh!

github-actions Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Health

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

natebosch Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

Varadraj75 commented Sep 15, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Aug 28, 2026 •

edited

Loading

github-actions Bot commented Aug 28, 2026 •

edited

Loading