diff --git a/pkg/nnbd_migration/lib/nnbd_migration.dart b/pkg/nnbd_migration/lib/nnbd_migration.dart index d7d29f4a0cd..7388f0cb946 100644 --- a/pkg/nnbd_migration/lib/nnbd_migration.dart +++ b/pkg/nnbd_migration/lib/nnbd_migration.dart @@ -45,6 +45,21 @@ class NullabilityFixDescription { kind: NullabilityFixKind.checkExpressionDueToHint, ); + /// A compound assignment's combiner operator returns a type that isn't + /// assignable to the LHS of the assignment. + static const compoundAssignmentHasBadCombinedType = + const NullabilityFixDescription._( + appliedMessage: 'Compound assignment has bad combined type', + kind: NullabilityFixKind.compoundAssignmentHasBadCombinedType, + ); + + /// A compound assignment's LHS has a nullable type. + static const compoundAssignmentHasNullableSource = + const NullabilityFixDescription._( + appliedMessage: 'Compound assignment has nullable source', + kind: NullabilityFixKind.compoundAssignmentHasNullableSource, + ); + /// Informative message: a condition of an if-test or conditional expression /// will always evaluate to `false` in strong checking mode. static const conditionFalseInStrongMode = const NullabilityFixDescription._( @@ -99,6 +114,14 @@ class NullabilityFixDescription { 'Null-aware access will be unnecessary in strong checking mode', kind: NullabilityFixKind.nullAwarenessUnnecessaryInStrongMode); + /// Informative message: a null-aware assignment won't be necessary in strong + /// checking mode. + static const nullAwareAssignmentUnnecessaryInStrongMode = + const NullabilityFixDescription._( + appliedMessage: + 'Null-aware assignment will be unnecessary in strong checking mode', + kind: NullabilityFixKind.nullAwareAssignmentUnnecessaryInStrongMode); + static const otherCastExpression = const NullabilityFixDescription._( appliedMessage: 'Added a cast to an expression (non-downcast)', kind: NullabilityFixKind.otherCastExpression, @@ -217,12 +240,15 @@ enum NullabilityFixKind { addType, checkExpression, checkExpressionDueToHint, + compoundAssignmentHasNullableSource, + compoundAssignmentHasBadCombinedType, conditionFalseInStrongMode, conditionTrueInStrongMode, downcastExpression, makeTypeNullable, makeTypeNullableDueToHint, nullAwarenessUnnecessaryInStrongMode, + nullAwareAssignmentUnnecessaryInStrongMode, otherCastExpression, removeAs, removeDeadCode, diff --git a/pkg/nnbd_migration/lib/src/edit_plan.dart b/pkg/nnbd_migration/lib/src/edit_plan.dart index 6e4f5aa3d92..d2004de87ee 100644 --- a/pkg/nnbd_migration/lib/src/edit_plan.dart +++ b/pkg/nnbd_migration/lib/src/edit_plan.dart @@ -392,6 +392,20 @@ class EditPlanner { return (plan as NodeProducingEditPlan)._getChanges(false); } + /// Creates a new edit plan that adds an informative message to the given + /// [token]. + /// + /// The created edit plan should be inserted into the list of inner plans for + /// a pass-through plan targeted at the [containingNode]. See [passThrough]. + EditPlan informativeMessageForToken(AstNode containingNode, Token token, + {AtomicEditInfo info}) { + return _TokenChangePlan(containingNode, { + token.offset: [ + AtomicEdit.delete(token.lexeme.length, info: info, isInformative: true) + ] + }); + } + /// Creates a new edit plan that inserts the text indicated by [edits] at the /// given [offset]. /// diff --git a/pkg/nnbd_migration/lib/src/fix_aggregator.dart b/pkg/nnbd_migration/lib/src/fix_aggregator.dart index 9200694ae3a..e1548abb2e3 100644 --- a/pkg/nnbd_migration/lib/src/fix_aggregator.dart +++ b/pkg/nnbd_migration/lib/src/fix_aggregator.dart @@ -301,6 +301,70 @@ class NodeChangeForAsExpression extends NodeChangeForExpression { } } +/// Implementation of [NodeChange] specialized for operating on +/// [AssignmentExpression] nodes. +class NodeChangeForAssignment + extends NodeChangeForExpression { + /// Indicates whether the user should be warned that the assignment is a + /// compound assignment with a bad combined type (the return type of the + /// combiner isn't assignable to the the write type of the LHS). + bool isCompoundAssignmentWithBadCombinedType = false; + + /// Indicates whether the user should be warned that the assignment is a + /// compound assignment with a nullable source type. + bool isCompoundAssignmentWithNullableSource = false; + + /// Indicates whether the user should be warned that the assignment is a + /// null-aware assignment that will have no effect when strong checking is + /// enabled. + bool isWeakNullAware = false; + + @override + Iterable get _toStringParts => [ + ...super._toStringParts, + if (isCompoundAssignmentWithBadCombinedType) + 'isCompoundAssignmentWithBadCombinedType', + if (isCompoundAssignmentWithNullableSource) + 'isCompoundAssignmentWithNullableSource', + if (isWeakNullAware) 'isWeakNullAware' + ]; + + @override + NodeProducingEditPlan _apply( + AssignmentExpression node, FixAggregator aggregator) { + var lhsPlan = aggregator.planForNode(node.leftHandSide); + EditPlan operatorPlan; + if (isCompoundAssignmentWithNullableSource) { + operatorPlan = aggregator.planner.informativeMessageForToken( + node, node.operator, + info: AtomicEditInfo( + NullabilityFixDescription.compoundAssignmentHasNullableSource, + const {})); + } else if (isCompoundAssignmentWithBadCombinedType) { + operatorPlan = aggregator.planner.informativeMessageForToken( + node, node.operator, + info: AtomicEditInfo( + NullabilityFixDescription.compoundAssignmentHasBadCombinedType, + const {})); + } else if (isWeakNullAware) { + operatorPlan = aggregator.planner.informativeMessageForToken( + node, node.operator, + info: AtomicEditInfo( + NullabilityFixDescription + .nullAwareAssignmentUnnecessaryInStrongMode, + const {})); + } + var rhsPlan = aggregator.planForNode(node.rightHandSide); + var innerPlans = [ + lhsPlan, + if (operatorPlan != null) operatorPlan, + rhsPlan + ]; + return _applyExpression(aggregator, + aggregator.planner.passThrough(node, innerPlans: innerPlans)); + } +} + /// Implementation of [NodeChange] specialized for operating on /// [CompilationUnit] nodes. class NodeChangeForCompilationUnit extends NodeChange { @@ -832,6 +896,10 @@ class _NodeChangeVisitor extends GeneralizingAstVisitor> { NodeChange visitAsExpression(AsExpression node) => NodeChangeForAsExpression(); + @override + NodeChange visitAssignmentExpression(AssignmentExpression node) => + NodeChangeForAssignment(); + @override NodeChange visitCompilationUnit(CompilationUnit node) => NodeChangeForCompilationUnit(); diff --git a/pkg/nnbd_migration/lib/src/fix_builder.dart b/pkg/nnbd_migration/lib/src/fix_builder.dart index 60abe3f7c05..bbec67fc0aa 100644 --- a/pkg/nnbd_migration/lib/src/fix_builder.dart +++ b/pkg/nnbd_migration/lib/src/fix_builder.dart @@ -25,6 +25,7 @@ import 'package:analyzer/src/generated/migration.dart'; import 'package:analyzer/src/generated/resolver.dart'; import 'package:analyzer/src/generated/source.dart'; import 'package:analyzer/src/generated/utilities_dart.dart'; +import 'package:analyzer/src/task/strong/checker.dart'; import 'package:nnbd_migration/fix_reason_target.dart'; import 'package:nnbd_migration/instrumentation.dart'; import 'package:nnbd_migration/nnbd_migration.dart'; @@ -288,6 +289,9 @@ class MigrationResolutionHooksImpl implements MigrationResolutionHooks { final Expando _shouldStayNullAware = Expando(); + final Map + _assignmentExpressionHandlers = {}; + FlowAnalysis _flowAnalysis; @@ -423,37 +427,14 @@ class MigrationResolutionHooksImpl implements MigrationResolutionHooks { @override DartType modifyExpressionType(Expression node, DartType type) => _wrapExceptions(node, () => type, () { - var hint = - _fixBuilder._variables.getNullCheckHint(_fixBuilder.source, node); - if (hint != null) { - type = _addNullCheck(node, type, - info: AtomicEditInfo( - NullabilityFixDescription.checkExpressionDueToHint, - { - FixReasonTarget.root: - FixReason_NullCheckHint(CodeReference.fromAstNode(node)) - }, - hintComment: hint), - hint: hint); + var parent = node.parent; + if (parent is AssignmentExpression) { + return (_assignmentExpressionHandlers[parent] ??= + _AssignmentExpressionHandler(parent)) + .modifySubexpressionType(this, node, type); + } else { + return _modifyRValueType(node, type); } - if (type.isDynamic) return type; - var ancestor = _findNullabilityContextAncestor(node); - DartType context = _getNullabilityContext(ancestor); - if (!_fixBuilder._typeSystem.isSubtypeOf(type, context)) { - // Either a cast or a null check is needed. We prefer to do a null - // check if we can. - var nonNullType = _fixBuilder._typeSystem.promoteToNonNull(type); - if (_fixBuilder._typeSystem.isSubtypeOf(nonNullType, context)) { - return _addNullCheck(node, type); - } else { - return _addCast(node, type, context); - } - } - if (!_fixBuilder._typeSystem.isNullable(type)) return type; - if (_needsNullCheckDueToStructure(ancestor)) { - return _addNullCheck(node, type); - } - return type; }); @override @@ -519,33 +500,40 @@ class MigrationResolutionHooksImpl implements MigrationResolutionHooks { } } - DartType _getNullabilityContext(Expression node) { - var parent = node.parent; - if (parent is AssignmentExpression) { - var lhs = parent.leftHandSide; - if (lhs is SimpleIdentifier) { - var lhsElement = lhs.staticElement; - if (lhsElement is PromotableElement) { - var operatorType = parent.operator.type; - switch (operatorType) { - case TokenType.EQ: - case TokenType.QUESTION_QUESTION_EQ: - // When visiting an assignment to a local variable, if the - // variable type is promoted, the resolver uses the promoted type - // of the variable as the inference context, but it's ok to assign - // a different type to the variable (un-doing the promotion). So - // for migration purposes, we need to consider the context type to - // be the unpromoted type. See - // https://github.com/dart-lang/sdk/issues/41411. - return lhsElement.type; - default: - break; - } - } + DartType _modifyRValueType(Expression node, DartType type, + {DartType context}) { + var hint = + _fixBuilder._variables.getNullCheckHint(_fixBuilder.source, node); + if (hint != null) { + type = _addNullCheck(node, type, + info: AtomicEditInfo( + NullabilityFixDescription.checkExpressionDueToHint, + { + FixReasonTarget.root: + FixReason_NullCheckHint(CodeReference.fromAstNode(node)) + }, + hintComment: hint), + hint: hint); + } + if (type.isDynamic) return type; + var ancestor = _findNullabilityContextAncestor(node); + context ??= + InferenceContext.getContext(ancestor) ?? DynamicTypeImpl.instance; + if (!_fixBuilder._typeSystem.isSubtypeOf(type, context)) { + // Either a cast or a null check is needed. We prefer to do a null + // check if we can. + var nonNullType = _fixBuilder._typeSystem.promoteToNonNull(type); + if (_fixBuilder._typeSystem.isSubtypeOf(nonNullType, context)) { + return _addNullCheck(node, type); + } else { + return _addCast(node, type, context); } } - var context = InferenceContext.getContext(node) ?? DynamicTypeImpl.instance; - return context; + if (!_fixBuilder._typeSystem.isNullable(type)) return type; + if (_needsNullCheckDueToStructure(ancestor)) { + return _addNullCheck(node, type); + } + return type; } bool _needsNullCheckDueToStructure(Expression node) { @@ -639,6 +627,110 @@ class NonNullableUnnamedOptionalParameter implements Problem { /// Common supertype for problems reported by [FixBuilder._addProblem]. abstract class Problem {} +/// Data structure keeping track of intermediate results when the fix builder +/// is handling an assignment expression. +class _AssignmentExpressionHandler { + /// The assignment expression in question. + final AssignmentExpression node; + + /// For compound and null-aware assignments, the type read from the LHS. + /*late final*/ DartType readType; + + /// The type that may be written to the LHS. + /*late final*/ DartType writeType; + + /// The type that should be used as a context type when inferring the RHS. + DartType rhsContextType; + + _AssignmentExpressionHandler(this.node); + + /// Called after visiting the RHS of the assignment, to verify that for + /// compound assignments, the return value of the assignment is assignable to + /// [writeType]. + void handleAssignmentRhs( + MigrationResolutionHooksImpl hooks, DartType rhsType) { + MethodElement combiner = node.staticElement; + if (combiner != null) { + var fixBuilder = hooks._fixBuilder; + var combinerReturnType = + fixBuilder._typeSystem.refineBinaryExpressionType( + readType, + node.operator.type, + rhsType, + combiner.returnType, + ); + if (!fixBuilder._typeSystem.isSubtypeOf(combinerReturnType, writeType)) { + (fixBuilder._getChange(node) as NodeChangeForAssignment) + .isCompoundAssignmentWithBadCombinedType = true; + } + } + } + + /// Called after visiting the LHS of the assignment. Records the [readType], + /// [writeType], and [rhsContextType]. Also verifies that for compound + /// assignments, the [readType] is non-nullable, and that for null-aware + /// assignments, the [readType] is nullable. + void handleLValueType(MigrationResolutionHooksImpl hooks, + TokenType operatorType, DartType resolvedType) { + assert(resolvedType.nullabilitySuffix != NullabilitySuffix.star); + // Provisionally store the resolved type as the type of the lhs, so that + // getReadType can fall back on it if necessary. + var lhs = node.leftHandSide; + lhs.staticType = resolvedType; + // The type passed in by the resolver for the LHS of an assignment is the + // "write type". + var writeType = resolvedType; + if (lhs is SimpleIdentifier) { + var element = lhs.staticElement; + if (element is PromotableElement) { + // However, if the LHS is a reference to a local variable that has + // been promoted, the resolver passes in the promoted type. We + // want to use the variable element's type, so that we consider it + // ok to assign a value to the variable that un-does the + // promotion. See https://github.com/dart-lang/sdk/issues/41411. + writeType = element.type; + } + } + assert(writeType.nullabilitySuffix != NullabilitySuffix.star); + this.writeType = writeType; + var fixBuilder = hooks._fixBuilder; + if (operatorType == TokenType.EQ) { + rhsContextType = writeType; + } else { + readType = getReadType(lhs); + assert(readType.nullabilitySuffix != NullabilitySuffix.star); + if (operatorType == TokenType.QUESTION_QUESTION_EQ) { + rhsContextType = writeType; + if (fixBuilder._typeSystem.isNonNullable(readType)) { + (fixBuilder._getChange(node) as NodeChangeForAssignment) + .isWeakNullAware = true; + } + } else { + if (!readType.isDynamic && + fixBuilder._typeSystem.isPotentiallyNullable(readType)) { + (fixBuilder._getChange(node) as NodeChangeForAssignment) + .isCompoundAssignmentWithNullableSource = true; + } + } + } + } + + /// Called after visiting the LHS or the RHS of the assignment. + DartType modifySubexpressionType(MigrationResolutionHooksImpl hooks, + Expression subexpression, DartType type) { + if (identical(subexpression, node.leftHandSide)) { + handleLValueType(hooks, node.operator.type, type); + return type; + } else { + assert(identical(subexpression, node.rightHandSide)); + type = + hooks._modifyRValueType(subexpression, type, context: rhsContextType); + handleAssignmentRhs(hooks, type); + return type; + } + } +} + /// Visitor that computes additional migrations on behalf of [FixBuilder] that /// should be run after resolution class _FixBuilderPostVisitor extends GeneralizingAstVisitor diff --git a/pkg/nnbd_migration/lib/src/front_end/info_builder.dart b/pkg/nnbd_migration/lib/src/front_end/info_builder.dart index 86c298cc5e5..e0639633d9b 100644 --- a/pkg/nnbd_migration/lib/src/front_end/info_builder.dart +++ b/pkg/nnbd_migration/lib/src/front_end/info_builder.dart @@ -186,13 +186,18 @@ class InfoBuilder { // We could add an edit to add a `/*?*/` hint, but the offset is a // little tricky. break; - case NullabilityFixKind.nullAwarenessUnnecessaryInStrongMode: - case NullabilityFixKind.conditionTrueInStrongMode: case NullabilityFixKind.conditionFalseInStrongMode: + case NullabilityFixKind.conditionTrueInStrongMode: + case NullabilityFixKind.nullAwarenessUnnecessaryInStrongMode: + case NullabilityFixKind.nullAwareAssignmentUnnecessaryInStrongMode: // We don't offer any edits around weak-only code. // TODO(paulberry): offer edits to delete the code that would be dead in // strong mode (https://github.com/dart-lang/sdk/issues/41554). break; + case NullabilityFixKind.compoundAssignmentHasBadCombinedType: + case NullabilityFixKind.compoundAssignmentHasNullableSource: + // We don't offer any edits around bad compound assignments. + break; } return edits; } diff --git a/pkg/nnbd_migration/lib/src/front_end/unit_renderer.dart b/pkg/nnbd_migration/lib/src/front_end/unit_renderer.dart index 29aded69995..d6653abbac0 100644 --- a/pkg/nnbd_migration/lib/src/front_end/unit_renderer.dart +++ b/pkg/nnbd_migration/lib/src/front_end/unit_renderer.dart @@ -23,9 +23,12 @@ class UnitRenderer { /// "proposed edits" area, in the order in which they should be displayed. @visibleForTesting static const List kindPriorityOrder = [ + NullabilityFixKind.compoundAssignmentHasBadCombinedType, + NullabilityFixKind.compoundAssignmentHasNullableSource, NullabilityFixKind.removeDeadCode, NullabilityFixKind.conditionTrueInStrongMode, NullabilityFixKind.conditionFalseInStrongMode, + NullabilityFixKind.nullAwareAssignmentUnnecessaryInStrongMode, NullabilityFixKind.nullAwarenessUnnecessaryInStrongMode, NullabilityFixKind.otherCastExpression, NullabilityFixKind.checkExpression, @@ -265,6 +268,12 @@ class UnitRenderer { return '$count null check$s added'; case NullabilityFixKind.checkExpressionDueToHint: return '$count null check hint$s converted to null check$s'; + case NullabilityFixKind.compoundAssignmentHasBadCombinedType: + return '$count compound assignment$s could not be migrated (bad ' + 'combined type)'; + case NullabilityFixKind.compoundAssignmentHasNullableSource: + return '$count compound assignment$s could not be migrated (nullable ' + 'source)'; case NullabilityFixKind.conditionTrueInStrongMode: return '$count condition$s will be true in strong checking mode'; break; @@ -278,6 +287,9 @@ class UnitRenderer { case NullabilityFixKind.nullAwarenessUnnecessaryInStrongMode: return '$count null-aware access$es will be unnecessary in strong ' 'checking mode'; + case NullabilityFixKind.nullAwareAssignmentUnnecessaryInStrongMode: + return '$count null-aware assignment$s will be unnecessary in strong ' + 'checking mode'; case NullabilityFixKind.removeAs: return '$count cast$s now unnecessary'; case NullabilityFixKind.removeDeadCode: diff --git a/pkg/nnbd_migration/test/edit_plan_test.dart b/pkg/nnbd_migration/test/edit_plan_test.dart index b9e52c09fad..910e7aa96fd 100644 --- a/pkg/nnbd_migration/test/edit_plan_test.dart +++ b/pkg/nnbd_migration/test/edit_plan_test.dart @@ -535,6 +535,25 @@ class EditPlanTest extends AbstractSingleUnitTest { 'var x = 0; var y = 0;'); } + Future test_informativeMessageForToken() async { + await analyze('f(x) => x + 1;'); + var sum = findNode.binary('+'); + var info = _MockInfo(); + var changes = checkPlan( + planner.passThrough(sum, innerPlans: [ + planner.informativeMessageForToken(sum, sum.operator, info: info) + ]), + 'f(x) => x + 1;', + expectedIncludingInformative: 'f(x) => x 1;'); + var expectedOffset = sum.operator.offset; + expect(changes.keys, unorderedEquals([expectedOffset])); + expect(changes[expectedOffset], hasLength(1)); + expect(changes[expectedOffset][0].length, '+'.length); + expect(changes[expectedOffset][0].replacement, ''); + expect(changes[expectedOffset][0].isInformative, isTrue); + expect(changes[expectedOffset][0].info, same(info)); + } + Future test_insertText() async { await analyze('final x = 1;'); var variableDeclarationList = findNode.variableDeclarationList('final'); @@ -1581,6 +1600,10 @@ g(a, c) => a..b = throw (c..d); } } +class _MockInfo implements AtomicEditInfo { + noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + class _PrecedenceChecker extends UnifyingAstVisitor { final EditPlanner planner; diff --git a/pkg/nnbd_migration/test/fix_aggregator_test.dart b/pkg/nnbd_migration/test/fix_aggregator_test.dart index f3e3447cf1c..671c1f3033a 100644 --- a/pkg/nnbd_migration/test/fix_aggregator_test.dart +++ b/pkg/nnbd_migration/test/fix_aggregator_test.dart @@ -10,6 +10,7 @@ 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/dart/element/type_provider.dart'; +import 'package:nnbd_migration/nnbd_migration.dart'; import 'package:nnbd_migration/src/decorated_type.dart'; import 'package:nnbd_migration/src/edit_plan.dart'; import 'package:nnbd_migration/src/fix_aggregator.dart'; @@ -54,6 +55,95 @@ class FixAggregatorTest extends FixAggregatorTestBase { expect(previewInfo.applyTo(code), 'f(a, b) => (a! + b!)!;'); } + Future test_assignment_add_null_check() async { + var content = 'f(int x, int y) => x += y;'; + await analyze(content); + var previewInfo = run({ + findNode.assignment('+='): NodeChangeForAssignment()..addNullCheck(null) + }); + expect(previewInfo.applyTo(code), 'f(int x, int y) => (x += y)!;'); + } + + Future test_assignment_change_lhs() async { + var content = 'f(List x, int y) => x[0] += y;'; + await analyze(content); + var previewInfo = run({ + findNode.assignment('+='): NodeChangeForAssignment(), + findNode.index('[0]').target: NodeChangeForExpression() + ..addNullCheck(null) + }); + expect(previewInfo.applyTo(code), 'f(List x, int y) => x![0] += y;'); + } + + Future test_assignment_change_rhs() async { + var content = 'f(int x, int y) => x += y;'; + await analyze(content); + var assignment = findNode.assignment('+='); + var previewInfo = run({ + assignment: NodeChangeForAssignment(), + assignment.rightHandSide: NodeChangeForExpression()..addNullCheck(null) + }); + expect(previewInfo.applyTo(code), 'f(int x, int y) => x += y!;'); + } + + Future test_assignment_compound_with_bad_combined_type() async { + var content = 'f(int x, int y) => x += y;'; + await analyze(content); + var previewInfo = run({ + findNode.assignment('+='): NodeChangeForAssignment() + ..isCompoundAssignmentWithBadCombinedType = true + }); + expect(previewInfo.applyTo(code), content); + expect(previewInfo, hasLength(1)); + var edit = previewInfo[content.indexOf('+=')].single; + expect(edit.info.description, + NullabilityFixDescription.compoundAssignmentHasBadCombinedType); + expect(edit.isInformative, isTrue); + expect(edit.length, '+='.length); + } + + Future test_assignment_compound_with_nullable_source() async { + var content = 'f(int x, int y) => x += y;'; + await analyze(content); + var previewInfo = run({ + findNode.assignment('+='): NodeChangeForAssignment() + ..isCompoundAssignmentWithNullableSource = true + }); + expect(previewInfo.applyTo(code), content); + expect(previewInfo, hasLength(1)); + var edit = previewInfo[content.indexOf('+=')].single; + expect(edit.info.description, + NullabilityFixDescription.compoundAssignmentHasNullableSource); + expect(edit.isInformative, isTrue); + expect(edit.length, '+='.length); + } + + Future test_assignment_introduce_as() async { + var content = 'f(int x, int y) => x += y;'; + await analyze(content); + var previewInfo = run({ + findNode.assignment('+='): NodeChangeForAssignment() + ..introduceAs(nnbdTypeProvider.intType, null) + }); + expect(previewInfo.applyTo(code), 'f(int x, int y) => (x += y) as int;'); + } + + Future test_assignment_weak_null_aware() async { + var content = 'f(int x, int y) => x ??= y;'; + await analyze(content); + var previewInfo = run({ + findNode.assignment('??='): NodeChangeForAssignment() + ..isWeakNullAware = true + }, warnOnWeakCode: true); + expect(previewInfo.applyTo(code), content); + expect(previewInfo, hasLength(1)); + var edit = previewInfo[content.indexOf('??=')].single; + expect(edit.info.description, + NullabilityFixDescription.nullAwareAssignmentUnnecessaryInStrongMode); + expect(edit.isInformative, isTrue); + expect(edit.length, '??='.length); + } + Future test_eliminateDeadIf_changesInKeptCode() async { await analyze(''' f(int i, int/*?*/ j) { diff --git a/pkg/nnbd_migration/test/fix_builder_test.dart b/pkg/nnbd_migration/test/fix_builder_test.dart index e8b600b47cb..69c96cbeb23 100644 --- a/pkg/nnbd_migration/test/fix_builder_test.dart +++ b/pkg/nnbd_migration/test/fix_builder_test.dart @@ -47,6 +47,18 @@ class FixBuilderTest extends EdgeBuilderTestBase { TypeMatcher() .having((c) => c.addRequiredKeyword, 'addRequiredKeyword', true); + static final isCompoundAssignmentNullableSource = + TypeMatcher().having( + (c) => c.isCompoundAssignmentWithNullableSource, + 'isCompoundAssignmentWithNullableSource', + true); + + static final isCompoundAssignmentBadCombinedType = + TypeMatcher().having( + (c) => c.isCompoundAssignmentWithBadCombinedType, + 'isCompoundAssignmentWithBadCombinedType', + true); + static final isMakeNullable = TypeMatcher() .having((c) => c.makeNullable, 'makeNullable', true) .having((c) => c.nullabilityHint, 'nullabilityHint', isNull); @@ -84,6 +96,10 @@ class FixBuilderTest extends EdgeBuilderTestBase { TypeMatcher().having( (c) => c.changeToRequiredKeyword, 'changeToRequiredKeyword', true); + static final isWeakNullAwareAssignment = + TypeMatcher() + .having((c) => c.isWeakNullAware, 'isWeakNullAware', true); + DartType get dynamicType => postMigrationTypeProvider.dynamicType; DartType get objectType => postMigrationTypeProvider.objectType; @@ -349,7 +365,8 @@ _f(bool/*?*/ x, bool/*?*/ y) => x != null && (x ??= y) != null; // On the RHS of the `&&`, `x` is promoted to non-nullable, but it is still // considered to be a nullable assignment target, so no null check is // generated for `y`. - visitSubexpression(findNode.binary('&&'), 'bool'); + visitSubexpression(findNode.binary('&&'), 'bool', + changes: {findNode.assignment('??='): isWeakNullAwareAssignment}); } Future @@ -1047,6 +1064,48 @@ f() => true; visitSubexpression(findNode.booleanLiteral('true'), 'bool'); } + Future test_compound_assignment_nullable_result_bad() async { + await analyze(''' +abstract class C { + C/*?*/ operator+(int i); +} +f(C c) { + c += 1; +} +'''); + var assignment = findNode.assignment('+='); + visitSubexpression(assignment, 'C?', + changes: {assignment: isCompoundAssignmentBadCombinedType}); + } + + Future test_compound_assignment_nullable_result_ok() async { + await analyze(''' +abstract class C { + C/*?*/ operator+(int i); +} +abstract class D { + void set x(C/*?*/ value); + C/*!*/ get x; + f() { + x += 1; + } +} +'''); + var assignment = findNode.assignment('+='); + visitSubexpression(assignment, 'C?'); + } + + Future test_compound_assignment_nullable_source() async { + await analyze(''' +_f(int/*?*/ x) { + x += 1; +} +'''); + var assignment = findNode.assignment('+='); + visitSubexpression(assignment, 'int', + changes: {assignment: isCompoundAssignmentNullableSource}); + } + Future test_conditionalExpression_dead_else_remove() async { await analyze('_f(int x, int/*?*/ y) => x != null ? x + 1 : y + 1.0;'); var expression = findNode.conditionalExpression('x != null'); @@ -1814,6 +1873,49 @@ _f(_C/*?*/ c) => c.toString(); visitSubexpression(findNode.methodInvocation('c.toString'), 'String'); } + Future test_null_aware_assignment_non_nullable_source() async { + await analyze(''' +abstract class C { + int/*!*/ f(); + g(int/*!*/ x) { + x ??= f(); + } +} +'''); + var assignment = findNode.assignment('??='); + visitSubexpression(assignment, 'int', + changes: {assignment: isWeakNullAwareAssignment}); + } + + Future test_null_aware_assignment_nullable_rhs_needs_check() async { + await analyze(''' +abstract class C { + void set x(int/*!*/ value); + int/*?*/ get x; + int/*?*/ f(); + g() { + x ??= f(); + } +} +'''); + var assignment = findNode.assignment('??='); + visitSubexpression(assignment, 'int', + changes: {assignment.rightHandSide: isNullCheck}); + } + + Future test_null_aware_assignment_nullable_rhs_ok() async { + await analyze(''' +abstract class C { + int/*?*/ f(); + g(int/*?*/ x) { + x ??= f(); + } +} +'''); + var assignment = findNode.assignment('??='); + visitSubexpression(assignment, 'int?'); + } + Future test_nullAssertion_promotes() async { await analyze(''' _f(bool/*?*/ x) => x && x; diff --git a/pkg/nnbd_migration/test/front_end/info_builder_test.dart b/pkg/nnbd_migration/test/front_end/info_builder_test.dart index b9e06f24a9e..ece8325f824 100644 --- a/pkg/nnbd_migration/test/front_end/info_builder_test.dart +++ b/pkg/nnbd_migration/test/front_end/info_builder_test.dart @@ -364,6 +364,56 @@ void main() { kind: NullabilityFixKind.addLateDueToTestSetup); } + Future test_compound_assignment_nullable_result() async { + var unit = await buildInfoForSingleTestFile(''' +abstract class C { + C/*?*/ operator+(int i); +} +void f(C/*!*/ a, int b) { + a += b; +} +''', migratedContent: ''' +abstract class C { + C/*?*/ operator+(int i); +} +void f(C/*!*/ a, int b) { + a += b; +} +'''); + var operator = '+='; + var operatorOffset = unit.content.indexOf(operator); + var region = + unit.regions.where((region) => region.offset == operatorOffset).single; + assertRegion( + region: region, + length: operator.length, + explanation: 'Compound assignment has bad combined type', + kind: NullabilityFixKind.compoundAssignmentHasBadCombinedType, + edits: isEmpty); + } + + Future test_compound_assignment_nullable_source() async { + var unit = await buildInfoForSingleTestFile(''' +void f(int/*?*/ a, int b) { + a += b; +} +''', migratedContent: ''' +void f(int/*?*/ a, int b) { + a += b; +} +'''); + var operator = '+='; + var operatorOffset = unit.content.indexOf(operator); + var region = + unit.regions.where((region) => region.offset == operatorOffset).single; + assertRegion( + region: region, + length: operator.length, + explanation: 'Compound assignment has nullable source', + kind: NullabilityFixKind.compoundAssignmentHasNullableSource, + edits: isEmpty); + } + Future test_conditionFalseInStrongMode_expression() async { var unit = await buildInfoForSingleTestFile( 'int f(String s) => s == null ? 0 : s.length;',