Nullability of readNode in crawlAsync

[simolus3]

In 6adde6d, the return type of readNode in crawlAsync was changed to a potentially non-nullable FutureOr<V>. Quoting the documentation of crawlAsync:

/// If [readNode] returns null for any key it will be ignored from the rest of
/// the graph. 

Should we make this a FutureOr<V?> Function(K) then? Or are users supposed to provide a nullable V and then filter out nulls in the end? If that's the intention, maybe the documentation needs some tweaking. Note that a nullable V wouldn't really work, since null values are still not added to the result.

cc @natebosch

[natebosch]

Good find. We should either go with V extends Object with FutureOr<V?> or we should change the documentation and avoid ignoring the nulls.

[simolus3]

Any preferences? In build_resolvers we're returning null to ignore assets from a later build phase, so I'm leaning towards the non-nullable V approach to keep that simple. IMO it also reflects the pre-nnbd intentions better.

A downside is that adding an upper bound for V might be considered a breaking change. But so is not ignoring nulls...

[natebosch]

A downside is that adding an upper bound for V might be considered a breaking change. But so is not ignoring nulls...

Yeah, unfortunately I think we're going to need to publish as a breaking release either way. I don't think this should be a huge problem.

Any preferences? In build_resolvers we're returning null to ignore assets from a later build phase, so I'm leaning towards the non-nullable V approach to keep that simple. IMO it also reflects the pre-nnbd intentions better.

I think you are correct that V extends Object reflects the pre-nnbd intentions better. It would also be less breaking than changing the semantics. However, my gut is that with the additional expressiveness we gain with null safety it is worth changing the semantics as well. If we do, we'd leave it up to the caller to decide if null should be accepted. At least that is the way I'd first write the API in a null safe world. It might be worth trying both approaches and seeing how the calling code feels to get a better idea - I'm going to try both out now.

cc @jakemac53

[natebosch]

I wanted to evaluate the impact of the behavioral breaking change. The diff in build_resolvers doesn't seem too bad.

diff --git a/build_resolvers/lib/src/build_asset_uri_resolver.dart b/build_resolvers/lib/src/build_asset_uri_resolver.dart
index cc33a9aa..b474f32a 100644
--- a/build_resolvers/lib/src/build_asset_uri_resolver.dart
+++ b/build_resolvers/lib/src/build_asset_uri_resolver.dart
@@ -95,16 +95,15 @@ class BuildAssetUriResolver extends UriResolver {
       {Set<AssetId> /*?*/ transitivelyResolved}) async {
     final path = assetPath(id);
     if (!await buildStep.canRead(id)) {
-      if (globallySeenAssets.contains(id)) {
-        // ignore from this graph, some later build step may still be using it
-        // so it shouldn't be removed from [resourceProvider], but we also
-        // don't care about it's transitive imports.
-        return null;
-      }
-      _cachedAssetDependencies.remove(id);
-      _cachedAssetDigests.remove(id);
-      if (resourceProvider.getFile(path).exists) {
-        resourceProvider.deleteFile(path);
+      if (!globallySeenAssets.contains(id)) {
+        // No other build step is using this, so it should be removed.
+        // If this asset was globally seens some other step may be using it so
+        // it won't be removed, but we don't care about it's transitive imports.
+        _cachedAssetDependencies.remove(id);
+        _cachedAssetDigests.remove(id);
+        if (resourceProvider.getFile(path).exists) {
+          resourceProvider.deleteFile(path);
+        }
       }
       return _AssetState(path, const []);
     }

In build_modules the part I don't like is the need to import stream_transform to get whereType. That will be a little bit better once it's in package:async/stream_transform, but still it isn't very nice.

diff --git a/build_modules/lib/src/kernel_builder.dart b/build_modules/lib/src/kernel_builder.dart
index 0bedfe6d..0c08e4fd 100644
--- a/build_modules/lib/src/kernel_builder.dart
+++ b/build_modules/lib/src/kernel_builder.dart
@@ -16,6 +16,7 @@ import 'package:graphs/graphs.dart' show crawlAsync;
 import 'package:meta/meta.dart';
 import 'package:path/path.dart' as p;
 import 'package:scratch_space/scratch_space.dart';
+import 'package:stream_transform/stream_transform.dart';
 
 import 'errors.dart';
 import 'module_builder.dart';
@@ -305,7 +306,7 @@ Future<void> _findModuleDeps(
 Future<List<Module>> _resolveTransitiveModules(
     Module root, BuildStep buildStep) async {
   var missing = <AssetId>{};
-  var modules = await crawlAsync<AssetId, Module>(
+  var modules = await crawlAsync<AssetId, Module /*?*/>(
           [root.primarySource],
           (id) => buildStep.fetchResource(moduleCache).then((c) async {
                 var moduleId =
@@ -318,8 +319,9 @@ Future<List<Module>> _resolveTransitiveModules(
                 }
                 return module;
               }),
-          (id, module) => module.directDependencies)
+          (id, module) => module?.directDependencies ?? [])
       .skip(1) // Skip the root.
+      .whereType<Module>()
       .toList();
 
   if (missing.isNotEmpty) {

On the whole, I'm in leaning slightly in favor of pushing this handling of null explicitly into the clients, but I could be convinced otherwise. It feels more consistent to me that readNode avoids special behavior around a null return, in the same way that edges does, and it also feels somewhat "fair" to me that the build_modules use case has some complexity around the missing case. On the other hand It's a little simpler at both the typical and somewhat more tricky call site to let null be meaningful to the algorithm. I can't really think of a good reason that null should meaningful to the caller in a different way than it's meaningful to this algorithm, so I could see an argument for keeping the same behavior. It's also easier from a breaking change rollout perspective.

[jakemac53]

I don't have a strong opinion either way here, although if we are doing another breaking change anyways I would probably lean towards just removing the handling of null inside the algorithm. It seems like we probably could land the build_resolvers and build_modules changes ahead of time making the transition relatively smooth?