Skip to content

Commit

Permalink
Migration: prioritize changes differently if they're due to hints.
Browse files Browse the repository at this point in the history 
Change-Id: I4dfdd0287e488dfac21923c71741d2218a2c070c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/143334
Reviewed-by: Samuel Rawlins <srawlins@google.com>
  • Loading branch information
@stereotype441
stereotype441 authored and commit-bot@chromium.org committed Apr 14, 2020
1 parent 46b49d2 commit 1d3105e
Show file tree
Hide file tree
Showing 11 changed files with 244 additions and 58 deletions.
11 changes: 11 additions & 0 deletions pkg/analysis_server/lib/src/edit/nnbd_migration/info_builder.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,9 @@ class InfoBuilder {
// associated with a parameter and insert an assert if it is.
edits.add(EditDetail('Add /*!*/ hint', offset, 0, '/*!*/'));
break;
case NullabilityFixKind.checkExpressionDueToHint:
edits.add(EditDetail('Remove /*!*/ hint', offset, 5, ''));
break;
case NullabilityFixKind.downcastExpression:
case NullabilityFixKind.otherCastExpression:
// There's no useful hint to apply to casts.
Expand All @@ -158,6 +161,14 @@ class InfoBuilder {
edits.add(EditDetail('Add /*!*/ hint', offset, 0, '/*!*/'));
edits.add(EditDetail('Add /*?*/ hint', offset, 0, '/*?*/'));
break;
case NullabilityFixKind.makeTypeNullableDueToHint:
edits.add(EditDetail('Add /*!*/ hint', offset, 5, '/*!*/'));
edits.add(EditDetail('Remove /*?*/ hint', offset, 5, ''));
break;
case NullabilityFixKind.typeNotMadeNullableDueToHint:
edits.add(EditDetail('Remove /*!*/ hint', offset, 5, ''));
edits.add(EditDetail('Add /*?*/ hint', offset, 5, '/*?*/'));
break;
}
return edits;
}
Expand Down
8 changes: 8 additions & 0 deletions pkg/analysis_server/lib/src/edit/nnbd_migration/unit_renderer.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ class UnitRenderer {
NullabilityFixKind.addType,
NullabilityFixKind.replaceVar,
NullabilityFixKind.removeAs,
NullabilityFixKind.checkExpressionDueToHint,
NullabilityFixKind.makeTypeNullableDueToHint,
NullabilityFixKind.removeLanguageVersionComment
];

Expand Down Expand Up @@ -246,10 +248,14 @@ class UnitRenderer {
return '$count cast$s (non-downcast) added';
case NullabilityFixKind.checkExpression:
return '$count null check$s added';
case NullabilityFixKind.checkExpressionDueToHint:
return '$count null check hint$s converted to null check$s';
case NullabilityFixKind.addType:
return '$count type$s added';
case NullabilityFixKind.makeTypeNullable:
return '$count type$s made nullable';
case NullabilityFixKind.makeTypeNullableDueToHint:
return '$count nullability hint$s converted to ?$s';
case NullabilityFixKind.removeAs:
return '$count cast$s now unnecessary';
case NullabilityFixKind.removeDeadCode:
Expand All @@ -262,6 +268,8 @@ class UnitRenderer {
return "$count 'var' declaration$s replaced";
case NullabilityFixKind.typeNotMadeNullable:
return '$count type$s not made nullable';
case NullabilityFixKind.typeNotMadeNullableDueToHint:
return '$count type$s not made nullable due to hint$s';
}
throw StateError('Null kind');
}
Expand Down
119 changes: 99 additions & 20 deletions pkg/analysis_server/test/src/edit/nnbd_migration/info_builder_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,14 @@ class InfoBuilderTest extends NnbdMigrationTestBase {
expect(entryInfo.description, descriptionMatcher);
}

List<RegionInfo> getNonInformativeRegions(List<RegionInfo> regions) {
return regions
.where((region) =>
region.kind != NullabilityFixKind.typeNotMadeNullable &&
region.kind != NullabilityFixKind.typeNotMadeNullableDueToHint)
.toList();
}

Future<void> test_discardCondition() async {
var unit = await buildInfoForSingleTestFile('''
void g(int i) {
Expand Down Expand Up @@ -293,10 +301,7 @@ void g(int i) {
var migratedContent = 'int /*!*/ f(num /*!*/ n) => n as int;';
var unit = await buildInfoForSingleTestFile(content,
migratedContent: migratedContent);
var regions = unit.regions
.where(
(region) => region.kind != NullabilityFixKind.typeNotMadeNullable)
.toList();
var regions = getNonInformativeRegions(unit.regions);
expect(regions, hasLength(1));
var region = regions.single;
var regionTarget = ' as int';
Expand All @@ -316,10 +321,7 @@ void g(int i) {
var migratedContent = 'int?/*?*/ f(num /*!*/ n) => n as int?;';
var unit = await buildInfoForSingleTestFile(content,
migratedContent: migratedContent);
var regions = unit.regions
.where(
(region) => region.kind != NullabilityFixKind.typeNotMadeNullable)
.toList();
var regions = getNonInformativeRegions(unit.regions);
var regionTarget = ' as int?';
var offset = migratedContent.indexOf(regionTarget);
var region = regions.where((region) => region.offset == offset).single;
Expand All @@ -340,10 +342,7 @@ void g(int i) {
var migratedContent = 'int?/*?*/ f(num?/*?*/ n) => n as int?;';
var unit = await buildInfoForSingleTestFile(content,
migratedContent: migratedContent);
var regions = unit.regions
.where(
(region) => region.kind != NullabilityFixKind.typeNotMadeNullable)
.toList();
var regions = getNonInformativeRegions(unit.regions);
var regionTarget = ' as int?';
var offset = migratedContent.indexOf(regionTarget);
var region = regions.where((region) => region.offset == offset).single;
Expand All @@ -361,10 +360,7 @@ void g(int i) {
var migratedContent = 'int /*!*/ f(num?/*?*/ n) => n as int;';
var unit = await buildInfoForSingleTestFile(content,
migratedContent: migratedContent);
var regions = unit.regions
.where(
(region) => region.kind != NullabilityFixKind.typeNotMadeNullable)
.toList();
var regions = getNonInformativeRegions(unit.regions);
var regionTarget = ' as int';
var offset = migratedContent.indexOf(regionTarget);
var region = regions.where((region) => region.offset == offset).single;
Expand Down Expand Up @@ -540,6 +536,30 @@ C /*!*/ _f(C c) => (c + c)!;
kind: NullabilityFixKind.checkExpression);
}

Future<void> test_nullCheck_dueToHint() async {
var content = 'int f(int/*?*/ x) => x/*!*/;';
var migratedContent = 'int f(int?/*?*/ x) => x!/*!*/;';
var unit = await buildInfoForSingleTestFile(content,
migratedContent: migratedContent);
var regions = unit.fixRegions;
expect(regions, hasLength(2));
// regions[0] is `int?`.
var region = regions[1];
assertRegion(
region: region,
offset: migratedContent.indexOf('!/*!*/'),
explanation: 'Accepted a null check hint',
kind: NullabilityFixKind.checkExpressionDueToHint);
// Note that traces are still included.
expect(region.traces, isNotEmpty);
var textToRemove = '/*!*/';
assertEdit(
edit: region.edits.single,
offset: content.indexOf(textToRemove),
length: textToRemove.length,
replacement: '');
}

Future<void> test_nullCheck_onMemberAccess() async {
var unit = await buildInfoForSingleTestFile('''
class C {
Expand Down Expand Up @@ -680,10 +700,7 @@ int f(Object o) {
''';
var unit = await buildInfoForSingleTestFile(content,
migratedContent: migratedContent);
var regions = unit.regions
.where(
(region) => region.kind != NullabilityFixKind.typeNotMadeNullable)
.toList();
var regions = getNonInformativeRegions(unit.regions);
expect(regions, hasLength(1));
var region = regions.single;
var regionTarget = ' as int';
Expand Down Expand Up @@ -911,6 +928,36 @@ String? g() => 1 == 2 ? "Hello" : null;
explanation: "Changed type 'String' to be nullable");
}

Future<void> test_type_made_nullable_due_to_hint() async {
var content = 'int/*?*/ x = 0;';
var migratedContent = 'int?/*?*/ x = 0;';
var unit = await buildInfoForSingleTestFile(content,
migratedContent: migratedContent);
var region = unit.fixRegions.single;
assertRegion(
region: region,
offset: migratedContent.indexOf('?/*?*/'),
explanation:
"Changed type 'int' to be nullable, due to a nullability hint",
kind: NullabilityFixKind.makeTypeNullableDueToHint);
// Note that traces are still included.
expect(region.traces, isNotNull);
var textToRemove = '/*?*/';
var edits = region.edits;
expect(edits, hasLength(2));
var editsByDescription = {for (var edit in edits) edit.description: edit};
assertEdit(
edit: editsByDescription['Add /*!*/ hint'],
offset: content.indexOf(textToRemove),
length: textToRemove.length,
replacement: '/*!*/');
assertEdit(
edit: editsByDescription['Remove /*?*/ hint'],
offset: content.indexOf(textToRemove),
length: textToRemove.length,
replacement: '');
}

Future<void> test_type_not_made_nullable() async {
var unit = await buildInfoForSingleTestFile('int i = 0;',
migratedContent: 'int i = 0;');
Expand All @@ -925,4 +972,36 @@ String? g() => 1 == 2 ? "Hello" : null;
expect(region.traces, isEmpty);
expect(region.kind, NullabilityFixKind.typeNotMadeNullable);
}

Future<void> test_type_not_made_nullable_due_to_hint() async {
var content = 'int/*!*/ i = 0;';
var migratedContent = 'int /*!*/ i = 0;';
var unit = await buildInfoForSingleTestFile(content,
migratedContent: migratedContent);
var region = unit.regions
.where((regionInfo) =>
regionInfo.offset == migratedContent.indexOf(' /*!*/ i'))
.single;
assertRegion(
region: region,
offset: migratedContent.indexOf(' /*!*/ i'),
explanation: "Type 'int' was not made nullable due to a hint",
kind: NullabilityFixKind.typeNotMadeNullableDueToHint);
// Note that traces are still included.
expect(region.traces, isNotNull);
var textToRemove = '/*!*/';
var edits = region.edits;
expect(edits, hasLength(2));
var editsByDescription = {for (var edit in edits) edit.description: edit};
assertEdit(
edit: editsByDescription['Add /*?*/ hint'],
offset: content.indexOf(textToRemove),
length: textToRemove.length,
replacement: '/*?*/');
assertEdit(
edit: editsByDescription['Remove /*!*/ hint'],
offset: content.indexOf(textToRemove),
length: textToRemove.length,
replacement: '');
}
}
5 changes: 4 additions & 1 deletion pkg/analysis_server/test/src/edit/nnbd_migration/unit_renderer_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,10 @@ class <span id="...">C</span> {
}
// The only kinds that should not be displayed are those associated with a
// place where nothing interesting occurred.
expect(nonDisplayedKinds, {NullabilityFixKind.typeNotMadeNullable});
expect(nonDisplayedKinds, {
NullabilityFixKind.typeNotMadeNullable,
NullabilityFixKind.typeNotMadeNullableDueToHint
});
}

Future<void> test_navContentContainsEscapedHtml() async {
Expand Down
26 changes: 26 additions & 0 deletions pkg/nnbd_migration/lib/nnbd_migration.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ class NullabilityFixDescription {
kind: NullabilityFixKind.checkExpression,
);

/// An expression's value will be null-checked due to a hint.
static const checkExpressionDueToHint = const NullabilityFixDescription._(
appliedMessage: 'Accepted a null check hint',
kind: NullabilityFixKind.checkExpressionDueToHint,
);

/// An unnecessary downcast has been discarded.
static const removeLanguageVersionComment = const NullabilityFixDescription._(
appliedMessage: 'Removed language version comment so that NNBD features '
Expand Down Expand Up @@ -110,6 +116,15 @@ class NullabilityFixDescription {
kind: NullabilityFixKind.makeTypeNullable,
);

/// An explicit type mentioned in the source program will be made
/// nullable due to a nullability hint.
factory NullabilityFixDescription.makeTypeNullableDueToHint(String type) =>
NullabilityFixDescription._(
appliedMessage:
"Changed type '$type' to be nullable, due to a nullability hint",
kind: NullabilityFixKind.makeTypeNullableDueToHint,
);

/// A 'var' declaration needs to be replaced with an explicit type.
factory NullabilityFixDescription.replaceVar(String typeText) =>
NullabilityFixDescription._(
Expand All @@ -125,6 +140,14 @@ class NullabilityFixDescription {
kind: NullabilityFixKind.typeNotMadeNullable,
);

/// An explicit type mentioned in the source program does not need to be made
/// nullable.
factory NullabilityFixDescription.typeNotMadeNullableDueToHint(String type) =>
NullabilityFixDescription._(
appliedMessage: "Type '$type' was not made nullable due to a hint",
kind: NullabilityFixKind.typeNotMadeNullableDueToHint,
);

const NullabilityFixDescription._(
{@required this.appliedMessage, @required this.kind});

Expand All @@ -151,15 +174,18 @@ class NullabilityFixDescription {
enum NullabilityFixKind {
addRequired,
checkExpression,
checkExpressionDueToHint,
downcastExpression,
addType,
makeTypeNullable,
makeTypeNullableDueToHint,
otherCastExpression,
removeAs,
removeDeadCode,
removeLanguageVersionComment,
replaceVar,
typeNotMadeNullable,
typeNotMadeNullableDueToHint,
}

/// Provisional API for DartFix to perform nullability migration.
Expand Down
54 changes: 39 additions & 15 deletions pkg/nnbd_migration/lib/src/fix_aggregator.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -597,37 +597,61 @@ class NodeChangeForSimpleFormalParameter
/// Implementation of [NodeChange] specialized for operating on [TypeAnnotation]
/// nodes.
class NodeChangeForTypeAnnotation extends NodeChange<TypeAnnotation> {
/// Indicates whether the type should be made nullable by adding a `?`.
bool makeNullable = false;
bool _makeNullable = false;

bool _nullabilityDueToHint = false;

/// The decorated type of the type annotation, or `null` if there is no
/// decorated type info of interest. If [makeNullable] is `true`, the node
/// from this type will be attached to the edit that adds the `?`. If
/// [makeNullable] is `false`, the node from this type will be attached to the
/// [_makeNullable] is `false`, the node from this type will be attached to the
/// information about why the node wasn't made nullable.
DecoratedType decoratedType;
DecoratedType _decoratedType;

NodeChangeForTypeAnnotation() : super._();

@override
bool get isInformative => !makeNullable;
bool get isInformative => !_makeNullable;

/// Indicates whether the type should be made nullable by adding a `?`.
bool get makeNullable => _makeNullable;

/// Indicates whether we are making the type nullable due to a hint.
bool get makeNullableDueToHint => _nullabilityDueToHint;

void recordNullability(DecoratedType decoratedType, bool makeNullable,
{bool nullabilityDueToHint: false}) {
_decoratedType = decoratedType;
_makeNullable = makeNullable;
_nullabilityDueToHint = nullabilityDueToHint;
}

@override
EditPlan _apply(TypeAnnotation node, FixAggregator aggregator) {
var innerPlan = aggregator.innerPlanForNode(node);
if (decoratedType == null) return innerPlan;
if (makeNullable) {
if (_decoratedType == null) return innerPlan;
var typeName = _decoratedType.type.getDisplayString(withNullability: false);
var fixReasons = {FixReasonTarget.root: _decoratedType.node};
if (_makeNullable) {
NullabilityFixDescription description;
if (_nullabilityDueToHint) {
description =
NullabilityFixDescription.makeTypeNullableDueToHint(typeName);
} else {
description = NullabilityFixDescription.makeTypeNullable(typeName);
}
return aggregator.planner.makeNullable(innerPlan,
info: AtomicEditInfo(
NullabilityFixDescription.makeTypeNullable(
decoratedType.type.getDisplayString(withNullability: false)),
{FixReasonTarget.root: decoratedType.node}));
info: AtomicEditInfo(description, fixReasons));
} else {
NullabilityFixDescription description;
if (_nullabilityDueToHint) {
description =
NullabilityFixDescription.typeNotMadeNullableDueToHint(typeName);
} else {
description = NullabilityFixDescription.typeNotMadeNullable(typeName);
}
return aggregator.planner.explainNonNullable(innerPlan,
info: AtomicEditInfo(
NullabilityFixDescription.typeNotMadeNullable(
decoratedType.type.getDisplayString(withNullability: false)),
{FixReasonTarget.root: decoratedType.node}));
info: AtomicEditInfo(description, fixReasons));
}
}
}
Expand Down
Loading

0 comments on commit 1d3105e

Please sign in to comment.