Skip to content

Commit

Permalink
fix #27259, implement covariance checking for strong mode and DDC
Browse files Browse the repository at this point in the history 
R=vsm@google.com

Review-Url: https://codereview.chromium.org/2954523002 .
  • Loading branch information
Jennifer Messerly committed Jul 6, 2017
1 parent c4ea80a commit 90380df
Show file tree
Hide file tree
Showing 25 changed files with 2,305 additions and 396 deletions.
58 changes: 39 additions & 19 deletions pkg/analyzer/lib/src/dart/element/type.dart
View file
Expand Up @@ -718,7 +718,7 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
// variables, and see if the result is equal.
if (typeFormals.isNotEmpty) {
List<DartType> freshVariables =
relateTypeFormals(this, object, (t, s) => t == s);
relateTypeFormals(this, object, (t, s, _, __) => t == s);
if (freshVariables == null) {
return false;
}
Expand Down Expand Up @@ -895,7 +895,7 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
return relate(
this,
type,
(DartType t, DartType s, _, __) =>
(DartType t, DartType s) =>
(t as TypeImpl).isMoreSpecificThan(s, withDynamic),
new TypeSystemImpl(null).instantiateToBounds);
}
Expand All @@ -906,7 +906,7 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
return relate(
typeSystem.instantiateToBounds(this),
typeSystem.instantiateToBounds(type),
(DartType t, DartType s, _, __) => t.isAssignableTo(s),
(DartType t, DartType s) => t.isAssignableTo(s),
typeSystem.instantiateToBounds);
}

Expand Down Expand Up @@ -1076,16 +1076,16 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
* structural rules for handling optional parameters and arity, but use their
* own relation for comparing corresponding parameters or return types.
*
* If [returnRelation] is omitted, uses [parameterRelation] for both.
* If [parameterRelation] is omitted, uses [returnRelation] for both. This
* is convenient for Dart 1 type system methods.
*/
static bool relate(
FunctionType t,
DartType other,
bool parameterRelation(
DartType t, DartType s, bool tIsCovariant, bool sIsCovariant),
bool returnRelation(DartType t, DartType s),
DartType instantiateToBounds(DartType t),
{bool returnRelation(DartType t, DartType s)}) {
returnRelation ??= (t, s) => parameterRelation(t, s, false, false);
{bool parameterRelation(ParameterElement t, ParameterElement s)}) {
parameterRelation ??= (t, s) => returnRelation(t.type, s.type);

// Trivial base cases.
if (other == null) {
Expand All @@ -1102,7 +1102,8 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
// This type cast is safe, because we checked it above.
FunctionType s = other as FunctionType;
if (t.typeFormals.isNotEmpty) {
List<DartType> freshVariables = relateTypeFormals(t, s, returnRelation);
List<DartType> freshVariables =
relateTypeFormals(t, s, (s, t, _, __) => returnRelation(s, t));
if (freshVariables == null) {
return false;
}
Expand All @@ -1119,14 +1120,34 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
}

// Test the parameter types.
return relateParameters(t.parameters, s.parameters, parameterRelation);
}

/**
* Compares parameters [tParams] and [sParams] of two function types, taking
* corresponding parameters from the lists, and see if they match
* [parameterRelation].
*
* Corresponding parameters are defined as a pair `(t, s)` where `t` is a
* parameter from [tParams] and `s` is a parameter from [sParams], and both
* `t` and `s` are at the same position (for positional parameters)
* or have the same name (for named parameters).
*
* Used for the various relations on function types which have the same
* structural rules for handling optional parameters and arity, but use their
* own relation for comparing the parameters.
*/
static bool relateParameters(
List<ParameterElement> tParams,
List<ParameterElement> sParams,
bool parameterRelation(ParameterElement t, ParameterElement s)) {
// TODO(jmesserly): this could be implemented with less allocation if we
// wanted, by taking advantage of the fact that positional arguments must
// appear before named ones.
var tRequired = <ParameterElement>[];
var tOptional = <ParameterElement>[];
var tNamed = <String, ParameterElement>{};
for (var p in t.parameters) {
for (var p in tParams) {
var kind = p.parameterKind;
if (kind == ParameterKind.REQUIRED) {
tRequired.add(p);
Expand All @@ -1141,7 +1162,7 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
var sRequired = <ParameterElement>[];
var sOptional = <ParameterElement>[];
var sNamed = <String, ParameterElement>{};
for (var p in s.parameters) {
for (var p in sParams) {
var kind = p.parameterKind;
if (kind == ParameterKind.REQUIRED) {
sRequired.add(p);
Expand Down Expand Up @@ -1174,8 +1195,7 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
return false;
}
var sParam = sNamed[key];
if (!parameterRelation(
tParam.type, sParam.type, tParam.isCovariant, sParam.isCovariant)) {
if (!parameterRelation(tParam, sParam)) {
return false;
}
}
Expand Down Expand Up @@ -1204,10 +1224,7 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
}

for (int i = 0; i < sPositional.length; i++) {
var tParam = tPositional[i];
var sParam = sPositional[i];
if (!parameterRelation(
tParam.type, sParam.type, tParam.isCovariant, sParam.isCovariant)) {
if (!parameterRelation(tPositional[i], sPositional[i])) {
return false;
}
}
Expand All @@ -1227,7 +1244,10 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
* `F` to get `F -> F` and `F -> F`, which we can see are equal.
*/
static List<DartType> relateTypeFormals(
FunctionType f1, FunctionType f2, bool relation(DartType t, DartType s)) {
FunctionType f1,
FunctionType f2,
bool relation(DartType bound2, DartType bound1,
TypeParameterElement formal2, TypeParameterElement formal1)) {
List<TypeParameterElement> params1 = f1.typeFormals;
List<TypeParameterElement> params2 = f2.typeFormals;
int count = params1.length;
Expand Down Expand Up @@ -1258,7 +1278,7 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
bound1 = bound1.substitute2(variablesFresh, variables1);
bound2 = bound2.substitute2(variablesFresh, variables2);
pFresh.bound = bound2;
if (!relation(bound2, bound1)) {
if (!relation(bound2, bound1, p2, p1)) {
return null;
}
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/analyzer/lib/src/generated/resolver.dart
View file
Expand Up @@ -6515,10 +6515,10 @@ class ResolverVisitor extends ScopedVisitor {
!FunctionTypeImpl.relate(
expectedClosureType,
staticClosureType,
(DartType t, DartType s, _, __) =>
(t as TypeImpl).isMoreSpecificThan(s),
(s, t) => true,
new TypeSystemImpl(typeProvider).instantiateToBounds,
returnRelation: (s, t) => true)) {
parameterRelation: (t, s) =>
(t.type as TypeImpl).isMoreSpecificThan(s.type))) {
return;
}
// set propagated type for the closure
Expand Down
45 changes: 25 additions & 20 deletions pkg/analyzer/lib/src/generated/type_system.dart
View file
Expand Up @@ -497,19 +497,25 @@ class StrongTypeSystemImpl extends TypeSystem {
!nonnullableTypes.contains(_getTypeFullyQualifiedName(type));
}

/// Check that [f1] is a subtype of [f2] for an override.
/// Check that [f1] is a subtype of [f2] for a member override.
///
/// This is different from the normal function subtyping in two ways:
/// - we know the function types are strict arrows,
/// - it allows opt-in covariant parameters.
bool isOverrideSubtypeOf(FunctionType f1, FunctionType f2) {
return FunctionTypeImpl.relate(
f1,
f2,
(t1, t2, t1Covariant, _) =>
isSubtypeOf(t2, t1) || t1Covariant && isSubtypeOf(t1, t2),
instantiateToBounds,
returnRelation: isSubtypeOf);
return FunctionTypeImpl.relate(f1, f2, isSubtypeOf, instantiateToBounds,
parameterRelation: isOverrideSubtypeOfParameter);
}

/// Check that parameter [p2] is a subtype of [p1], given that we are
/// checking `f1 <: f2` where `p1` is a parameter of `f1` and `p2` is a
/// parameter of `f2`.
///
/// Parameters are contravariant, so we must check `p2 <: p1` to
/// determine if `f1 <: f2`. This is used by [isOverrideSubtypeOf].
bool isOverrideSubtypeOfParameter(ParameterElement p1, ParameterElement p2) {
return isSubtypeOf(p2.type, p1.type) ||
p1.isCovariant && isSubtypeOf(p1.type, p2.type);
}

@override
Expand Down Expand Up @@ -789,13 +795,10 @@ class StrongTypeSystemImpl extends TypeSystem {
/// that dynamic parameters of f1 and f2 are treated as bottom.
bool _isFunctionSubtypeOf(
FunctionType f1, FunctionType f2, Set<TypeImpl> visitedTypes) {
return FunctionTypeImpl.relate(
f1,
f2,
(t1, t2, _, __) =>
_isSubtypeOf(t2, t1, visitedTypes, dynamicIsBottom: true),
instantiateToBounds,
returnRelation: isSubtypeOf);
return FunctionTypeImpl.relate(f1, f2, isSubtypeOf, instantiateToBounds,
parameterRelation: (p1, p2) => _isSubtypeOf(
p2.type, p1.type, visitedTypes,
dynamicIsBottom: true));
}

bool _isInterfaceSubtypeOf(
Expand Down Expand Up @@ -2176,14 +2179,16 @@ class _GenericInferrer {
FunctionTypeImpl.relate(
t1,
t2,
(t1, t2, _, __) {
_matchSubtypeOf(t2, t1, null, origin,
covariant: !covariant, dynamicIsBottom: true);
(t1, t2) {
// TODO(jmesserly): should we flip covariance when we're relating
// type formal bounds? They're more like parameters.
matchSubtype(t1, t2);
return true;
},
_typeSystem.instantiateToBounds,
returnRelation: (t1, t2) {
matchSubtype(t1, t2);
parameterRelation: (p1, p2) {
_matchSubtypeOf(p2.type, p1.type, null, origin,
covariant: !covariant, dynamicIsBottom: true);
return true;
});
}
Expand Down
65 changes: 56 additions & 9 deletions pkg/analyzer/lib/src/task/strong/ast_properties.dart
View file
Expand Up @@ -7,17 +7,27 @@
/// These properties are not public, but provided by use of back-ends such as
/// Dart Dev Compiler.
import 'package:analyzer/analyzer.dart';
import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/dart/element/type.dart';

const String _hasImplicitCasts = '_hasImplicitCasts';
const String _implicitAssignmentCast = '_implicitAssignmentCast';
const String _implicitOperationCast = '_implicitAssignmentCast';
const String _implicitCast = '_implicitCast';
const String _isDynamicInvoke = '_isDynamicInvoke';
const String _classCovariantParameters = '_classCovariantParameters';
const String _superclassCovariantParameters = '_superclassCovariantParameters';
const String _covariantGenericReturn = '_covariantGenericReturn';
const String _covariantPrivateFields = '_covariantPrivateFields';
const String _covariantPrivateMembers = '_covariantPrivateMembers';

/// If this op-assign has an implicit cast on the assignment, returns the type
/// it is coerced to, otherwise returns null.
DartType getImplicitAssignmentCast(Expression node) {
return node.getProperty/*<DartType>*/(_implicitAssignmentCast);
/// If this expression needs an implicit cast on a subexpression that cannot be
/// expressed anywhere else, returns the type it is coerced to.
///
/// For example, op-assign can have an implicit cast on the final assignment,
/// and MethodInvocation calls on functions (`obj.f()` where `obj.f` is an
/// accessor and not a real method) can need a cast on the function.
DartType getImplicitOperationCast(Expression node) {
return node.getProperty/*<DartType>*/(_implicitOperationCast);
}

/// If this expression has an implicit cast, returns the type it is coerced to,
Expand All @@ -44,17 +54,54 @@ void setHasImplicitCasts(CompilationUnit node, bool value) {
node.setProperty(_hasImplicitCasts, value == true ? true : null);
}

/// Sets the result of [getImplicitAssignmentCast] for this node.
void setImplicitAssignmentCast(Expression node, DartType type) {
node.setProperty(_implicitAssignmentCast, type);
/// Sets the result of [getImplicitOperationCast] for this node.
void setImplicitOperationCast(Expression node, DartType type) {
node.setProperty(_implicitOperationCast, type);
}

/// Sets the result of [getImplicitCast] for this node.
void setImplicitCast(Expression node, DartType type) {
node.setProperty(_implicitCast, type);
}

/// Sets [isDynamicInvoke] property for this expression
/// Sets [isDynamicInvoke] property for this expression.
void setIsDynamicInvoke(Expression node, bool value) {
node.setProperty(_isDynamicInvoke, value == true ? true : null);
}

/// Returns a list of parameters and method type parameters in this class
/// declaration that need a check at runtime to ensure soundness.
Set<Element> getClassCovariantParameters(Declaration node) {
return node.getProperty(_classCovariantParameters);
}

/// Sets [getClassCovariantParameters] property for this class.
void setClassCovariantParameters(Declaration node, Set<Element> value) {
node.setProperty(_classCovariantParameters, value);
}

/// Returns a list of parameters and method type parameters from mixins and
/// superclasses of this class that need a stub method to check their type at
/// runtime for soundness.
Set<Element> getSuperclassCovariantParameters(Declaration node) {
return node.getProperty(_superclassCovariantParameters);
}

/// Sets [getSuperclassCovariantParameters] property for this class.
void setSuperclassCovariantParameters(Declaration node, Set<Element> value) {
node.setProperty(_superclassCovariantParameters, value);
}

/// Gets the private setters and methods that are accessed unsafely from
/// this compilation unit.
///
/// These members will require a check.
Set<ExecutableElement> getCovariantPrivateMembers(CompilationUnit node) {
return node.getProperty(_covariantPrivateMembers);
}

/// Sets [getCovariantPrivateMembers] property for this compilation unit.
void setCovariantPrivateMembers(
CompilationUnit node, Set<ExecutableElement> value) {
node.setProperty(_covariantPrivateMembers, value);
}

0 comments on commit 90380df

Please sign in to comment.