[Diagnostics] Add inaccessible member diagnostic

Author: xedin

lib/Sema/CSDiag.cpp

@@ -854,11 +854,17 @@ void FailureDiagnosis::diagnoseUnviableLookupResults(
       // visible to us, but the conforming type is. In this case, we need to
       // clamp the formal access for diagnostics purposes to the formal access
       // of the protocol itself.
-      diagnose(nameLoc, diag::candidate_inaccessible, decl->getBaseName(),
-               decl->getFormalAccessScope().accessLevelForDiagnostics());
-      for (auto cand : result.UnviableCandidates)
-        diagnose(cand.getDecl(), diag::decl_declared_here, memberName);
-
+      InaccessibleMemberFailure failure(expr, CS, decl,
+                                        CS.getConstraintLocator(E));
+      auto diagnosed = failure.diagnoseAsError();
+      assert(diagnosed && "failed to produce expected diagnostic");
+      for (auto cand : result.UnviableCandidates) {
+        auto *choice = cand.getDecl();
+        // failure is going to highlight candidate given to it,
+        // we just need to handle the rest here.
+        if (choice != decl)
+          diagnose(choice, diag::decl_declared_here, choice->getFullName());
+      }
       return;
     }
     }

lib/Sema/CSDiagnostics.cpp

@@ -2246,3 +2246,57 @@ bool OutOfOrderArgumentFailure::diagnoseAsError() {
 
   return true;
 }
+
+bool InaccessibleMemberFailure::diagnoseAsError() {
+  auto *anchor = getRawAnchor();
+  // Let's try to avoid over-diagnosing chains of inaccessible
+  // members e.g.:
+  //
+  // struct A {
+  //   struct B {
+  //     struct C {}
+  //   }
+  // }
+  //
+  // _ = A.B.C()
+  //
+  // We'll have a fix for each `B', `C` and `C.init` but it makes
+  // sense to diagnose only `B` and consider the rest hidden.
+  Expr *baseExpr = nullptr;
+  DeclNameLoc nameLoc;
+  if (auto *UDE = dyn_cast<UnresolvedDotExpr>(anchor)) {
+    baseExpr = UDE->getBase();
+    nameLoc = UDE->getNameLoc();
+  } else if (auto *UME = dyn_cast<UnresolvedMemberExpr>(anchor)) {
+    nameLoc = UME->getNameLoc();
+  } else if (auto *SE = dyn_cast<SubscriptExpr>(anchor)) {
+    baseExpr = SE->getBase();
+  } else if (auto *call = dyn_cast<CallExpr>(anchor)) {
+    baseExpr = call->getFn();
+  }
+
+  if (baseExpr) {
+    auto &cs = getConstraintSystem();
+    auto *locator =
+        cs.getConstraintLocator(baseExpr, ConstraintLocator::Member);
+    if (llvm::any_of(cs.getFixes(), [&](const ConstraintFix *fix) {
+          return fix->getLocator() == locator;
+        }))
+      return false;
+  }
+
+  auto loc = nameLoc.isValid() ? nameLoc.getStartLoc() : anchor->getLoc();
+  auto accessLevel = Member->getFormalAccessScope().accessLevelForDiagnostics();
+  if (auto *CD = dyn_cast<ConstructorDecl>(Member)) {
+    emitDiagnostic(loc, diag::init_candidate_inaccessible,
+                   CD->getResultInterfaceType(), accessLevel)
+        .highlight(nameLoc.getSourceRange());
+  } else {
+    emitDiagnostic(loc, diag::candidate_inaccessible, Member->getBaseName(),
+                   accessLevel)
+        .highlight(nameLoc.getSourceRange());
+  }
+
+  emitDiagnostic(Member, diag::decl_declared_here, Member->getFullName());
+  return true;
+}

lib/Sema/CSDiagnostics.h

@@ -956,6 +956,29 @@ class ClosureParamDestructuringFailure final : public FailureDiagnostic {
   }
 };
 
+/// Diagnose an attempt to reference inaccessible member e.g.
+///
+/// ```swift
+/// struct S {
+///   var foo: String
+///
+///   private init(_ v: String) {
+///     self.foo = v
+///   }
+/// }
+/// _ = S("ultimate question")
+/// ```
+class InaccessibleMemberFailure final : public FailureDiagnostic {
+  ValueDecl *Member;
+
+public:
+  InaccessibleMemberFailure(Expr *root, ConstraintSystem &cs, ValueDecl *member,
+                            ConstraintLocator *locator)
+      : FailureDiagnostic(root, cs, locator), Member(member) {}
+
+  bool diagnoseAsError() override;
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSFix.cpp

@@ -386,7 +386,9 @@ MoveOutOfOrderArgument *MoveOutOfOrderArgument::create(
 }
 
 bool AllowInaccessibleMember::diagnose(Expr *root, bool asNote) const {
-  return false;
+  InaccessibleMemberFailure failure(root, getConstraintSystem(), Member,
+                                    getLocator());
+  return failure.diagnose(asNote);
 }
 
 AllowInaccessibleMember *

lib/Sema/CalleeCandidateInfo.cpp

@@ -16,6 +16,7 @@
 
 #include "ConstraintSystem.h"
 #include "CSDiag.h"
+#include "CSDiagnostics.h"
 #include "CalleeCandidateInfo.h"
 #include "TypeCheckAvailability.h"
 #include "swift/AST/GenericEnvironment.h"
@@ -1108,18 +1109,17 @@ bool CalleeCandidateInfo::diagnoseSimpleErrors(const Expr *E) {
   if (closeness == CC_Inaccessible) {
     auto decl = candidates[0].getDecl();
     assert(decl && "Only decl-based candidates may be marked inaccessible");
-    if (auto *CD = dyn_cast<ConstructorDecl>(decl)) {
-      CS.TC.diagnose(loc, diag::init_candidate_inaccessible,
-                     CD->getResultInterfaceType(), decl->getFormalAccess());
-      
-    } else {
-      CS.TC.diagnose(loc, diag::candidate_inaccessible, decl->getBaseName(),
-                     decl->getFormalAccess());
-    }
+
+    InaccessibleMemberFailure failure(
+        nullptr, CS, decl, CS.getConstraintLocator(const_cast<Expr *>(E)));
+    auto diagnosed = failure.diagnoseAsError();
+    assert(diagnosed && "failed to produce expected diagnostic");
+
     for (auto cand : candidates) {
-      if (auto decl = cand.getDecl()) {
-        CS.TC.diagnose(decl, diag::decl_declared_here, decl->getFullName());
-      }
+      auto *candidate = cand.getDecl();
+      if (candidate && candidate != decl)
+        CS.TC.diagnose(candidate, diag::decl_declared_here,
+                       candidate->getFullName());
     }
 
     return true;

test/Compatibility/optional_visibility.swift

@@ -5,7 +5,7 @@
 }
 
 class Conforms : Opt {
-  private func f(callback: () -> ()) {} // expected-note {{'f' declared here}}
+  private func f(callback: () -> ()) {} // expected-note {{'f(callback:)' declared here}}
 }
 
 func g(x: Conforms) {

test/Constraints/construction.swift

@@ -177,3 +177,35 @@ func init_via_non_const_metatype(_ s1: S1.Type) {
   _ = s1(i: 42) // expected-error {{initializing from a metatype value must reference 'init' explicitly}} {{9-9=.init}}
   _ = s1.init(i: 42) // ok
 }
+
+// rdar://problem/45535925 - diagnostic is attached to invalid source location
+func rdar_45535925() {
+  struct S {
+    var addr: String
+    var port: Int
+
+    private init(addr: String, port: Int?) {
+      // expected-note@-1 {{'init(addr:port:)' declared here}}
+      self.addr = addr
+      self.port = port ?? 31337
+    }
+
+    private init(port: Int) {
+      self.addr = "localhost"
+      self.port = port
+    }
+
+    private func foo(_: Int) {}  // expected-note {{'foo' declared here}}
+    private static func bar() {} // expected-note {{'bar()' declared here}}
+  }
+
+  _ = S(addr: "localhost", port: nil)
+  // expected-error@-1 {{'S' initializer is inaccessible due to 'private' protection level}}
+
+  func baz(_ s: S) {
+    s.foo(42)
+    // expected-error@-1 {{'foo' is inaccessible due to 'private' protection level}}
+    S.bar()
+    // expected-error@-1 {{'bar' is inaccessible due to 'private' protection level}}
+  }
+}

test/Constraints/dynamic_lookup.swift

@@ -217,8 +217,8 @@ let uopt : AnyObject! = nil
 uopt.wibble!()
 
 // Should not be able to see private or internal @objc methods.
-uopt.privateFoo!() // expected-error{{value of type 'AnyObject?' has no member 'privateFoo'}}
-uopt.internalFoo!() // expected-error{{value of type 'AnyObject?' has no member 'internalFoo'}}
+uopt.privateFoo!() // expected-error{{'privateFoo' is inaccessible due to 'private' protection level}}
+uopt.internalFoo!() // expected-error{{'internalFoo' is inaccessible due to 'internal' protection level}}
 
 let anyValue: Any = X()
 _ = anyValue.bar() // expected-error {{value of type 'Any' has no member 'bar'}}

test/Constraints/super_constructor.swift

@@ -55,13 +55,13 @@ 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' declared here}}
+  private init() {} // expected-note {{'init()' declared here}}
+  private init(a: Int) {}
 }
 
 class Impl_2484 : SR_2484 {
   init() {
-    super.init() // expected-error {{'init' is inaccessible due to 'private' protection level}}
+    super.init() // expected-error {{'SR_2484' initializer is inaccessible due to 'private' protection level}}
   }
 }
 

test/NameBinding/Inputs/accessibility_other.swift

@@ -7,7 +7,7 @@ private let c = 0
 extension Foo {
   public static func a() {}
   internal static func b() {}
-  private static func c() {}  // expected-note {{'c' declared here}}
+  private static func c() {}  // expected-note {{'c()' declared here}}
 }
 
 struct PrivateInit {