Remove NullabilityMigrationAssumptions class.
This was a lot of bookkeeping for very little added value. If we want
to experiment with changing the behavior of the migration tool in the
future, we should experimentally change it and see whether the quality
of the migration improves; it's not worth speculatively keeping
multiple behaviors around while the tool is being developed.
Change-Id: I9a59e2626be7e9e84a937e287421abcdc971aba5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/104482
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
diff --git a/pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart b/pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart
index f12f55f..f4bd6dd 100644
--- a/pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart
+++ b/pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart
@@ -7,7 +7,6 @@
import 'package:analysis_server/src/nullability/decorated_type.dart';
import 'package:analysis_server/src/nullability/expression_checks.dart';
import 'package:analysis_server/src/nullability/nullability_node.dart';
-import 'package:analysis_server/src/nullability/transitional_api.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/token.dart';
import 'package:analyzer/dart/ast/visitor.dart';
@@ -32,8 +31,6 @@
final bool _permissive;
- final NullabilityMigrationAssumptions assumptions;
-
final NullabilityGraph _graph;
/// The file being analyzed.
@@ -77,7 +74,7 @@
bool _inConditionalControlFlow = false;
ConstraintGatherer(TypeProvider typeProvider, this._variables, this._graph,
- this._source, this._permissive, this.assumptions)
+ this._source, this._permissive)
: _notNullType =
DecoratedType(typeProvider.objectType, NullabilityNode.never),
_nonNullableBoolType =
@@ -214,15 +211,10 @@
if (node.declaredElement.hasRequired) {
// Nothing to do; the implicit default value of `null` will never be
// reached.
- } else if (node.declaredElement.isOptionalPositional ||
- assumptions.namedNoDefaultParameterHeuristic ==
- NamedNoDefaultParameterHeuristic.assumeNullable) {
+ } else {
NullabilityNode.recordAssignment(NullabilityNode.always,
getOrComputeElementType(node.declaredElement).node, _guards, _graph,
hard: false);
- } else {
- assert(assumptions.namedNoDefaultParameterHeuristic ==
- NamedNoDefaultParameterHeuristic.assumeRequired);
}
} else {
_handleAssignment(
diff --git a/pkg/analysis_server/lib/src/nullability/constraint_variable_gatherer.dart b/pkg/analysis_server/lib/src/nullability/constraint_variable_gatherer.dart
index 814aa90..5d86653 100644
--- a/pkg/analysis_server/lib/src/nullability/constraint_variable_gatherer.dart
+++ b/pkg/analysis_server/lib/src/nullability/constraint_variable_gatherer.dart
@@ -6,7 +6,6 @@
import 'package:analysis_server/src/nullability/decorated_type.dart';
import 'package:analysis_server/src/nullability/expression_checks.dart';
import 'package:analysis_server/src/nullability/nullability_node.dart';
-import 'package:analysis_server/src/nullability/transitional_api.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:analyzer/dart/element/element.dart';
@@ -39,14 +38,12 @@
final bool _permissive;
- final NullabilityMigrationAssumptions assumptions;
-
final NullabilityGraph _graph;
final TypeProvider _typeProvider;
ConstraintVariableGatherer(this._variables, this._source, this._permissive,
- this.assumptions, this._graph, this._typeProvider);
+ this._graph, this._typeProvider);
/// Creates and stores a [DecoratedType] object corresponding to the given
/// [type] AST, and returns it.
diff --git a/pkg/analysis_server/lib/src/nullability/provisional_api.dart b/pkg/analysis_server/lib/src/nullability/provisional_api.dart
index 36b741b..02b8921 100644
--- a/pkg/analysis_server/lib/src/nullability/provisional_api.dart
+++ b/pkg/analysis_server/lib/src/nullability/provisional_api.dart
@@ -13,9 +13,6 @@
import 'package:analyzer/src/generated/source.dart';
import 'package:meta/meta.dart';
-export 'package:analysis_server/src/nullability/transitional_api.dart'
- show NamedNoDefaultParameterHeuristic, NullabilityMigrationAssumptions;
-
/// Kinds of fixes that might be performed by nullability migration.
class NullabilityFixKind {
/// An import needs to be added.
@@ -73,12 +70,9 @@
/// as far as possible even though the migration algorithm is not yet
/// complete. TODO(paulberry): remove this mode once the migration algorithm
/// is fully implemented.
- NullabilityMigration(this.listener,
- {bool permissive: false,
- analyzer.NullabilityMigrationAssumptions assumptions:
- const analyzer.NullabilityMigrationAssumptions()})
- : _analyzerMigration = analyzer.NullabilityMigration(
- permissive: permissive, assumptions: assumptions);
+ NullabilityMigration(this.listener, {bool permissive: false})
+ : _analyzerMigration =
+ analyzer.NullabilityMigration(permissive: permissive);
void finish() {
for (var entry in _analyzerMigration.finish().entries) {
diff --git a/pkg/analysis_server/lib/src/nullability/transitional_api.dart b/pkg/analysis_server/lib/src/nullability/transitional_api.dart
index 06b7c12..b7cd542 100644
--- a/pkg/analysis_server/lib/src/nullability/transitional_api.dart
+++ b/pkg/analysis_server/lib/src/nullability/transitional_api.dart
@@ -89,38 +89,6 @@
}
}
-/// Enum encapsulating the various options proposed at
-/// https://github.com/dart-lang/language/issues/156#issuecomment-460525075
-enum DefaultParameterHandling {
- /// Option 2: Add required named parameters
- ///
- /// - `{int x}` implicitly means `x` is required
- /// - required-ness goes into the function type:
- /// `int Function({required int x})`
- /// - `{required int? x}` is allowed
- /// - means that something must be passed
- /// - passing null is allowed
- /// - `{int x = 3}` is allowed
- /// - `x` is optional
- /// - passing null to it is an error
- /// - passing nothing to it results in it getting the default value
- /// - `[int x]` is an error
- /// - `[int x = 3]` is allowed
- option2_addRequiredNamedParameters,
-}
-
-/// Enum representing the possible heuristics for handling named parameters with
-/// no default value.
-enum NamedNoDefaultParameterHeuristic {
- /// Assume that the parameter should be considered nullable, unless the user
- /// has explicitly marked it as `@required`.
- assumeNullable,
-
- /// Assume that the parameter should be considered required, unless the user
- /// has explicitly marked it as nullable.
- assumeRequired,
-}
-
/// Transitional migration API.
///
/// Usage: pass each input source file to [prepareInput]. Then pass each input
@@ -132,8 +100,6 @@
class NullabilityMigration {
final bool _permissive;
- final NullabilityMigrationAssumptions assumptions;
-
final Variables _variables;
final NullabilityGraph _graph;
@@ -144,13 +110,10 @@
/// as far as possible even though the migration algorithm is not yet
/// complete. TODO(paulberry): remove this mode once the migration algorithm
/// is fully implemented.
- NullabilityMigration(
- {bool permissive: false,
- NullabilityMigrationAssumptions assumptions:
- const NullabilityMigrationAssumptions()})
- : this._(permissive, assumptions, NullabilityGraph());
+ NullabilityMigration({bool permissive: false})
+ : this._(permissive, NullabilityGraph());
- NullabilityMigration._(this._permissive, this.assumptions, this._graph)
+ NullabilityMigration._(this._permissive, this._graph)
: _variables = Variables(_graph);
Map<Source, List<PotentialModification>> finish() {
@@ -159,42 +122,16 @@
}
void prepareInput(CompilationUnit unit, TypeProvider typeProvider) {
- unit.accept(ConstraintVariableGatherer(
- _variables,
- unit.declaredElement.source,
- _permissive,
- assumptions,
- _graph,
- typeProvider));
+ unit.accept(ConstraintVariableGatherer(_variables,
+ unit.declaredElement.source, _permissive, _graph, typeProvider));
}
void processInput(CompilationUnit unit, TypeProvider typeProvider) {
unit.accept(ConstraintGatherer(typeProvider, _variables, _graph,
- unit.declaredElement.source, _permissive, assumptions));
+ unit.declaredElement.source, _permissive));
}
}
-/// Assumptions affecting the behavior of the nullability migration tool.
-///
-/// These options generally reflect design decisions that have not yet been
-/// made. They don't reflect behavioral differences we would want to expose to
-/// the user.
-///
-/// TODO(paulberry): hardcode these assumptions once decisions have been made.
-class NullabilityMigrationAssumptions {
- /// Handling of default parameters.
- final DefaultParameterHandling defaultParameterHandling;
-
- /// Heuristic for handling named parameters with no default value.
- final NamedNoDefaultParameterHeuristic namedNoDefaultParameterHeuristic;
-
- const NullabilityMigrationAssumptions(
- {this.defaultParameterHandling:
- DefaultParameterHandling.option2_addRequiredNamedParameters,
- this.namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeNullable});
-}
-
/// Records information about the possible addition of an import
/// to the source code.
class PotentiallyAddImport extends PotentialModification {
diff --git a/pkg/analysis_server/test/src/nullability/migration_visitor_test.dart b/pkg/analysis_server/test/src/nullability/migration_visitor_test.dart
index 73918d4..e4914fa 100644
--- a/pkg/analysis_server/test/src/nullability/migration_visitor_test.dart
+++ b/pkg/analysis_server/test/src/nullability/migration_visitor_test.dart
@@ -289,51 +289,21 @@
hard: true);
}
- test_functionDeclaration_parameter_named_no_default_assume_nullable() async {
+ test_functionDeclaration_parameter_named_no_default() async {
await analyze('''
void f({int i}) {}
-''',
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeNullable));
+''');
assertEdge(NullabilityNode.always, decoratedTypeAnnotation('int').node,
hard: false);
}
- test_functionDeclaration_parameter_named_no_default_assume_required() async {
- await analyze('''
-void f({int i}) {}
-''',
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeRequired));
-
- assertNoUpstreamNullability(decoratedTypeAnnotation('int').node);
- }
-
- test_functionDeclaration_parameter_named_no_default_required_assume_nullable() async {
+ test_functionDeclaration_parameter_named_no_default_required() async {
addMetaPackage();
await analyze('''
import 'package:meta/meta.dart';
void f({@required int i}) {}
-''',
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeNullable));
-
- assertNoUpstreamNullability(decoratedTypeAnnotation('int').node);
- }
-
- test_functionDeclaration_parameter_named_no_default_required_assume_required() async {
- addMetaPackage();
- await analyze('''
-import 'package:meta/meta.dart';
-void f({@required int i}) {}
-''',
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeRequired));
+''');
assertNoUpstreamNullability(decoratedTypeAnnotation('int').node);
}
@@ -364,20 +334,6 @@
hard: false);
}
- test_functionDeclaration_parameter_positionalOptional_no_default_assume_required() async {
- // Note: the `assumeRequired` behavior shouldn't affect the behavior here
- // because it only affects named parameters.
- await analyze('''
-void f([int i]) {}
-''',
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeRequired));
-
- assertEdge(NullabilityNode.always, decoratedTypeAnnotation('int').node,
- hard: false);
- }
-
test_functionDeclaration_resets_unconditional_control_flow() async {
await analyze('''
void f(bool b, int i, int j) {
@@ -775,12 +731,10 @@
/// Analyzes the given source code, producing constraint variables and
/// constraints for it.
@override
- Future<CompilationUnit> analyze(String code,
- {NullabilityMigrationAssumptions assumptions:
- const NullabilityMigrationAssumptions()}) async {
+ Future<CompilationUnit> analyze(String code) async {
var unit = await super.analyze(code);
- unit.accept(ConstraintGatherer(
- typeProvider, _variables, graph, testSource, false, assumptions));
+ unit.accept(
+ ConstraintGatherer(typeProvider, _variables, graph, testSource, false));
return unit;
}
}
@@ -944,12 +898,10 @@
TypeProvider get typeProvider => testAnalysisResult.typeProvider;
- Future<CompilationUnit> analyze(String code,
- {NullabilityMigrationAssumptions assumptions:
- const NullabilityMigrationAssumptions()}) async {
+ Future<CompilationUnit> analyze(String code) async {
await resolveTestUnit(code);
testUnit.accept(ConstraintVariableGatherer(
- _variables, testSource, false, assumptions, graph, typeProvider));
+ _variables, testSource, false, graph, typeProvider));
findNode = FindNode(code, testUnit);
return testUnit;
}
diff --git a/pkg/analysis_server/test/src/nullability/provisional_api_test.dart b/pkg/analysis_server/test/src/nullability/provisional_api_test.dart
index 8a4bb19..e230e2a 100644
--- a/pkg/analysis_server/test/src/nullability/provisional_api_test.dart
+++ b/pkg/analysis_server/test/src/nullability/provisional_api_test.dart
@@ -36,15 +36,13 @@
/// Verifies that migration of the files in [input] produces the output in
/// [expectedOutput].
Future<void> _checkMultipleFileChanges(
- Map<String, String> input, Map<String, String> expectedOutput,
- {NullabilityMigrationAssumptions assumptions:
- const NullabilityMigrationAssumptions()}) async {
+ Map<String, String> input, Map<String, String> expectedOutput) async {
for (var path in input.keys) {
newFile(path, content: input[path]);
}
var listener = new _TestMigrationListener();
- var migration = NullabilityMigration(listener,
- permissive: _usePermissiveMode, assumptions: assumptions);
+ var migration =
+ NullabilityMigration(listener, permissive: _usePermissiveMode);
for (var path in input.keys) {
migration.prepareInput(await session.getResolvedUnit(path));
}
@@ -69,13 +67,10 @@
/// Verifies that migraiton of the single file with the given [content]
/// produces the [expected] output.
- Future<void> _checkSingleFileChanges(String content, String expected,
- {NullabilityMigrationAssumptions assumptions:
- const NullabilityMigrationAssumptions()}) async {
+ Future<void> _checkSingleFileChanges(String content, String expected) async {
var sourcePath = convertPath('/home/test/lib/test.dart');
await _checkMultipleFileChanges(
- {sourcePath: content}, {sourcePath: expected},
- assumptions: assumptions);
+ {sourcePath: content}, {sourcePath: expected});
}
}
@@ -318,7 +313,7 @@
await _checkSingleFileChanges(content, expected);
}
- test_named_parameter_no_default_unused_option2_assume_nullable() async {
+ test_named_parameter_no_default_unused() async {
var content = '''
void f({String s}) {}
main() {
@@ -331,13 +326,10 @@
f();
}
''';
- await _checkSingleFileChanges(content, expected,
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeNullable));
+ await _checkSingleFileChanges(content, expected);
}
- test_named_parameter_no_default_unused_option2_assume_nullable_propagate() async {
+ test_named_parameter_no_default_unused_propagate() async {
var content = '''
void f(String s) {}
void g({String s}) {
@@ -356,57 +348,10 @@
g();
}
''';
- await _checkSingleFileChanges(content, expected,
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeNullable));
+ await _checkSingleFileChanges(content, expected);
}
- test_named_parameter_no_default_unused_option2_assume_required() async {
- var content = '''
-void f({String s}) {}
-main() {
- f();
-}
-''';
- var expected = '''
-void f({String? s}) {}
-main() {
- f();
-}
-''';
- await _checkSingleFileChanges(content, expected,
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeRequired));
- }
-
- test_named_parameter_no_default_unused_option2_assume_required_propagate() async {
- var content = '''
-void f(String s) {}
-void g({String s}) {
- f(s);
-}
-main() {
- g();
-}
-''';
- var expected = '''
-void f(String? s) {}
-void g({String? s}) {
- f(s);
-}
-main() {
- g();
-}
-''';
- await _checkSingleFileChanges(content, expected,
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeRequired));
- }
-
- test_named_parameter_no_default_unused_required_option2_assume_nullable() async {
+ test_named_parameter_no_default_unused_required() async {
// The `@required` annotation overrides the assumption of nullability.
// The call at `f()` is presumed to be in error.
addMetaPackage();
@@ -424,38 +369,10 @@
f();
}
''';
- await _checkSingleFileChanges(content, expected,
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeNullable));
+ await _checkSingleFileChanges(content, expected);
}
- test_named_parameter_no_default_unused_required_option2_assume_required() async {
- // Since the `@required` annotation is already present, it is not added
- // again.
- // The call at `f()` is presumed to be in error.
- addMetaPackage();
- var content = '''
-import 'package:meta/meta.dart';
-void f({@required String s}) {}
-main() {
- f();
-}
-''';
- var expected = '''
-import 'package:meta/meta.dart';
-void f({@required String s}) {}
-main() {
- f();
-}
-''';
- await _checkSingleFileChanges(content, expected,
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeRequired));
- }
-
- test_named_parameter_no_default_used_non_null_option2_assume_nullable() async {
+ test_named_parameter_no_default_used_non_null() async {
var content = '''
void f({String s}) {}
main() {
@@ -468,13 +385,10 @@
f(s: 'x');
}
''';
- await _checkSingleFileChanges(content, expected,
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeNullable));
+ await _checkSingleFileChanges(content, expected);
}
- test_named_parameter_no_default_used_non_null_option2_assume_nullable_propagate() async {
+ test_named_parameter_no_default_used_non_null_propagate() async {
var content = '''
void f(String s) {}
void g({String s}) {
@@ -493,80 +407,7 @@
g(s: 'x');
}
''';
- await _checkSingleFileChanges(content, expected,
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeNullable));
- }
-
- test_named_parameter_no_default_used_non_null_option2_assume_required() async {
- var content = '''
-void f({String s}) {}
-main() {
- f(s: 'x');
-}
-''';
- var expected = '''
-import 'package:meta/meta.dart';
-void f({@required String s}) {}
-main() {
- f(s: 'x');
-}
-''';
- await _checkSingleFileChanges(content, expected,
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeRequired));
- }
-
- test_named_parameter_no_default_used_non_null_option2_assume_required_propagate() async {
- var content = '''
-void f(String s) {}
-void g({String s}) {
- f(s);
-}
-main() {
- g(s: 'x');
-}
-''';
- var expected = '''
-import 'package:meta/meta.dart';
-void f(String s) {}
-void g({@required String s}) {
- f(s);
-}
-main() {
- g(s: 'x');
-}
-''';
- await _checkSingleFileChanges(content, expected,
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeRequired));
- }
-
- test_named_parameter_no_default_used_non_null_required_option2_assume_required() async {
- // Even if we are using the "assumeRequired" heuristic, we should not add a
- // duplicate `@required` annotation.
- addMetaPackage();
- var content = '''
-import 'package:meta/meta.dart';
-void f({@required String s}) {}
-main() {
- f(s: 'x');
-}
-''';
- var expected = '''
-import 'package:meta/meta.dart';
-void f({@required String s}) {}
-main() {
- f(s: 'x');
-}
-''';
- await _checkSingleFileChanges(content, expected,
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeRequired));
+ await _checkSingleFileChanges(content, expected);
}
test_named_parameter_no_default_used_null_option2() async {
@@ -585,7 +426,7 @@
await _checkSingleFileChanges(content, expected);
}
- test_named_parameter_no_default_used_null_required_option2_assume_nullable() async {
+ test_named_parameter_no_default_used_null_required() async {
// Explicitly passing `null` forces the parameter to be nullable even though
// it is required.
addMetaPackage();
@@ -603,34 +444,7 @@
f(s: null);
}
''';
- await _checkSingleFileChanges(content, expected,
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeNullable));
- }
-
- test_named_parameter_no_default_used_null_required_option2_assume_required() async {
- // Explicitly passing `null` forces the parameter to be nullable even though
- // it is required.
- addMetaPackage();
- var content = '''
-import 'package:meta/meta.dart';
-void f({@required String s}) {}
-main() {
- f(s: null);
-}
-''';
- var expected = '''
-import 'package:meta/meta.dart';
-void f({@required String? s}) {}
-main() {
- f(s: null);
-}
-''';
- await _checkSingleFileChanges(content, expected,
- assumptions: NullabilityMigrationAssumptions(
- namedNoDefaultParameterHeuristic:
- NamedNoDefaultParameterHeuristic.assumeRequired));
+ await _checkSingleFileChanges(content, expected);
}
test_named_parameter_with_non_null_default_unused_option2() async {
