[5.0][Sema] Add dedicated fix-it for NSObject.hashValue overrides (#22173)

* [Sema] Add dedicated fix-it for NSObject.hashValue overrides

NSObject.hashValue used to be declared `@objc open` by historical accident. This has been corrected to `@nonobjc public` in Swift 5’s SDK overlays, to catch accidental overrides. (These never did work correctly, and shouldn’t have been allowed.)

Help migration by adding a dedicated error message for NSObject.hashValue overrides, with a nice fix-it.

rdar://problem/45674813
(cherry picked from commit df8eba4)

* [test] Test new fix-it for NSObject.hashValue overrides

(cherry picked from commit 6af814d)

Author: lorentey

include/swift/AST/DiagnosticsSema.def

@@ -4281,9 +4281,13 @@ WARNING(non_exhaustive_switch_unknown_only,none,
         "switch covers known cases, but %0 may have additional unknown values"
         "%select{|, possibly added in future versions}1", (Type, bool))
 
-WARNING(override_nsobject_hashvalue,none,
+WARNING(override_nsobject_hashvalue_warning,none,
         "override of 'NSObject.hashValue' is deprecated; "
-        "override 'NSObject.hash' to get consistent hashing behavior", ())
+        "did you mean to override 'NSObject.hash'?", ())
+ERROR(override_nsobject_hashvalue_error,none,
+      "'NSObject.hashValue' is not overridable; "
+      "did you mean to override 'NSObject.hash'?", ())
+
 WARNING(hashvalue_implementation,none,
         "'Hashable.hashValue' is deprecated as a protocol requirement; "
         "conform type %0 to 'Hashable' by implementing 'hash(into:)' instead",

lib/Sema/TypeCheckDeclOverride.cpp

@@ -540,6 +540,22 @@ static bool parameterTypesMatch(const ValueDecl *derivedDecl,
   return true;
 }
 
+/// Returns true if the given declaration is for the `NSObject.hashValue`
+/// property.
+static bool isNSObjectHashValue(ValueDecl *baseDecl) {
+  ASTContext &ctx = baseDecl->getASTContext();
+
+  if (auto baseVar = dyn_cast<VarDecl>(baseDecl)) {
+    if (auto classDecl = baseVar->getDeclContext()->getSelfClassDecl()) {
+      return baseVar->getName() == ctx.Id_hashValue &&
+        classDecl->getName().is("NSObject") &&
+        (classDecl->getModuleContext()->getName() == ctx.Id_Foundation ||
+         classDecl->getModuleContext()->getName() == ctx.Id_ObjectiveC);
+    }
+  }
+  return false;
+}
+
 namespace {
   /// Class that handles the checking of a particular declaration against
   /// superclass entities that it could override.
@@ -826,9 +842,15 @@ bool OverrideMatcher::checkOverride(ValueDecl *baseDecl,
       baseDecl->getModuleContext() != decl->getModuleContext() &&
       !isa<ConstructorDecl>(decl) &&
       !isa<ProtocolDecl>(decl->getDeclContext())) {
-    diags.diagnose(decl, diag::override_of_non_open,
-                   decl->getDescriptiveKind());
-
+    // NSObject.hashValue was made non-overridable in Swift 5; one should
+    // override NSObject.hash instead.
+    if (isNSObjectHashValue(baseDecl)) {
+      diags.diagnose(decl, diag::override_nsobject_hashvalue_error)
+        .fixItReplace(SourceRange(decl->getNameLoc()), "hash");
+    } else {
+      diags.diagnose(decl, diag::override_of_non_open,
+                     decl->getDescriptiveKind());
+    }
   } else if (baseHasOpenAccess &&
              classDecl->hasOpenAccess(dc) &&
              decl->getFormalAccess() != AccessLevel::Open &&
@@ -1516,6 +1538,13 @@ static bool checkSingleOverride(ValueDecl *override, ValueDecl *base) {
       (isa<ExtensionDecl>(base->getDeclContext()) ||
        isa<ExtensionDecl>(override->getDeclContext())) &&
       !base->isObjC()) {
+    // Suppress this diagnostic for overrides of a non-open NSObject.hashValue
+    // property; these are diagnosed elsewhere. An error message complaining
+    // about extensions would be misleading in this case; the correct fix is to
+    // override NSObject.hash instead.
+    if (isNSObjectHashValue(base) && 
+        !base->hasOpenAccess(override->getDeclContext()))
+      return true;
     bool baseCanBeObjC = canBeRepresentedInObjC(base);
     diags.diagnose(override, diag::override_decl_extension, baseCanBeObjC,
                    !isa<ExtensionDecl>(base->getDeclContext()));
@@ -1616,15 +1645,12 @@ static bool checkSingleOverride(ValueDecl *override, ValueDecl *base) {
 
   // Overrides of NSObject.hashValue are deprecated; one should override
   // NSObject.hash instead.
-  if (auto baseVar = dyn_cast<VarDecl>(base)) {
-    if (auto classDecl = baseVar->getDeclContext()->getSelfClassDecl()) {
-      if (baseVar->getName() == ctx.Id_hashValue &&
-          classDecl->getName().is("NSObject") &&
-          (classDecl->getModuleContext()->getName() == ctx.Id_Foundation ||
-           classDecl->getModuleContext()->getName() == ctx.Id_ObjectiveC)) {
-        override->diagnose(diag::override_nsobject_hashvalue);
-      }
-    }
+  // FIXME: Remove this when NSObject.hashValue becomes non-open in
+  // swift-corelibs-foundation.
+  if (isNSObjectHashValue(base) &&
+      base->hasOpenAccess(override->getDeclContext())) {
+    override->diagnose(diag::override_nsobject_hashvalue_warning)
+      .fixItReplace(SourceRange(override->getNameLoc()), "hash");
   }
 
   /// Check attributes associated with the base; some may need to merged with

test/ClangImporter/objc_override.swift

@@ -110,7 +110,8 @@ class CallbackSubC : CallbackBase {
 
 //
 class MyHashableNSObject: NSObject {
-  override var hashValue: Int { // expected-error{{overriding non-open property outside of its defining module}} expected-error{{overriding non-@objc declarations from extensions is not supported}}
+  override var hashValue: Int {
+    // expected-error@-1 {{'NSObject.hashValue' is not overridable; did you mean to override 'NSObject.hash'?}}
     return 0
   }
 }

test/Migrator/fixit-NSObject-hashValue.swift

@@ -0,0 +1,14 @@
+// RUN: %empty-directory(%t)
+// RUN: not %target-swift-frontend -typecheck -update-code -primary-file %s -emit-migrated-file-path %t/fixit-NSObject-hashValue.result
+// RUN: diff -u %s.expected %t/fixit-NSObject-hashValue.result
+// RUN: %target-swift-frontend -typecheck %s.expected
+
+// REQUIRES: objc_interop
+
+import ObjectiveC
+
+class MyClass: NSObject {
+  override var hashValue: Int {
+    return 42
+  }
+}

test/Migrator/fixit-NSObject-hashValue.swift.expected

@@ -0,0 +1,14 @@
+// RUN: %empty-directory(%t)
+// RUN: not %target-swift-frontend -typecheck -update-code -primary-file %s -emit-migrated-file-path %t/fixit-NSObject-hashValue.result
+// RUN: diff -u %s.expected %t/fixit-NSObject-hashValue.result
+// RUN: %target-swift-frontend -typecheck %s.expected
+
+// REQUIRES: objc_interop
+
+import ObjectiveC
+
+class MyClass: NSObject {
+  override var hash: Int {
+    return 42
+  }
+}

test/stdlib/NSObject-hashing.swift

@@ -6,10 +6,13 @@
 import ObjectiveC
 
 class Foo: NSObject {
-  override var hashValue: Int { // expected-error {{overriding non-open property outside of its defining module}} expected-error {{overriding non-@objc declarations from extensions is not supported}}
+  override var hashValue: Int {
+    // expected-error@-1 {{'NSObject.hashValue' is not overridable; did you mean to override 'NSObject.hash'?}}
     return 0
   }
 
-  override func hash(into hasher: inout Hasher) { // expected-error {{overriding non-open instance method outside of its defining module}} expected-error {{overriding declarations in extensions is not supported}}
+  override func hash(into hasher: inout Hasher) {
+    // expected-error@-1 {{overriding non-open instance method outside of its defining module}}
+    // expected-error@-2 {{overriding declarations in extensions is not supported}}
   }
 }