Merge pull request #23416 from xedin/diag-inaccessible-via-fixes-5.1

[5.1] [ConstraintSystem] Introduce a fix for inaccessible members

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

@@ -384,3 +384,15 @@ MoveOutOfOrderArgument *MoveOutOfOrderArgument::create(
   return new (cs.getAllocator())
       MoveOutOfOrderArgument(cs, argIdx, prevArgIdx, bindings, locator);
 }
+
+bool AllowInaccessibleMember::diagnose(Expr *root, bool asNote) const {
+  InaccessibleMemberFailure failure(root, getConstraintSystem(), Member,
+                                    getLocator());
+  return failure.diagnose(asNote);
+}
+
+AllowInaccessibleMember *
+AllowInaccessibleMember::create(ConstraintSystem &cs, ValueDecl *member,
+                                ConstraintLocator *locator) {
+  return new (cs.getAllocator()) AllowInaccessibleMember(cs, member, locator);
+}

lib/Sema/CSFix.h

@@ -132,6 +132,10 @@ enum class FixKind : uint8_t {
 
   /// If there is out-of-order argument, let's fix that by re-ordering.
   MoveOutOfOrderArgument,
+
+  /// If there is a matching inaccessible member - allow it as if there
+  /// no access control.
+  AllowInaccessibleMember,
 };
 
 class ConstraintFix {
@@ -713,6 +717,26 @@ class MoveOutOfOrderArgument final : public ConstraintFix {
                                         ConstraintLocator *locator);
 };
 
+class AllowInaccessibleMember final : public ConstraintFix {
+  ValueDecl *Member;
+
+  AllowInaccessibleMember(ConstraintSystem &cs, ValueDecl *member,
+                          ConstraintLocator *locator)
+      : ConstraintFix(cs, FixKind::AllowInaccessibleMember, locator),
+        Member(member) {}
+
+public:
+  std::string getName() const override {
+    return "allow inaccessible member reference";
+  }
+
+  bool diagnose(Expr *root, bool asNote = false) const override;
+
+  static AllowInaccessibleMember *create(ConstraintSystem &cs,
+                                         ValueDecl *member,
+                                         ConstraintLocator *locator);
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSSimplify.cpp

@@ -4160,10 +4160,12 @@ fixMemberRef(ConstraintSystem &cs, Type baseTy,
     case MemberLookupResult::UR_TypeMemberOnInstance:
       return AllowTypeOrInstanceMember::create(cs, baseTy, memberName, locator);
 
+    case MemberLookupResult::UR_Inaccessible:
+      return AllowInaccessibleMember::create(cs, decl, locator);
+
     case MemberLookupResult::UR_MutatingMemberOnRValue:
     case MemberLookupResult::UR_MutatingGetterOnRValue:
     case MemberLookupResult::UR_LabelMismatch:
-    case MemberLookupResult::UR_Inaccessible:
     case MemberLookupResult::UR_UnavailableInExistential:
       break;
     }
@@ -4187,9 +4189,9 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
 
   auto locator = getConstraintLocator(locatorB);
   MemberLookupResult result =
-    performMemberLookup(kind, member, baseTy, functionRefKind, locator,
-                        /*includeInaccessibleMembers*/false);
-  
+      performMemberLookup(kind, member, baseTy, functionRefKind, locator,
+                          /*includeInaccessibleMembers*/ shouldAttemptFixes());
+
   switch (result.OverallResult) {
   case MemberLookupResult::Unsolved:
     // If requested, generate a constraint.
@@ -6195,6 +6197,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::AllowInvalidInitRef:
   case FixKind::AllowClosureParameterDestructuring:
   case FixKind::MoveOutOfOrderArgument:
+  case FixKind::AllowInaccessibleMember:
     llvm_unreachable("handled elsewhere");
   }
 

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 {

test/Sema/Inputs/accessibility_multi_other.swift

@@ -14,18 +14,18 @@ private protocol PrivateProtocol {}
 
 extension PrivateProtocol {
   public func publicExtensionMember() {}
-  // expected-note@-1 {{'publicExtensionMember' declared here}}
+  // expected-note@-1 {{'publicExtensionMember()' declared here}}
 
   internal func internalExtensionMember() {}
-  // expected-note@-1 {{'internalExtensionMember' declared here}}
+  // expected-note@-1 {{'internalExtensionMember()' declared here}}
 }
 
 fileprivate protocol FilePrivateProtocol {}
 
 extension FilePrivateProtocol {
   public func publicFPExtensionMember() {}
-  // expected-note@-1 {{'publicFPExtensionMember' declared here}}
+  // expected-note@-1 {{'publicFPExtensionMember()' declared here}}
 
   internal func internalFPExtensionMember() {}
-  // expected-note@-1 {{'internalFPExtensionMember' declared here}}
+  // expected-note@-1 {{'internalFPExtensionMember()' declared here}}
 }

test/decl/protocol/special/coding/class_codable_inheritance.swift

@@ -30,7 +30,9 @@ class SimpleChildClass : SimpleClass {
 
     // The enum should have a case for each of the vars.
     // NOTE: This expected error will need to be removed in the future.
-    let _ = SimpleChildClass.CodingKeys.w // expected-error {{'CodingKeys' is inaccessible due to 'private' protection level}}
+    let _ = SimpleChildClass.CodingKeys.w
+    // expected-error@-1 {{'CodingKeys' is inaccessible due to 'private' protection level}}
+    // expected-error@-2 {{type 'SimpleClass.CodingKeys' has no member 'w'}}
 
     // Inherited vars should not be part of the CodingKeys enum.
     // NOTE: This expected error will need to be updated in the future.

test/expr/primary/selector/Inputs/property_helper.swift

@@ -7,5 +7,5 @@ class OtherObjCClass: NSObject {
 
   @objc internal func internalFunc() {}
 
-  private func privateFunc() {} // expected-note 2{{'privateFunc' declared here}}
+  private func privateFunc() {} // expected-note 2{{'privateFunc()' declared here}}
 }