Diagnose @objcImpl kind and settability errors

Sema now diagnoses @objcImpl implementations with:

• The wrong settability (i.e. a `let` used for a `readwrite` property)
• The wrong kind (i.e. a method used for a property)

Author: beccadax

include/swift/AST/DiagnosticsSema.def

@@ -1624,6 +1624,15 @@ ERROR(objc_implementation_wrong_category,none,
       "%select{main class interface|category %3}3",
       (DescriptiveDeclKind, ValueDecl *, Identifier, Identifier))
 
+ERROR(objc_implementation_wrong_decl_kind,none,
+      "%0 %1 does not match the %2 declared by the header",
+      (DescriptiveDeclKind, ValueDecl *, DescriptiveDeclKind))
+
+ERROR(objc_implementation_must_be_settable,none,
+      "%0 %1 should be settable to match the settable %2 declared by the "
+      "header",
+      (DescriptiveDeclKind, ValueDecl *, DescriptiveDeclKind))
+
 ERROR(objc_implementation_wrong_objc_name,none,
       "selector %0 for %1 %2 not found in header; did you mean %3?",
       (ObjCSelector, DescriptiveDeclKind, ValueDecl *, ObjCSelector))

lib/Sema/TypeCheckDeclObjC.cpp

@@ -3005,6 +3005,9 @@ class ObjCImplementationChecker {
     WrongImplicitObjCName,
     WrongStaticness,
     WrongCategory,
+    WrongDeclKind,
+//    WrongType,
+    WrongWritability,
 
     Match,
     MatchWithExplicitObjCName,
@@ -3228,11 +3231,24 @@ class ObjCImplementationChecker {
     return false;
   }
 
+  static bool areSwiftNamesEqual(DeclName lhs, DeclName rhs) {
+    // Conflate `foo()` and `foo`. This allows us to diagnose
+    // method-vs.-property mistakes more nicely.
+
+    if (lhs.isCompoundName() && lhs.getArgumentNames().empty())
+      lhs = lhs.getBaseName();
+
+    if (rhs.isCompoundName() && rhs.getArgumentNames().empty())
+      rhs = rhs.getBaseName();
+
+    return lhs == rhs;
+  }
+
   MatchOutcome matches(ValueDecl *req, ValueDecl *cand,
                        ObjCSelector explicitObjCName) const {
     bool hasObjCNameMatch =
         req->getObjCRuntimeName() == cand->getObjCRuntimeName();
-    bool hasSwiftNameMatch = req->getName() == cand->getName();
+    bool hasSwiftNameMatch = areSwiftNamesEqual(req->getName(), cand->getName());
 
     // If neither the ObjC nor Swift names match, there's absolutely no reason
     // to think these two methods are related.
@@ -3261,10 +3277,16 @@ class ObjCImplementationChecker {
           != req->getDeclContext())
       return MatchOutcome::WrongCategory;
 
-    // FIXME: Diagnose candidate without a required setter
-    // FIXME: Diagnose declaration kind mismatches
+    if (cand->getKind() != req->getKind())
+      return MatchOutcome::WrongDeclKind;
+
     // FIXME: Diagnose type mismatches (with allowance for extra optionality)
 
+    if (auto reqVar = dyn_cast<AbstractStorageDecl>(req))
+      if (reqVar->isSettable(nullptr) &&
+            !cast<AbstractStorageDecl>(cand)->isSettable(nullptr))
+        return MatchOutcome::WrongWritability;
+
     // If we got here, everything matched. But at what quality?
     if (explicitObjCName)
       return MatchOutcome::MatchWithExplicitObjCName;
@@ -3325,6 +3347,16 @@ class ObjCImplementationChecker {
                getCategoryName(cand->getDeclContext()->
                                  getImplementedObjCContext()));
       return;
+
+    case MatchOutcome::WrongDeclKind:
+      diagnose(cand, diag::objc_implementation_wrong_decl_kind,
+               cand->getDescriptiveKind(), cand, req->getDescriptiveKind());
+      return;
+
+    case MatchOutcome::WrongWritability:
+      diagnose(cand, diag::objc_implementation_must_be_settable,
+               cand->getDescriptiveKind(), cand, req->getDescriptiveKind());
+      return;
     }
 
     llvm_unreachable("Unknown MatchOutcome");

test/decl/ext/Inputs/objc_implementation.h

@@ -18,6 +18,7 @@
 - (void)methodFromHeader2:(int)param;
 - (void)methodFromHeader3:(int)param;
 - (void)methodFromHeader4:(int)param;
+- (int)methodFromHeader5;
 
 @property int propertyFromHeader1;
 @property int propertyFromHeader2;
@@ -28,6 +29,7 @@
 @property int propertyFromHeader7;
 @property int propertyFromHeader8;
 @property int propertyFromHeader9;
+@property int propertyFromHeader10;
 
 @property (readonly) int readonlyPropertyFromHeader1;
 @property (readonly) int readonlyPropertyFromHeader2;

test/decl/ext/objc_implementation.swift

@@ -38,6 +38,9 @@
     // expected-note@-3 {{add 'final' to define a Swift instance method that cannot be overridden}} {{3-3=final }}
   }
 
+  var methodFromHeader5: CInt
+  // expected-error@-1 {{property 'methodFromHeader5' does not match the instance method declared by the header}}
+
   var propertyFromHeader1: CInt
   // OK, provides an implementation with a stored property
 
@@ -57,16 +60,21 @@
   }
 
   @objc let propertyFromHeader5: CInt
-  // FIXME: bad, needs to be settable
+  // expected-error@-1 {{property 'propertyFromHeader5' should be settable to match the settable property declared by the header}}
 
   @objc var propertyFromHeader6: CInt {
-    // FIXME: bad, needs a setter
+    // expected-error@-1 {{property 'propertyFromHeader6' should be settable to match the settable property declared by the header}}
     get { return 1 }
   }
 
   final var propertyFromHeader8: CInt
   // FIXME: Should complain about final not fulfilling the @objc requirement
 
+  func propertyFromHeader10() -> CInt {
+    // expected-error@-1 {{instance method 'propertyFromHeader10()' does not match the property declared by the header}}
+    return 1
+  }
+
   var readonlyPropertyFromHeader1: CInt
   // OK, provides an implementation with a stored property that's nonpublicly settable