More inputs invalidation test; fix for deletions (#3987)

* Add invalidation test coverage for other inputs.

* Fix invalidation when a non-primary input is deleted.

Author: davidmorgan

build_runner/test/generate/watch_test.dart

@@ -361,6 +361,10 @@ void main() {
           readerWriter,
         );
 
+        var aTxtId = makeAssetId('a|web/a.txt');
+        var aTxtNode = AssetNode.missingSource(aTxtId);
+        var aTxtCopyId = makeAssetId('a|web/a.txt.copy');
+        var aTxtCopyNode = AssetNode.missingSource(aTxtCopyId);
         var bCopyId = makeAssetId('a|web/b.txt.copy');
         var bTxtId = makeAssetId('a|web/b.txt');
         var bCopyNode = AssetNode.generated(
@@ -374,7 +378,10 @@ void main() {
           inputs: [makeAssetId('a|web/b.txt')],
           isHidden: false,
         );
+
         expectedGraph
+          ..add(aTxtNode)
+          ..add(aTxtCopyNode)
           ..add(bCopyNode)
           ..add(
             makeAssetNode('a|web/b.txt', [

build_runner_core/lib/src/asset_graph/graph.dart

@@ -201,8 +201,6 @@ class AssetGraph implements GeneratedAssetHider {
     var existing = get(node.id);
     if (existing != null) {
       if (existing.type == NodeType.missingSource) {
-        // Don't call _removeRecursive, that recursively removes all transitive
-        // primary outputs. We only want to remove this node.
         _nodesByPackage[existing.id.package]!.remove(existing.id.path);
         node = node.rebuild((b) {
           b.primaryOutputs.addAll(existing.primaryOutputs);
@@ -263,60 +261,23 @@ class AssetGraph implements GeneratedAssetHider {
     );
   }
 
-  /// Removes the node representing [id] from the graph, and all of its
-  /// `primaryOutput`s.
-  ///
-  /// Also removes all edges between all removed nodes and remaining nodes.
-  ///
-  /// Returns the IDs of removed asset nodes.
-  Set<AssetId> _removeRecursive(AssetId id, {Set<AssetId>? removedIds}) {
+  /// Changes [id] and its transitive`primaryOutput`s to `missingSource` nodes.
+  void _setMissingRecursive(AssetId id, {Set<AssetId>? removedIds}) {
     removedIds ??= <AssetId>{};
     var node = get(id);
-    if (node == null) return removedIds;
+    if (node == null) return;
     removedIds.add(id);
     for (var output in node.primaryOutputs.toList()) {
-      _removeRecursive(output, removedIds: removedIds);
-    }
-    final outputs = computeOutputs();
-    for (var output in (outputs[node.id] ?? const <AssetId>{})) {
-      updateNodeIfPresent(output, (nodeBuilder) {
-        if (nodeBuilder.type == NodeType.generated) {
-          nodeBuilder.generatedNodeState.inputs.remove(id);
-        } else if (nodeBuilder.type == NodeType.glob) {
-          nodeBuilder.globNodeState
-            ..inputs.remove(id)
-            ..results.remove(id);
-        }
-      });
-    }
-
-    if (node.type == NodeType.generated) {
-      for (var input in node.generatedNodeState!.inputs) {
-        // We may have already removed this node entirely.
-        updateNodeIfPresent(input, (nodeBuilder) {
-          nodeBuilder.primaryOutputs.remove(id);
-        });
-      }
-    } else if (node.type == NodeType.glob) {
-      for (var input in node.globNodeState!.inputs) {
-        // We may have already removed this node entirely.
-        updateNodeIfPresent(input, (nodeBuilder) {
-          nodeBuilder.primaryOutputs.remove(id);
-        });
-      }
-    }
-
-    // Missing source nodes need to be kept to retain dependency tracking.
-    if (node.type != NodeType.missingSource) {
-      _nodesByPackage[id.package]!.remove(id.path);
+      _setMissingRecursive(output, removedIds: removedIds);
     }
+    updateNode(id, (nodeBuilder) {
+      nodeBuilder.replace(AssetNode.missingSource(id));
+    });
 
     // Remove post build action applications with removed assets as inputs.
     for (final packageOutputs in _postProcessBuildStepOutputs.values) {
       packageOutputs.removeWhere((id, _) => removedIds!.contains(id.input));
     }
-
-    return removedIds;
   }
 
   /// Computes node outputs: the inverse of the graph described by the `inputs`
@@ -401,8 +362,12 @@ class AssetGraph implements GeneratedAssetHider {
   /// Updates graph structure, invalidating and deleting any outputs that were
   /// affected.
   ///
-  /// Returns the list of [AssetId]s that were invalidated.
-  Future<Set<AssetId>> updateAndInvalidate(
+  /// Outputs that are deleted from the filesystem are retained in the graph as
+  /// `missingSource` nodes.
+  ///
+  /// Returns the set of [AssetId]s that were deleted, and the set that was
+  /// invalidated.
+  Future<(Set<AssetId>, Set<AssetId>)> updateAndInvalidate(
     BuildPhases buildPhases,
     Map<AssetId, ChangeType> updates,
     String rootPackage,
@@ -572,23 +537,22 @@ class AssetGraph implements GeneratedAssetHider {
       }
     }
 
-    // Delete all the invalidated assets, then remove them from the graph. This
-    // order is important because some `AssetWriter`s throw if the id is not in
-    // the graph.
+    // Delete all the invalidated assets.
     await Future.wait(idsToDelete.map(delete));
 
-    // Remove all deleted source assets from the graph, which also recursively
-    // removes all their primary outputs.
+    // Change deleted source assets and their transitive primary outputs to
+    // `missingSource` nodes, rather than deleting them. This allows them to
+    // remain referenced in `inputs` in order to trigger rebuilds if necessary.
     for (final id in removeIds) {
       final node = get(id);
       if (node != null && node.type == NodeType.source) {
         invalidateNodeAndDeps(id);
-        _removeRecursive(id);
+        _setMissingRecursive(id);
       }
     }
 
     _outputs = null;
-    return invalidatedIds;
+    return (idsToDelete, invalidatedIds);
   }
 
   /// Crawl up primary inputs to see if the original Source file matches the
@@ -713,9 +677,9 @@ class AssetGraph implements GeneratedAssetHider {
   ///
   /// If there are existing [AssetNode.source]s or [AssetNode.missingSource]s
   /// that overlap the [AssetNode.generated]s, then they will be replaced with
-  /// [AssetNode.generated]s, and all their `primaryOutputs` will be removed
-  /// from from the graph as well. The return value is the set of assets that
-  /// were removed from the graph.
+  /// [AssetNode.generated]s, and their transitive `primaryOutputs` will be
+  /// changed to `missingSource` nodes. The return value is the set of assets
+  /// that were removed from the graph.
   Set<AssetId> _addGeneratedOutputs(
     Iterable<AssetId> outputs,
     int phaseNumber,
@@ -745,7 +709,7 @@ class AssetGraph implements GeneratedAssetHider {
             buildPhases.inBuildPhases[phaseNumber].builderLabel,
           );
         }
-        _removeRecursive(output, removedIds: removed);
+        _setMissingRecursive(output, removedIds: removed);
       }
 
       var newNode = AssetNode.generated(
@@ -769,9 +733,16 @@ class AssetGraph implements GeneratedAssetHider {
   @override
   String toString() => allNodes.toList().toString();
 
-  // TODO remove once tests are updated
   void add(AssetNode node) => _add(node);
-  Set<AssetId> remove(AssetId id) => _removeRecursive(id);
+
+  /// Removes a generated node that was output by a post process build step.
+  /// TODO(davidmorgan): look at removing them from the graph altogether.
+  void removePostProcessOutput(AssetId id) {
+    _nodesByPackage[id.package]!.remove(id.path);
+  }
+
+  void removeForTest(AssetId id) =>
+      _nodesByPackage[id.package]?.remove(id.path);
 
   /// Adds [input] to all [outputs] if they track inputs.
   ///

build_runner_core/lib/src/generate/build.dart

@@ -77,6 +77,12 @@ class Build {
   /// Filled from the `updates` passed in to the build.
   final Set<AssetId> changedInputs = {};
 
+  /// Assets that were deleted since the last build.
+  ///
+  /// This is used to distinguish between `missingSource` nodes that were
+  /// already missing and `missingSource` nodes that are newly missing.
+  final Set<AssetId> deletedAssets = {};
+
   /// Outputs that changed since the last build.
   ///
   /// Filled during the build as each output is produced and its digest is
@@ -169,14 +175,23 @@ class Build {
 
   Future<void> _updateAssetGraph(Map<AssetId, ChangeType> updates) async {
     await logTimedAsync(_logger, 'Updating asset graph', () async {
-      changedInputs.addAll(updates.keys.toSet());
-      var invalidated = await assetGraph.updateAndInvalidate(
+      changedInputs.clear();
+      deletedAssets.clear();
+      for (final update in updates.entries) {
+        if (update.value == ChangeType.REMOVE) {
+          deletedAssets.add(update.key);
+        } else {
+          changedInputs.add(update.key);
+        }
+      }
+      final (deleted, invalidated) = await assetGraph.updateAndInvalidate(
         buildPhases,
         updates,
         options.packageGraph.root.name,
         _delete,
         readerWriter,
       );
+      deletedAssets.addAll(deleted);
       await readerWriter.cache.invalidate(invalidated);
     });
   }
@@ -626,7 +641,7 @@ class Build {
     );
     await _cleanUpStaleOutputs(existingOutputs);
     for (final output in existingOutputs) {
-      assetGraph.remove(output);
+      assetGraph.removePostProcessOutput(output);
     }
     assetGraph.updateNode(inputNode.id, (nodeBuilder) {
       nodeBuilder.deletedBy.remove(postProcessBuildStepId);
@@ -842,6 +857,16 @@ class Build {
           }
           return true;
         }
+      } else if (node.type == NodeType.missingSource) {
+        if (deletedAssets.contains(input)) {
+          if (logFine) {
+            _logger.fine(
+              'Build ${renderer.build(forInput, outputs)} because '
+              '${renderer.id(input)} was deleted.',
+            );
+          }
+          return true;
+        }
       }
     }
 

build_runner_core/test/asset_graph/graph_test.dart

@@ -71,8 +71,8 @@ void main() {
           nodes.add(testAddNode());
         }
         graph
-          ..remove(nodes[1].id)
-          ..remove(nodes[4].id);
+          ..removeForTest(nodes[1].id)
+          ..removeForTest(nodes[4].id);
 
         expectNodeExists(nodes[0]);
         expectNodeDoesNotExist(nodes[1]);
@@ -82,7 +82,7 @@ void main() {
 
         graph
           // Doesn't throw.
-          ..remove(nodes[1].id)
+          ..removeForTest(nodes[1].id)
           // Can be added back
           ..add(nodes[1]);
         expectNodeExists(nodes[1]);
@@ -306,8 +306,8 @@ void main() {
             (id) async => deletes.add(id),
             digestReader,
           );
-          expect(graph.contains(primaryInputId), isFalse);
-          expect(graph.contains(primaryOutputId), isFalse);
+          expect(graph.get(primaryInputId)!.type, NodeType.missingSource);
+          expect(graph.get(primaryOutputId)!.type, NodeType.missingSource);
           expect(deletes, equals([primaryOutputId]));
           expect(graph.postProcessBuildStepIds(package: 'foo'), isEmpty);
         });
@@ -414,8 +414,8 @@ void main() {
               digestReader,
             );
 
-            expect(graph.contains(primaryInputId), isFalse);
-            expect(graph.contains(primaryOutputId), isFalse);
+            expect(graph.get(primaryInputId)!.type, NodeType.missingSource);
+            expect(graph.get(primaryOutputId)!.type, NodeType.missingSource);
             expect(
               graph.computeOutputs()[secondaryId] ?? const <AssetId>{},
               isNot(contains(primaryOutputId)),
@@ -513,14 +513,6 @@ void main() {
             expect(globNode.globNodeState!.inputs, contains(coolAssetId));
             expect(globNode.globNodeState!.results, contains(coolAssetId));
             await checkChangeType(ChangeType.REMOVE);
-            expect(
-              globNode.globNodeState!.inputs,
-              isNot(contains(coolAssetId)),
-            );
-            expect(
-              globNode.globNodeState!.results,
-              isNot(contains(coolAssetId)),
-            );
           },
         );
       });
@@ -678,16 +670,25 @@ void main() {
             ..inputs.add(outputReadingNode);
         });
 
-        final invalidatedNodes = await graph.updateAndInvalidate(
+        await graph.updateAndInvalidate(
           buildPhases,
           {makeAssetId('foo|lib/a.txt'): ChangeType.ADD},
           'foo',
           (_) async {},
           digestReader,
         );
 
-        expect(invalidatedNodes, contains(outputReadingNode));
-        expect(invalidatedNodes, contains(lastPrimaryOutputNode));
+        expect(
+          graph.get(outputReadingNode)!.generatedNodeState!.pendingBuildAction,
+          PendingBuildAction.buildIfInputsChanged,
+        );
+        expect(
+          graph
+              .get(lastPrimaryOutputNode)!
+              .generatedNodeState!
+              .pendingBuildAction,
+          PendingBuildAction.buildIfInputsChanged,
+        );
       });
 
       test('https://github.com/dart-lang/build/issues/1804', () async {
@@ -736,8 +737,8 @@ void main() {
           digestReader,
         );
 
-        // The old generated part file should no longer exist
-        expect(graph.get(generatedPart), isNull);
+        // The old generated part file should be marked as missing.
+        expect(graph.get(generatedPart)!.type, NodeType.missingSource);
 
         // The generated part file should not exist in outputs of the new
         // generated dart file

build_runner_core/test/generate/build_test.dart

@@ -1673,7 +1673,10 @@ void main() {
               makeAssetId('a|$assetGraphPath'),
             ),
           );
-          expect(graph.get(makeAssetId('a|lib/a.txt.copy')), isNull);
+          expect(
+            graph.get(makeAssetId('a|lib/a.txt.copy'))!.type,
+            NodeType.missingSource,
+          );
         },
       );
     });
@@ -1702,15 +1705,15 @@ void main() {
         resumeFrom: result,
       );
 
-      /// Should be deleted using the writer, and removed from the new graph.
+      /// Should be deleted using the writer, and converted to missingSource.
       var newGraph = AssetGraph.deserialize(
         result.readerWriter.testing.readBytes(makeAssetId('a|$assetGraphPath')),
       );
       var aNodeId = makeAssetId('a|lib/a.txt');
       var aCopyNodeId = makeAssetId('a|lib/a.txt.copy');
       var aCloneNodeId = makeAssetId('a|lib/a.txt.copy.clone');
-      expect(newGraph.contains(aNodeId), isFalse);
-      expect(newGraph.contains(aCopyNodeId), isFalse);
+      expect(newGraph.get(aNodeId)!.type, NodeType.missingSource);
+      expect(newGraph.get(aCopyNodeId)!.type, NodeType.missingSource);
       expect(newGraph.contains(aCloneNodeId), isFalse);
       expect(result.readerWriter.testing.exists(aNodeId), isFalse);
       expect(result.readerWriter.testing.exists(aCopyNodeId), isFalse);