From dbb56e5e10adb22805edf25cfd9a0ee5086c19cf Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Thu, 12 Dec 2019 01:44:43 +0000 Subject: [PATCH] Connect g/setters which override fields (w/ implicit g/setters) Change-Id: I4eeb507e23962fbf53f1026e14f4e2ae607aa7e6 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/128105 Reviewed-by: Mike Fairhurst Commit-Queue: Samuel Rawlins --- .../nnbd_migration/info_builder_test.dart | 30 ++-- pkg/nnbd_migration/lib/src/edge_builder.dart | 133 +++++++++++------- pkg/nnbd_migration/test/api_test.dart | 94 ++++++++++++- .../test/edge_builder_test.dart | 30 ++++ 4 files changed, 215 insertions(+), 72 deletions(-) diff --git a/pkg/analysis_server/test/src/edit/nnbd_migration/info_builder_test.dart b/pkg/analysis_server/test/src/edit/nnbd_migration/info_builder_test.dart index d379bfb15dc..22364e556ae 100644 --- a/pkg/analysis_server/test/src/edit/nnbd_migration/info_builder_test.dart +++ b/pkg/analysis_server/test/src/edit/nnbd_migration/info_builder_test.dart @@ -1011,21 +1011,15 @@ class B extends A { details: ["A nullable value is assigned"]); } - @FailingTest( - reason: "Currently crashes with: Bad state: A decorated type for void " - "set m(int _m) should have been stored by the NodeBuilder via " - "recordDecoratedElementType") test_parameter_fromOverriddenField_explicit() async { - await buildInfoForSingleTestFile(''' + UnitInfo unit = await buildInfoForSingleTestFile(''' class A { int m; } class B extends A { void set m(Object p) {} } -void f(A a) { - a.m = null; -} +void f(A a) => a.m = null; ''', migratedContent: ''' class A { int? m; @@ -1033,13 +1027,21 @@ class A { class B extends A { void set m(Object? p) {} } -void f(A a) { - a.m = null; -} +void f(A a) => a.m = null; '''); - // TODO(srawlins): Write expectations similar to - // test_parameter_fromMultipleOverridden_explicit above, once the test stops - // crashing. + List regions = unit.fixRegions; + expect(regions, hasLength(2)); + assertRegion(region: regions[0], offset: 15, details: [ + // TODO(srawlins): I suspect this should be removed... + "A nullable value is assigned", + "An explicit 'null' is assigned in the function 'f'", + ]); + assertRegion(region: regions[1], offset: 61, details: [ + // TODO(srawlins): Improve this message to include "B.m". + "The corresponding parameter in the overridden method is nullable" + ]); + assertDetail(detail: regions[0].details[1], offset: 90, length: 4); + assertDetail(detail: regions[1].details[0], offset: 12, length: 3); } test_parameter_named_omittedInCall() async { diff --git a/pkg/nnbd_migration/lib/src/edge_builder.dart b/pkg/nnbd_migration/lib/src/edge_builder.dart index 4732c4e0e84..b04e15fb11e 100644 --- a/pkg/nnbd_migration/lib/src/edge_builder.dart +++ b/pkg/nnbd_migration/lib/src/edge_builder.dart @@ -1811,65 +1811,90 @@ class EdgeBuilder extends GeneralizingAstVisitor overriddenElement = overriddenElement.declaration; var overriddenClass = overriddenElement.enclosingElement as ClassElement; - var decoratedOverriddenFunctionType = - _variables.decoratedElementType(overriddenElement); var decoratedSupertype = _decoratedClassHierarchy .getDecoratedSupertype(classElement, overriddenClass); var substitution = decoratedSupertype.asSubstitution; - var overriddenFunctionType = - decoratedOverriddenFunctionType.substitute(substitution); - if (returnType == null) { - _unionDecoratedTypes( - _currentFunctionType.returnType, - overriddenFunctionType.returnType, - ReturnTypeInheritanceOrigin(source, node)); + if (overriddenElement is PropertyAccessorElement && + overriddenElement.isSynthetic) { + assert(node is MethodDeclaration); + var method = node as MethodDeclaration; + var decoratedOverriddenField = + _variables.decoratedElementType(overriddenElement.variable); + var overriddenFieldType = + decoratedOverriddenField.substitute(substitution); + if (method.isGetter) { + _checkAssignment(ReturnTypeInheritanceOrigin(source, node), + source: _currentFunctionType.returnType, + destination: overriddenFieldType, + hard: true); + } else { + assert(method.isSetter); + DecoratedType currentParameterType = + _currentFunctionType.positionalParameters.single; + DecoratedType overriddenParameterType = overriddenFieldType; + _checkAssignment(ParameterInheritanceOrigin(source, node), + source: overriddenParameterType, + destination: currentParameterType, + hard: true); + } } else { - _checkAssignment(ReturnTypeInheritanceOrigin(source, node), - source: _currentFunctionType.returnType, - destination: overriddenFunctionType.returnType, - hard: true); - } - if (parameters != null) { - int positionalParameterCount = 0; - for (var parameter in parameters.parameters) { - NormalFormalParameter normalParameter; - if (parameter is NormalFormalParameter) { - normalParameter = parameter; - } else { - normalParameter = - (parameter as DefaultFormalParameter).parameter; - } - DecoratedType currentParameterType; - DecoratedType overriddenParameterType; - if (parameter.isNamed) { - var name = normalParameter.identifier.name; - currentParameterType = - _currentFunctionType.namedParameters[name]; - overriddenParameterType = - overriddenFunctionType.namedParameters[name]; - } else { - if (positionalParameterCount < - _currentFunctionType.positionalParameters.length) { - currentParameterType = _currentFunctionType - .positionalParameters[positionalParameterCount]; - } - if (positionalParameterCount < - overriddenFunctionType.positionalParameters.length) { - overriddenParameterType = overriddenFunctionType - .positionalParameters[positionalParameterCount]; - } - positionalParameterCount++; - } - if (overriddenParameterType != null) { - var origin = ParameterInheritanceOrigin(source, node); - if (_isUntypedParameter(normalParameter)) { - _unionDecoratedTypes( - overriddenParameterType, currentParameterType, origin); + var decoratedOverriddenFunctionType = + _variables.decoratedElementType(overriddenElement); + var overriddenFunctionType = + decoratedOverriddenFunctionType.substitute(substitution); + if (returnType == null) { + _unionDecoratedTypes( + _currentFunctionType.returnType, + overriddenFunctionType.returnType, + ReturnTypeInheritanceOrigin(source, node)); + } else { + _checkAssignment(ReturnTypeInheritanceOrigin(source, node), + source: _currentFunctionType.returnType, + destination: overriddenFunctionType.returnType, + hard: true); + } + if (parameters != null) { + int positionalParameterCount = 0; + for (var parameter in parameters.parameters) { + NormalFormalParameter normalParameter; + if (parameter is NormalFormalParameter) { + normalParameter = parameter; } else { - _checkAssignment(origin, - source: overriddenParameterType, - destination: currentParameterType, - hard: true); + normalParameter = + (parameter as DefaultFormalParameter).parameter; + } + DecoratedType currentParameterType; + DecoratedType overriddenParameterType; + if (parameter.isNamed) { + var name = normalParameter.identifier.name; + currentParameterType = + _currentFunctionType.namedParameters[name]; + overriddenParameterType = + overriddenFunctionType.namedParameters[name]; + } else { + if (positionalParameterCount < + _currentFunctionType.positionalParameters.length) { + currentParameterType = _currentFunctionType + .positionalParameters[positionalParameterCount]; + } + if (positionalParameterCount < + overriddenFunctionType.positionalParameters.length) { + overriddenParameterType = overriddenFunctionType + .positionalParameters[positionalParameterCount]; + } + positionalParameterCount++; + } + if (overriddenParameterType != null) { + var origin = ParameterInheritanceOrigin(source, node); + if (_isUntypedParameter(normalParameter)) { + _unionDecoratedTypes( + overriddenParameterType, currentParameterType, origin); + } else { + _checkAssignment(origin, + source: overriddenParameterType, + destination: currentParameterType, + hard: true); + } } } } diff --git a/pkg/nnbd_migration/test/api_test.dart b/pkg/nnbd_migration/test/api_test.dart index 6ee875721e1..d6c1d25fc56 100644 --- a/pkg/nnbd_migration/test/api_test.dart +++ b/pkg/nnbd_migration/test/api_test.dart @@ -1760,6 +1760,68 @@ void g(C c) { // (3) await _checkSingleFileChanges(content, expected); } + test_getter_implicit_returnType_overrides_implicit_getter() async { + var content = ''' +class A { + final String s = "x"; +} +class C implements A { + get s => false ? "y" : null; +} +'''; + var expected = ''' +class A { + final String? s = "x"; +} +class C implements A { + get s => false ? "y" : null; +} +'''; + await _checkSingleFileChanges(content, expected); + } + + test_getter_overrides_implicit_getter() async { + var content = ''' +class A { + final String s = "x"; +} +class C implements A { + String get s => false ? "y" : null; +} +'''; + var expected = ''' +class A { + final String? s = "x"; +} +class C implements A { + String? get s => false ? "y" : null; +} +'''; + await _checkSingleFileChanges(content, expected); + } + + test_getter_overrides_implicit_getter_with_generics() async { + var content = ''' +class A { + final T value; + A(this.value); +} +class C implements A { + String get value => false ? "y" : null; +} +'''; + var expected = ''' +class A { + final T? value; + A(this.value); +} +class C implements A { + String? get value => false ? "y" : null; +} +'''; + await _checkSingleFileChanges(content, expected); + } + test_getter_topLevel() async { var content = ''' int get g => 0; @@ -2514,7 +2576,7 @@ main() { test_null_aware_setter_invocation_null_target() async { var content = ''' class C { - void set x(int value); + void set x(int value) {} } int f(C c) => c?.x = 1; main() { @@ -2523,7 +2585,7 @@ main() { '''; var expected = ''' class C { - void set x(int value); + void set x(int value) {} } int? f(C? c) => c?.x = 1; main() { @@ -2536,7 +2598,7 @@ main() { test_null_aware_setter_invocation_null_value() async { var content = ''' class C { - void set x(int value); + void set x(int value) {} } int f(C c) => c?.x = 1; main() { @@ -2545,7 +2607,7 @@ main() { '''; var expected = ''' class C { - void set x(int value); + void set x(int value) {} } int? f(C? c) => c?.x = 1; main() { @@ -2995,6 +3057,30 @@ int? recover() { await _checkSingleFileChanges(content, expected); } + test_setter_overrides_implicit_setter() async { + var content = ''' +class A { + String s = "x"; +} +class C implements A { + String get s => "x"; + void set s(String value) {} +} +f() => A().s = null; +'''; + var expected = ''' +class A { + String? s = "x"; +} +class C implements A { + String get s => "x"; + void set s(String? value) {} +} +f() => A().s = null; +'''; + await _checkSingleFileChanges(content, expected); + } + test_single_file_multiple_changes() async { var content = ''' int f() => null; diff --git a/pkg/nnbd_migration/test/edge_builder_test.dart b/pkg/nnbd_migration/test/edge_builder_test.dart index 5783ed6b310..b2a925a1cd1 100644 --- a/pkg/nnbd_migration/test/edge_builder_test.dart +++ b/pkg/nnbd_migration/test/edge_builder_test.dart @@ -2418,6 +2418,20 @@ bool f(Object x) => x is _P; // a more specific test with assertions. } + test_getter_overrides_implicit_getter() async { + await analyze(''' +class A { + final String/*1*/ s = "x"; +} +class C implements A { + String/*2*/ get s => false ? "y" : null; +} +'''); + var string1 = decoratedTypeAnnotation('String/*1*/'); + var string2 = decoratedTypeAnnotation('String/*2*/'); + assertEdge(string2.node, string1.node, hard: true); + } + test_if_condition() async { await analyze(''' void f(bool b) { @@ -5124,6 +5138,22 @@ Set f() { hard: false); } + test_setter_overrides_implicit_setter() async { + await analyze(''' +class A { + String/*1*/ s = "x"; +} +class C implements A { + String get s => "x"; + void set s(String/*2*/ value) {} +} +f() => A().s = null; +'''); + var string1 = decoratedTypeAnnotation('String/*1*/'); + var string2 = decoratedTypeAnnotation('String/*2*/'); + assertEdge(string1.node, string2.node, hard: true); + } + test_simpleIdentifier_function() async { await analyze(''' int f() => null;