languages/dart-sdk
languages/dart-sdk
1
0
Fork 0
mirror of https://github.com/dart-lang/sdk synced 2024-10-14 12:30:03 +00:00
Code Issues Packages Projects Releases Wiki Activity

Remove NullabilityMigrationAssumptions class.

Browse source 
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>
This commit is contained in:
Paul Berry 2019-06-03 02:20:26 +00:00 committed by commit-bot@chromium.org
parent 709df79404
commit e31af09992
6 changed files with 38 additions and 352 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

12
pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart
View file

@ -7,7 +7,6 @@ import 'package:analysis_server/src/nullability/constraint_variable_gatherer.dar
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 @@ class ConstraintGatherer extends GeneralizingAstVisitor<DecoratedType> {
final bool _permissive;
final NullabilityMigrationAssumptions assumptions;
final NullabilityGraph _graph;
/// The file being analyzed.
@ -77,7 +74,7 @@ class ConstraintGatherer extends GeneralizingAstVisitor<DecoratedType> {
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 @@ class ConstraintGatherer extends GeneralizingAstVisitor<DecoratedType> {
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(

5
pkg/analysis_server/lib/src/nullability/constraint_variable_gatherer.dart
View file

@ -6,7 +6,6 @@ import 'package:analysis_server/src/nullability/conditional_discard.dart';
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 @@ class ConstraintVariableGatherer extends GeneralizingAstVisitor<DecoratedType> {
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.

12
pkg/analysis_server/lib/src/nullability/provisional_api.dart
View file

@ -13,9 +13,6 @@ import 'package:analyzer/dart/analysis/results.dart';
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 @@ class NullabilityMigration {
/// 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) {

75
pkg/analysis_server/lib/src/nullability/transitional_api.dart
View file

@ -89,38 +89,6 @@ class ConditionalModification extends PotentialModification {
}
}
/// 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 @@ enum NamedNoDefaultParameterHeuristic {
class NullabilityMigration {
final bool _permissive;
final NullabilityMigrationAssumptions assumptions;
final Variables _variables;
final NullabilityGraph _graph;
@ -144,13 +110,10 @@ class NullabilityMigration {
/// 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 @@ class NullabilityMigration {
}
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 {

66
pkg/analysis_server/test/src/nullability/migration_visitor_test.dart
View file

@ -289,51 +289,21 @@ void f({int i = null}) {}
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 @@ void f([int i]) {}
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 @@ abstract class ConstraintsTestBase extends MigrationVisitorTestBase {
/// 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 @@ class MigrationVisitorTestBase extends AbstractSingleUnitTest {
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;
}

220
pkg/analysis_server/test/src/nullability/provisional_api_test.dart
View file

@ -36,15 +36,13 @@ abstract class _ProvisionalApiTestBase extends AbstractContextTest {
/// 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 @@ abstract class _ProvisionalApiTestBase extends AbstractContextTest {
/// 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 @@ int f(int i) {
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 @@ main() {
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 @@ main() {
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 @@ main() {
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 @@ main() {
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 @@ main() {
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 @@ main() {
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 @@ main() {
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 {