Change in dart/sdk[master]: Issue 41371. Report an error for covariant parameter in the interface...
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov would like Erik Ernst and Brian Wilkerson to review this change.
Issue 41371. Report an error for covariant parameter in the interface, but not in the implementation.
Bug: https://github.com/dart-lang/sdk/issues/41371
Change-Id: Ife57b54c1f2609ef1202ccc6c918cf85bb8c0df7
---
M pkg/analyzer/lib/error/error.dart
M pkg/analyzer/lib/src/error/codes.dart
M pkg/analyzer/lib/src/error/correct_override.dart
M pkg/analyzer/lib/src/error/inheritance_override.dart
A pkg/analyzer/test/src/diagnostics/covariant_interface_parameter_with_non_covariant_implementation_test.dart
M pkg/analyzer/test/src/diagnostics/test_all.dart
M tests/language/covariant_override/tear_off_type_test.dart
M tests/language_2/covariant_override/tear_off_type_test.dart
M tests/language_2/regress/regress31596_covariant_declaration_test.dart
9 files changed, 179 insertions(+), 4 deletions(-)
diff --git a/pkg/analyzer/lib/error/error.dart b/pkg/analyzer/lib/error/error.dart
index 1ecfb41..65f6f71 100644
--- a/pkg/analyzer/lib/error/error.dart
+++ b/pkg/analyzer/lib/error/error.dart
@@ -126,6 +126,8 @@
CompileTimeErrorCode.CONST_WITH_UNDEFINED_CONSTRUCTOR,
CompileTimeErrorCode.CONST_WITH_UNDEFINED_CONSTRUCTOR_DEFAULT,
CompileTimeErrorCode.CONTINUE_LABEL_ON_SWITCH,
+ CompileTimeErrorCode
+ .COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION,
CompileTimeErrorCode.DEFAULT_LIST_CONSTRUCTOR,
CompileTimeErrorCode.DEFAULT_VALUE_IN_FUNCTION_TYPED_PARAMETER,
CompileTimeErrorCode.DEFAULT_VALUE_IN_REDIRECTING_FACTORY_CONSTRUCTOR,
diff --git a/pkg/analyzer/lib/src/error/codes.dart b/pkg/analyzer/lib/src/error/codes.dart
index 3a45736..09e9b27 100644
--- a/pkg/analyzer/lib/src/error/codes.dart
+++ b/pkg/analyzer/lib/src/error/codes.dart
@@ -1468,6 +1468,22 @@
"A continue label resolves to switch, must be loop or switch member");
/**
+ * Parameters:
+ * 0: the name of the formal parameter.
+ * 1: the name of the member.
+ * 2: the name of the class with the implementation of the member.
+ */
+ static const CompileTimeErrorCode
+ COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION =
+ CompileTimeErrorCode(
+ 'COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION',
+ "The parameter '{0}' of '{1}' is covariant in the interface of the class, "
+ "but is not covariant in the implementation '{2}.{1}'.",
+ correction: "Try removing the covariant modifier in the interface, "
+ "or overriding the method in the class and calling super.",
+ );
+
+ /**
* No parameters.
*/
// #### Description
diff --git a/pkg/analyzer/lib/src/error/correct_override.dart b/pkg/analyzer/lib/src/error/correct_override.dart
index 7d67e40..7ff8919 100644
--- a/pkg/analyzer/lib/src/error/correct_override.dart
+++ b/pkg/analyzer/lib/src/error/correct_override.dart
@@ -9,6 +9,7 @@
import 'package:analyzer/error/listener.dart';
import 'package:analyzer/src/dart/analysis/session.dart';
import 'package:analyzer/src/dart/element/element.dart';
+import 'package:analyzer/src/dart/element/inheritance_manager3.dart';
import 'package:analyzer/src/dart/element/type.dart';
import 'package:analyzer/src/dart/element/type_algebra.dart';
import 'package:analyzer/src/error/codes.dart';
@@ -16,6 +17,49 @@
import 'package:analyzer/src/generated/type_system.dart';
import 'package:meta/meta.dart';
+/// It is an error if a formal parameter is covariant in the interface, but
+/// the corresponding formal parameter in the concrete declaration is not
+/// covariant.
+///
+/// This happens when the concrete declaration is inherited, not when it
+/// is declared in the class being verified, otherwise it would inherit
+/// the covariant-by-declaration flag.
+void verifyConcreteForCovariantInterface({
+ @required InheritanceManager3 inheritance,
+ @required Name name,
+ @required ExecutableElement concreteMember,
+ @required List<InterfaceType> directInterfaces,
+ @required ErrorReporter errorReporter,
+ @required AstNode errorNode,
+}) {
+ for (var directInterface in directInterfaces) {
+ var interfaceMember = inheritance.getMember(directInterface, name);
+ if (interfaceMember == null) continue;
+
+ var interfaceParameters = interfaceMember.parameters;
+ for (var i = 0; i < interfaceParameters.length; i++) {
+ var interfaceParameter = interfaceParameters[i];
+ if (interfaceParameter.isCovariant) {
+ var concreteParameter =
+ CovariantParametersVerifier._correspondingParameter(
+ concreteMember.parameters, interfaceParameter, i);
+ if (concreteParameter != null && !concreteParameter.isCovariant) {
+ errorReporter.reportErrorForNode(
+ CompileTimeErrorCode
+ .COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION,
+ errorNode,
+ [
+ interfaceParameter.name,
+ interfaceMember.displayName,
+ concreteMember.enclosingElement.displayName,
+ ],
+ );
+ }
+ }
+ }
+ }
+}
+
class CorrectOverrideHelper {
final LibraryElementImpl _library;
final TypeSystemImpl _typeSystem;
diff --git a/pkg/analyzer/lib/src/error/inheritance_override.dart b/pkg/analyzer/lib/src/error/inheritance_override.dart
index 7d6fc78..00e6fa3 100644
--- a/pkg/analyzer/lib/src/error/inheritance_override.dart
+++ b/pkg/analyzer/lib/src/error/inheritance_override.dart
@@ -237,6 +237,19 @@
}
_reportInheritedAbstractMembers(inheritedAbstract);
+
+ for (var entry in interface.implemented.entries) {
+ var name = entry.key;
+ var concreteElement = entry.value;
+ verifyConcreteForCovariantInterface(
+ inheritance: inheritance,
+ name: name,
+ concreteMember: concreteElement,
+ directInterfaces: directSuperInterfaces,
+ errorReporter: reporter,
+ errorNode: classNameNode,
+ );
+ }
}
}
diff --git a/pkg/analyzer/test/src/diagnostics/covariant_interface_parameter_with_non_covariant_implementation_test.dart b/pkg/analyzer/test/src/diagnostics/covariant_interface_parameter_with_non_covariant_implementation_test.dart
new file mode 100644
index 0000000..3a51587
--- /dev/null
+++ b/pkg/analyzer/test/src/diagnostics/covariant_interface_parameter_with_non_covariant_implementation_test.dart
@@ -0,0 +1,67 @@
+// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
+// 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:analyzer/src/error/codes.dart';
+import 'package:test_reflective_loader/test_reflective_loader.dart';
+
+import '../dart/resolution/driver_resolution.dart';
+
+main() {
+ defineReflectiveSuite(() {
+ defineReflectiveTests(
+ CovariantInterfaceParameterWithNonCovariantImplementationTest);
+ });
+}
+
+@reflectiveTest
+class CovariantInterfaceParameterWithNonCovariantImplementationTest
+ extends DriverResolutionTest {
+ test_method_covariantBoth() async {
+ await assertNoErrorsInCode(r'''
+class C {
+ void foo(covariant int x) {}
+}
+
+abstract class I {
+ void foo(covariant int x);
+}
+
+class D extends C implements I {}
+''');
+ }
+
+ test_method_covariantImplementation() async {
+ await assertNoErrorsInCode(r'''
+class C {
+ void foo(covariant int x) {}
+}
+
+abstract class I {
+ void foo(int x);
+}
+
+class D extends C implements I {}
+''');
+ }
+
+ test_method_covariantInterface() async {
+ await assertErrorsInCode(r'''
+class C {
+ void foo(int x) {}
+}
+
+abstract class I {
+ void foo(covariant int x);
+}
+
+class D extends C implements I {}
+''', [
+ error(
+ CompileTimeErrorCode
+ .COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION,
+ 91,
+ 1),
+ ]);
+ }
+}
diff --git a/pkg/analyzer/test/src/diagnostics/test_all.dart b/pkg/analyzer/test/src/diagnostics/test_all.dart
index 76664cc..6955167 100644
--- a/pkg/analyzer/test/src/diagnostics/test_all.dart
+++ b/pkg/analyzer/test/src/diagnostics/test_all.dart
@@ -56,6 +56,8 @@
import 'const_spread_expected_list_or_set_test.dart'
as const_spread_expected_list_or_set;
import 'const_spread_expected_map_test.dart' as const_spread_expected_map;
+import 'covariant_interface_parameter_with_non_covariant_implementation_test.dart'
+ as covariant_interface_parameter_with_non_covariant_implementation;
import 'dead_code_test.dart' as dead_code;
import 'dead_null_aware_expression_test.dart' as dead_null_aware_expression;
import 'default_list_constructor_test.dart' as default_list_constructor;
@@ -545,6 +547,7 @@
const_set_element_type_implements_equals.main();
const_spread_expected_list_or_set.main();
const_spread_expected_map.main();
+ covariant_interface_parameter_with_non_covariant_implementation.main();
dead_code.main();
dead_null_aware_expression.main();
default_list_constructor.main();
diff --git a/tests/language/covariant_override/tear_off_type_test.dart b/tests/language/covariant_override/tear_off_type_test.dart
index 732aaf6..3e911a7 100644
--- a/tests/language/covariant_override/tear_off_type_test.dart
+++ b/tests/language/covariant_override/tear_off_type_test.dart
@@ -26,6 +26,8 @@
}
class C extends Object with M1, M2 {}
+// ^
+// [analyzer] COMPILE_TIME_ERROR.COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION
class Direct {
void positional(covariant int a, int b, covariant int c, int d, int e) {}
@@ -96,8 +98,20 @@
}
class Mixed extends Superclass
- with Mixin1, Mixin2
- implements Interface1, Interface2 {}
+// ^^^^^
+// [analyzer] COMPILE_TIME_ERROR.COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION
+// ^^^^^
+// [analyzer] COMPILE_TIME_ERROR.COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION
+// ^^^^^
+// [analyzer] COMPILE_TIME_ERROR.COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION
+// ^^^^^
+// [analyzer] COMPILE_TIME_ERROR.COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION
+ with
+ Mixin1,
+ Mixin2
+ implements
+ Interface1,
+ Interface2 {}
void main() {
testDirect();
diff --git a/tests/language_2/covariant_override/tear_off_type_test.dart b/tests/language_2/covariant_override/tear_off_type_test.dart
index 59991b8..cfb1f48 100644
--- a/tests/language_2/covariant_override/tear_off_type_test.dart
+++ b/tests/language_2/covariant_override/tear_off_type_test.dart
@@ -26,6 +26,8 @@
}
class C extends Object with M1, M2 {}
+// ^
+// [analyzer] COMPILE_TIME_ERROR.COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION
class Direct {
void positional(covariant int a, int b, covariant int c, int d, int e) {}
@@ -94,8 +96,20 @@
}
class Mixed extends Superclass
- with Mixin1, Mixin2
- implements Interface1, Interface2 {}
+// ^^^^^
+// [analyzer] COMPILE_TIME_ERROR.COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION
+// ^^^^^
+// [analyzer] COMPILE_TIME_ERROR.COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION
+// ^^^^^
+// [analyzer] COMPILE_TIME_ERROR.COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION
+// ^^^^^
+// [analyzer] COMPILE_TIME_ERROR.COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION
+ with
+ Mixin1,
+ Mixin2
+ implements
+ Interface1,
+ Interface2 {}
void main() {
testDirect();
diff --git a/tests/language_2/regress/regress31596_covariant_declaration_test.dart b/tests/language_2/regress/regress31596_covariant_declaration_test.dart
index 20defe2..f3cfe46 100644
--- a/tests/language_2/regress/regress31596_covariant_declaration_test.dart
+++ b/tests/language_2/regress/regress31596_covariant_declaration_test.dart
@@ -20,5 +20,7 @@
// ^
// [analyzer] COMPILE_TIME_ERROR.INVALID_OVERRIDE
// [cfe] unspecified
+// ^
+// [analyzer] COMPILE_TIME_ERROR.COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION
main() {}
To view, visit change 146120. To unsubscribe, or for help writing mail filters, visit settings.
Erik Ernst (Gerrit)
LGTM!
One thing noted in comments: The error arises when a dynamic type check on an actual argument is absent in the inherited implementation, but required due to a formal parameter which is covariant-by-declaration in the class interface. This means that it is enough for the inherited implementation to have the check due to the parameter being covariant-by-class or -by-declaration. I believe the implementation and comments below omit the -by-class case.
Patch set 4:Code-Review +1
7 comments:
File pkg/analyzer/lib/error/error.dart:
Patch Set #4, Line 130:
.COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATION,Wow, I can even understand the long identifier. ;-)
File pkg/analyzer/lib/src/error/codes.dart:
Patch Set #4, Line 1483:
"or overriding the method in the class and calling super.",Maybe give the advice in the opposite order (because the override _will_ resolve the issue, but editing the superinterface(s) where that parameter is made covariant-by-declaration may break other subtypes).
File pkg/analyzer/lib/src/error/correct_override.dart:
Patch Set #4, Line 22:
/// covariant.It would not be an error if the parameter is covariant-by-class, but I suppose the word 'covariant' is generally taken to mean 'covariant-by-declaration' in these libraries?
Actually, we can have a situation where the parameter in the interface is covariant-by-declaration, and the parameter of the inherited implementation is covariant-by-class, and in that situation the inherited implementation would already perform the type check on the actual argument, so there is no need for a generated forwarder. Because of that, I used the phrase `which is not covariant` rather than `which is not covariant-by-declaration` in line 3 of language issue #925.
So I'd consider the following wording safer:
/// It is an error if a formal parameter is covariant-by-declaration in the
/// interface, but the corresponding formal parameter in the concrete declaration
/// is not covariant.Patch Set #4, Line 42:
if (interfaceParameter.isCovariant) {`isCovariant` here should mean 'is-covariant-by-declaration'.
Patch Set #4, Line 46:
if (concreteParameter != null && !concreteParameter.isCovariant) {`isCovariant` here should mean 'is-covariant-by-declaration || is-covariant-by-class'.
Could have this case, too, expecting no errors:
```
class C<X> {
void foo(X x) {}
}abstract class I {
void foo(covariant List<int> x);
}class D extends C<List<num>> implements I {}
```
File tests/language_2/covariant_override/tear_off_type_test.dart:
Patch Set #4, Line 106:
// [analyzer] COMPILE_TIME_ERROR.COVARIANT_INTERFACE_PARAMETER_WITH_NON_COVARIANT_IMPLEMENTATIONI guess there is no way we can see that it is `d` that doesn't have an error? ;-)
To view, visit change 146120. To unsubscribe, or for help writing mail filters, visit settings.
Brian Wilkerson (Gerrit)
Patch set 4:Code-Review +1
2 comments:
File pkg/analyzer/lib/src/error/codes.dart:
Patch Set #4, Line 1480:
the classCan we replace "the class" with the name of the class that defines '0' as being covariant? I think that would make it easier for users to understand one of the places to look in order to fix the problem.
File pkg/analyzer/lib/src/error/correct_override.dart:
Patch Set #4, Line 47:
reportErrorForNodeI would love to have a context message here pointing users to the interface parameter, preferably by adding a method to `DiagnosticFactory` so that context messages are handled uniformly.
To view, visit change 146120. To unsubscribe, or for help writing mail filters, visit settings.
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov abandoned this change.
To view, visit change 146120. To unsubscribe, or for help writing mail filters, visit settings.