From 175385825a81b935402d8cf7ba28ba477ed7bfca Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Fri, 30 Sep 2022 13:14:31 +0000 Subject: [PATCH] Migration: clean up handling of type substitutions. Previously, when the migration tool needed to change one type variable to another during a substitution, it created a DecortedType to represent the substitution using `DecoratedType._forTypeParameterSubstitution`. This was problematic because the resulting DecoratedType had a null nullability `node`. This in turn forced nearly every reference to `DecoratedType.node` to be followed by a `!` operator. With this change, we now have a new base class, `SubstitutedType`, which can either be a `DecoratedType` (representing a fully general substitution) or a `_TypeVariableReplacement` (representing a simple change from one type variable to another). This paves the way for a follow-up CL that will make `DecoratedType.node` non-nullable. Change-Id: I7fdaef57994c7678ecd517ab4fe84d00f0d81424 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/261941 Reviewed-by: Konstantin Shcheglov Reviewed-by: Samuel Rawlins Commit-Queue: Paul Berry --- .../lib/src/decorated_class_hierarchy.dart | 4 +- .../lib/src/decorated_type.dart | 106 +++++++++++------- pkg/nnbd_migration/lib/src/edge_builder.dart | 10 +- .../test/edge_builder_test.dart | 2 +- 4 files changed, 73 insertions(+), 49 deletions(-) diff --git a/pkg/nnbd_migration/lib/src/decorated_class_hierarchy.dart b/pkg/nnbd_migration/lib/src/decorated_class_hierarchy.dart index af7ecad4eb1..074da14eaa7 100644 --- a/pkg/nnbd_migration/lib/src/decorated_class_hierarchy.dart +++ b/pkg/nnbd_migration/lib/src/decorated_class_hierarchy.dart @@ -84,10 +84,10 @@ class DecoratedClassHierarchy { var supertype = decoratedSupertype.type as InterfaceType; // Compute a type substitution to determine how [class_] relates to // this specific [superclass]. - Map substitution = {}; + Map substitution = {}; for (int i = 0; i < supertype.typeArguments.length; i++) { substitution[supertype.element2.typeParameters[i]] = - decoratedSupertype.typeArguments[i]; + decoratedSupertype.typeArguments[i]!; } // Apply that substitution to the relation between [superclass] and // each of its transitive superclasses, to determine the relation diff --git a/pkg/nnbd_migration/lib/src/decorated_type.dart b/pkg/nnbd_migration/lib/src/decorated_type.dart index a4c032a5a1a..a413b60f703 100644 --- a/pkg/nnbd_migration/lib/src/decorated_type.dart +++ b/pkg/nnbd_migration/lib/src/decorated_type.dart @@ -16,7 +16,7 @@ import 'package:nnbd_migration/src/nullability_node_target.dart'; /// Representation of a type in the code to be migrated. In addition to /// tracking the (unmigrated) [DartType], we track the [ConstraintVariable]s /// indicating whether the type, and the types that compose it, are nullable. -class DecoratedType implements DecoratedTypeInfo { +class DecoratedType implements DecoratedTypeInfo, SubstitutedType { @override final DartType? type; @@ -169,25 +169,6 @@ class DecoratedType implements DecoratedTypeInfo { } } - /// Creates a [DecoratedType] for a synthetic type parameter, to be used - /// during comparison of generic function types. - DecoratedType._forTypeParameterSubstitution(TypeParameterElement parameter) - : type = TypeParameterTypeImpl( - element2: parameter, - nullabilitySuffix: NullabilitySuffix.star, - ), - node = null, - returnType = null, - positionalParameters = const [], - namedParameters = const {}, - typeArguments = const [] { - // We'll be storing the type parameter bounds in - // [_decoratedTypeParameterBounds] so the type parameter needs to have an - // enclosing element of `null`. - assert(parameter.enclosingElement == null, - '$parameter should not have parent ${parameter.enclosingElement}'); - } - /// If `this` represents an interface type, returns the substitution necessary /// to produce this type using the class's type as a starting point. /// Otherwise throws an exception. @@ -195,11 +176,13 @@ class DecoratedType implements DecoratedTypeInfo { /// For instance, if `this` represents `List`, returns the substitution /// `{T: int?1}`, where `T` is the [TypeParameterElement] for `List`'s type /// parameter. - Map get asSubstitution { + Map get asSubstitution { var type = this.type; if (type is InterfaceType) { - return Map.fromIterables( - type.element2.typeParameters, typeArguments); + return { + for (int i = 0; i < typeArguments.length; i++) + type.element2.typeParameters[i]: typeArguments[i]! + }; } else { throw StateError( 'Tried to convert a non-interface type to a substitution'); @@ -311,14 +294,14 @@ class DecoratedType implements DecoratedTypeInfo { /// [undecoratedResult] is the result of the substitution, as determined by /// the normal type system. If not supplied, it is inferred. DecoratedType substitute( - Map substitution, + Map substitution, [DartType? undecoratedResult]) { if (substitution.isEmpty) return this; if (undecoratedResult == null) { var type = this.type!; undecoratedResult = Substitution.fromPairs( substitution.keys.toList(), - substitution.values.map((d) => d!.type!).toList(), + substitution.values.map((d) => d.type!).toList(), ).substituteType(type); if (undecoratedResult is FunctionType && type is FunctionType) { for (int i = 0; i < undecoratedResult.typeFormals.length; i++) { @@ -398,9 +381,15 @@ class DecoratedType implements DecoratedTypeInfo { namedParameters: namedParameters, typeArguments: typeArguments); + @override + DecoratedType _performSubstitution( + DecoratedType other, DartType undecoratedResult) => + withNodeAndType( + NullabilityNode.forSubstitution(node, other.node), undecoratedResult); + /// Internal implementation of [_substitute], used as a recursion target. DecoratedType _substitute( - Map substitution, + Map substitution, DartType? undecoratedResult) { var type = this.type; if (type is FunctionType && undecoratedResult is FunctionType) { @@ -411,13 +400,12 @@ class DecoratedType implements DecoratedTypeInfo { // substitutions, so we need to reflect that in our decorations by // substituting to use the type variables the analyzer used. substitution = - Map.from(substitution); + Map.from(substitution); for (int i = 0; i < typeFormals.length; i++) { // Check if it's a fresh type variable. if (undecoratedResult.typeFormals[i].enclosingElement == null) { substitution[typeFormals[i]] = - DecoratedType._forTypeParameterSubstitution( - undecoratedResult.typeFormals[i]); + _TypeVariableReplacement(undecoratedResult.typeFormals[i]); } } for (int i = 0; i < typeFormals.length; i++) { @@ -461,9 +449,7 @@ class DecoratedType implements DecoratedTypeInfo { if (inner == null) { return this; } else { - return inner.withNodeAndType( - NullabilityNode.forSubstitution(inner.node, node), - undecoratedResult); + return inner._performSubstitution(this, undecoratedResult!); } } else if (type!.isVoid || type.isDynamic) { return this; @@ -476,7 +462,7 @@ class DecoratedType implements DecoratedTypeInfo { /// is [undecoratedResult], and whose return type, positional parameters, and /// named parameters are formed by performing the given [substitution]. DecoratedType _substituteFunctionAfterFormals(FunctionType undecoratedResult, - Map substitution) { + Map substitution) { var newPositionalParameters = []; var numRequiredParameters = undecoratedResult.normalParameterTypes.length; for (int i = 0; i < positionalParameters!.length; i++) { @@ -604,17 +590,16 @@ class RenamedDecoratedFunctionTypes { } // Create a fresh set of type variables and substitute so we can // compare safely. - var substitution1 = {}; - var substitution2 = {}; + var substitution1 = {}; + var substitution2 = {}; var newParameters = []; for (int i = 0; i < type1.typeFormals!.length; i++) { var newParameter = TypeParameterElementImpl.synthetic(type1.typeFormals![i].name); newParameters.add(newParameter); - var newParameterType = - DecoratedType._forTypeParameterSubstitution(newParameter); - substitution1[type1.typeFormals![i]] = newParameterType; - substitution2[type2.typeFormals![i]] = newParameterType; + var newType = _TypeVariableReplacement(newParameter); + substitution1[type1.typeFormals![i]] = newType; + substitution2[type2.typeFormals![i]] = newType; } for (int i = 0; i < type1.typeFormals!.length; i++) { var bound1 = DecoratedTypeParameterBounds.current! @@ -656,13 +641,13 @@ class RenamedDecoratedFunctionTypes { } static List _substituteList(List list, - Map substitution) { + Map substitution) { return list.map((t) => t!.substitute(substitution)).toList(); } static Map _substituteMap( Map map, - Map substitution) { + Map substitution) { var result = {}; for (var entry in map.entries) { result[entry.key] = entry.value!.substitute(substitution); @@ -670,3 +655,42 @@ class RenamedDecoratedFunctionTypes { return result; } } + +/// Abstract base class for anything that can appear in the "value" position of +/// a decorated type substitution map (in other words, anything that a type +/// variable can be replaced with when performing a substitution). A +/// substituted type is either a [DecoratedType] (in which case the substitution +/// will be fully general, and the resulting decorated type will have a +/// nullability that's based on both the original type and the substituted +/// type), or it can be a [_TypeVariableReplacement], which is used when one +/// type variable is being replaced with another, but there is no change in +/// nullability. +abstract class SubstitutedType { + /// The undecorated type to be substituted + DartType? get type; + + /// Called by substitute methods to perform the actual substitution. [other] + /// is the decorated type to be substituted, and [undecoratedResult] is the + /// expected undecorated result of the substitution. + DecoratedType _performSubstitution( + DecoratedType other, DartType undecoratedResult); +} + +/// Data structure used as a value in a substitution map if the only +/// substitution that needs to be performed is to replace one type variable with +/// another. +class _TypeVariableReplacement implements SubstitutedType { + @override + final DartType type; + + _TypeVariableReplacement(TypeParameterElement newTypeVariable) + : type = TypeParameterTypeImpl( + element2: newTypeVariable, + nullabilitySuffix: NullabilitySuffix.star, + ); + + @override + DecoratedType _performSubstitution( + DecoratedType other, DartType undecoratedResult) => + other.withNodeAndType(other.node!, undecoratedResult); +} diff --git a/pkg/nnbd_migration/lib/src/edge_builder.dart b/pkg/nnbd_migration/lib/src/edge_builder.dart index 31fe0473851..3c532352b96 100644 --- a/pkg/nnbd_migration/lib/src/edge_builder.dart +++ b/pkg/nnbd_migration/lib/src/edge_builder.dart @@ -282,7 +282,7 @@ class EdgeBuilder extends GeneralizingAstVisitor /// (receiver) for a method, getter, or setter invocation. DecoratedType getOrComputeElementType(AstNode node, Element element, {DecoratedType? targetType, Expression? targetExpression}) { - Map? substitution; + Map? substitution; Element? baseElement = element.declaration; if (targetType != null) { var enclosingElement = baseElement!.enclosingElement; @@ -2214,7 +2214,7 @@ class EdgeBuilder extends GeneralizingAstVisitor astNode, type, left.substitute( - {typeParam: _variables.decoratedTypeParameterBound(typeParam)}), + {typeParam: _variables.decoratedTypeParameterBound(typeParam)!}), right, isLUB, node: node); @@ -2227,7 +2227,7 @@ class EdgeBuilder extends GeneralizingAstVisitor type, left, right.substitute( - {typeParam: _variables.decoratedTypeParameterBound(typeParam)}), + {typeParam: _variables.decoratedTypeParameterBound(typeParam)!}), isLUB, node: node); } @@ -2979,8 +2979,8 @@ class EdgeBuilder extends GeneralizingAstVisitor setType = _variables.decoratedElementType(setter).positionalParameters!.single; } - Map getterSubstitution = const {}; - Map setterSubstitution = const {}; + Map getterSubstitution = const {}; + Map setterSubstitution = const {}; if (class_ != null) { var getterClass = getter.enclosingElement as ClassElement; if (!identical(class_, getterClass)) { diff --git a/pkg/nnbd_migration/test/edge_builder_test.dart b/pkg/nnbd_migration/test/edge_builder_test.dart index 981f4047137..4ebf354b3f7 100644 --- a/pkg/nnbd_migration/test/edge_builder_test.dart +++ b/pkg/nnbd_migration/test/edge_builder_test.dart @@ -8268,7 +8268,7 @@ class _DecoratedClassHierarchyForTesting implements DecoratedClassHierarchy { } if (class_.name == 'MyListOfList' && superclass.name == 'List') { return assignmentCheckerTest._myListOfListSupertype - .substitute({class_.typeParameters[0]: type.typeArguments[0]}); + .substitute({class_.typeParameters[0]: type.typeArguments[0]!}); } if (class_.name == 'List' && superclass.name == 'Iterable') { return DecoratedType(