Skip to content

Commit

Permalink
Migration: fix handling of assignments involving FutureOr.
Browse files Browse the repository at this point in the history
Fixes ~96 exceptions whose stack trace contains the line:

DecoratedClassHierarchy.getDecoratedSupertype (package:nnbd_migration/src/decorated_class_hierarchy.dart:59:10)

Change-Id: I0a4d31f4822107453b2881618817c8a7c20e0e84
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/115740
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
  • Loading branch information
stereotype441 authored and commit-bot@chromium.org committed Sep 5, 2019
1 parent ec48af6 commit 89dde05
Show file tree
Hide file tree
Showing 5 changed files with 183 additions and 4 deletions.
21 changes: 21 additions & 0 deletions pkg/nnbd_migration/lib/src/already_migrated_code_decorator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// 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.

import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/dart/element/type.dart';
import 'package:analyzer/src/dart/element/type.dart';
import 'package:analyzer/src/generated/resolver.dart';
Expand Down Expand Up @@ -76,4 +77,24 @@ class AlreadyMigratedCodeDecorator {
'Unable to decorate already-migrated type $type');
}
}

/// Get all the decorated immediate supertypes of the non-migrated class
/// [class_].
Iterable<DecoratedType> getImmediateSupertypes(ClassElement class_) {
var allSupertypes = <DartType>[];
var supertype = class_.supertype;
if (supertype != null) {
allSupertypes.add(supertype);
}
allSupertypes.addAll(class_.superclassConstraints);
allSupertypes.addAll(class_.interfaces);
allSupertypes.addAll(class_.mixins);
var type = class_.type;
if (type.isDartAsyncFuture) {
// Add FutureOr<T> as a supertype of Future<T>.
allSupertypes
.add(_typeProvider.futureOrType.instantiate(type.typeArguments));
}
return allSupertypes.map(decorate);
}
}
18 changes: 18 additions & 0 deletions pkg/nnbd_migration/lib/src/edge_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1923,6 +1923,15 @@ mixin _AssignmentChecker {
@required DecoratedType destination,
@required bool hard}) {
_connect(source.node, destination.node, origin, hard: hard);
_checkAssignment_recursion(origin,
source: source, destination: destination);
}

/// Does the recursive part of [_checkAssignment], visiting all of the types
/// constituting [source] and [destination], and creating the appropriate
/// edges between them.
void _checkAssignment_recursion(EdgeOrigin origin,
{@required DecoratedType source, @required DecoratedType destination}) {
var sourceType = source.type;
var destinationType = destination.type;
if (sourceType.isBottom || sourceType.isDartCoreNull) {
Expand Down Expand Up @@ -1957,6 +1966,15 @@ mixin _AssignmentChecker {
destinationType is InterfaceType) {
if (_typeSystem.isSubtypeOf(sourceType, destinationType)) {
// Ordinary (upcast) assignment. No cast necessary.
if (destinationType.isDartAsyncFutureOr) {
if (_typeSystem.isSubtypeOf(
sourceType, destinationType.typeArguments[0])) {
// We are looking at T <: FutureOr<U>. So treat this as T <: U.
_checkAssignment_recursion(origin,
source: source, destination: destination.typeArguments[0]);
return;
}
}
var rewrittenSource = _decoratedClassHierarchy.asInstanceOf(
source, destinationType.element);
assert(rewrittenSource.typeArguments.length ==
Expand Down
5 changes: 2 additions & 3 deletions pkg/nnbd_migration/lib/src/variables.dart
Original file line number Diff line number Diff line change
Expand Up @@ -248,9 +248,8 @@ class Variables implements VariableRecorder, VariableRepository {
Map<ClassElement, DecoratedType> _decorateDirectSupertypes(
ClassElement class_) {
var result = <ClassElement, DecoratedType>{};
for (var supertype in class_.allSupertypes) {
var decoratedSupertype =
_alreadyMigratedCodeDecorator.decorate(supertype);
for (var decoratedSupertype
in _alreadyMigratedCodeDecorator.getImmediateSupertypes(class_)) {
assert(identical(decoratedSupertype.node, _graph.never));
var class_ = (decoratedSupertype.type as InterfaceType).element;
if (class_ is ClassElementHandle) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@
// 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.

import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/dart/element/type.dart';
import 'package:analyzer/src/dart/element/element.dart';
import 'package:analyzer/src/dart/element/type.dart';
import 'package:analyzer/src/generated/resolver.dart';
import 'package:analyzer/src/generated/testing/element_factory.dart';
import 'package:analyzer/src/generated/testing/test_type_provider.dart';
import 'package:analyzer/src/generated/utilities_dart.dart';
import 'package:nnbd_migration/src/already_migrated_code_decorator.dart';
Expand Down Expand Up @@ -45,6 +47,15 @@ class _AlreadyMigratedCodeDecoratorTest {
expect(decoratedType.node, same(always));
}

void checkFutureOr(
DecoratedType decoratedType,
NullabilityNode expectedNullability,
void Function(DecoratedType) checkArgument) {
expect(decoratedType.type.element, typeProvider.futureOrType.element);
expect(decoratedType.node, expectedNullability);
checkArgument(decoratedType.typeArguments[0]);
}

void checkInt(
DecoratedType decoratedType, NullabilityNode expectedNullability) {
expect(decoratedType.type.element, typeProvider.intType.element);
Expand Down Expand Up @@ -76,7 +87,7 @@ class _AlreadyMigratedCodeDecoratorTest {
void checkTypeParameter(
DecoratedType decoratedType,
NullabilityNode expectedNullability,
TypeParameterElementImpl expectedElement) {
TypeParameterElement expectedElement) {
var type = decoratedType.type as TypeParameterTypeImpl;
expect(type.element, same(expectedElement));
expect(decoratedType.node, expectedNullability);
Expand Down Expand Up @@ -202,4 +213,63 @@ class _AlreadyMigratedCodeDecoratorTest {
test_decorate_void() {
checkVoid(decorate(typeProvider.voidType));
}

test_getImmediateSupertypes_future() {
var element = typeProvider.futureType.element;
var decoratedSupertypes =
decorator.getImmediateSupertypes(element).toList();
var typeParam = element.typeParameters[0];
expect(decoratedSupertypes, hasLength(2));
checkObject(decoratedSupertypes[0], never);
// Since Future<T> is a subtype of FutureOr<T>, we consider FutureOr<T> to
// be an immediate supertype, even though the class declaration for Future
// doesn't mention FutureOr.
checkFutureOr(decoratedSupertypes[1], never,
(t) => checkTypeParameter(t, never, typeParam));
}

test_getImmediateSupertypes_generic() {
var t = ElementFactory.typeParameterElement('T');
var class_ = ElementFactory.classElement3(
name: 'C',
typeParameters: [t],
supertype: typeProvider.iterableType.instantiate([t.type]));
var decoratedSupertypes = decorator.getImmediateSupertypes(class_).toList();
expect(decoratedSupertypes, hasLength(1));
checkIterable(decoratedSupertypes[0], never,
(type) => checkTypeParameter(type, never, t));
}

test_getImmediateSupertypes_interface() {
var class_ = ElementFactory.classElement('C', typeProvider.objectType);
class_.interfaces = [typeProvider.numType];
var decoratedSupertypes = decorator.getImmediateSupertypes(class_).toList();
expect(decoratedSupertypes, hasLength(2));
checkObject(decoratedSupertypes[0], never);
checkNum(decoratedSupertypes[1], never);
}

test_getImmediateSupertypes_mixin() {
var class_ = ElementFactory.classElement('C', typeProvider.objectType);
class_.mixins = [typeProvider.numType];
var decoratedSupertypes = decorator.getImmediateSupertypes(class_).toList();
expect(decoratedSupertypes, hasLength(2));
checkObject(decoratedSupertypes[0], never);
checkNum(decoratedSupertypes[1], never);
}

test_getImmediateSupertypes_superclassConstraint() {
var class_ = ElementFactory.mixinElement(
name: 'C', constraints: [typeProvider.numType]);
var decoratedSupertypes = decorator.getImmediateSupertypes(class_).toList();
expect(decoratedSupertypes, hasLength(1));
checkNum(decoratedSupertypes[0], never);
}

test_getImmediateSupertypes_supertype() {
var class_ = ElementFactory.classElement('C', typeProvider.objectType);
var decoratedSupertypes = decorator.getImmediateSupertypes(class_).toList();
expect(decoratedSupertypes, hasLength(1));
checkObject(decoratedSupertypes[0], never);
}
}
71 changes: 71 additions & 0 deletions pkg/nnbd_migration/test/edge_builder_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,77 @@ int f(dynamic d) => d;
hard: true);
}

test_assign_future_to_futureOr_complex() async {
await analyze('''
import 'dart:async';
FutureOr<List<int>> f(Future<List<int>> x) => x;
''');
// If `x` is `Future<List<int?>>`, then the only way to migrate is to make
// the return type `FutureOr<List<int?>>`.
assertEdge(decoratedTypeAnnotation('int>> x').node,
decoratedTypeAnnotation('int>> f').node,
hard: false);
assertNoEdge(decoratedTypeAnnotation('int>> x').node,
decoratedTypeAnnotation('List<int>> f').node);
assertNoEdge(decoratedTypeAnnotation('int>> x').node,
decoratedTypeAnnotation('FutureOr<List<int>> f').node);
}

test_assign_future_to_futureOr_simple() async {
await analyze('''
import 'dart:async';
FutureOr<int> f(Future<int> x) => x;
''');
// If `x` is nullable, then there are two migrations possible: we could make
// the return type `FutureOr<int?>` or we could make it `FutureOr<int>?`.
// We choose `FutureOr<int>?` because it's strictly more conservative (it's
// a subtype of `FutureOr<int?>`).
assertEdge(decoratedTypeAnnotation('Future<int> x').node,
decoratedTypeAnnotation('FutureOr<int>').node,
hard: true);
assertNoEdge(decoratedTypeAnnotation('Future<int> x').node,
decoratedTypeAnnotation('int> f').node);
// If `x` is `Future<int?>`, then the only way to migrate is to make the
// return type `FutureOr<int?>`.
assertEdge(substitutionNode(decoratedTypeAnnotation('int> x').node, never),
decoratedTypeAnnotation('int> f').node,
hard: false);
assertNoEdge(decoratedTypeAnnotation('int> x').node,
decoratedTypeAnnotation('FutureOr<int>').node);
}

test_assign_non_future_to_futureOr_complex() async {
await analyze('''
import 'dart:async';
FutureOr<List<int>> f(List<int> x) => x;
''');
// If `x` is `List<int?>`, then the only way to migrate is to make the
// return type `FutureOr<List<int?>>`.
assertEdge(decoratedTypeAnnotation('int> x').node,
decoratedTypeAnnotation('int>> f').node,
hard: false);
assertNoEdge(decoratedTypeAnnotation('int> x').node,
decoratedTypeAnnotation('List<int>> f').node);
assertNoEdge(decoratedTypeAnnotation('int> x').node,
decoratedTypeAnnotation('FutureOr<List<int>> f').node);
}

test_assign_non_future_to_futureOr_simple() async {
await analyze('''
import 'dart:async';
FutureOr<int> f(int x) => x;
''');
// If `x` is nullable, then there are two migrations possible: we could make
// the return type `FutureOr<int?>` or we could make it `FutureOr<int>?`.
// We choose `FutureOr<int>?` because it's strictly more conservative (it's
// a subtype of `FutureOr<int?>`).
assertEdge(decoratedTypeAnnotation('int x').node,
decoratedTypeAnnotation('FutureOr<int>').node,
hard: true);
assertNoEdge(decoratedTypeAnnotation('int x').node,
decoratedTypeAnnotation('int>').node);
}

test_assign_null_to_generic_type() async {
await analyze('''
main() {
Expand Down

0 comments on commit 89dde05

Please sign in to comment.