Skip to content

Commit c0f23ce

Browse files
MichaelRFairhurstMichaelRFairhurst
committed
[nnbd_migration] fix kind -> description, drop empty, tests
Change-Id: I7224167ae26344336f665827b145c57319076d73 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/111382 Reviewed-by: Dan Rubel <[email protected]> Reviewed-by: Paul Berry <[email protected]>
1 parent 330a401 commit c0f23ce

File tree

5 files changed

+205
-63
lines changed

5 files changed

+205
-63
lines changed

‎pkg/analysis_server/lib/src/edit/fix/non_nullable_fix.dart‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,6 @@ class NullabilityMigrationAdapter implements NullabilityMigrationListener {
196196

197197
@override
198198
void addFix(SingleNullabilityFix fix) {
199-
// TODO(danrubel): Update the description based upon the [fix.kind]
200-
listener.addSuggestion(fix.kind.appliedMessage, fix.location);
199+
listener.addSuggestion(fix.description.appliedMessage, fix.location);
201200
}
202201
}

‎pkg/nnbd_migration/lib/nnbd_migration.dart‎

Lines changed: 32 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -8,43 +8,50 @@ import 'package:analyzer/src/generated/source.dart';
88
import 'package:meta/meta.dart';
99
import 'package:nnbd_migration/src/nullability_migration_impl.dart';
1010

11-
/// Kinds of fixes that might be performed by nullability migration.
12-
class NullabilityFixKind {
13-
/// An import needs to be added.
14-
static const addImport =
15-
const NullabilityFixKind._(appliedMessage: 'Add an import');
16-
17-
/// A formal parameter needs to have a required modifier added.
18-
static const addRequired =
19-
const NullabilityFixKind._(appliedMessage: "Add a 'required' modifier");
20-
21-
/// An expression's value needs to be null-checked.
22-
static const checkExpression = const NullabilityFixKind._(
23-
appliedMessage: 'Added a null check to an expression',
24-
);
25-
26-
/// An explicit type mentioned in the source program needs to be made
27-
/// nullable.
28-
static const makeTypeNullable = const NullabilityFixKind._(
29-
appliedMessage: 'Changed a type to be nullable',
30-
);
31-
11+
/// Description of fixes that might be performed by nullability migration.
12+
class NullabilityFixDescription {
3213
/// An if-test or conditional expression needs to have its "then" branch
3314
/// discarded.
34-
static const discardThen = const NullabilityFixKind._(
15+
static const discardThen = const NullabilityFixDescription._(
3516
appliedMessage: 'Discarded an unreachable conditional then branch',
3617
);
3718

3819
/// An if-test or conditional expression needs to have its "else" branch
3920
/// discarded.
40-
static const discardElse = const NullabilityFixKind._(
21+
static const discardElse = const NullabilityFixDescription._(
4122
appliedMessage: 'Discarded an unreachable conditional else branch',
4223
);
4324

25+
/// An expression's value needs to be null-checked.
26+
static const checkExpression = const NullabilityFixDescription._(
27+
appliedMessage: 'Added a non-null assertion to nullable expression',
28+
);
29+
4430
/// A message used by dartfix to indicate a fix has been applied.
4531
final String appliedMessage;
4632

47-
const NullabilityFixKind._({@required this.appliedMessage});
33+
/// An import needs to be added.
34+
factory NullabilityFixDescription.addImport(String uri) =>
35+
NullabilityFixDescription._(appliedMessage: 'Add import $uri');
36+
37+
/// A formal parameter needs to have a required modifier added.
38+
factory NullabilityFixDescription.addRequired(
39+
String className, String functionName, String paramName) =>
40+
NullabilityFixDescription._(
41+
appliedMessage:
42+
"Add 'required' modifier to parameter $paramName in " +
43+
(className == null
44+
? functionName
45+
: '$className.$functionName'));
46+
47+
/// An explicit type mentioned in the source program needs to be made
48+
/// nullable.
49+
factory NullabilityFixDescription.makeTypeNullable(String type) =>
50+
NullabilityFixDescription._(
51+
appliedMessage: 'Changed type ${type} to be nullable',
52+
);
53+
54+
const NullabilityFixDescription._({@required this.appliedMessage});
4855
}
4956

5057
/// Provisional API for DartFix to perform nullability migration.
@@ -89,7 +96,7 @@ abstract class NullabilityMigrationListener {
8996
/// achieve.
9097
abstract class SingleNullabilityFix {
9198
/// What kind of fix this is.
92-
NullabilityFixKind get kind;
99+
NullabilityFixDescription get description;
93100

94101
/// Location of the change, for reporting to the user.
95102
Location get location;

‎pkg/nnbd_migration/lib/src/nullability_migration_impl.dart‎

Lines changed: 47 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import 'package:analysis_server/src/protocol_server.dart';
66
import 'package:analyzer/dart/analysis/results.dart';
77
import 'package:analyzer/src/generated/source.dart';
8+
import 'package:meta/meta.dart';
89
import 'package:nnbd_migration/nnbd_migration.dart';
910
import 'package:nnbd_migration/src/decorated_type.dart';
1011
import 'package:nnbd_migration/src/edge_builder.dart';
@@ -39,16 +40,7 @@ class NullabilityMigrationImpl implements NullabilityMigration {
3940

4041
void finish() {
4142
_graph.propagate();
42-
for (var entry in _variables.getPotentialModifications().entries) {
43-
var source = entry.key;
44-
for (var potentialModification in entry.value) {
45-
var fix = _SingleNullabilityFix(source, potentialModification);
46-
listener.addFix(fix);
47-
for (var edit in potentialModification.modifications) {
48-
listener.addEdit(fix, edit);
49-
}
50-
}
51-
}
43+
broadcast(_variables, listener);
5244
}
5345

5446
void prepareInput(ResolvedUnitResult result) {
@@ -62,6 +54,27 @@ class NullabilityMigrationImpl implements NullabilityMigration {
6254
unit.accept(EdgeBuilder(result.typeProvider, result.typeSystem, _variables,
6355
_graph, unit.declaredElement.source, _permissive ? listener : null));
6456
}
57+
58+
@visibleForTesting
59+
static void broadcast(
60+
Variables variables, NullabilityMigrationListener listener) {
61+
for (var entry in variables.getPotentialModifications().entries) {
62+
var source = entry.key;
63+
final lineInfo = LineInfo.fromContent(source.contents.data);
64+
for (var potentialModification in entry.value) {
65+
var modifications = potentialModification.modifications;
66+
if (modifications.isEmpty) {
67+
continue;
68+
}
69+
var fix =
70+
_SingleNullabilityFix(source, potentialModification, lineInfo);
71+
listener.addFix(fix);
72+
for (var edit in modifications) {
73+
listener.addEdit(fix, edit);
74+
}
75+
}
76+
}
77+
}
6578
}
6679

6780
/// Implementation of [SingleNullabilityFix] used internally by
@@ -71,51 +84,57 @@ class _SingleNullabilityFix extends SingleNullabilityFix {
7184
final Source source;
7285

7386
@override
74-
final NullabilityFixKind kind;
87+
final NullabilityFixDescription description;
88+
89+
Location _location;
7590

76-
factory _SingleNullabilityFix(
77-
Source source, PotentialModification potentialModification) {
91+
factory _SingleNullabilityFix(Source source,
92+
PotentialModification potentialModification, LineInfo lineInfo) {
7893
// TODO(paulberry): once everything is migrated into the analysis server,
7994
// the migration engine can just create SingleNullabilityFix objects
8095
// directly and set their kind appropriately; we won't need to translate the
8196
// kinds using a bunch of `is` checks.
82-
NullabilityFixKind kind;
97+
NullabilityFixDescription desc;
8398
if (potentialModification is ExpressionChecks) {
84-
kind = NullabilityFixKind.checkExpression;
99+
desc = NullabilityFixDescription.checkExpression;
85100
} else if (potentialModification is DecoratedTypeAnnotation) {
86-
kind = NullabilityFixKind.makeTypeNullable;
101+
desc = NullabilityFixDescription.makeTypeNullable(
102+
potentialModification.type.toString());
87103
} else if (potentialModification is ConditionalModification) {
88-
kind = potentialModification.discard.keepFalse
89-
? NullabilityFixKind.discardThen
90-
: NullabilityFixKind.discardElse;
104+
desc = potentialModification.discard.keepFalse
105+
? NullabilityFixDescription.discardThen
106+
: NullabilityFixDescription.discardElse;
91107
} else if (potentialModification is PotentiallyAddImport) {
92-
kind = NullabilityFixKind.addImport;
108+
desc =
109+
NullabilityFixDescription.addImport(potentialModification.importPath);
93110
} else if (potentialModification is PotentiallyAddRequired) {
94-
kind = NullabilityFixKind.addRequired;
111+
desc = NullabilityFixDescription.addRequired(
112+
potentialModification.className,
113+
potentialModification.methodName,
114+
potentialModification.parameterName);
95115
} else {
96116
throw new UnimplementedError('TODO(paulberry)');
97117
}
98118

99119
Location location;
100120

101-
// TODO(devoncarew): Calculate line and column info from the source+offset.
102121
if (potentialModification.modifications.isNotEmpty) {
122+
final locationInfo = lineInfo
123+
.getLocation(potentialModification.modifications.first.offset);
103124
location = new Location(
104125
source.fullName,
105126
potentialModification.modifications.first.offset,
106127
potentialModification.modifications.first.length,
107-
0, // TODO(devoncarew): calculate the startLine info
108-
0, // TODO(devoncarew): calculate the startColumn info
128+
locationInfo.lineNumber,
129+
locationInfo.columnNumber,
109130
);
110131
}
111132

112-
return _SingleNullabilityFix._(source, kind, location: location);
133+
return _SingleNullabilityFix._(source, desc, location: location);
113134
}
114135

115-
_SingleNullabilityFix._(this.source, this.kind, {Location location})
136+
_SingleNullabilityFix._(this.source, this.description, {Location location})
116137
: this._location = location;
117138

118139
Location get location => _location;
119-
120-
Location _location;
121140
}

‎pkg/nnbd_migration/lib/src/potential_modification.dart‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -88,16 +88,17 @@ class PotentiallyAddImport extends PotentialModification {
8888
final _usages = <PotentialModification>[];
8989

9090
final int _offset;
91-
final String _importPath;
91+
final String importPath;
9292

9393
PotentiallyAddImport(
94-
AstNode beforeNode, this._importPath, PotentialModification usage)
95-
: _offset = beforeNode.offset {
94+
AstNode beforeNode, String importPath, PotentialModification usage)
95+
: this.forOffset(beforeNode.offset, importPath, usage);
96+
97+
PotentiallyAddImport.forOffset(
98+
this._offset, this.importPath, PotentialModification usage) {
9699
_usages.add(usage);
97100
}
98101

99-
get importPath => _importPath;
100-
101102
@override
102103
bool get isEmpty {
103104
for (PotentialModification usage in _usages) {
@@ -111,7 +112,7 @@ class PotentiallyAddImport extends PotentialModification {
111112
// TODO(danrubel): change all of dartfix NNBD to use DartChangeBuilder
112113
@override
113114
Iterable<SourceEdit> get modifications =>
114-
isEmpty ? const [] : [SourceEdit(_offset, 0, "import '$_importPath';\n")];
115+
isEmpty ? const [] : [SourceEdit(_offset, 0, "import '$importPath';\n")];
115116

116117
void addUsage(PotentialModification usage) {
117118
_usages.add(usage);
@@ -124,9 +125,21 @@ class PotentiallyAddRequired extends PotentialModification {
124125
final NullabilityNode _node;
125126

126127
final int _offset;
128+
final String className;
129+
final String methodName;
130+
final String parameterName;
131+
132+
factory PotentiallyAddRequired(
133+
DefaultFormalParameter parameter, NullabilityNode node) {
134+
final element = parameter.declaredElement;
135+
final method = element.enclosingElement;
136+
final cls = method.enclosingElement;
137+
return PotentiallyAddRequired._(
138+
node, parameter.offset, cls.name, method.name, element.name);
139+
}
127140

128-
PotentiallyAddRequired(DefaultFormalParameter parameter, this._node)
129-
: _offset = parameter.offset;
141+
PotentiallyAddRequired._(this._node, this._offset, this.className,
142+
this.methodName, this.parameterName);
130143

131144
@override
132145
bool get isEmpty => _node.isNullable;

‎pkg/nnbd_migration/test/nullability_migration_impl_test.dart‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
import 'package:analyzer/src/generated/source.dart';
6+
import 'package:analyzer/src/generated/timestamped_data.dart';
7+
import 'package:analyzer_plugin/protocol/protocol_common.dart';
8+
import 'package:mockito/mockito.dart';
9+
import 'package:nnbd_migration/nnbd_migration.dart';
10+
import 'package:nnbd_migration/src/nullability_migration_impl.dart';
11+
import 'package:nnbd_migration/src/potential_modification.dart';
12+
import 'package:nnbd_migration/src/variables.dart';
13+
import 'package:test/test.dart';
14+
import 'package:test_reflective_loader/test_reflective_loader.dart';
15+
16+
main() {
17+
defineReflectiveSuite(() {
18+
defineReflectiveTests(NullabilityMigrationImplTest);
19+
});
20+
}
21+
22+
@reflectiveTest
23+
class NullabilityMigrationImplTest {
24+
VariablesMock variables;
25+
26+
void setUp() {
27+
variables = VariablesMock();
28+
}
29+
30+
void test_modification_columnLineInfo() {
31+
final innerModification = PotentialModificationMock();
32+
final potentialModification =
33+
PotentiallyAddImport.forOffset(10, 'foo', innerModification);
34+
final listener = NullabilityMigrationListenerMock();
35+
final source = SourceMock('0123456\n8910');
36+
when(variables.getPotentialModifications()).thenReturn({
37+
source: [potentialModification]
38+
});
39+
40+
when(innerModification.isEmpty).thenReturn(false);
41+
42+
NullabilityMigrationImpl.broadcast(variables, listener);
43+
44+
final fix = verify(listener.addFix(captureAny)).captured.single
45+
as SingleNullabilityFix;
46+
expect(fix.description.appliedMessage, 'Add import foo');
47+
expect(fix.source, source);
48+
expect(fix.location.offset, 10);
49+
expect(fix.location.length, 0);
50+
expect(fix.location.file, '/test.dart');
51+
expect(fix.location.startLine, 2);
52+
expect(fix.location.startColumn, 3);
53+
verifyNever(listener.addDetail(any));
54+
final edit =
55+
verify(listener.addEdit(fix, captureAny)).captured.single as SourceEdit;
56+
expect(edit.offset, 10);
57+
expect(edit.length, 0);
58+
expect(edit.replacement, "import 'foo';\n");
59+
}
60+
61+
void test_noModifications_notReported() {
62+
final potentialModification = PotentialModificationMock();
63+
final listener = NullabilityMigrationListenerMock();
64+
final source = SourceMock('');
65+
when(variables.getPotentialModifications()).thenReturn({
66+
source: [potentialModification]
67+
});
68+
69+
when(potentialModification.modifications).thenReturn([]);
70+
71+
NullabilityMigrationImpl.broadcast(variables, listener);
72+
73+
verifyNever(listener.addFix(any));
74+
verifyNever(listener.addDetail(any));
75+
verifyNever(listener.addEdit(any, any));
76+
}
77+
78+
void test_noPotentialChanges_notReported() {
79+
final listener = NullabilityMigrationListenerMock();
80+
final source = SourceMock('');
81+
when(variables.getPotentialModifications()).thenReturn({source: []});
82+
83+
NullabilityMigrationImpl.broadcast(variables, listener);
84+
85+
verifyNever(listener.addFix(any));
86+
verifyNever(listener.addDetail(any));
87+
verifyNever(listener.addEdit(any, any));
88+
}
89+
}
90+
91+
class NullabilityMigrationListenerMock extends Mock
92+
implements NullabilityMigrationListener {}
93+
94+
class PotentialModificationMock extends Mock implements PotentialModification {}
95+
96+
class SourceMock extends Mock implements Source {
97+
final String _contents;
98+
99+
SourceMock(this._contents);
100+
TimestampedData<String> get contents => TimestampedData<String>(0, _contents);
101+
String get fullName => '/test.dart';
102+
}
103+
104+
class VariablesMock extends Mock implements Variables {}

0 commit comments

Comments
 (0)