[dart2js] Add replacementImpact to conditional impacts.

Adds a conditional impact that represents that impact if the replacement node is added to the kernel AST. This is a soundness fix for conditional impacts as the replacement node might have a different set of impacts than the "source".

It's worth noting that the only way this could lead to an incorrect compilation for protobufs is if *every* metadata field was erased. The impact of each "source" is a superset of the impact for every "replacement" node.

This code is new and behind a flag so this bug certainly won't have been visible to anyone.

Change-Id: Ifa3cb7d6b9aa46221c736a1566aa79d73f99917c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/353940
Commit-Queue: Nate Biggs <natebiggs@google.com>
Reviewed-by: Mayank Patke <fishythefish@google.com>

pkg/compiler/lib/src/deferred_load/entity_data_info.dart

@@ -236,8 +236,13 @@ class EntityDataInfoBuilder {
   }
 
   void _addFromConditionalUse(ConditionalUse conditionalUse) {
-    if (conditionalUse.conditions.any(closedWorld.isMemberUsed)) {
+    if (conditionalUse.originalConditions.any(closedWorld.isMemberUsed)) {
       _addDependenciesFromImpact(conditionalUse.impact);
+    } else {
+      final replacementImpact = conditionalUse.replacementImpact;
+      if (replacementImpact != null) {
+        _addDependenciesFromImpact(replacementImpact);
+      }
     }
   }
 

pkg/compiler/lib/src/enqueue.dart

@@ -41,7 +41,7 @@ abstract class EnqueuerListener {
   WorldImpact registerUsedElement(MemberEntity member);
 
   /// Called to register that [use] is a pending condition.
-  /// [ConditionalUse.conditions] must only contain members that are not known
+  /// [ConditionalUse.originalConditions] must only contain members that are not known
   /// to be live yet.
   void registerPendingConditionalUse(ConditionalUse use);
 

pkg/compiler/lib/src/ir/impact_data.dart

@@ -37,11 +37,10 @@ abstract class ConditionalImpactHandler {
   /// otherwise.
   ImpactData? beforeInstanceInvocation(ir.InstanceInvocation node);
 
-  /// Invoked after children of [node] are analyzed. [registry] allows any extra
-  /// impacts to be registered. This is typically used to register conditional
-  /// impacts in the registry.
-  void afterInstanceInvocation(
-      ir.InstanceInvocation node, ImpactRegistry registry);
+  /// Invoked after children of [node] are analyzed. Returns a
+  /// [ConditionalImpactData] containing the conditional data to register for
+  /// [node]. Returns null if [node] is not relevant to this handler.
+  ConditionalImpactData? afterInstanceInvocation(ir.InstanceInvocation node);
 }
 
 class _ConditionalImpactBuilder extends ImpactBuilder {
@@ -58,8 +57,19 @@ class _ConditionalImpactBuilder extends ImpactBuilder {
 
     super.visitInstanceInvocation(node);
 
-    _data = oldData;
-    _conditionalHandler.afterInstanceInvocation(node, this);
+    final conditionalData = _conditionalHandler.afterInstanceInvocation(node);
+    if (conditionalData != null) {
+      final replacement = conditionalData.replacement;
+      if (replacement != null) {
+        final replacementImpact = _data = ImpactData();
+        replacement.accept(this);
+        conditionalData.replacementImpactData = replacementImpact;
+      }
+      _data = oldData;
+      registerConditionalImpact(conditionalData);
+    } else {
+      _data = oldData;
+    }
   }
 }
 
@@ -1149,13 +1159,14 @@ class ImpactBuilder extends ir.RecursiveVisitor implements ImpactRegistry {
 }
 
 class ConditionalImpactData {
-  final ir.TreeNode? source;
+  final ir.TreeNode? original;
   final ir.TreeNode? replacement;
-  final List<ir.Member> conditions;
+  final List<ir.Member> originalConditions;
   final ImpactData impactData;
+  late ImpactData replacementImpactData;
 
-  ConditionalImpactData(this.conditions, this.impactData,
-      {this.source, this.replacement});
+  ConditionalImpactData(this.originalConditions, this.impactData,
+      {this.original, this.replacement});
 }
 
 /// Data object that contains the world impact data derived purely from kernel.

pkg/compiler/lib/src/ir/protobuf_impacts.dart

@@ -7,7 +7,6 @@ import 'package:kernel/clone.dart' as ir;
 import 'package:kernel/type_algebra.dart' as ir;
 
 import '../kernel/element_map.dart';
-import 'impact.dart';
 import 'impact_data.dart';
 
 /// Handles conditional impact creation for protobuf metadata.
@@ -172,10 +171,9 @@ class ProtobufImpactHandler implements ConditionalImpactHandler {
   }
 
   @override
-  void afterInstanceInvocation(
-      ir.InstanceInvocation node, ImpactRegistry registry) {
+  ConditionalImpactData? afterInstanceInvocation(ir.InstanceInvocation node) {
     // This instance invocation is not a metadata initializer.
-    if (_impactData == null) return;
+    if (_impactData == null) return null;
 
     // The tag number is always the first argument in a metadata initializer.
     final tagNumber = ((node.arguments.positional[0] as ir.ConstantExpression)
@@ -203,9 +201,8 @@ class ProtobufImpactHandler implements ConditionalImpactHandler {
         }
       }
     }
-    registry.registerConditionalImpact(ConditionalImpactData(
-        accessors, _impactData!,
-        source: node, replacement: _buildProtobufMetadataPlaceholder(node)));
+    return ConditionalImpactData(accessors, _impactData!,
+        original: node, replacement: _buildProtobufMetadataPlaceholder(node));
   }
 }
 

pkg/compiler/lib/src/js_backend/resolution_listener.dart

@@ -183,24 +183,26 @@ class ResolutionEnqueuerListener extends EnqueuerListener {
       _backendUsage.isNoSuchMethodUsed = true;
     }
 
+    // Update the Kernel for any unused conditional impacts.
+    _pendingConditionalUses.forEach((_, uses) {
+      for (final use in uses) {
+        final source = use.original;
+        if (source?.parent != null) {
+          // Make sure the source node is still in the AST.
+          source?.replaceWith(use.replacement!);
+          enqueuer.applyImpact(use.replacementImpact!);
+        }
+      }
+    });
+    _pendingConditionalUses.clear();
+
     if (!enqueuer.queueIsEmpty) return false;
 
     return true;
   }
 
   @override
-  void onQueueClosed() {
-    // Update the Kernel for any unused conditional impacts.
-    _pendingConditionalUses.forEach((_, uses) {
-      for (final use in uses) {
-        final source = use.source;
-        if (source?.parent != null) {
-          // Make sure the source node is still in the AST.
-          source?.replaceWith(use.replacement!);
-        }
-      }
-    });
-  }
+  void onQueueClosed() {}
 
   /// Adds the impact of [constant] to [impactBuilder].
   void _computeImpactForCompileTimeConstant(
@@ -266,7 +268,7 @@ class ResolutionEnqueuerListener extends EnqueuerListener {
       // from the pending list of other members.
       for (final conditionalUse in conditionalUses) {
         worldImpact.addImpact(conditionalUse.impact);
-        for (final condition in conditionalUse.conditions) {
+        for (final condition in conditionalUse.originalConditions) {
           _pendingConditionalUses[condition]?.remove(conditionalUse);
         }
       }
@@ -500,7 +502,7 @@ class ResolutionEnqueuerListener extends EnqueuerListener {
 
   @override
   void registerPendingConditionalUse(ConditionalUse use) {
-    for (final condition in use.conditions) {
+    for (final condition in use.originalConditions) {
       (_pendingConditionalUses[condition] ??= {}).add(use);
     }
   }

pkg/compiler/lib/src/kernel/kernel_impact.dart

@@ -797,10 +797,12 @@ class KernelImpactConverter implements ImpactRegistry {
 
   @override
   void registerConditionalImpact(ConditionalImpactData impact) {
-    final conditionalUse = ConditionalUse(
+    final conditionalUse = ConditionalUse.withReplacement(
         impact: convert(impact.impactData),
-        conditions: impact.conditions.map(elementMap.getMember).toList(),
-        source: impact.source,
+        replacementImpact: convert(impact.replacementImpactData),
+        originalConditions:
+            impact.originalConditions.map(elementMap.getMember).toList(),
+        original: impact.original,
         replacement: impact.replacement);
     impactBuilder.registerConditionalUse(conditionalUse);
   }

pkg/compiler/lib/src/resolution/enqueuer.dart

@@ -341,7 +341,7 @@ class ResolutionEnqueuer extends Enqueuer {
     // live. If any condition member is live then immediately apply the impact.
     // `worldBuilder.isMemberProcessed` checks whether the member is used
     // including all static and instance members.
-    if (conditionalUse.conditions.any(worldBuilder.isMemberProcessed)) {
+    if (conditionalUse.originalConditions.any(worldBuilder.isMemberProcessed)) {
       applyImpact(conditionalUse.impact);
     } else {
       listener.registerPendingConditionalUse(conditionalUse);

pkg/compiler/lib/src/universe/use.dart

@@ -1047,19 +1047,43 @@ class ConstantUse {
 
 /// Conditional impact and Kernel nodes for replacement if it isn't applied.
 ///
-/// If one of source or replacement is provided, the other must also be
-/// provided.
+/// If one of [original], [replacement], or [replacementImpact] is provided, the
+/// others must also be provided.
 class ConditionalUse {
-  final ir.TreeNode? source;
-  final ir.TreeNode? replacement;
-  final WorldImpact impact;
-  final List<MemberEntity> conditions;
+  /// If any of the members in this list are reachable from the program then
+  /// these conditions are considered "satisfied", [original] is kept in the
+  /// Kernel tree and [impact] is applied to the world. Otherwise [original] is
+  /// replaced with [replacement] in the Kernel tree and [replacementImpact] is
+  /// instead applied to the world.
+  final List<MemberEntity> originalConditions;
 
-  ConditionalUse(
-      {this.source,
-      this.replacement,
+  /// The node to replace if [originalConditions] are not satisfied.
+  /// [impact] is the impact implied by this node.
+  final ir.TreeNode? original;
+
+  /// The node to replace [original] with is [originalConditions] are not
+  /// satisfied.
+  final ir.TreeNode? replacement;
+
+  /// The impact to apply for [original] if [originalConditions] are satisfied.
+  final WorldImpact impact;
+
+  /// The impact to apply for [replacement] if [originalConditions] are not
+  /// satisifed.
+  final WorldImpact? replacementImpact;
+
+  ConditionalUse.noReplacement(
+      {required this.impact, required this.originalConditions})
+      : original = null,
+        replacement = null,
+        replacementImpact = null,
+        assert(originalConditions.isNotEmpty);
+
+  ConditionalUse.withReplacement(
+      {required this.original,
+      required this.replacement,
+      required this.replacementImpact,
       required this.impact,
-      required this.conditions})
-      : assert((source == null) == (replacement == null)),
-        assert(conditions.isNotEmpty);
+      required this.originalConditions})
+      : assert(originalConditions.isNotEmpty);
 }