[Protocol conformance] Simplify/unify checking for @objc/non-@objc conflicts.

When a non-@objc witness matches an @objc requirement except for
@objc-ness, treat it the same way whether it's an optional requirement
or not, except that it's a warning for the optional case. Should
finish off rdar://problem/25159872.

Author: DougGregor

include/swift/AST/DiagnosticsSema.def

@@ -1269,27 +1269,22 @@ NOTE(protocol_conformance_implied_here,none,
      "implied protocol conformance %0 here can be made explicit", (Identifier))
 
 // "Near matches"
-WARNING(optional_req_nonobjc_near_match,none,
-        "non-@objc %select{initializer %1|method %1|property %1|subscript}0 "
-        "does not satisfy optional requirement of @objc protocol %2",
-        (RequirementKind, DeclName, DeclName))
-NOTE(optional_req_nonobjc_near_match_add_objc,none,
-     "add '@objc' to provide an Objective-C entrypoint", ())
-NOTE(optional_req_nonobjc_near_match_silence,none,
-     "add '@nonobjc' to silence this warning", ())
-
 WARNING(optional_req_near_match,none,
         "%0 %1 nearly matches optional requirement %2 of protocol %3",
         (DescriptiveDeclKind, DeclName, DeclName, DeclName))
 NOTE(optional_req_near_match_rename,none,
      "rename to %0 to satisfy this requirement",
      (DeclName))
+NOTE(optional_req_nonobjc_near_match_add_objc,none,
+     "add '@objc' to provide an Objective-C entrypoint", ())
+NOTE(optional_req_nonobjc_to_objc,none,
+     "rename to %0 to satisfy this requirement",
+     (DeclName))
 NOTE(optional_req_near_match_move,none,
      "move %0 to %select{an|another}1 extension to silence this warning",
      (DeclName, unsigned))
 NOTE(optional_req_near_match_nonobjc,none,
-     "move %0 to %select{an|another}1 extension or add '@nonobjc' to "
-     "silence this warning", (DeclName))
+     "add '@nonobjc' to silence this %select{warning|error}0", (bool))
 NOTE(optional_req_near_match_accessibility,none,
      "make %0 %select{ERROR|private|private or internal}1 to silence this "
      "warning", (DeclName, Accessibility))
@@ -2700,12 +2695,24 @@ NOTE(objc_optional_requirement_swift_rename,none,
      "requirement %1", (bool, DeclName))
 
 ERROR(witness_non_objc,none,
-      "non-'@objc' " OBJC_DIAG_SELECT " does not satisfy '@objc' requirement in "
-      "protocol %2", (unsigned, DeclName, DeclName))
+      "non-'@objc' " OBJC_DIAG_SELECT " does not satisfy requirement "
+      "of '@objc' protocol %2",
+      (unsigned, DeclName, DeclName))
+
+WARNING(witness_non_objc_optional,none,
+        "non-'@objc' " OBJC_DIAG_SELECT " does not satisfy optional "
+        "requirement of '@objc' protocol %2",
+        (unsigned, DeclName, DeclName))
 
 ERROR(witness_non_objc_storage,none,
-      "non-'@objc' %select{property %1|subscript}0 does not satisfy '@objc' "
-      "requirement in protocol %2", (bool, DeclName, DeclName))
+      "non-'@objc' %select{property %1|subscript}0 does not satisfy "
+      "requirement of '@objc' protocol %2",
+      (bool, DeclName, DeclName))
+
+WARNING(witness_non_objc_storage_optional,none,
+      "non-'@objc' %select{property %1|subscript}0 does not satisfy "
+      "optional requirement of '@objc' protocol %2",
+      (bool, DeclName, DeclName))
 
 ERROR(nonobjc_not_allowed,none,
       "declaration is %" OBJC_ATTR_SELECT "0, and cannot be marked @nonobjc",

lib/AST/ASTContext.cpp

@@ -2487,7 +2487,7 @@ bool ASTContext::diagnoseObjCUnsatisfiedOptReqConflicts(SourceFile &sf) {
     if (auto objcAttr = conflicts[0]->getAttrs().getAttribute<ObjCAttr>())
       hasExplicitObjCAttribute = !objcAttr->isImplicit();
     if (!hasExplicitObjCAttribute)
-      Diags.diagnose(conflicts[0], diag::optional_req_nonobjc_near_match_silence)
+      Diags.diagnose(conflicts[0], diag::optional_req_near_match_nonobjc, true)
         .fixItInsert(
           conflicts[0]->getAttributeInsertionLoc(/*forModifier=*/false),
           "@nonobjc ");

lib/Sema/TypeCheckProtocol.cpp

@@ -1395,15 +1395,6 @@ namespace {
     /// Record that the given optional requirement has no witness.
     void recordOptionalWitness(ValueDecl *requirement);
 
-    /// Check whether there is a potential non-@objc witness for an
-    /// @objc optional requirement.
-    void checkOptionalWitnessNonObjCMatch(ValueDecl *requirement);
-
-    /// Check whether there is a potential non-@objc witness for an
-    /// @objc optional requirement.
-    bool diagnoseOptionalWitnessNonObjCMatch(ValueDecl *requirement,
-                                             ValueDecl *witness);
-
     /// Record a type witness.
     ///
     /// \param assocType The associated type whose witness is being recorded.
@@ -1870,62 +1861,6 @@ void ConformanceChecker::recordOptionalWitness(ValueDecl *requirement) {
 
   // Record that there is no witness.
   Conformance->setWitness(requirement, ConcreteDeclRef());
-
-  // If the requirement is @objc, note that we have an unsatisfied
-  // optional @objc requirement.
-  if (requirement->isObjC() &&
-      !requirement->getAttrs().isUnavailable(TC.Context)) {
-    // If we're not suppressing diagnostics, look for a non-@objc near
-    // match.
-    if (!SuppressDiagnostics)
-      checkOptionalWitnessNonObjCMatch(requirement);
-  }
-}
-
-void ConformanceChecker::checkOptionalWitnessNonObjCMatch(
-       ValueDecl *requirement) {
-  for (auto witness : lookupValueWitnesses(requirement, nullptr)) {
-    // If the witness is in a different DeclContext, ignore it.
-    if (witness->getDeclContext() != DC)
-      continue;
-
-    // If the witness is @objc, selector-collision diagnostics will
-    // handle this if there actually is a collision.
-    if (witness->isObjC())
-      continue;
-
-    // Diagnose if needed.
-    if (diagnoseOptionalWitnessNonObjCMatch(requirement, witness))
-      break;
-  }
-}
-
-bool ConformanceChecker::diagnoseOptionalWitnessNonObjCMatch(
-       ValueDecl *requirement, ValueDecl *witness) {
-  // This is silenced by writing @nonobjc.
-  if (witness->getAttrs().hasAttribute<NonObjCAttr>())
-    return false;;
-
-  // Diagnose the non-@objc declaration.
-  TC.diagnose(witness, diag::optional_req_nonobjc_near_match,
-              getRequirementKind(requirement), requirement->getFullName(),
-              Proto->getFullName());
-
-  // Add the appropriate "@objc" attribute to make the witness match.
-  {
-    auto diag = TC.diagnose(witness,
-                            diag::optional_req_nonobjc_near_match_add_objc);
-    fixDeclarationObjCName(diag, witness, requirement->getObjCRuntimeName());
-  }
-
-  // Add the "@nonobjc" attribute to suppress the warning.
-  TC.diagnose(witness, diag::optional_req_nonobjc_near_match_silence)
-    .fixItInsert(witness->getAttributeInsertionLoc(/*forModifier=*/false),
-                 "@nonobjc ");
-
-  TC.diagnose(requirement, diag::protocol_requirement_here,
-              requirement->getFullName());
-  return true;
 }
 
 void ConformanceChecker::recordTypeWitness(AssociatedTypeDecl *assocType,
@@ -3695,35 +3630,29 @@ void ConformanceChecker::checkConformance() {
     auto finalizeWitness = [&] {
       // Find the witness.
       auto witness = Conformance->getWitness(requirement, nullptr).getDecl();
-      if (!witness) {
-        // If this is an unsatisfied @objc optional requirement,
-        // diagnose it.
-        if (requirement->isObjC())
-          checkOptionalWitnessNonObjCMatch(requirement);
-
-        return;
-      }
+      if (!witness) return;
 
       // Objective-C checking for @objc requirements.
       if (requirement->isObjC()) {
         // The witness must also be @objc.
         if (!witness->isObjC()) {
-          // If the requirement is optional, warn.
-          if (requirement->getAttrs().hasAttribute<OptionalAttr>()) {
-            diagnoseOptionalWitnessNonObjCMatch(requirement, witness);
-            return;
-          }
-
+          bool isOptional =
+            requirement->getAttrs().hasAttribute<OptionalAttr>();
           if (auto witnessFunc = dyn_cast<AbstractFunctionDecl>(witness)) {
             auto diagInfo = getObjCMethodDiagInfo(witnessFunc);
-            auto diag = TC.diagnose(witness, diag::witness_non_objc,
+            auto diag = TC.diagnose(witness,
+                                    isOptional ? diag::witness_non_objc_optional
+                                               : diag::witness_non_objc,
                                     diagInfo.first, diagInfo.second,
                                     Proto->getFullName());
             fixDeclarationObjCName(
               diag, witness,
               cast<AbstractFunctionDecl>(requirement)->getObjCSelector());
           } else if (isa<VarDecl>(witness)) {
-            auto diag = TC.diagnose(witness, diag::witness_non_objc_storage,
+            auto diag = TC.diagnose(witness,
+                                    isOptional
+                                      ? diag::witness_non_objc_storage_optional
+                                      : diag::witness_non_objc_storage,
                                     /*isSubscript=*/false,
                                     witness->getFullName(),
                                     Proto->getFullName());
@@ -3732,10 +3661,23 @@ void ConformanceChecker::checkConformance() {
                ObjCSelector(requirement->getASTContext(), 0,
                             cast<VarDecl>(requirement)->getObjCPropertyName()));
           } else if (isa<SubscriptDecl>(witness)) {
-            TC.diagnose(witness, diag::witness_non_objc_storage,
+            TC.diagnose(witness,
+                        isOptional
+                          ? diag::witness_non_objc_storage_optional
+                          : diag::witness_non_objc_storage,
                         /*isSubscript=*/true,
                         witness->getFullName(),
-                        Proto->getFullName());
+                        Proto->getFullName())
+              .fixItInsert(witness->getAttributeInsertionLoc(false),
+                           "@objc ");
+          }
+
+          // If the requirement is optional, @nonobjc suppresses the
+          // diagnostic.
+          if (isOptional) {
+            TC.diagnose(witness, diag::optional_req_near_match_nonobjc, false)
+              .fixItInsert(witness->getAttributeInsertionLoc(false),
+                           "@nonobjc ");
           }
 
           TC.diagnose(requirement, diag::protocol_requirement_here,
@@ -4354,15 +4296,20 @@ static void diagnosePotentialWitness(TypeChecker &tc, ValueDecl *req,
               req->getFullName(),
               proto->getFullName());
 
-  // Warning to fix the names.
   if (req->getFullName() != witness->getFullName()) {
+    // Note to fix the names.
     auto diag = tc.diagnose(witness, diag::optional_req_near_match_rename,
                             req->getFullName());
     fixDeclarationName(diag, witness, req->getFullName());
 
     // Also fix the Objective-C name, if needed.
     if (req->isObjC())
       fixDeclarationObjCName(diag, witness, req->getObjCRuntimeName());
+  } else if (req->isObjC() && !witness->isObjC()) {
+    // Note to add @objc.
+    auto diag = tc.diagnose(witness,
+                            diag::optional_req_nonobjc_near_match_add_objc);
+    fixDeclarationObjCName(diag, witness, req->getObjCRuntimeName());    
   }
 
   // If moving the declaration can help, suggest that.
@@ -4383,7 +4330,7 @@ static void diagnosePotentialWitness(TypeChecker &tc, ValueDecl *req,
 
   // If adding @nonobjc can help, suggest that.
   if (canSuppressPotentialWitnessWarningWithNonObjC(req, witness)) {
-    tc.diagnose(witness, diag::optional_req_nonobjc_near_match_silence)
+    tc.diagnose(witness, diag::optional_req_near_match_nonobjc, false)
       .fixItInsert(witness->getAttributeInsertionLoc(false), "@nonobjc ");
   }
 
@@ -4527,11 +4474,6 @@ void TypeChecker::checkConformancesInContext(DeclContext *dc,
           bestOptionalReqs.push_back(req);
       }
 
-      // If we found an exactly-matching name, don't diagnose anything here.
-      // FIXME: We want to diagnose here with specific detail.
-      if (bestScore == 0)
-        bestOptionalReqs.clear();
-
       // If we found some requirements with nearly-matching names, diagnose
       // the first one.
       if (bestScore < UINT_MAX) {

test/ClangModules/objc_parse.swift

@@ -264,7 +264,7 @@ func classAnyObject(_ obj: NSObject) {
 class Wobbler : NSWobbling { // expected-note{{candidate has non-matching type '()'}}
   @objc func wobble() { }
 
-  func returnMyself() -> Self { return self } // expected-error{{non-'@objc' method 'returnMyself()' does not satisfy '@objc' requirement in protocol 'NSWobbling'}}{{3-3=@objc }}
+  func returnMyself() -> Self { return self } // expected-error{{non-'@objc' method 'returnMyself()' does not satisfy requirement of '@objc' protocol 'NSWobbling'}}{{3-3=@objc }}
 }
 
 extension Wobbler : NSMaybeInitWobble { // expected-error{{type 'Wobbler' does not conform to protocol 'NSMaybeInitWobble'}}

test/attr/attr_objc.swift

@@ -2028,8 +2028,11 @@ class ConformsToProtocolThrowsObjCName1 : ProtocolThrowsObjCName {
   @objc func doThing(_ x: String) throws -> String { return x } // okay
 }
 
-class ConformsToProtocolThrowsObjCName2 : ProtocolThrowsObjCName { // expected-note{{class 'ConformsToProtocolThrowsObjCName2' declares conformance to protocol 'ProtocolThrowsObjCName' here}}
-  @objc func doThing(_ x: Int) throws -> String { return "" } // expected-error{{Objective-C method 'doThing:error:' provided by method 'doThing' conflicts with optional requirement method 'doThing' in protocol 'ProtocolThrowsObjCName'}}
+class ConformsToProtocolThrowsObjCName2 : ProtocolThrowsObjCName {
+  @objc func doThing(_ x: Int) throws -> String { return "" }
+  // expected-warning@-1{{instance method 'doThing' nearly matches optional requirement 'doThing' of protocol 'ProtocolThrowsObjCName'}}
+  // expected-note@-2{{move 'doThing' to an extension to silence this warning}}
+  // expected-note@-3{{make 'doThing' private to silence this warning}}{{9-9=private }}
 }
 
 @objc class DictionaryTest {

test/decl/protocol/objc.swift

@@ -29,9 +29,9 @@
 }
 
 class C1b : P1 {
-  func method1() { } // expected-error{{non-'@objc' method 'method1()' does not satisfy '@objc' requirement in protocol 'P1'}}{{3-3=@objc }}
-  var property1: ObjCClass = ObjCClass() // expected-error{{non-'@objc' property 'property1' does not satisfy '@objc' requirement in protocol 'P1'}}{{3-3=@objc }}
-  var property2: ObjCClass = ObjCClass() // expected-error{{non-'@objc' property 'property2' does not satisfy '@objc' requirement in protocol 'P1'}}{{3-3=@objc }}
+  func method1() { } // expected-error{{non-'@objc' method 'method1()' does not satisfy requirement of '@objc' protocol 'P1'}}{{3-3=@objc }}
+  var property1: ObjCClass = ObjCClass() // expected-error{{non-'@objc' property 'property1' does not satisfy requirement of '@objc' protocol 'P1'}}{{3-3=@objc }}
+  var property2: ObjCClass = ObjCClass() // expected-error{{non-'@objc' property 'property2' does not satisfy requirement of '@objc' protocol 'P1'}}{{3-3=@objc }}
 }
 
 @objc protocol P2 {
@@ -42,9 +42,9 @@ class C1b : P1 {
 }
 
 class C2a : P2 {
-  func method(_: Int, class: ObjCClass) { } // expected-error{{non-'@objc' method 'method(_:class:)' does not satisfy '@objc' requirement in protocol 'P2'}}{{3-3=@objc(methodWithInt:withClass:) }}
+  func method(_: Int, class: ObjCClass) { } // expected-error{{non-'@objc' method 'method(_:class:)' does not satisfy requirement of '@objc' protocol 'P2'}}{{3-3=@objc(methodWithInt:withClass:) }}
 
-  var empty: Bool { // expected-error{{non-'@objc' property 'empty' does not satisfy '@objc' requirement in protocol 'P2'}}{{3-3=@objc }}
+  var empty: Bool { // expected-error{{non-'@objc' property 'empty' does not satisfy requirement of '@objc' protocol 'P2'}}{{3-3=@objc }}
     get { }
   }
 }
@@ -103,5 +103,5 @@ class C3a : P3 {
 }
 
 class Bar: Foo {
-  required init() {} // expected-error{{non-'@objc' initializer 'init()' does not satisfy '@objc' requirement in protocol 'Foo'}}{{3-3=@objc }}
+  required init() {} // expected-error{{non-'@objc' initializer 'init()' does not satisfy requirement of '@objc' protocol 'Foo'}}{{3-3=@objc }}
 }