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

@@ -277,39 +277,9 @@ class AssetGraph implements GeneratedAssetHider {
     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);
-    }
+    updateNode(id, (nodeBuilder) {
+      nodeBuilder.replace(AssetNode.missingSource(id));
+    });
 
     // Remove post build action applications with removed assets as inputs.
     for (final packageOutputs in _postProcessBuildStepOutputs.values) {
@@ -401,8 +371,9 @@ 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(
+  /// 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,
@@ -588,7 +559,7 @@ class AssetGraph implements GeneratedAssetHider {
     }
 
     _outputs = null;
-    return invalidatedIds;
+    return (idsToDelete, invalidatedIds);
   }
 
   /// Crawl up primary inputs to see if the original Source file matches the
@@ -769,9 +740,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);

build_runner_core/test/invalidation/asset_input_invalidation_test.dart

@@ -6,7 +6,8 @@ import 'package:test/test.dart';
 
 import 'invalidation_tester.dart';
 
-/// Invalidation tests in which the inputs are individually read arbitrary assets.
+/// Invalidation tests in which the inputs are individually read arbitrary
+/// assets.
 void main() {
   late InvalidationTester tester;
 
@@ -35,10 +36,7 @@ void main() {
       await tester.build();
       expect(
         await tester.build(delete: 'z'),
-        // TODO(davidmorgan): a.1 should be rebuilt, but it's not. This is a
-        // regression since the last version on pub: fix it!
-        // Result(written: ['a.1'], deleted: ['z.1'])
-        Result(deleted: ['z.1']),
+        Result(written: ['a.1'], deleted: ['z.1']),
       );
     });
 
@@ -73,14 +71,11 @@ void main() {
       );
     });
 
-    test('delete a.1, a.2+b.4 are rebuilt', () async {
+    test('delete a.1, a.2 is deleted and b.4 is rebuilt', () async {
       await tester.build();
       expect(
         await tester.build(delete: 'a.1'),
-        // TODO(davidmorgan): a.1 should be rebuilt, but it's not. This is a
-        // regression since the last version on pub: fix it!
-        // Result(written: ['a.2', 'b.4']),
-        Result(deleted: ['a.2']),
+        Result(deleted: ['a.2'], written: ['b.4']),
       );
     });
 

build_runner_core/test/invalidation/invalidation_tester.dart

@@ -6,7 +6,6 @@ import 'dart:async';
 import 'dart:convert';
 
 import 'package:build/build.dart';
-import 'package:build_runner_core/src/changes/asset_updates.dart';
 import 'package:build_test/build_test.dart';
 import 'package:built_collection/built_collection.dart';
 import 'package:crypto/crypto.dart';