Add `Filesystem` underneath `Reader`, simplify tests further. #3860

davidmorgan · 2025-02-14T12:33:48Z

github-actions · 2025-02-14T12:35:42Z

PR Health

Changelog Entry ✔️

Package	Changed Files

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

jensjoha · 2025-02-17T14:34:30Z

build/lib/src/state/filesystem.dart

+/// Methods behave as the `dart:io` methods with the same names, with some
+/// exceptions noted.
+abstract interface class Filesystem {
+  Future<bool> exists(AssetId id);


A existsSync wouldn't be useful? Or sync versions of read? Are there any "rules" for when calling the sync version or not?

It certainly will :) in this PR I just added what's already used.

The generator-facing interfaces, AssetReader/AssetWriter, are only async. We can add sync methods but we can't move existing async use to sync.

But for build_runner internal use we can switch things to sync and it's probably better--benchmarking showed it gives about a 2x speedup. So one of the goals of this refactoring is to give build_runner internals the choice.

jensjoha · 2025-02-18T07:54:07Z

build/lib/src/state/reader_state.dart

+  ///
+  /// Warning: this access to the filesystem bypasses reader functionality
+  /// such as read tracking, caching and visibility restriction.
+  Filesystem get filesystem;


Does this need to be on the interface?
It - at least mostly - seems to be used on the InMemoryAssetReaderWriter.
And it generally seems like something that shouldn't be used (having a warning and all) - are there other use cases than testing? If not, should it perhaps also be called something else? (e.g. filesystemForTesting or something).

You are correct that only test code uses this for now.

That's because non-test code was restricted to using the Reader interface, which hides the underlying files.

Each generator has a different view on the filesystem, with some set of files hidden because they are "generated after" the current generator runs. Currently the import graph computation deals with that by checking the whole thing for each generator.

Through this refactoring build_runner internals are allowed to bypass that abstraction and see the actual filesystem; in the next PRs they will also get access to information about what is different about the current generator's view.

Then they can be fast :) by building the base state once then applying the diff for the current generator on top.

jensjoha · 2025-02-18T08:08:41Z

build_runner_core/lib/src/asset/file_based.dart

-    });
-  }
+          {Encoding encoding = utf8}) =>
+      filesystem.writeAsString(id, contents);


Shouldn't you pass along the encoding as well?
Or not have options for enconding and always use utf8...

Done, thanks.

This is from the generator-facing AssetWriter interface, so no choice about the signature :)

jensjoha · 2025-02-18T08:25:52Z

build_test/lib/src/in_memory_reader.dart

 /// An implementation of [AssetReader] and [AssetWriter] with primed in-memory
 /// assets.
+///
+/// TODO(davidmorgan): merge into `FileBasedReader` and `FileBasedWriter`.


Why? Memory based and file based seems at odds.

I think it will end up as one class, with a different Filesystem implementation for in-memory and file based.

Once all the optional functionality is refactored from inheritance hierarchy to members like AssetFinder, it should be possible to get rid of AssetReaderState, get rid of delegation, and have one concrete AssetReaderWriter with all the interesting/optional functionality provided via those members.

The key thing allowing this refactoring is that the inheritance hierarchy / delegates are always built so they run in the same order--so for example caching always happens underneath per-generator visibility restriction. So actually that can just be "do I have visibility restriction, if so apply it, do I have a cache, if so use it" and so on :)

jensjoha · 2025-02-18T08:30:01Z

build_test/lib/src/in_memory_reader.dart

  @override
  Future writeAsBytes(AssetId id, List<int> bytes) async {
-    assets[id] = bytes;
+    assets[id] = Uint8List.fromList(bytes);


maybe bytes is Uint8List ? bytes : Uint8List.fromList(bytes);?
In general I think the interface should be changed, but probably not in this cl...

Done, thanks.

This is generator-facing API so we can't easily change it.

davidmorgan

Thanks :)

Some quite detailed responses about where this is headed--hopefully they make sense, happy to elaborate / discuss as needed. Thanks!

davidmorgan · 2025-02-18T08:45:53Z

build/lib/src/state/filesystem.dart

+/// Methods behave as the `dart:io` methods with the same names, with some
+/// exceptions noted.
+abstract interface class Filesystem {
+  Future<bool> exists(AssetId id);


It certainly will :) in this PR I just added what's already used.

The generator-facing interfaces, AssetReader/AssetWriter, are only async. We can add sync methods but we can't move existing async use to sync.

But for build_runner internal use we can switch things to sync and it's probably better--benchmarking showed it gives about a 2x speedup. So one of the goals of this refactoring is to give build_runner internals the choice.

davidmorgan · 2025-02-18T08:59:16Z

build/lib/src/state/reader_state.dart

+  ///
+  /// Warning: this access to the filesystem bypasses reader functionality
+  /// such as read tracking, caching and visibility restriction.
+  Filesystem get filesystem;


You are correct that only test code uses this for now.

That's because non-test code was restricted to using the Reader interface, which hides the underlying files.

Each generator has a different view on the filesystem, with some set of files hidden because they are "generated after" the current generator runs. Currently the import graph computation deals with that by checking the whole thing for each generator.

Through this refactoring build_runner internals are allowed to bypass that abstraction and see the actual filesystem; in the next PRs they will also get access to information about what is different about the current generator's view.

Then they can be fast :) by building the base state once then applying the diff for the current generator on top.

davidmorgan · 2025-02-18T09:00:16Z

build_runner_core/lib/src/asset/file_based.dart

-    });
-  }
+          {Encoding encoding = utf8}) =>
+      filesystem.writeAsString(id, contents);


Done, thanks.

This is from the generator-facing AssetWriter interface, so no choice about the signature :)

davidmorgan · 2025-02-18T09:07:17Z

build_test/lib/src/in_memory_reader.dart

 /// An implementation of [AssetReader] and [AssetWriter] with primed in-memory
 /// assets.
+///
+/// TODO(davidmorgan): merge into `FileBasedReader` and `FileBasedWriter`.


I think it will end up as one class, with a different Filesystem implementation for in-memory and file based.

Once all the optional functionality is refactored from inheritance hierarchy to members like AssetFinder, it should be possible to get rid of AssetReaderState, get rid of delegation, and have one concrete AssetReaderWriter with all the interesting/optional functionality provided via those members.

The key thing allowing this refactoring is that the inheritance hierarchy / delegates are always built so they run in the same order--so for example caching always happens underneath per-generator visibility restriction. So actually that can just be "do I have visibility restriction, if so apply it, do I have a cache, if so use it" and so on :)

davidmorgan · 2025-02-18T09:08:23Z

build_test/lib/src/in_memory_reader.dart

  @override
  Future writeAsBytes(AssetId id, List<int> bytes) async {
-    assets[id] = bytes;
+    assets[id] = Uint8List.fromList(bytes);


Done, thanks.

This is generator-facing API so we can't easily change it.

davidmorgan force-pushed the reader-filesystem branch 4 times, most recently from 6bce827 to 8d83144 Compare February 17, 2025 12:54

Add Filesystem underneath Reader, simplify tests further.

55c36f0

davidmorgan force-pushed the reader-filesystem branch from 8d83144 to 55c36f0 Compare February 17, 2025 13:13

davidmorgan marked this pull request as ready for review February 17, 2025 13:29

davidmorgan requested a review from jensjoha February 17, 2025 13:29

davidmorgan mentioned this pull request Feb 17, 2025

Simplify test readers and writers #3865

Merged

jensjoha approved these changes Feb 18, 2025

View reviewed changes

Address review comments.

2ca3750

davidmorgan commented Feb 18, 2025

View reviewed changes

davidmorgan added 2 commits February 19, 2025 11:57

Merge branch 'master' into reader-filesystem

e708cb8

Fix after merge from master.

28fd288

davidmorgan merged commit e6c9b0f into dart-lang:master Feb 19, 2025
75 checks passed

davidmorgan deleted the reader-filesystem branch February 19, 2025 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Filesystem` underneath `Reader`, simplify tests further. #3860

Add `Filesystem` underneath `Reader`, simplify tests further. #3860

davidmorgan commented Feb 14, 2025 •

edited

Loading

github-actions bot commented Feb 14, 2025 •

edited

Loading

jensjoha Feb 17, 2025

davidmorgan Feb 18, 2025

jensjoha Feb 18, 2025

davidmorgan Feb 18, 2025

jensjoha Feb 18, 2025

davidmorgan Feb 18, 2025

jensjoha Feb 18, 2025

davidmorgan Feb 18, 2025

jensjoha Feb 18, 2025

davidmorgan Feb 18, 2025

davidmorgan left a comment

davidmorgan Feb 18, 2025

davidmorgan Feb 18, 2025

davidmorgan Feb 18, 2025

davidmorgan Feb 18, 2025

davidmorgan Feb 18, 2025

Add Filesystem underneath Reader, simplify tests further. #3860

Add Filesystem underneath Reader, simplify tests further. #3860

Conversation

davidmorgan commented Feb 14, 2025 • edited Loading

github-actions bot commented Feb 14, 2025 • edited Loading

PR Health

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `Filesystem` underneath `Reader`, simplify tests further. #3860

Add `Filesystem` underneath `Reader`, simplify tests further. #3860

davidmorgan commented Feb 14, 2025 •

edited

Loading

github-actions bot commented Feb 14, 2025 •

edited

Loading