Revert "Revert "[analyzer] new warning for nullable '==' parameter type""

This reverts commit 885457e.

[analyzer] new warning for nullable '==' parameter type

This rule checks that a parameter to an `operator ==` implementation has
a non-nullable type.

I intentionally did not enforce, in this rule, that the parameter is
exactly `Object`. It is legal to narrow the parameter type to a
different non-nullable type, like `int`. I can't imagine doing it, but
it seems to be unrelated to whether the type should be nullable or not.

Fixes https://github.com/dart-lang/linter/issues/3441

Replaces dart-archive/linter#3923

Change-Id: Ic0be2bfebaf59b0336e9a3a58e5b7f5359eb8646
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/291042
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Stephen Adams <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>

Author: srawlins

pkg/_fe_analyzer_shared/lib/src/parser/identifier_context_impl.dart

@@ -449,7 +449,7 @@ class ExtensionShowHideElementIdentifierContext extends IdentifierContext {
   }
 
   @override
-  bool operator ==(dynamic other) {
+  bool operator ==(Object other) {
     return other is ExtensionShowHideElementIdentifierContext &&
         _kind == other._kind;
   }

pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml

@@ -3699,6 +3699,11 @@ WarningCode.NON_CONST_CALL_TO_LITERAL_CONSTRUCTOR_USING_NEW:
   status: needsFix
   notes: |-
     The fix is to replace `new` with `const`.
+WarningCode.NON_NULLABLE_EQUALS_PARAMETER:
+  status: needsFix
+  notes: |-
+    The fix is to remove the question mark if the parameter has one (if the
+    written type is an alias, it may not).
 WarningCode.NULLABLE_TYPE_IN_CATCH_CLAUSE:
   status: hasFix
 WarningCode.NULL_ARGUMENT_TO_NON_NULL_TYPE:

pkg/analysis_server/test/src/services/correction/fix/remove_comparison_test.dart

@@ -688,7 +688,7 @@ class Person {
   final String name = '';
 
   @override
-  operator ==(Object? other) =>
+  operator ==(Object other) =>
           other != null &&
           other is Person &&
           name == other.name;
@@ -699,7 +699,7 @@ class Person {
   final String name = '';
 
   @override
-  operator ==(Object? other) =>
+  operator ==(Object other) =>
           other is Person &&
           name == other.name;
 }
@@ -712,7 +712,7 @@ class Person {
   final String name = '';
 
   @override
-  operator ==(Object? other) {
+  operator ==(Object other) {
     return other != null &&
           other is Person &&
           name == other.name;
@@ -724,7 +724,7 @@ class Person {
   final String name = '';
 
   @override
-  operator ==(Object? other) {
+  operator ==(Object other) {
     return other is Person &&
           name == other.name;
   }
@@ -738,7 +738,7 @@ class Person {
   final String name = '';
 
   @override
-  operator ==(Object? other) {
+  operator ==(Object other) {
     if (other is! Person) return false;
     other ??= Person();
     return other.name == name;

pkg/analysis_server/tool/code_completion/metrics_util.dart

@@ -458,7 +458,7 @@ class Place {
   int get rank => _numerator;
 
   @override
-  bool operator ==(dynamic other) =>
+  bool operator ==(Object other) =>
       other is Place &&
       _numerator == other._numerator &&
       _denominator == other._denominator;

pkg/analyzer/lib/src/error/best_practices_verifier.dart

@@ -593,6 +593,7 @@ class BestPracticesVerifier extends RecursiveAstVisitor<void> {
       _checkForMissingReturn(node.body, node);
       _mustCallSuperVerifier.checkMethodDeclaration(node);
       _checkForUnnecessaryNoSuchMethod(node);
+      _checkForNullableEqualsParameterType(node);
 
       var name = Name(_currentLibrary.source.uri, element.name);
       var elementIsOverride = element is ClassMemberElement &&
@@ -1262,6 +1263,44 @@ class BestPracticesVerifier extends RecursiveAstVisitor<void> {
     }
   }
 
+  void _checkForNullableEqualsParameterType(MethodDeclaration node) {
+    if (!_typeSystem.isNonNullableByDefault) {
+      // Cannot specify non-nullable types before null safety.
+      return;
+    }
+
+    if (node.name.type != TokenType.EQ_EQ) {
+      return;
+    }
+
+    var parameters = node.parameters;
+    if (parameters == null) {
+      return;
+    }
+
+    if (parameters.parameters.length != 1) {
+      return;
+    }
+
+    var parameter = parameters.parameters.first;
+    var parameterElement = parameter.declaredElement;
+    if (parameterElement == null) {
+      return;
+    }
+
+    var type = parameterElement.type;
+    if (!type.isDartCoreObject && type is! DynamicType) {
+      // There is no legal way to define a nullable parameter type, which is not
+      // `dynamic` or `Object?`, so avoid double reporting here.
+      return;
+    }
+
+    if (_typeSystem.isNullable(parameterElement.type)) {
+      _errorReporter.reportErrorForToken(
+          WarningCode.NON_NULLABLE_EQUALS_PARAMETER, node.name);
+    }
+  }
+
   void _checkForNullableTypeInCatchClause(CatchClause node) {
     if (!_isNonNullableByDefault) {
       return;

pkg/analyzer/lib/src/error/codes.g.dart

@@ -6894,6 +6894,13 @@ class WarningCode extends AnalyzerErrorCode {
     uniqueName: 'NON_CONST_CALL_TO_LITERAL_CONSTRUCTOR_USING_NEW',
   );
 
+  ///  No parameters.
+  static const WarningCode NON_NULLABLE_EQUALS_PARAMETER = WarningCode(
+    'NON_NULLABLE_EQUALS_PARAMETER',
+    "The parameter type of '==' operators should be non-nullable.",
+    correctionMessage: "Try using a non-nullable type.",
+  );
+
   ///  No parameters.
   static const WarningCode NULLABLE_TYPE_IN_CATCH_CLAUSE = WarningCode(
     'NULLABLE_TYPE_IN_CATCH_CLAUSE',

pkg/analyzer/lib/src/error/error_code_values.g.dart

@@ -1038,6 +1038,7 @@ const List<ErrorCode> errorCodeValues = [
   WarningCode.MUST_CALL_SUPER,
   WarningCode.NON_CONST_CALL_TO_LITERAL_CONSTRUCTOR,
   WarningCode.NON_CONST_CALL_TO_LITERAL_CONSTRUCTOR_USING_NEW,
+  WarningCode.NON_NULLABLE_EQUALS_PARAMETER,
   WarningCode.NULLABLE_TYPE_IN_CATCH_CLAUSE,
   WarningCode.NULL_ARGUMENT_TO_NON_NULL_TYPE,
   WarningCode.NULL_CHECK_ALWAYS_FAILS,

pkg/analyzer/messages.yaml

@@ -6906,7 +6906,7 @@ CompileTimeErrorCode:
 
       ```dart
       mixin M on Enum {
-        bool operator [!==!](Object? other) => false;
+        bool operator [!==!](Object other) => false;
       }
       ```
 
@@ -24835,6 +24835,11 @@ WarningCode:
 
       void f() => const C();
       ```
+  NON_NULLABLE_EQUALS_PARAMETER:
+    problemMessage: "The parameter type of '==' operators should be non-nullable."
+    correctionMessage: Try using a non-nullable type.
+    hasPublishedDocs: false
+    comment: No parameters.
   NULLABLE_TYPE_IN_CATCH_CLAUSE:
     problemMessage: "A potentially nullable type can't be used in an 'on' clause because it isn't valid to throw a nullable expression."
     correctionMessage: Try using a non-nullable type.

pkg/analyzer/test/src/diagnostics/extension_declares_member_of_object_test.dart

@@ -28,18 +28,18 @@ extension E on String {
   test_instance_sameKind() async {
     await assertErrorsInCode('''
 extension E on String {
-  bool operator==(_) => false;
+  bool operator==(Object _) => false;
   int get hashCode => 0;
   String toString() => '';
   dynamic get runtimeType => null;
   dynamic noSuchMethod(_) => null;
 }
 ''', [
       error(CompileTimeErrorCode.EXTENSION_DECLARES_MEMBER_OF_OBJECT, 39, 2),
-      error(CompileTimeErrorCode.EXTENSION_DECLARES_MEMBER_OF_OBJECT, 65, 8),
-      error(CompileTimeErrorCode.EXTENSION_DECLARES_MEMBER_OF_OBJECT, 89, 8),
-      error(CompileTimeErrorCode.EXTENSION_DECLARES_MEMBER_OF_OBJECT, 121, 11),
-      error(CompileTimeErrorCode.EXTENSION_DECLARES_MEMBER_OF_OBJECT, 152, 12),
+      error(CompileTimeErrorCode.EXTENSION_DECLARES_MEMBER_OF_OBJECT, 72, 8),
+      error(CompileTimeErrorCode.EXTENSION_DECLARES_MEMBER_OF_OBJECT, 96, 8),
+      error(CompileTimeErrorCode.EXTENSION_DECLARES_MEMBER_OF_OBJECT, 128, 11),
+      error(CompileTimeErrorCode.EXTENSION_DECLARES_MEMBER_OF_OBJECT, 159, 12),
     ]);
   }
 

pkg/analyzer/test/src/diagnostics/extension_type_declares_member_of_object_test.dart

@@ -29,7 +29,7 @@ extension type E(int it) {
   test_instance_sameKind() async {
     await assertErrorsInCode('''
 extension type E(int it) {
-  bool operator==(_) => false;
+  bool operator==(Object _) => false;
   int get hashCode => 0;
   String toString() => '';
   dynamic get runtimeType => null;
@@ -39,12 +39,12 @@ extension type E(int it) {
       error(
           CompileTimeErrorCode.EXTENSION_TYPE_DECLARES_MEMBER_OF_OBJECT, 42, 2),
       error(
-          CompileTimeErrorCode.EXTENSION_TYPE_DECLARES_MEMBER_OF_OBJECT, 68, 8),
+          CompileTimeErrorCode.EXTENSION_TYPE_DECLARES_MEMBER_OF_OBJECT, 75, 8),
       error(
-          CompileTimeErrorCode.EXTENSION_TYPE_DECLARES_MEMBER_OF_OBJECT, 92, 8),
-      error(CompileTimeErrorCode.EXTENSION_TYPE_DECLARES_MEMBER_OF_OBJECT, 124,
+          CompileTimeErrorCode.EXTENSION_TYPE_DECLARES_MEMBER_OF_OBJECT, 99, 8),
+      error(CompileTimeErrorCode.EXTENSION_TYPE_DECLARES_MEMBER_OF_OBJECT, 131,
           11),
-      error(CompileTimeErrorCode.EXTENSION_TYPE_DECLARES_MEMBER_OF_OBJECT, 155,
+      error(CompileTimeErrorCode.EXTENSION_TYPE_DECLARES_MEMBER_OF_OBJECT, 162,
           12),
     ]);
   }

pkg/analyzer/test/src/diagnostics/non_nullable_equals_parameter_test.dart

@@ -0,0 +1,98 @@
+// Copyright (c) 2023, 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:analyzer/src/utilities/legacy.dart';
+import 'package:test_reflective_loader/test_reflective_loader.dart';
+
+import '../dart/resolution/context_collection_resolution.dart';
+
+main() {
+  defineReflectiveSuite(() {
+    defineReflectiveTests(NonNullableEqualsParameterTest);
+  });
+}
+
+@reflectiveTest
+class NonNullableEqualsParameterTest extends PubPackageResolutionTest {
+  test_dynamic() async {
+    await assertErrorsInCode(r'''
+class C {
+  @override
+  bool operator ==(dynamic other) => false;
+}
+''', [
+      error(WarningCode.NON_NULLABLE_EQUALS_PARAMETER, 38, 2),
+    ]);
+  }
+
+  test_inheritedDynamic() async {
+    await assertErrorsInCode(r'''
+class C {
+  @override
+  bool operator ==(dynamic other) => false;
+}
+class D extends C {
+  @override
+  bool operator ==(other) => false;
+}
+''', [
+      error(WarningCode.NON_NULLABLE_EQUALS_PARAMETER, 38, 2),
+      error(WarningCode.NON_NULLABLE_EQUALS_PARAMETER, 116, 2),
+    ]);
+  }
+
+  test_inheritedFromObject() async {
+    await assertNoErrorsInCode(r'''
+class C {
+  @override
+  bool operator ==(other) => false;
+}
+''');
+  }
+
+  test_inheritedFromObject_preNullSafe() async {
+    try {
+      noSoundNullSafety = false;
+      await assertNoErrorsInCode(r'''
+// @dart=2.9
+class C {
+  @override
+  bool operator ==(other) => false;
+}
+''');
+    } finally {
+      noSoundNullSafety = true;
+    }
+  }
+
+  test_int() async {
+    await assertNoErrorsInCode(r'''
+class C {
+  @override
+  bool operator ==(covariant int other) => false;
+}
+''');
+  }
+
+  test_nullableObject() async {
+    await assertErrorsInCode(r'''
+class C {
+  @override
+  bool operator ==(Object? other) => false;
+}
+''', [
+      error(WarningCode.NON_NULLABLE_EQUALS_PARAMETER, 38, 2),
+    ]);
+  }
+
+  test_object() async {
+    await assertNoErrorsInCode(r'''
+class C {
+  @override
+  bool operator ==(Object other) => false;
+}
+''');
+  }
+}

pkg/analyzer/test/src/diagnostics/test_all.dart

@@ -637,6 +637,7 @@ import 'non_generative_implicit_constructor_test.dart'
 import 'non_native_function_type_argument_to_pointer_test.dart'
     as non_native_function_type_argument_to_pointer;
 import 'non_null_opt_out_test.dart' as non_null_opt_out;
+import 'non_nullable_equals_parameter_test.dart' as non_null_equals_parameters;
 import 'non_positive_array_dimension_test.dart' as non_positive_array_dimension;
 import 'non_sized_type_argument_test.dart' as non_sized_type_argument;
 import 'non_type_as_type_argument_test.dart' as non_type_as_type_argument;
@@ -1327,6 +1328,7 @@ main() {
     non_generative_implicit_constructor.main();
     non_native_function_type_argument_to_pointer.main();
     non_null_opt_out.main();
+    non_null_equals_parameters.main();
     non_positive_array_dimension.main();
     non_sized_type_argument.main();
     non_type_as_type_argument.main();

pkg/analyzer/tool/diagnostics/diagnostics.md

@@ -8189,7 +8189,7 @@ has `Enum` in the `on` clause, declares an explicit operator named `==`:
 
 {% prettify dart tag=pre+code %}
 mixin M on Enum {
-  bool operator [!==!](Object? other) => false;
+  bool operator [!==!](Object other) => false;
 }
 {% endprettify %}
 

pkg/compiler/lib/src/inferrer/locals_handler.dart

@@ -254,7 +254,7 @@ class ArgumentsTypes extends IterableMixin<TypeInformation> {
   String toString() => "{ positional = $positional, named = $named }";
 
   @override
-  bool operator ==(Object? other) {
+  bool operator ==(Object other) {
     if (other is! ArgumentsTypes) return false;
     if (positional.length != other.positional.length) return false;
     if (named.length != other.named.length) return false;