[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: I61d4a7b1ab8318dc9403da1633c352de95bfac61
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/277700
Reviewed-by: Mark Zhou <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>

Author: srawlins

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

@@ -447,7 +447,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

@@ -27,7 +27,7 @@
 # - 655 "needsEvaluation"
 #   - 282 for CompileTimeErrorCodes
 #   - 220 for ParserErrorCodes
-# -  92 "needsFix"
+# -  93 "needsFix"
 # - 310 "hasFix"
 # -  94 "noFix"
 
@@ -2751,6 +2751,11 @@ StaticWarningCode.INVALID_NULL_AWARE_OPERATOR_AFTER_SHORT_CIRCUIT:
   status: hasFix
 StaticWarningCode.MISSING_ENUM_CONSTANT_IN_SWITCH:
   status: hasFix
+StaticWarningCode.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).
 StaticWarningCode.SDK_VERSION_AS_EXPRESSION_IN_CONST_CONTEXT:
   status: hasFix
 StaticWarningCode.SDK_VERSION_ASYNC_EXPORTED_FROM_CORE:

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

@@ -281,7 +281,7 @@ class Person {
   final String name = '';
 
   @override
-  operator ==(Object? other) =>
+  operator ==(Object other) =>
           other != null &&
           other is Person &&
           name == other.name;
@@ -292,7 +292,7 @@ class Person {
   final String name = '';
 
   @override
-  operator ==(Object? other) =>
+  operator ==(Object other) =>
           other is Person &&
           name == other.name;
 }
@@ -305,7 +305,7 @@ class Person {
   final String name = '';
 
   @override
-  operator ==(Object? other) {
+  operator ==(Object other) {
     return other != null &&
           other is Person &&
           name == other.name;
@@ -317,7 +317,7 @@ class Person {
   final String name = '';
 
   @override
-  operator ==(Object? other) {
+  operator ==(Object other) {
     return other is Person &&
           name == other.name;
   }
@@ -331,7 +331,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;
@@ -349,7 +349,7 @@ class Person {
   final String name = '';
 
   @override
-  operator ==(Object? other) {
+  operator ==(Object other) {
     if (other is! Person) return false;
     final toCompare = other ?? Person();
     return toCompare.name == name;
@@ -362,7 +362,7 @@ class Person {
   final String name = '';
 
   @override
-  operator ==(Object? other) {
+  operator ==(Object other) {
     if (other is! Person) return false;
     final toCompare = other;
     return toCompare.name == name;
@@ -381,7 +381,7 @@ class Person {
   final String name = '';
 
   @override
-  operator ==(Object? other) {
+  operator ==(Object other) {
     if (other == null) return false;
     return other is Person &&
           name == other.name;
@@ -393,7 +393,7 @@ class Person {
   final String name = '';
 
   @override
-  operator ==(Object? other) {
+  operator ==(Object other) {
     return other is Person &&
           name == other.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

@@ -683,6 +683,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 &&
@@ -1390,6 +1391,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.isDynamic) {
+      // 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(
+          StaticWarningCode.NON_NULLABLE_EQUALS_PARAMETER, node.name);
+    }
+  }
+
   void _checkForNullableTypeInCatchClause(CatchClause node) {
     if (!_isNonNullableByDefault) {
       return;

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

@@ -5645,6 +5645,14 @@ class StaticWarningCode extends AnalyzerErrorCode {
     hasPublishedDocs: true,
   );
 
+  ///  No parameters.
+  static const StaticWarningCode NON_NULLABLE_EQUALS_PARAMETER =
+      StaticWarningCode(
+    'NON_NULLABLE_EQUALS_PARAMETER',
+    "The parameter type of '==' operators should be non-nullable.",
+    correctionMessage: "Try using a non-nullable type.",
+  );
+
   ///  Parameters:
   ///  0: the name of the class
   static const StaticWarningCode SDK_VERSION_ASYNC_EXPORTED_FROM_CORE =

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

@@ -964,6 +964,7 @@ const List<ErrorCode> errorCodeValues = [
   StaticWarningCode.INVALID_NULL_AWARE_OPERATOR,
   StaticWarningCode.INVALID_NULL_AWARE_OPERATOR_AFTER_SHORT_CIRCUIT,
   StaticWarningCode.MISSING_ENUM_CONSTANT_IN_SWITCH,
+  StaticWarningCode.NON_NULLABLE_EQUALS_PARAMETER,
   StaticWarningCode.SDK_VERSION_ASYNC_EXPORTED_FROM_CORE,
   StaticWarningCode.SDK_VERSION_AS_EXPRESSION_IN_CONST_CONTEXT,
   StaticWarningCode.SDK_VERSION_BOOL_OPERATOR_IN_CONST_CONTEXT,

pkg/analyzer/messages.yaml

@@ -5635,7 +5635,7 @@ CompileTimeErrorCode:
 
       ```dart
       mixin M on Enum {
-        bool operator [!==!](Object? other) => false;
+        bool operator [!==!](Object other) => false;
       }
       ```
 
@@ -22789,6 +22789,11 @@ StaticWarningCode:
       ```
       TODO(brianwilkerson) This documentation will need to be updated when NNBD
        ships.
+  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.
   SDK_VERSION_ASYNC_EXPORTED_FROM_CORE:
     problemMessage: "The class '{0}' wasn't exported from 'dart:core' until version 2.1, but this code is required to be able to run on earlier versions."
     correctionMessage: "Try either importing 'dart:async' or updating the SDK constraints."

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/non_nullable_equals_parameter_test.dart

@@ -0,0 +1,98 @@
+// Copyright (c) 2019, 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(StaticWarningCode.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(StaticWarningCode.NON_NULLABLE_EQUALS_PARAMETER, 38, 2),
+      error(StaticWarningCode.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(StaticWarningCode.NON_NULLABLE_EQUALS_PARAMETER, 38, 2),
+    ]);
+  }
+
+  test_object() async {
+    await assertNoErrorsInCode(r'''
+class C {
+  @override
+  bool operator ==(Object other) => false;
+}
+''');
+  }
+}