[dart2js] Fix timing issue in protobuf conditional impact enqueueing.

Prior to this fix, the "processedMembers" field on the ResolutionWorldBuilder was getting used as the signal that a member was active and therefore any conditional uses dependent on it should be active.

In some cases the following would occur:
1) The listener gets a signal a member is active. It checks if there are any conditional uses for that member and sees none.
2) A conditional use is registered and the member is not indicated as active yet so the use is stored as pending in ResolutionEnqueuerListener.
3) "processedMembers" in ResolutionWorldBuilder is updated for a member that would mark the conditional use active. This doesn't trigger a check of the pending conditional uses.

If the enqueues happen in this order (which is not always the case), the  conditional impact will not be marked active and the field will be erroneously tree shaken.

The solution here is to remove step 3 and add an analogous "usedMembers" update in step 1. Step 1 will always occur before step 2 so the conditional impact ledger and the usedMembers ledger will always stay in sync.

Change-Id: Ib732a4fe5dcc8fc7cbe9a6d37e1c239e287595ff
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/395740
Reviewed-by: Mayank Patke <[email protected]>
Commit-Queue: Nate Biggs <[email protected]>

Author: natebiggs

pkg/compiler/lib/src/enqueue.dart

@@ -40,10 +40,10 @@ abstract class EnqueuerListener {
   /// backend specific [WorldImpact] of this is returned.
   WorldImpact registerUsedElement(MemberEntity member);
 
-  /// Called to register that [use] is a pending condition.
-  /// [ConditionalUse.originalConditions] must only contain members that are not known
-  /// to be live yet.
-  void registerPendingConditionalUse(ConditionalUse use);
+  /// Called to register a new conditional impact, [use]. If the conditional
+  /// impact's condition is satisfied, the returned [WorldImpact] will contain
+  /// the implied impact.
+  WorldImpact registerConditionalUse(ConditionalUse use);
 
   /// Called to register that [value] is statically known to be used. Any
   /// backend specific [WorldImpact] of this is returned.

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

@@ -357,7 +357,7 @@ class CodegenEnqueuerListener extends EnqueuerListener {
   }
 
   @override
-  void registerPendingConditionalUse(ConditionalUse use) {
+  WorldImpact registerConditionalUse(ConditionalUse use) {
     throw UnsupportedError(
         'Codegen enqueuer does not support conditional impacts.');
   }

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

@@ -18,11 +18,11 @@ import '../universe/call_structure.dart' show CallStructure;
 import '../universe/use.dart' show ConditionalUse, StaticUse, TypeUse;
 import '../universe/world_impact.dart'
     show WorldImpact, WorldImpactBuilder, WorldImpactBuilderImpl;
-import 'field_analysis.dart';
 import 'backend_impact.dart';
 import 'backend_usage.dart';
 import 'checked_mode_helpers.dart';
 import 'custom_elements_analysis.dart';
+import 'field_analysis.dart';
 import 'interceptor_data.dart';
 import 'native_data.dart' show NativeBasicData;
 import 'no_such_method_registry.dart';
@@ -258,10 +258,13 @@ class ResolutionEnqueuerListener extends EnqueuerListener {
     return impactBuilder;
   }
 
+  final Set<MemberEntity> _usedMembers = {};
+
   @override
   WorldImpact registerUsedElement(MemberEntity member) {
     WorldImpactBuilderImpl worldImpact = WorldImpactBuilderImpl();
     _customElementsAnalysis.registerStaticUse(member);
+    _usedMembers.add(member);
     final conditionalUses = _pendingConditionalUses.remove(member);
     if (conditionalUses != null) {
       // Apply any newly satisfied conditional impacts and remove the condition
@@ -501,9 +504,16 @@ class ResolutionEnqueuerListener extends EnqueuerListener {
   }
 
   @override
-  void registerPendingConditionalUse(ConditionalUse use) {
-    for (final condition in use.originalConditions) {
-      (_pendingConditionalUses[condition] ??= {}).add(use);
+  WorldImpact registerConditionalUse(ConditionalUse use) {
+    WorldImpactBuilderImpl impactBuilder = WorldImpactBuilderImpl();
+    if (use.originalConditions.any(_usedMembers.contains)) {
+      // If any condition is already satisfied, apply the impact immediately.
+      impactBuilder.addImpact(use.impact);
+    } else {
+      for (final condition in use.originalConditions) {
+        (_pendingConditionalUses[condition] ??= {}).add(use);
+      }
     }
+    return impactBuilder;
   }
 }

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

@@ -8,10 +8,11 @@ import '../common.dart';
 import '../common/elements.dart' show ElementEnvironment;
 import '../common/tasks.dart' show CompilerTask;
 import '../common/work.dart' show WorkItem;
-import '../enqueue.dart';
 import '../elements/entities.dart';
 import '../elements/types.dart';
+import '../enqueue.dart';
 import '../js_backend/annotations.dart';
+import '../js_backend/resolution_listener.dart';
 import '../universe/member_usage.dart';
 import '../universe/resolution_world_builder.dart' show ResolutionWorldBuilder;
 import '../universe/use.dart'
@@ -32,7 +33,7 @@ class ResolutionEnqueuer extends Enqueuer {
   final CompilerTask task;
   final String name;
   @override
-  final EnqueuerListener listener;
+  final ResolutionEnqueuerListener listener;
 
   final Set<ClassEntity> _recentClasses = Setlet<ClassEntity>();
   bool _recentConstants = false;
@@ -337,14 +338,6 @@ class ResolutionEnqueuer extends Enqueuer {
 
   @override
   void processConditionalUse(ConditionalUse conditionalUse) {
-    // Only register a conditional use as pending if no condition member is
-    // 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.originalConditions.any(worldBuilder.isMemberProcessed)) {
-      applyImpact(conditionalUse.impact);
-    } else {
-      listener.registerPendingConditionalUse(conditionalUse);
-    }
+    applyImpact(listener.registerConditionalUse(conditionalUse));
   }
 }