Migrator: Add /*required*/ hints when meta not imported.
Currently the migrator can add `@required` from the preview, which will crash
the preview upon rerunning, if package:meta is not imported.
This change instead adds a /*required*/ hint when meta is not imported.
Additionally, /*required*/ is understood by the migrator just as if it were
`@required`.
Fixes https://github.com/dart-lang/sdk/issues/43751
Change-Id: I1ea532246b956feacedd179146372b13c65003df
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/169900
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
diff --git a/pkg/nnbd_migration/lib/src/edge_builder.dart b/pkg/nnbd_migration/lib/src/edge_builder.dart
index 282b622..d10f7f4 100644
--- a/pkg/nnbd_migration/lib/src/edge_builder.dart
+++ b/pkg/nnbd_migration/lib/src/edge_builder.dart
@@ -212,6 +212,8 @@
final Set<PromotableElement> _lateHintedLocals = {};
+ final Set<PromotableElement> _requiredHintedParameters = {};
+
final Map<Token, HintComment> _nullCheckHints = {};
/// Helper that assists us in transforming Iterable methods to their "OrNull"
@@ -745,6 +747,10 @@
if (node.declaredElement.hasRequired) {
// Nothing to do; the implicit default value of `null` will never be
// reached.
+ } else if (_variables.getRequiredHint(source, node) != null) {
+ // Nothing to do; assume the implicit default value of `null` will never
+ // be reached.
+ _requiredHintedParameters.add(node.declaredElement);
} else {
_graph.makeNullable(getOrComputeElementType(node.declaredElement).node,
OptionalFormalParameterOrigin(source, node));
@@ -1536,6 +1542,7 @@
if (!node.inDeclarationContext() &&
node.inGetterContext() &&
!_lateHintedLocals.contains(staticElement) &&
+ !_requiredHintedParameters.contains(staticElement) &&
!_flowAnalysis.isAssigned(staticElement)) {
_graph.makeNullable(type.node, UninitializedReadOrigin(source, node));
}
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 2f9f0fb..b1d3f84 100644
--- a/pkg/nnbd_migration/lib/src/front_end/info_builder.dart
+++ b/pkg/nnbd_migration/lib/src/front_end/info_builder.dart
@@ -123,14 +123,16 @@
'Reason', [_makeTraceEntry(info.description, info.codeReference)]));
}
- /// Return an edit that can be applied.
+ /// Returns a list of edits that can be applied.
List<EditDetail> _computeEdits(
- AtomicEditInfo fixInfo, int offset, String content) {
- EditDetail _removeHint(String description) => EditDetail.fromSourceEdit(
+ AtomicEditInfo fixInfo, int offset, ResolvedUnitResult result) {
+ var content = result.content;
+
+ EditDetail removeHint(String description) => EditDetail.fromSourceEdit(
description,
fixInfo.hintComment.changesToRemove(content).toSourceEdits().single);
- EditDetail _changeHint(String description, String replacement) =>
+ EditDetail changeHint(String description, String replacement) =>
EditDetail.fromSourceEdit(
description,
fixInfo.hintComment
@@ -142,16 +144,27 @@
var fixKind = fixInfo.description.kind;
switch (fixKind) {
case NullabilityFixKind.addLateDueToHint:
- edits.add(_removeHint('Remove /*late*/ hint'));
+ edits.add(removeHint('Remove /*late*/ hint'));
break;
case NullabilityFixKind.addLateFinalDueToHint:
- edits.add(_removeHint('Remove /*late final*/ hint'));
+ edits.add(removeHint('Remove /*late final*/ hint'));
break;
case NullabilityFixKind.addRequired:
- // TODO(brianwilkerson) This doesn't verify that the meta package has
- // been imported.
- edits
- .add(EditDetail("Mark with '@required'.", offset, 0, '@required '));
+ var metaImport =
+ _findImportDirective(result.unit, 'package:meta/meta.dart');
+ if (metaImport == null) {
+ edits.add(
+ EditDetail('Add /*required*/ hint', offset, 0, '/*required*/ '));
+ } else {
+ var prefix = metaImport.prefix?.name;
+ if (prefix == null) {
+ edits.add(
+ EditDetail("Mark with '@required'", offset, 0, '@required '));
+ } else {
+ edits.add(EditDetail(
+ "Mark with '@required'", offset, 0, '@$prefix.required '));
+ }
+ }
break;
case NullabilityFixKind.checkExpression:
// TODO(brianwilkerson) Determine whether we can know that the fix is
@@ -159,7 +172,7 @@
edits.add(EditDetail('Add /*!*/ hint', offset, 0, '/*!*/'));
break;
case NullabilityFixKind.checkExpressionDueToHint:
- edits.add(_removeHint('Remove /*!*/ hint'));
+ edits.add(removeHint('Remove /*!*/ hint'));
break;
case NullabilityFixKind.downcastExpression:
case NullabilityFixKind.otherCastExpression:
@@ -180,12 +193,12 @@
edits.add(EditDetail('Add /*?*/ hint', offset, 0, '/*?*/'));
break;
case NullabilityFixKind.makeTypeNullableDueToHint:
- edits.add(_changeHint('Change to /*!*/ hint', '/*!*/'));
- edits.add(_removeHint('Remove /*?*/ hint'));
+ edits.add(changeHint('Change to /*!*/ hint', '/*!*/'));
+ edits.add(removeHint('Remove /*?*/ hint'));
break;
case NullabilityFixKind.typeNotMadeNullableDueToHint:
- edits.add(_removeHint('Remove /*!*/ hint'));
- edits.add(_changeHint('Change to /*?*/ hint', '/*?*/'));
+ edits.add(removeHint('Remove /*!*/ hint'));
+ edits.add(changeHint('Change to /*?*/ hint', '/*?*/'));
break;
case NullabilityFixKind.addLate:
case NullabilityFixKind.addLateDueToTestSetup:
@@ -363,7 +376,7 @@
}
var info = edit.info;
var edits = info != null
- ? _computeEdits(info, sourceOffset, result.content)
+ ? _computeEdits(info, sourceOffset, result)
: <EditDetail>[];
var lineNumber = lineInfo.getLocation(sourceOffset).lineNumber;
var traces = info == null
@@ -426,6 +439,17 @@
return unitInfo;
}
+ /// Searches [unit] for an import directive whose URI matches [uri], returning
+ /// it if found, or `null` if not found.
+ ImportDirective _findImportDirective(CompilationUnit unit, String uri) {
+ for (var directive in unit.directives) {
+ if (directive is ImportDirective && directive.uriContent == uri) {
+ return directive;
+ }
+ }
+ return null;
+ }
+
TraceEntryInfo _makeTraceEntry(
String description, CodeReference codeReference,
{List<HintAction> hintActions = const []}) {
diff --git a/pkg/nnbd_migration/lib/src/node_builder.dart b/pkg/nnbd_migration/lib/src/node_builder.dart
index 7baf8e8..ad52876 100644
--- a/pkg/nnbd_migration/lib/src/node_builder.dart
+++ b/pkg/nnbd_migration/lib/src/node_builder.dart
@@ -2,6 +2,7 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
+import 'package:_fe_analyzer_shared/src/scanner/token.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:analyzer/dart/element/element.dart';
@@ -228,11 +229,15 @@
@override
DecoratedType visitDefaultFormalParameter(DefaultFormalParameter node) {
var decoratedType = node.parameter.accept(this);
+ var hint = getPrefixHint(node.firstTokenAfterCommentAndMetadata);
if (node.defaultValue != null) {
node.defaultValue.accept(this);
return null;
} else if (node.declaredElement.hasRequired) {
return null;
+ } else if (hint != null && hint.kind == HintCommentKind.required) {
+ _variables.recordRequiredHint(source, node, hint);
+ return null;
}
if (decoratedType == null) {
throw StateError('No type computed for ${node.parameter.runtimeType} '
@@ -889,3 +894,40 @@
throw UnimplementedError(buffer.toString());
}
}
+
+extension on FormalParameter {
+ // TODO(srawlins): Add this to FormalParameter interface.
+ Token get firstTokenAfterCommentAndMetadata {
+ var parameter = this is DefaultFormalParameter
+ ? (this as DefaultFormalParameter).parameter
+ : this as NormalFormalParameter;
+ if (parameter is FieldFormalParameter) {
+ if (parameter.keyword != null) {
+ return parameter.keyword;
+ } else if (parameter.type != null) {
+ return parameter.type.beginToken;
+ } else {
+ return parameter.thisKeyword;
+ }
+ } else if (parameter is FunctionTypedFormalParameter) {
+ if (parameter.covariantKeyword != null) {
+ return parameter.covariantKeyword;
+ } else if (parameter.returnType != null) {
+ return parameter.returnType.beginToken;
+ } else {
+ return parameter.identifier.token;
+ }
+ } else if (parameter is SimpleFormalParameter) {
+ if (parameter.covariantKeyword != null) {
+ return parameter.covariantKeyword;
+ } else if (parameter.keyword != null) {
+ return parameter.keyword;
+ } else if (parameter.type != null) {
+ return parameter.type.beginToken;
+ } else {
+ return parameter.identifier.token;
+ }
+ }
+ return null;
+ }
+}
diff --git a/pkg/nnbd_migration/lib/src/utilities/hint_utils.dart b/pkg/nnbd_migration/lib/src/utilities/hint_utils.dart
index 471149c..28177af 100644
--- a/pkg/nnbd_migration/lib/src/utilities/hint_utils.dart
+++ b/pkg/nnbd_migration/lib/src/utilities/hint_utils.dart
@@ -67,6 +67,17 @@
lateOffset + 'late final'.length,
commentToken.end,
token.offset);
+ } else if (commentText == 'required') {
+ var requiredOffset =
+ commentOffset + commentToken.lexeme.indexOf('required');
+ return HintComment(
+ HintCommentKind.required,
+ commentOffset,
+ commentOffset,
+ requiredOffset,
+ requiredOffset + 'required'.length,
+ commentToken.end,
+ token.offset);
}
}
}
@@ -210,4 +221,8 @@
/// The comment `/*late final*/`, which indicates that the variable
/// declaration should be late and final.
lateFinal,
+
+ /// The comment `/*required*/`, which indicates that the parameter should be
+ /// required.
+ required,
}
diff --git a/pkg/nnbd_migration/lib/src/variables.dart b/pkg/nnbd_migration/lib/src/variables.dart
index c054c01..a3ddc70 100644
--- a/pkg/nnbd_migration/lib/src/variables.dart
+++ b/pkg/nnbd_migration/lib/src/variables.dart
@@ -62,6 +62,8 @@
final _nullabilityHints = <Source, Map<int, HintComment>>{};
+ final _requiredHints = <Source, Map<int, HintComment>>{};
+
final _unnecessaryCasts = <Source, Set<int>>{};
final AlreadyMigratedCodeDecorator _alreadyMigratedCodeDecorator;
@@ -185,6 +187,12 @@
{})[uniqueIdentifierForSpan(node.offset, node.end)];
}
+ /// If the given [node] is preceded by a `/*required*/` hint, returns the
+ /// HintComment for it; otherwise returns `null`. See [recordRequiredHint].
+ HintComment getRequiredHint(Source source, FormalParameter node) {
+ return (_requiredHints[source] ?? {})[node.offset];
+ }
+
/// If the given [expression] is followed by a null check hint (`/*!*/`),
/// returns the HintComment for it; otherwise returns `null`. See
/// [recordNullCheckHint].
@@ -260,6 +268,12 @@
{})[uniqueIdentifierForSpan(node.offset, node.end)] = hintComment;
}
+ /// Records that the given [node] was preceded by a `/*required*/` hint.
+ void recordRequiredHint(
+ Source source, FormalParameter node, HintComment hint) {
+ (_requiredHints[source] ??= {})[node.offset] = hint;
+ }
+
/// Records that the given [expression] is followed by a null check hint
/// (`/*!*/`), for later recall by [hasNullCheckHint].
void recordNullCheckHint(
diff --git a/pkg/nnbd_migration/test/edge_builder_test.dart b/pkg/nnbd_migration/test/edge_builder_test.dart
index fc33d2e..0497d63 100644
--- a/pkg/nnbd_migration/test/edge_builder_test.dart
+++ b/pkg/nnbd_migration/test/edge_builder_test.dart
@@ -3469,6 +3469,15 @@
}
Future<void>
+ test_functionDeclaration_parameter_named_no_default_required_hint() async {
+ await analyze('''
+void f({/*required*/ int i}) {}
+''');
+
+ assertNoUpstreamNullability(decoratedTypeAnnotation('int').node);
+ }
+
+ Future<void>
test_functionDeclaration_parameter_positionalOptional_default_notNull() async {
await analyze('''
void f([int i = 1]) {}
@@ -3618,6 +3627,20 @@
assertNoUpstreamNullability(nullable_i);
}
+ Future<void>
+ test_functionInvocation_parameter_named_missing_required_hint() async {
+ verifyNoTestUnitErrors = false;
+ await analyze('''
+void f({/*required*/ int i}) {}
+void g() {
+ f();
+}
+''');
+ // The call at `f()` is presumed to be in error; no constraint is recorded.
+ var nullable_i = decoratedTypeAnnotation('int i').node;
+ assertNoUpstreamNullability(nullable_i);
+ }
+
Future<void> test_functionInvocation_parameter_null() async {
await analyze('''
void f(int i) {}
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 cee694a..b4f76ed 100644
--- a/pkg/nnbd_migration/test/front_end/info_builder_test.dart
+++ b/pkg/nnbd_migration/test/front_end/info_builder_test.dart
@@ -796,7 +796,7 @@
edits: isEmpty);
}
- Future<void> test_insertedRequired_fieldFormal() async {
+ Future<void> test_insertedRequired_fieldFormal_hint() async {
var unit = await buildInfoForSingleTestFile('''
class C {
int level;
@@ -821,10 +821,41 @@
explanation: "Add 'required' keyword to parameter 'level' in 'C.'",
kind: NullabilityFixKind.addRequired);
assertEdit(
- edit: edits[0], offset: 42, length: 0, replacement: '@required ');
+ edit: edits[0], offset: 42, length: 0, replacement: '/*required*/ ');
}
- Future<void> test_insertedRequired_parameter() async {
+ Future<void> test_insertedRequired_fieldFormal() async {
+ addMetaPackage();
+ var unit = await buildInfoForSingleTestFile('''
+import 'package:meta/meta.dart';
+class C {
+ int level;
+ int level2;
+ C({this.level}) : this.level2 = level + 1;
+}
+''', migratedContent: '''
+import 'package:meta/meta.dart';
+class C {
+ int level;
+ int level2;
+ C({required this.level}) : this.level2 = level + 1;
+}
+''');
+ var regions = unit.fixRegions;
+ expect(regions, hasLength(1));
+ var region = regions[0];
+ var edits = region.edits;
+ assertRegion(
+ region: region,
+ offset: 77,
+ length: 9,
+ explanation: "Add 'required' keyword to parameter 'level' in 'C.'",
+ kind: NullabilityFixKind.addRequired);
+ assertEdit(
+ edit: edits[0], offset: 75, length: 0, replacement: '@required ');
+ }
+
+ Future<void> test_insertedRequired_parameter_hint() async {
var unit = await buildInfoForSingleTestFile('''
class C {
int level = 0;
@@ -847,7 +878,59 @@
explanation: "Add 'required' keyword to parameter 'lvl' in 'C.f'",
kind: NullabilityFixKind.addRequired);
assertEdit(
- edit: edits[0], offset: 37, length: 0, replacement: '@required ');
+ edit: edits[0], offset: 37, length: 0, replacement: '/*required*/ ');
+ }
+
+ Future<void> test_insertedRequired_parameter_metaPrefixed() async {
+ addMetaPackage();
+ var unit = await buildInfoForSingleTestFile('''
+import 'package:meta/meta.dart' as meta;
+class C {
+ int level = 0;
+ bool f({int lvl}) => lvl >= level;
+}
+''', migratedContent: '''
+import 'package:meta/meta.dart' as meta;
+class C {
+ int level = 0;
+ bool f({required int lvl}) => lvl >= level;
+}
+''');
+ var regions = unit.fixRegions;
+ expect(regions, hasLength(1));
+ var region = regions[0];
+ var edits = region.edits;
+ assertEdit(
+ edit: edits[0], offset: 78, length: 0, replacement: '@meta.required ');
+ }
+
+ Future<void> test_insertedRequired_parameter() async {
+ addMetaPackage();
+ var unit = await buildInfoForSingleTestFile('''
+import 'package:meta/meta.dart';
+class C {
+ int level = 0;
+ bool f({int lvl}) => lvl >= level;
+}
+''', migratedContent: '''
+import 'package:meta/meta.dart';
+class C {
+ int level = 0;
+ bool f({required int lvl}) => lvl >= level;
+}
+''');
+ var regions = unit.fixRegions;
+ expect(regions, hasLength(1));
+ var region = regions[0];
+ var edits = region.edits;
+ assertRegion(
+ region: region,
+ offset: 72,
+ length: 9,
+ explanation: "Add 'required' keyword to parameter 'lvl' in 'C.f'",
+ kind: NullabilityFixKind.addRequired);
+ assertEdit(
+ edit: edits[0], offset: 70, length: 0, replacement: '@required ');
}
Future<void> test_insertParens() async {
diff --git a/pkg/nnbd_migration/test/node_builder_test.dart b/pkg/nnbd_migration/test/node_builder_test.dart
index b85ac69..f729846 100644
--- a/pkg/nnbd_migration/test/node_builder_test.dart
+++ b/pkg/nnbd_migration/test/node_builder_test.dart
@@ -829,6 +829,62 @@
expect(ctorParamType.returnType.node.isImmutable, false);
}
+ Future<void>
+ test_fieldFormalParameter_named_no_default_required_hint() async {
+ await analyze('''
+class C {
+ String s;
+ C({/*required*/ this.s});
+}
+''');
+ expect(
+ variables.getRequiredHint(
+ testSource, findNode.fieldFormalParameter('this.s')),
+ isNotNull);
+ }
+
+ Future<void>
+ test_fieldFormalParameter_named_no_default_required_hint_annotation() async {
+ await analyze('''
+class C {
+ String s;
+ C({@deprecated /*required*/ this.s});
+}
+''');
+ expect(
+ variables.getRequiredHint(
+ testSource, findNode.fieldFormalParameter('this.s')),
+ isNotNull);
+ }
+
+ Future<void>
+ test_fieldFormalParameter_named_no_default_required_hint_function_typed() async {
+ await analyze('''
+class C {
+ String Function() s;
+ C({/*required*/ String this.s()});
+}
+''');
+ expect(
+ variables.getRequiredHint(
+ testSource, findNode.fieldFormalParameter('this.s')),
+ isNotNull);
+ }
+
+ Future<void>
+ test_fieldFormalParameter_named_no_default_required_hint_type() async {
+ await analyze('''
+class C {
+ String s;
+ C({/*required*/ String this.s});
+}
+''');
+ expect(
+ variables.getRequiredHint(
+ testSource, findNode.fieldFormalParameter('this.s')),
+ isNotNull);
+ }
+
Future<void> test_fieldFormalParameter_typed() async {
await analyze('''
class C {
@@ -863,6 +919,62 @@
// field.
}
+ Future<void> test_formalParameter_named_no_default_required_hint() async {
+ await analyze('''
+void f({/*required*/ String s}) {}
+''');
+ expect(variables.getRequiredHint(testSource, findNode.simpleParameter('s')),
+ isNotNull);
+ }
+
+ Future<void>
+ test_formalParameter_named_no_default_required_hint_annotation() async {
+ await analyze('''
+void f({@deprecated /*required*/ String s}) {}
+''');
+ expect(variables.getRequiredHint(testSource, findNode.simpleParameter('s')),
+ isNotNull);
+ }
+
+ Future<void>
+ test_formalParameter_named_no_default_required_hint_covariant() async {
+ await analyze('''
+class C {
+ void f({/*required*/ covariant String s}) {}
+}
+''');
+ expect(
+ variables.getRequiredHint(
+ testSource, findNode.simpleParameter('String s')),
+ isNotNull);
+ }
+
+ Future<void>
+ test_formalParameter_named_no_default_required_hint_final_type() async {
+ await analyze('''
+void f({/*required*/ final String s}) {}
+''');
+ expect(variables.getRequiredHint(testSource, findNode.simpleParameter('s')),
+ isNotNull);
+ }
+
+ Future<void>
+ test_formalParameter_named_no_default_required_hint_no_type() async {
+ await analyze('''
+void f({/*required*/ s}) {}
+''');
+ expect(variables.getRequiredHint(testSource, findNode.simpleParameter('s')),
+ isNotNull);
+ }
+
+ Future<void> test_formalParameter_named_no_default_required_hint_var() async {
+ await analyze('''
+void f({/*required*/ var s}) {}
+''');
+ expect(variables.getRequiredHint(testSource, findNode.simpleParameter('s')),
+ isNotNull);
+ }
+
Future<void> test_function_explicit_returnType() async {
await analyze('''
class C {
@@ -985,6 +1097,28 @@
same(decoratedTypeAnnotation('String')));
}
+ Future<void>
+ test_functionTypedFormalParameter_named_no_default_required_hint() async {
+ await analyze('''
+void f({/*required*/ void s()}) {}
+''');
+ expect(
+ variables.getRequiredHint(
+ testSource, findNode.functionTypedFormalParameter('s')),
+ isNotNull);
+ }
+
+ Future<void>
+ test_functionTypedFormalParameter_named_no_default_required_hint_no_return() async {
+ await analyze('''
+void f({/*required*/ s()}) {}
+''');
+ expect(
+ variables.getRequiredHint(
+ testSource, findNode.functionTypedFormalParameter('s')),
+ isNotNull);
+ }
+
Future<void> test_functionTypedFormalParameter_namedParameter_typed() async {
await analyze('''
void f(void g({int i})) {}
@@ -1701,6 +1835,24 @@
expect(functionType.namedParameters['s'].node.isPossiblyOptional, false);
}
+ Future<void>
+ test_topLevelFunction_parameterType_named_no_default_required_hint() async {
+ addMetaPackage();
+ await analyze('''
+import 'package:meta/meta.dart';
+void f({/*required*/ String s}) {}
+''');
+ var decoratedType = decoratedTypeAnnotation('String');
+ var functionType = decoratedFunctionType('f');
+ expect(functionType.namedParameters['s'], same(decoratedType));
+ expect(decoratedType.node, isNotNull);
+ expect(decoratedType.node, isNot(never));
+ expect(decoratedType.node, isNot(always));
+ expect(functionType.namedParameters['s'].node.isPossiblyOptional, false);
+ expect(variables.getRequiredHint(testSource, findNode.simpleParameter('s')),
+ isNotNull);
+ }
+
Future<void> test_topLevelFunction_parameterType_named_with_default() async {
await analyze('''
void f({String s: 'x'}) {}
@@ -2030,6 +2182,14 @@
expect(decorated.node, same(never));
}
+ Future<void> test_variableDeclaration_late_final_hint_simple() async {
+ await analyze('/*late final*/ int i;');
+ expect(
+ variables.getLateHint(
+ testSource, findNode.variableDeclarationList('int i')),
+ isNotNull);
+ }
+
Future<void> test_variableDeclaration_late_hint_after_metadata() async {
await analyze('@deprecated /*late*/ int i;');
expect(
@@ -2054,14 +2214,6 @@
isNotNull);
}
- Future<void> test_variableDeclaration_late_final_hint_simple() async {
- await analyze('/*late final*/ int i;');
- expect(
- variables.getLateHint(
- testSource, findNode.variableDeclarationList('int i')),
- isNotNull);
- }
-
Future<void> test_variableDeclaration_late_hint_with_spaces() async {
await analyze('/* late */ int i;');
expect(
