Skip to content

Commit

Permalink
Migration: add support for most compound assignments.
Browse files Browse the repository at this point in the history
This should address ~37 exceptions whose stack trace includes the line:

EdgeBuilder.visitAssignmentExpression (package:nnbd_migration/src/edge_builder.dart:274:7)

Still not addressed: compound assignments using `??=`.

Change-Id: Iae97fcc4979c2a98c1dbb7ad57f62ebbfc4bb4b5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/116084
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
  • Loading branch information
stereotype441 authored and commit-bot@chromium.org committed Sep 8, 2019
1 parent daaa665 commit cbe1352
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 8 deletions.
57 changes: 49 additions & 8 deletions pkg/nnbd_migration/lib/src/edge_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -269,12 +269,16 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>

@override
DecoratedType visitAssignmentExpression(AssignmentExpression node) {
if (node.operator.type != TokenType.EQ) {
// TODO(paulberry)
_unimplemented(node, 'Assignment with operator ${node.operator.lexeme}');
_CompoundOperatorInfo compoundOperatorInfo;
if (node.operator.type == TokenType.QUESTION_QUESTION_EQ) {
_unimplemented(node, 'Assignment with operator ??=');
} else if (node.operator.type != TokenType.EQ) {
compoundOperatorInfo = _CompoundOperatorInfo(
node.staticElement, node.operator.offset, node.staticType);
}
var expressionType = _handleAssignment(node.rightHandSide,
destinationExpression: node.leftHandSide);
destinationExpression: node.leftHandSide,
compoundOperatorInfo: compoundOperatorInfo);
var conditionalNode = _conditionalNodes[node.leftHandSide];
if (conditionalNode != null) {
expressionType = expressionType.withNode(
Expand Down Expand Up @@ -1479,6 +1483,7 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
DecoratedType _handleAssignment(Expression expression,
{DecoratedType destinationType,
Expression destinationExpression,
_CompoundOperatorInfo compoundOperatorInfo,
bool canInsertChecks = true}) {
assert(
(destinationExpression == null) != (destinationType == null),
Expand Down Expand Up @@ -1508,10 +1513,38 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
expressionChecks = ExpressionChecks(expression.end);
_variables.recordExpressionChecks(source, expression, expressionChecks);
}
_checkAssignment(expressionChecks,
source: sourceType,
destination: destinationType,
hard: _postDominatedLocals.isReferenceInScope(expression));
if (compoundOperatorInfo != null) {
var compoundOperatorMethod = compoundOperatorInfo.method;
if (compoundOperatorMethod != null) {
_checkAssignment(
CompoundAssignmentOrigin(source, compoundOperatorInfo.offset),
source: destinationType,
destination: _notNullType,
hard:
_postDominatedLocals.isReferenceInScope(destinationExpression));
DecoratedType compoundOperatorType =
getOrComputeElementType(compoundOperatorMethod);
assert(compoundOperatorType.positionalParameters.length > 0);
_checkAssignment(expressionChecks,
source: sourceType,
destination: compoundOperatorType.positionalParameters[0],
hard: _postDominatedLocals.isReferenceInScope(expression));
sourceType = _fixNumericTypes(compoundOperatorType.returnType,
compoundOperatorInfo.undecoratedType);
_checkAssignment(
CompoundAssignmentOrigin(source, compoundOperatorInfo.offset),
source: sourceType,
destination: destinationType,
hard: false);
} else {
sourceType = _dynamicType;
}
} else {
_checkAssignment(expressionChecks,
source: sourceType,
destination: destinationType,
hard: _postDominatedLocals.isReferenceInScope(expression));
}
if (destinationExpression != null) {
_postDominatedLocals.removeReferenceFromAllScopes(destinationExpression);
}
Expand Down Expand Up @@ -2068,6 +2101,14 @@ mixin _AssignmentChecker {
DecoratedType _getTypeParameterTypeBound(DecoratedType type);
}

class _CompoundOperatorInfo {
final MethodElement method;
final int offset;
final DartType undecoratedType;

_CompoundOperatorInfo(this.method, this.offset, this.undecoratedType);
}

/// Information about a binary expression whose boolean value could possibly
/// affect nullability analysis.
class _ConditionInfo {
Expand Down
6 changes: 6 additions & 0 deletions pkg/nnbd_migration/lib/src/edge_origin.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ class AlwaysNullableTypeOrigin extends EdgeOriginWithLocation {
AlwaysNullableTypeOrigin(Source source, int offset) : super(source, offset);
}

/// Edge origin resulting from the use of a value on the LHS of a compound
/// assignment.
class CompoundAssignmentOrigin extends EdgeOriginWithLocation {
CompoundAssignmentOrigin(Source source, int offset) : super(source, offset);
}

/// Common interface for classes providing information about how an edge came
/// to be; that is, what was found in the source code that led the migration
/// tool to create the edge.
Expand Down
67 changes: 67 additions & 0 deletions pkg/nnbd_migration/test/edge_builder_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,73 @@ void g(List<int> x) {
hard: false);
}

test_assignmentExpression_compound_dynamic() async {
await analyze('''
void f(dynamic x, int y) {
x += y;
}
''');
// No assertions; just making sure this doesn't crash.
}

test_assignmentExpression_compound_simple() async {
var code = '''
abstract class C {
C operator+(C x);
}
C f(C y, C z) => (y += z);
''';
await analyze(code);
var targetEdge =
assertEdge(decoratedTypeAnnotation('C y').node, never, hard: true);
expect((targetEdge.origin as CompoundAssignmentOrigin).offset,
code.indexOf('+='));
assertNullCheck(
checkExpression('z);'),
assertEdge(decoratedTypeAnnotation('C z').node,
decoratedTypeAnnotation('C x').node,
hard: true));
var operatorReturnEdge = assertEdge(
decoratedTypeAnnotation('C operator').node,
decoratedTypeAnnotation('C y').node,
hard: false);
expect((operatorReturnEdge.origin as CompoundAssignmentOrigin).offset,
code.indexOf('+='));
var fReturnEdge = assertEdge(decoratedTypeAnnotation('C operator').node,
decoratedTypeAnnotation('C f').node,
hard: false);
assertNullCheck(checkExpression('(y += z)'), fReturnEdge);
}

test_assignmentExpression_compound_withSubstitution() async {
var code = '''
abstract class C<T> {
C<T> operator+(C<T> x);
}
C<int> f(C<int> y, C<int> z) => (y += z);
''';
await analyze(code);
var targetEdge =
assertEdge(decoratedTypeAnnotation('C<int> y').node, never, hard: true);
expect((targetEdge.origin as CompoundAssignmentOrigin).offset,
code.indexOf('+='));
assertNullCheck(
checkExpression('z);'),
assertEdge(decoratedTypeAnnotation('C<int> z').node,
decoratedTypeAnnotation('C<T> x').node,
hard: true));
var operatorReturnEdge = assertEdge(
decoratedTypeAnnotation('C<T> operator').node,
decoratedTypeAnnotation('C<int> y').node,
hard: false);
expect((operatorReturnEdge.origin as CompoundAssignmentOrigin).offset,
code.indexOf('+='));
var fReturnEdge = assertEdge(decoratedTypeAnnotation('C<T> operator').node,
decoratedTypeAnnotation('C<int> f').node,
hard: false);
assertNullCheck(checkExpression('(y += z)'), fReturnEdge);
}

test_assignmentExpression_field() async {
await analyze('''
class C {
Expand Down

0 comments on commit cbe1352

Please sign in to comment.