[dart2js] Fix global type analysis

https://github.com/dart-lang/sdk/issues/43366

Fixed: 43366
Change-Id: Iac8247c31fa268186c8f2e3f52e0be1cac062598
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/162247
Commit-Queue: Stephen Adams <sra@google.com>
Reviewed-by: Mayank Patke <fishythefish@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>

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

@@ -983,10 +983,11 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation> {
     if (variable != null) {
       Local local = _localsMap.getLocalVariable(variable);
       if (!_capturedVariables.contains(local)) {
+        // Receiver strengthening to non-null.
         DartType type = _localsMap.getLocalType(_elementMap, local);
         _state.updateLocal(
             _inferrer, _capturedAndBoxed, local, receiverType, node, type,
-            isNullable: selector.appliesToNullWithoutThrow());
+            excludeNull: !selector.appliesToNullWithoutThrow());
       }
     }
 
@@ -1445,11 +1446,16 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation> {
     ir.Expression operand = node.operand;
     if (operand is ir.VariableGet) {
       Local local = _localsMap.getLocalVariable(operand.variable);
-      DartType type = _elementMap.getDartType(node.type);
+      DartType localType = _elementMap.getDartType(node.type);
       LocalState stateAfterCheckWhenTrue = new LocalState.childPath(_state);
       LocalState stateAfterCheckWhenFalse = new LocalState.childPath(_state);
-      stateAfterCheckWhenTrue.narrowLocal(
-          _inferrer, _capturedAndBoxed, local, type, node);
+
+      // Narrow variable to tested type on true branch.
+      TypeInformation currentTypeInformation = stateAfterCheckWhenTrue
+          .readLocal(_inferrer, _capturedAndBoxed, local);
+      stateAfterCheckWhenTrue.updateLocal(_inferrer, _capturedAndBoxed, local,
+          currentTypeInformation, node, localType,
+          isCast: false);
       _setStateAfter(_state, stateAfterCheckWhenTrue, stateAfterCheckWhenFalse);
     }
   }
@@ -1462,10 +1468,18 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation> {
       DartType localType = _localsMap.getLocalType(_elementMap, local);
       LocalState stateAfterCheckWhenTrue = new LocalState.childPath(_state);
       LocalState stateAfterCheckWhenFalse = new LocalState.childPath(_state);
+
+      // Narrow tested variable to 'Null' on true branch.
       stateAfterCheckWhenTrue.updateLocal(_inferrer, _capturedAndBoxed, local,
           _types.nullType, node, localType);
-      stateAfterCheckWhenFalse.narrowLocal(_inferrer, _capturedAndBoxed, local,
-          _closedWorld.commonElements.objectType, node);
+
+      // Narrow tested variable to 'not null' on false branch.
+      TypeInformation currentTypeInformation = stateAfterCheckWhenFalse
+          .readLocal(_inferrer, _capturedAndBoxed, local);
+      stateAfterCheckWhenFalse.updateLocal(_inferrer, _capturedAndBoxed, local,
+          currentTypeInformation, node, _closedWorld.commonElements.objectType,
+          excludeNull: true);
+
       _setStateAfter(_state, stateAfterCheckWhenTrue, stateAfterCheckWhenFalse);
     }
   }
@@ -1620,7 +1634,7 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation> {
       DartType type = _localsMap.getLocalType(_elementMap, local);
       _state.updateLocal(
           _inferrer, _capturedAndBoxed, local, localFunctionType, node, type,
-          isNullable: false);
+          excludeNull: true);
     }
 
     // We don't put the closure in the work queue of the
@@ -1737,7 +1751,7 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation> {
       Local local = _localsMap.getLocalVariable(exception);
       _state.updateLocal(_inferrer, _capturedAndBoxed, local, mask, node,
           _dartTypes.dynamicType(),
-          isNullable: false /* `throw null` produces a NullThrownError */);
+          excludeNull: true /* `throw null` produces a NullThrownError */);
     }
     ir.VariableDeclaration stackTrace = node.stackTrace;
     if (stackTrace != null) {
@@ -2088,9 +2102,11 @@ class LocalState {
       TypeInformation type,
       ir.Node node,
       DartType staticType,
-      {isNullable: true}) {
+      {isCast: true,
+      excludeNull: false}) {
     assert(type != null);
-    type = inferrer.types.narrowType(type, staticType, isNullable: isNullable);
+    type = inferrer.types
+        .narrowType(type, staticType, isCast: isCast, excludeNull: excludeNull);
 
     FieldEntity field = capturedAndBoxed[local];
     if (field != null) {
@@ -2100,17 +2116,6 @@ class LocalState {
     }
   }
 
-  void narrowLocal(
-      InferrerEngine inferrer,
-      Map<Local, FieldEntity> capturedAndBoxed,
-      Local local,
-      DartType type,
-      ir.Node node) {
-    TypeInformation currentType = readLocal(inferrer, capturedAndBoxed, local);
-    updateLocal(inferrer, capturedAndBoxed, local, currentType, node, type,
-        isNullable: false);
-  }
-
   LocalState mergeFlow(InferrerEngine inferrer, LocalState other) {
     seenReturnOrThrow = false;
     seenBreakOrContinue = false;

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

@@ -7,6 +7,7 @@ import '../common.dart';
 import '../constants/values.dart' show BoolConstantValue;
 import '../elements/entities.dart';
 import '../elements/types.dart';
+import '../ir/static_type.dart' show ClassRelation;
 import '../world.dart';
 import 'abstract_value_domain.dart';
 import 'type_graph_nodes.dart';
@@ -332,46 +333,42 @@ class TypeSystem {
       _abstractValueDomain.isNull(type.typeAnnotation).isDefinitelyFalse;
 
   /// Returns the intersection between [type] and [annotation].
-  /// [isNullable] indicates whether the annotation implies a null
-  /// type.
+  ///
+  /// [isCast] indicates whether narrowing comes from a cast or parameter check
+  /// rather than an 'is' test. (In legacy semantics these differ on whether
+  /// `null` is accepted).
+  ///
+  /// If [excludeNull] is true, the intersection excludes `null` even if the
+  /// Dart type implies `null`.
   TypeInformation narrowType(TypeInformation type, DartType annotation,
-      {bool isNullable: true}) {
-    TypeInformation _narrowTo(AbstractValue otherType) {
-      if (_abstractValueDomain.isExact(type.type).isDefinitelyTrue) return type;
-      if (isNullable) {
-        otherType = _abstractValueDomain.includeNull(otherType);
-      }
-      TypeInformation newType =
-          new NarrowTypeInformation(_abstractValueDomain, type, otherType);
-      allocatedTypes.add(newType);
-      return newType;
+      {bool isCast: true, bool excludeNull: false}) {
+    // Avoid refining an input with an exact type. It we are almost always
+    // adding a narrowing to a subtype of the same class or a superclass.
+    if (_abstractValueDomain.isExact(type.type).isDefinitelyTrue) return type;
+
+    AbstractValueWithPrecision narrowing =
+        _abstractValueDomain.createFromStaticType(annotation,
+            classRelation: ClassRelation.subtype, nullable: isCast);
+
+    AbstractValue abstractValue = narrowing.abstractValue;
+    if (excludeNull) {
+      abstractValue = _abstractValueDomain.excludeNull(abstractValue);
     }
 
-    // TODO(fishythefish): Use nullability.
-    annotation = annotation.withoutNullability;
-    if (annotation is VoidType) return type;
-    if (_closedWorld.dartTypes.isTopType(annotation)) {
-      if (isNullable) return type;
+    if (_abstractValueDomain.containsAll(abstractValue).isPotentiallyTrue) {
+      // Top, or non-nullable Top.
+      if (_abstractValueDomain.isNull(abstractValue).isPotentiallyTrue) {
+        return type;
+      }
       // If the input is already narrowed to be not-null, there is no value
       // in adding another narrowing node.
       if (_isNonNullNarrow(type)) return type;
-      return _narrowTo(_abstractValueDomain.excludeNull(dynamicType.type));
-    } else if (annotation is NeverType) {
-      return _narrowTo(_abstractValueDomain.emptyType);
-    } else if (annotation is InterfaceType) {
-      return _narrowTo(
-          _abstractValueDomain.createNonNullSubtype(annotation.element));
-    } else if (annotation is FunctionType) {
-      return _narrowTo(functionType.type);
-    } else if (annotation is FutureOrType) {
-      // TODO(johnniwinther): Support narrowing of FutureOr.
-      return type;
-    } else if (annotation is TypeVariableType) {
-      // TODO(ngeoffray): Narrow to bound.
-      return type;
-    } else {
-      throw 'Unexpected annotation type $annotation';
     }
+
+    TypeInformation newType =
+        NarrowTypeInformation(_abstractValueDomain, type, abstractValue);
+    allocatedTypes.add(newType);
+    return newType;
   }
 
   ParameterTypeInformation getInferredTypeOfParameter(Local parameter) {

pkg/compiler/test/analyses/api_allowed.json

@@ -69,7 +69,6 @@
     "Dynamic access of 'dart.collection::_modifications'.": 5,
     "Dynamic access of 'dart.collection::_map'.": 4,
     "Dynamic access of 'dart.collection::_elements'.": 1,
-    "Dynamic access of 'dart.collection::_element'.": 1,
     "Dynamic access of 'dart.collection::_first'.": 1
   },
   "org-dartlang-sdk:///lib/html/dart2js/html_dart2js.dart": {
@@ -198,4 +197,4 @@
   "org-dartlang-sdk:///lib/_http/websocket_impl.dart": {
     "Dynamic invocation of 'dart._http::_toJSON'.": 1
   }
-}
+}

sdk/lib/_internal/js_runtime/lib/collection_patch.dart

@@ -357,7 +357,7 @@ class _HashMap<K, V> extends MapBase<K, V> implements HashMap<K, V> {
     JS('void', 'delete #[#]', table, key);
   }
 
-  List _getBucket(var table, var key) {
+  List? _getBucket(var table, var key) {
     var hash = _computeHashCode(key);
     return JS('var', '#[#]', table, hash);
   }
@@ -905,7 +905,7 @@ class _HashSet<E> extends _SetBase<E> implements HashSet<E> {
     var bucket = _getBucket(rest, object);
     var index = _findBucketIndex(bucket, object);
     if (index < 0) return null;
-    return bucket[index];
+    return JS('', '#[#]', bucket, index);
   }
 
   // Collection.
@@ -1091,7 +1091,7 @@ class _HashSet<E> extends _SetBase<E> implements HashSet<E> {
     JS('void', 'delete #[#]', table, key);
   }
 
-  List _getBucket(var table, var element) {
+  List? _getBucket(var table, var element) {
     var hash = _computeHashCode(element);
     return JS('var', '#[#]', table, hash);
   }
@@ -1357,7 +1357,7 @@ class _LinkedHashSet<E> extends _SetBase<E> implements LinkedHashSet<E> {
     var bucket = _getBucket(rest, object);
     var index = _findBucketIndex(bucket, object);
     if (index < 0) return null;
-    return bucket[index]._element;
+    return JS<_LinkedHashSetCell>('', '#[#]', bucket, index)._element;
   }
 
   void forEach(void action(E element)) {
@@ -1564,7 +1564,7 @@ class _LinkedHashSet<E> extends _SetBase<E> implements LinkedHashSet<E> {
     JS('void', 'delete #[#]', table, key);
   }
 
-  List _getBucket(var table, var element) {
+  List? _getBucket(var table, var element) {
     var hash = _computeHashCode(element);
     return JS('var', '#[#]', table, hash);
   }

tests/language/regress/regress43366_test.dart

@@ -0,0 +1,28 @@
+// Copyright (c) 2020, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+String upcase(String? s) {
+  if (s == null) return '';
+  return s.toUpperCase();
+}
+
+String format(dynamic thing) {
+  if (thing is String?) return upcase(thing);
+  if (thing is num) return '$thing';
+  return '?';
+}
+
+main() {
+  log(format(null));
+  log(format('hello'));
+  log(format([]));
+
+  if (trace != '[][HELLO][?]') throw 'Unexpected: "$trace"';
+}
+
+String trace = '';
+
+void log(String s) {
+  trace += '[$s]';
+}