[dart2js] Fix inferrer bug in handling exit conditions in try/catch/finally.

This is showing up in places like here where we are inferring `JSObject.toString` to return `String?`:
https://github.com/dart-lang/sdk/blob/main/sdk/lib/_internal/js_runtime/lib/js_patch.dart#L204

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

pkg/compiler/lib/src/inferrer/builder.dart

@@ -2014,7 +2014,7 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation?>
     _state = LocalState.tryBlock(stateBefore, node);
     _state.markInitializationAsIndefinite();
     visit(node.body);
-    final stateAfterBody = _state;
+    final stateAfterTry = _state;
     // If the try block contains a throw, then `stateAfterBody.aborts` will be
     // true. The catch needs to be aware of the results of inference from the
     // try block since we may get there via the abortive control flow:
@@ -2040,15 +2040,15 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation?>
     // } catch (_) {
     //   print(x + 42); <-- x cannot be 0 here.
     // }
-    _state =
-        stateBefore.mergeFlow(_inferrer, stateAfterBody, ignoreAborts: true);
+    _state = stateBefore.mergeTry(_inferrer, stateAfterTry);
     for (ir.Catch catchBlock in node.catches) {
       final stateBeforeCatch = _state;
       _state = LocalState.childPath(stateBeforeCatch);
       visit(catchBlock);
       final stateAfterCatch = _state;
-      _state = stateBeforeCatch.mergeFlow(_inferrer, stateAfterCatch);
+      _state = stateBeforeCatch.mergeCatch(_inferrer, stateAfterCatch);
     }
+
     return null;
   }
 
@@ -2058,7 +2058,6 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation?>
     _state = LocalState.tryBlock(stateBefore, node);
     _state.markInitializationAsIndefinite();
     visit(node.body);
-    final stateAfterBody = _state;
     // Even if the try block contains abortive control flow, the finally block
     // needs to be aware of the results of inference from the try block since we
     // still reach the finally after abortive control flow:
@@ -2071,9 +2070,20 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation?>
     // } finally {
     //   print(x + 42); <-- x may be 0 here.
     // }
-    _state =
-        stateBefore.mergeFlow(_inferrer, stateAfterBody, ignoreAborts: true);
+    _state = stateBefore.mergeTry(_inferrer, _state);
+    final stateBeforeFinalizer = _state;
+    // Use a child path to reset abort state before continuing into the
+    // `finally` block.
+    _state = LocalState.childPath(stateBeforeFinalizer);
     visit(node.finalizer);
+    // Continue with a copy of the state after the finalizer since control flow
+    // should continue linearly. Update abort state to account for try/catch
+    // aborting.
+    _state = LocalState.childPath(_state)
+      ..seenReturnOrThrow =
+          _state.seenReturnOrThrow || stateBeforeFinalizer.seenReturnOrThrow
+      ..seenBreakOrContinue = _state.seenBreakOrContinue ||
+          stateBeforeFinalizer.seenBreakOrContinue;
     return null;
   }
 
@@ -2467,18 +2477,25 @@ class LocalState {
     }
   }
 
-  LocalState mergeFlow(InferrerEngine inferrer, LocalState other,
-      {bool ignoreAborts = false}) {
-    seenReturnOrThrow = false;
-    seenBreakOrContinue = false;
-
-    if (!ignoreAborts && other.aborts) {
-      return this;
-    }
-    LocalsHandler locals = _locals.mergeFlow(inferrer, other._locals);
+  LocalState mergeTry(InferrerEngine inferrer, LocalState other) {
+    final locals = _locals.mergeFlow(inferrer, other._locals);
     return LocalState.internal(locals, _fields, _tryBlock,
-        seenReturnOrThrow: seenReturnOrThrow,
-        seenBreakOrContinue: seenBreakOrContinue);
+        seenReturnOrThrow: seenReturnOrThrow || other.seenReturnOrThrow,
+        seenBreakOrContinue: seenBreakOrContinue || other.seenBreakOrContinue);
+  }
+
+  LocalState mergeCatch(InferrerEngine inferrer, LocalState other) {
+    LocalsHandler locals;
+    if (aborts) {
+      locals = other._locals;
+    } else if (other.aborts) {
+      locals = _locals;
+    } else {
+      locals = _locals.mergeFlow(inferrer, other._locals);
+    }
+    return LocalState.internal(locals, _fields, _tryBlock,
+        seenReturnOrThrow: seenReturnOrThrow && other.seenReturnOrThrow,
+        seenBreakOrContinue: seenBreakOrContinue && other.seenReturnOrThrow);
   }
 
   LocalState mergeDiamondFlow(

pkg/compiler/lib/src/inferrer/locals_handler.dart

@@ -364,8 +364,7 @@ class LocalsHandler {
     }
   }
 
-  /// Returns the join between this locals handler and [other] which models the
-  /// flow through either this or [other].
+  /// Returns the locals handler modeling the union of this and [other].
   ///
   /// If [inPlace] is `true`, the variable types in this locals handler are
   /// replaced by the variables types in [other]. Otherwise the variable types

pkg/compiler/lib/src/inferrer_experimental/builder.dart

@@ -1294,9 +1294,9 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation?>
       return finish(firstArgument.value);
     } else if (firstArgument is ir.StaticGet) {
       MemberEntity member = _elementMap.getMember(firstArgument.target);
-      if (member is FieldEntity) {
+      if (member is JField) {
         FieldAnalysisData fieldData =
-            _closedWorld.fieldAnalysis.getFieldData(member as JField);
+            _closedWorld.fieldAnalysis.getFieldData(member);
         final constantValue = fieldData.constantValue;
         if (fieldData.isEffectivelyConstant &&
             constantValue is IntConstantValue) {
@@ -2014,7 +2014,7 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation?>
     _state = LocalState.tryBlock(stateBefore, node);
     _state.markInitializationAsIndefinite();
     visit(node.body);
-    final stateAfterBody = _state;
+    final stateAfterTry = _state;
     // If the try block contains a throw, then `stateAfterBody.aborts` will be
     // true. The catch needs to be aware of the results of inference from the
     // try block since we may get there via the abortive control flow:
@@ -2040,15 +2040,15 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation?>
     // } catch (_) {
     //   print(x + 42); <-- x cannot be 0 here.
     // }
-    _state =
-        stateBefore.mergeFlow(_inferrer, stateAfterBody, ignoreAborts: true);
+    _state = stateBefore.mergeTry(_inferrer, stateAfterTry);
     for (ir.Catch catchBlock in node.catches) {
       final stateBeforeCatch = _state;
       _state = LocalState.childPath(stateBeforeCatch);
       visit(catchBlock);
       final stateAfterCatch = _state;
-      _state = stateBeforeCatch.mergeFlow(_inferrer, stateAfterCatch);
+      _state = stateBeforeCatch.mergeCatch(_inferrer, stateAfterCatch);
     }
+
     return null;
   }
 
@@ -2058,7 +2058,6 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation?>
     _state = LocalState.tryBlock(stateBefore, node);
     _state.markInitializationAsIndefinite();
     visit(node.body);
-    final stateAfterBody = _state;
     // Even if the try block contains abortive control flow, the finally block
     // needs to be aware of the results of inference from the try block since we
     // still reach the finally after abortive control flow:
@@ -2071,9 +2070,20 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation?>
     // } finally {
     //   print(x + 42); <-- x may be 0 here.
     // }
-    _state =
-        stateBefore.mergeFlow(_inferrer, stateAfterBody, ignoreAborts: true);
+    _state = stateBefore.mergeTry(_inferrer, _state);
+    final stateBeforeFinalizer = _state;
+    // Use a child path to reset abort state before continuing into the
+    // `finally` block.
+    _state = LocalState.childPath(stateBeforeFinalizer);
     visit(node.finalizer);
+    // Continue with a copy of the state after the finalizer since control flow
+    // should continue linearly. Update abort state to account for try/catch
+    // aborting.
+    _state = LocalState.childPath(_state)
+      ..seenReturnOrThrow =
+          _state.seenReturnOrThrow || stateBeforeFinalizer.seenReturnOrThrow
+      ..seenBreakOrContinue = _state.seenBreakOrContinue ||
+          stateBeforeFinalizer.seenBreakOrContinue;
     return null;
   }
 
@@ -2467,18 +2477,25 @@ class LocalState {
     }
   }
 
-  LocalState mergeFlow(InferrerEngine inferrer, LocalState other,
-      {bool ignoreAborts = false}) {
-    seenReturnOrThrow = false;
-    seenBreakOrContinue = false;
-
-    if (!ignoreAborts && other.aborts) {
-      return this;
-    }
-    LocalsHandler locals = _locals.mergeFlow(inferrer, other._locals);
+  LocalState mergeTry(InferrerEngine inferrer, LocalState other) {
+    final locals = _locals.mergeFlow(inferrer, other._locals);
     return LocalState.internal(locals, _fields, _tryBlock,
-        seenReturnOrThrow: seenReturnOrThrow,
-        seenBreakOrContinue: seenBreakOrContinue);
+        seenReturnOrThrow: seenReturnOrThrow || other.seenReturnOrThrow,
+        seenBreakOrContinue: seenBreakOrContinue || other.seenBreakOrContinue);
+  }
+
+  LocalState mergeCatch(InferrerEngine inferrer, LocalState other) {
+    LocalsHandler locals;
+    if (aborts) {
+      locals = other._locals;
+    } else if (other.aborts) {
+      locals = _locals;
+    } else {
+      locals = _locals.mergeFlow(inferrer, other._locals);
+    }
+    return LocalState.internal(locals, _fields, _tryBlock,
+        seenReturnOrThrow: seenReturnOrThrow && other.seenReturnOrThrow,
+        seenBreakOrContinue: seenBreakOrContinue && other.seenReturnOrThrow);
   }
 
   LocalState mergeDiamondFlow(

pkg/compiler/lib/src/inferrer_experimental/locals_handler.dart

@@ -364,8 +364,7 @@ class LocalsHandler {
     }
   }
 
-  /// Returns the join between this locals handler and [other] which models the
-  /// flow through either this or [other].
+  /// Returns the locals handler modeling the union of this and [other].
   ///
   /// If [inPlace] is `true`, the variable types in this locals handler are
   /// replaced by the variables types in [other]. Otherwise the variable types

pkg/compiler/test/inference/data/throw.dart

@@ -54,7 +54,7 @@ conditionalThrowReturn() {
 /// Method that rethrows unconditionally.
 ////////////////////////////////////////////////////////////////////////////////
 
-/*member: unconditionalRethrow:[null]*/
+/*member: unconditionalRethrow:[empty]*/
 unconditionalRethrow() {
   try {
     throw 'foo';

pkg/compiler/test/inference/data/try_catch.dart

@@ -133,7 +133,7 @@ returnInt6() {
   return 42;
 }
 
-/*member: returnDyn6:[null|subclass=Object]*/
+/*member: returnDyn6:[subclass=Object]*/
 returnDyn6() {
   try {
     throw 42;
@@ -142,6 +142,15 @@ returnDyn6() {
   }
 }
 
+/*member: returnDyn7:[null|subclass=Object]*/
+returnDyn7() {
+  try {
+    // Do nothing
+  } catch (e) {
+    return e;
+  }
+}
+
 /*member: returnInt7:[exact=JSUInt31]*/
 returnInt7() {
   dynamic a = 'foo';
@@ -152,6 +161,286 @@ returnInt7() {
   return 2;
 }
 
+/*member: returnInt8:[exact=JSUInt31]*/
+returnInt8() {
+  dynamic a = 'foo';
+  try {
+    a = 42;
+    return a;
+  } catch (e) {
+    a = 29;
+    return a;
+  }
+  // ignore: dead_code
+  a = 'bar';
+  return a;
+}
+
+/*member: returnUnion1:[null|exact=JSUInt31]*/
+returnUnion1() {
+  dynamic a = 42;
+  if (a /*invoke: [exact=JSUInt31]*/ == 54) {
+    try {
+      a = 'foo';
+      throw a;
+    } catch (e) {
+      a = null;
+    }
+  }
+  return a;
+}
+
+/*member: returnUnion2:Union(null, [exact=JSBool], [exact=JSString], [exact=JSUInt31])*/
+returnUnion2() {
+  dynamic a = 42;
+  try {
+    a = 'foo';
+    a = null;
+  } catch (e) {
+    a = true;
+  }
+  return a;
+}
+
+/*member: returnUnion3:Union([exact=JSString], [exact=JSUInt31])*/
+returnUnion3() {
+  dynamic a = 42;
+  if (a /*invoke: [exact=JSUInt31]*/ == 54) {
+    try {
+      a = 'foo';
+      a = null;
+    } catch (e) {
+      a = true;
+    } finally {
+      a = 'bar';
+    }
+  }
+  return a;
+}
+
+/*member: returnUnion4:Union(null, [exact=JSString], [exact=JSUInt31])*/
+returnUnion4() {
+  dynamic a = 42;
+  if (a /*invoke: [exact=JSUInt31]*/ == 54) {
+    try {
+      a = 'foo';
+      a = null;
+    } catch (e) {}
+  }
+  return a;
+}
+
+/*member: returnUnion5:Union([exact=JSBool], [exact=JSUInt31])*/
+returnUnion5() {
+  dynamic a = 42;
+  if (a /*invoke: [exact=JSUInt31]*/ == 54) {
+    try {
+      a = 'foo';
+    } catch (e) {
+      a = null;
+    } finally {
+      a = true;
+    }
+  }
+  return a;
+}
+
+/*member: returnUnion6:Union(null, [exact=JSBool], [exact=JSString])*/
+returnUnion6() {
+  dynamic a = 42;
+  try {
+    return 'foo';
+  } catch (e) {
+    return null;
+  } finally {
+    return true;
+  }
+  // ignore: dead_code
+  return a;
+}
+
+/*member: returnUnion7:Union([exact=JSBool], [exact=JSString])*/
+returnUnion7() {
+  dynamic a = 42;
+  try {
+    return 'foo';
+  } catch (e) {
+    return true;
+  } finally {
+    a = 55;
+  }
+}
+
+/*member: returnUnion8:[null|exact=JSUInt31]*/
+returnUnion8() {
+  dynamic a = 5.5;
+  try {
+    a = 'foo';
+    throw a;
+  } catch (e) {
+    a = null;
+  } catch (e) {
+    a = true;
+    return 3;
+  }
+  return a;
+}
+
+/*member: returnUnion9:[exact=JSBool]*/
+returnUnion9() {
+  dynamic a = 5.5;
+  try {
+    a = 'foo';
+    throw a;
+  } catch (e) {
+    a = false;
+  } catch (e) {
+    a = true;
+  }
+  return a;
+}
+
+/*member: returnUnion10:Value([exact=JSBool], value: true)*/
+returnUnion10() {
+  dynamic a = 5;
+  try {
+    a = 6;
+    throw 0;
+  } catch (e) {
+    a = 7;
+    throw 0;
+  } finally {
+    a = 10;
+    a = true;
+    return a;
+  }
+}
+
+/*member: returnNull1:[null]*/
+returnNull1() {
+  dynamic a = 42;
+  try {
+    a = 'foo';
+  } catch (e) {
+    a = true;
+  } finally {
+    return null;
+  }
+  return a;
+}
+
+/*member: returnNull2:[null]*/
+returnNull2() {
+  dynamic a = 5.5;
+  try {
+    a = 'foo';
+    throw a;
+  } catch (e) {
+    a = null;
+  } catch (e) {
+    a = true;
+    throw 3;
+  }
+  return a;
+}
+
+/*member: A.:[exact=A]*/
+class A {
+  /*member: A.a:[null|exact=JSUInt31]*/
+  dynamic a;
+  /*member: A.b:Union(null, [exact=JSUInt31], [exact=JsLinkedHashMap])*/
+  dynamic b;
+  /*member: A.c:Union(null, [exact=JSBool], [exact=JSString])*/
+  dynamic c;
+  /*member: A.d:Value([null|exact=JSString], value: "foo")*/
+  dynamic d;
+  /*member: A.e:Union(null, [exact=JSBool], [exact=JSString])*/
+  dynamic e;
+  /*member: A.f:Union(null, [exact=JSBool], [exact=JSString])*/
+  dynamic f;
+  /*member: A.g:Union(null, [exact=JSExtendableArray], [exact=JSNumNotInt], [exact=JSString])*/
+  dynamic g;
+
+  /*member: A.testa:Union([exact=JSBool], [exact=JSString])*/
+  testa() {
+    try {
+      return 'foo';
+    } catch (e) {
+      return true;
+    } finally {
+      /*update: [exact=A]*/ a = 55;
+    }
+  }
+
+  /*member: A.testb:Union([exact=JSBool], [exact=JSString])*/
+  testb() {
+    try {
+      return 'foo';
+    } catch (e) {
+      return true;
+    } finally {
+      /*update: [exact=A]*/ b = 55;
+    }
+    return b;
+  }
+
+  /*member: A.testc:Union(null, [exact=JSBool], [exact=JSString])*/
+  testc() {
+    try {
+      /*update: [exact=A]*/ c = 'foo';
+      throw /*[exact=A]*/ c;
+    } catch (e) {
+      /*update: [exact=A]*/ c = false;
+    } catch (e) {
+      /*update: [exact=A]*/ c = true;
+    }
+    return /*[exact=A]*/ c;
+  }
+
+  /*member: A.testd:Value([null|exact=JSString], value: "foo")*/
+  testd() {
+    try {
+      /*update: [exact=A]*/ d = 'foo';
+    } catch (e) {
+      // Do nothing
+    }
+    return /*[exact=A]*/ d;
+  }
+
+  /*member: A.teste:Union(null, [exact=JSBool], [exact=JSString])*/
+  teste() {
+    try {
+      /*update: [exact=A]*/ e = 'foo';
+    } catch (_) {
+      /*update: [exact=A]*/ e = true;
+    }
+    return /*[exact=A]*/ e;
+  }
+
+  /*member: A.testf:Union(null, [exact=JSBool], [exact=JSString], [exact=JSUInt31])*/
+  testf() {
+    try {
+      /*update: [exact=A]*/ f = 'foo';
+      return 3;
+    } catch (e) {
+      /*update: [exact=A]*/ f = true;
+    }
+    return /*[exact=A]*/ f;
+  }
+
+  /*member: A.testg:Union(null, [exact=JSUInt31], [exact=JsLinkedHashMap])*/
+  testg() {
+    try {
+      /*update: [exact=A]*/ g = 'foo';
+      /*update: [exact=A]*/ g = 5.5;
+    } catch (e) {
+      /*update: [exact=A]*/ g = [];
+      /*update: [exact=A]*/ b = {};
+    }
+    return /*[exact=A]*/ b;
+  }
+}
+
 /*member: main:[null]*/
 main() {
   returnInt1();
@@ -166,5 +455,28 @@ main() {
   returnDyn5();
   returnInt6();
   returnDyn6();
+  returnDyn7();
   returnInt7();
+  returnInt8();
+  returnUnion1();
+  returnUnion2();
+  returnUnion3();
+  returnUnion4();
+  returnUnion5();
+  returnUnion6();
+  returnUnion7();
+  returnUnion8();
+  returnUnion9();
+  returnUnion10();
+  returnNull1();
+  returnNull2();
+
+  final a = A();
+  a. /*invoke: [exact=A]*/ testa();
+  a. /*invoke: [exact=A]*/ testb();
+  a. /*invoke: [exact=A]*/ testc();
+  a. /*invoke: [exact=A]*/ testd();
+  a. /*invoke: [exact=A]*/ teste();
+  a. /*invoke: [exact=A]*/ testf();
+  a. /*invoke: [exact=A]*/ testg();
 }