[Diagnostics] SR-2484: Improve diagnostic for incorrectly called private init

Author: xedin

lib/Sema/CSDiag.cpp

@@ -1877,8 +1877,11 @@ bool CalleeCandidateInfo::diagnoseSimpleErrors(const Expr *E) {
       CS->TC.diagnose(loc, diag::candidate_inaccessible, decl->getName(),
                       decl->getFormalAccess());
     }
-    for (auto cand : candidates)
-      CS->TC.diagnose(cand.getDecl(),diag::decl_declared_here, decl->getName());
+    for (auto cand : candidates) {
+      if (auto decl = cand.getDecl()) {
+        CS->TC.diagnose(decl, diag::decl_declared_here, decl->getFullName());
+      }
+    }
 
     return true;
   }
@@ -2235,7 +2238,8 @@ bool FailureDiagnosis::diagnoseGeneralMemberFailure(Constraint *constraint) {
   assert(isMemberConstraint(constraint));
 
   auto memberName = constraint->getMember();
-  
+  auto isInitializer = memberName.isSimpleName(CS->TC.Context.Id_init);
+
   // Get the referenced base expression from the failed constraint, along with
   // the SourceRange for the member ref.  In "x.y", this returns the expr for x
   // and the source range for y.
@@ -2328,12 +2332,21 @@ bool FailureDiagnosis::diagnoseGeneralMemberFailure(Constraint *constraint) {
       .highlight(anchor->getSourceRange()).highlight(memberRange);
     return true;
   }
-  
-  MemberLookupResult result =
-    CS->performMemberLookup(constraint->getKind(), constraint->getMember(),
-                            baseTy, constraint->getFunctionRefKind(),
-                            constraint->getLocator(),
-                            /*includeInaccessibleMembers*/true);
+
+  // If this is initializer/constructor lookup we are dealing this.
+  if (isInitializer) {
+    // Let's check what is the base type we are trying to look it up on
+    // because only MetatypeType is viable to find constructor on, as per
+    // rules in ConstraintSystem::performMemberLookup.
+    if (!baseTy->is<AnyMetatypeType>()) {
+      baseTy = MetatypeType::get(baseTy, CS->getASTContext());
+    }
+  }
+
+  MemberLookupResult result = CS->performMemberLookup(
+      constraint->getKind(), memberName, baseTy,
+      constraint->getFunctionRefKind(), constraint->getLocator(),
+      /*includeInaccessibleMembers*/ true);
 
   switch (result.OverallResult) {
   case MemberLookupResult::Unsolved:      
@@ -2349,48 +2362,72 @@ bool FailureDiagnosis::diagnoseGeneralMemberFailure(Constraint *constraint) {
   case MemberLookupResult::HasResults:
     break;
   }
-  
-  // If this is a failing lookup, it has no viable candidates here.
-  if (result.ViableCandidates.empty()) {
-    // Diagnose 'super.init', which can only appear inside another initializer,
-    // specially.
-    if (result.UnviableCandidates.empty() &&
-        memberName.isSimpleName(CS->TC.Context.Id_init) &&
-        !baseObjTy->is<MetatypeType>()) {
-      if (auto ctorRef = dyn_cast<UnresolvedDotExpr>(expr)) {
-        if (isa<SuperRefExpr>(ctorRef->getBase())) {
-          diagnose(anchor->getLoc(),
-                   diag::super_initializer_not_in_initializer);
-          return true;
-        }
-        
-        // Suggest inserting a call to 'type(of:)' to construct another object
-        // of the same dynamic type.
-        SourceRange fixItRng = ctorRef->getNameLoc().getSourceRange();
-        
-        // Surround the caller in `type(of:)`.
-        diagnose(anchor->getLoc(), diag::init_not_instance_member)
-          .fixItInsert(fixItRng.Start, "type(of: ")
-          .fixItInsertAfter(fixItRng.End, ")");
+
+  // Since the lookup was allowing inaccessible members, let's check
+  // if it found anything of that sort, which is easy to diagnose.
+  bool allUnavailable = !CS->TC.getLangOpts().DisableAvailabilityChecking;
+  for (auto &member : result.ViableCandidates) {
+    if (!member.isDecl()) {
+      // if there is no declaration, this choice is implicitly available.
+      allUnavailable = false;
+      continue;
+    }
+
+    auto decl = member.getDecl();
+    // Check availability of the found choice.
+    if (!decl->getAttrs().isUnavailable(CS->getASTContext()))
+      allUnavailable = false;
+
+    if (decl->isAccessibleFrom(CS->DC))
+      continue;
+
+    if (auto *CD = dyn_cast<ConstructorDecl>(decl)) {
+      CS->TC.diagnose(anchor->getLoc(), diag::init_candidate_inaccessible,
+                      CD->getResultType(), decl->getFormalAccess());
+    } else {
+      CS->TC.diagnose(anchor->getLoc(), diag::candidate_inaccessible,
+                      decl->getName(), decl->getFormalAccess());
+    }
+
+    for (auto &candidate : result.ViableCandidates) {
+      if (auto decl = candidate.getDecl()) {
+        CS->TC.diagnose(decl, diag::decl_declared_here, decl->getFullName());
+      }
+    }
+
+    return true;
+  }
+
+  // Diagnose 'super.init', which can only appear inside another initializer,
+  // specially.
+  if (result.UnviableCandidates.empty() && isInitializer &&
+      !baseObjTy->is<MetatypeType>()) {
+    if (auto ctorRef = dyn_cast<UnresolvedDotExpr>(expr)) {
+      if (isa<SuperRefExpr>(ctorRef->getBase())) {
+        diagnose(anchor->getLoc(), diag::super_initializer_not_in_initializer);
         return true;
       }
+
+      // Suggest inserting a call to 'type(of:)' to construct another object
+      // of the same dynamic type.
+      SourceRange fixItRng = ctorRef->getNameLoc().getSourceRange();
+
+      // Surround the caller in `type(of:)`.
+      diagnose(anchor->getLoc(), diag::init_not_instance_member)
+          .fixItInsert(fixItRng.Start, "type(of: ")
+          .fixItInsertAfter(fixItRng.End, ")");
+      return true;
     }
+  }
 
+  if (result.ViableCandidates.empty()) {
     // FIXME: Dig out the property DeclNameLoc.
     diagnoseUnviableLookupResults(result, baseObjTy, anchor, memberName,
                                   DeclNameLoc(memberRange.Start),
                                   anchor->getLoc());
     return true;
   }
 
-  
-  bool allUnavailable = !CS->TC.getLangOpts().DisableAvailabilityChecking;
-  for (auto match : result.ViableCandidates) {
-    if (!match.isDecl() ||
-        !match.getDecl()->getAttrs().isUnavailable(CS->getASTContext()))
-      allUnavailable = false;
-  }
-  
   if (allUnavailable) {
     auto firstDecl = result.ViableCandidates[0].getDecl();
     // FIXME: We need the enclosing CallExpr to rewrite the argument labels.

test/Constraints/super_constructor.swift

@@ -53,3 +53,14 @@ class B {
   }
 }
 
+// SR-2484: Bad diagnostic for incorrectly calling private init
+class SR_2484 {
+  private init() {} // expected-note {{'init()' declared here}}
+  private init(a: Int) {} // expected-note {{'init(a:)' declared here}}
+}
+
+class Impl_2484 : SR_2484 {
+  init() {
+    super.init() // expected-error {{'SR_2484' initializer is inaccessible due to 'private' protection level}}
+  }
+}

test/NameBinding/Inputs/accessibility_other.swift

@@ -11,7 +11,7 @@ extension Foo {
 }
 
 struct PrivateInit {
-  private init() {}  // expected-note {{'init' declared here}}
+  private init() {}  // expected-note {{'init()' declared here}}
 }
 
 extension Foo {