[Sema] Improve diagnostics for access level of protocol witness in extension. (#22235)

dingobye · jrose-apple · commit 0f493a68b352 · 2019-02-08T09:31:01.000-08:00
If the access level of a protocol witness does not satisfies a requirement, the compiler suggests marking it as the required level. This is not suitable when the witness is in an extension whose specified access level is less than the required level, since the fixit fights with other warnings in this case. This patch identifies such case and produces improved diagnostics. Resolves: SR-9793
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -1726,6 +1726,10 @@ ERROR(type_witness_objc_generic_parameter,none,
 NOTE(witness_fix_access,none,
      "mark the %0 as '%select{%error|fileprivate|internal|public|%error}1' to "
      "satisfy the requirement", (DescriptiveDeclKind, AccessLevel))
+NOTE(witness_move_to_another_extension,none,
+     "move the %0 to another extension where it can be declared "
+     "'%select{%error|%error|internal|public|%error}1' to "
+     "satisfy the requirement", (DescriptiveDeclKind, AccessLevel))
 WARNING(assoc_type_default_conformance_failed,none,
         "default type %0 for associated type %1 does not satisfy constraint "
         "%2: %3", (Type, DeclName, Type, Type))
diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp
@@ -2403,6 +2403,45 @@ class DiagnoseUsableFromInline {
 };
 }
 
+/// Helper function for diagnostics when a witness needs to be seated at a
+/// required access level.
+static void diagnoseWitnessFixAccessLevel(DiagnosticEngine &diags,
+                                          ValueDecl *decl,
+                                          AccessLevel requiredAccess,
+                                          bool isForSetter = false) {
+  bool shouldMoveToAnotherExtension = false;
+  bool shouldUseDefaultAccess = false;
+  if (auto extDecl = dyn_cast<ExtensionDecl>(decl->getDeclContext())) {
+    if (auto attr = extDecl->getAttrs().getAttribute<AccessControlAttr>()) {
+      auto extAccess = std::max(attr->getAccess(), AccessLevel::FilePrivate);
+      if (extAccess < requiredAccess) {
+        shouldMoveToAnotherExtension = true;
+      } else if (extAccess == requiredAccess) {
+        auto declAttr = decl->getAttrs().getAttribute<AccessControlAttr>();
+        assert(declAttr && declAttr->getAccess() < requiredAccess &&
+            "expect an explicitly specified access control level which is "
+            "less accessible than required.");
+        (void)declAttr;
+        shouldUseDefaultAccess = true;
+      }
+    }
+  }
+
+  // If decl lives in an extension that forbids the required level, we should
+  // move it to another extension where the required level is possible;
+  // otherwise, we simply mark decl as the required level.
+  if (shouldMoveToAnotherExtension) {
+    diags.diagnose(decl, diag::witness_move_to_another_extension,
+                   decl->getDescriptiveKind(), requiredAccess);
+  } else {
+    auto fixItDiag = diags.diagnose(decl, diag::witness_fix_access,
+                                    decl->getDescriptiveKind(),
+                                    requiredAccess);
+    fixItAccess(fixItDiag, decl, requiredAccess, isForSetter,
+                shouldUseDefaultAccess);
+  }
+}
+
 void ConformanceChecker::recordTypeWitness(AssociatedTypeDecl *assocType,
                                            Type type,
                                            TypeDecl *typeDecl) {
@@ -2444,10 +2483,7 @@ void ConformanceChecker::recordTypeWitness(AssociatedTypeDecl *assocType,
                        typeDecl->getFullName(),
                        requiredAccess,
                        proto->getName());
-        auto fixItDiag = diags.diagnose(typeDecl, diag::witness_fix_access,
-                                        typeDecl->getDescriptiveKind(),
-                                        requiredAccess);
-        fixItAccess(fixItDiag, typeDecl, requiredAccess);
+        diagnoseWitnessFixAccessLevel(diags, typeDecl, requiredAccess);
       });
     }
 
@@ -3103,10 +3139,8 @@ ConformanceChecker::resolveWitnessViaLookup(ValueDecl *requirement) {
             return;
           }
         }
-        auto fixItDiag = diags.diagnose(witness, diag::witness_fix_access,
-                                        witness->getDescriptiveKind(),
-                                        requiredAccess);
-        fixItAccess(fixItDiag, witness, requiredAccess, isSetter);
+        diagnoseWitnessFixAccessLevel(diags, witness, requiredAccess,
+                                      isSetter);
       });
       break;
     }
diff --git a/test/Compatibility/accessibility.swift b/test/Compatibility/accessibility.swift
@@ -58,6 +58,18 @@ private struct PrivateStruct: PublicProto, InternalProto, FilePrivateProto, Priv
   public var publicVar = 0
 }
 
+public struct PublicStructWithInternalExtension: PublicProto, InternalProto, FilePrivateProto, PrivateProto {}
+// expected-error@-1 {{method 'publicReq()' must be declared public because it matches a requirement in public protocol 'PublicProto'}} {{none}}
+// expected-error@-2 {{method 'internalReq()' must be declared internal because it matches a requirement in internal protocol 'InternalProto'}} {{none}}
+// expected-error@-3 {{method 'filePrivateReq()' must be declared fileprivate because it matches a requirement in fileprivate protocol 'FilePrivateProto'}} {{none}}
+// expected-error@-4 {{method 'privateReq()' must be declared fileprivate because it matches a requirement in private protocol 'PrivateProto'}} {{none}}
+internal extension PublicStructWithInternalExtension {
+  private func publicReq() {} // expected-note {{move the instance method to another extension where it can be declared 'public' to satisfy the requirement}} {{none}}
+  private func internalReq() {} // expected-note {{mark the instance method as 'internal' to satisfy the requirement}} {{3-11=}}
+  private func filePrivateReq() {} // expected-note {{mark the instance method as 'fileprivate' to satisfy the requirement}} {{3-10=fileprivate}}
+  private func privateReq() {} // expected-note {{mark the instance method as 'fileprivate' to satisfy the requirement}} {{3-10=fileprivate}}
+}
+
 extension PublicStruct {
   public init(x: Int) { self.init() }
 }
@@ -670,6 +682,22 @@ internal struct EquatablishOuterProblem3 {
 private func ==(lhs: EquatablishOuterProblem3.Inner, rhs: EquatablishOuterProblem3.Inner) {}
 // expected-note@-1 {{mark the operator function as 'internal' to satisfy the requirement}} {{1-8=internal}}
 
+internal struct EquatablishOuterProblem4 {
+  public struct Inner : Equatablish {} // expected-error {{method '==' must be as accessible as its enclosing type because it matches a requirement in protocol 'Equatablish'}} {{none}}
+}
+internal extension EquatablishOuterProblem4.Inner {
+  fileprivate static func ==(lhs: EquatablishOuterProblem4.Inner, rhs: EquatablishOuterProblem4.Inner) {}
+  // expected-note@-1 {{mark the operator function as 'internal' to satisfy the requirement}} {{3-15=}}
+}
+
+internal struct EquatablishOuterProblem5 {
+  public struct Inner : Equatablish {} // expected-error {{method '==' must be as accessible as its enclosing type because it matches a requirement in protocol 'Equatablish'}} {{none}}
+}
+private extension EquatablishOuterProblem5.Inner {
+  static func ==(lhs: EquatablishOuterProblem5.Inner, rhs: EquatablishOuterProblem5.Inner) {}
+  // expected-note@-1 {{move the operator function to another extension where it can be declared 'internal' to satisfy the requirement}} {{none}}
+}
+
 
 public protocol AssocTypeProto {
   associatedtype Assoc
@@ -693,6 +721,20 @@ internal struct AssocTypeOuterProblem2 {
   }
 }
 
+internal struct AssocTypeOuterProblem3 {
+  public struct Inner : AssocTypeProto {} // expected-error {{type alias 'Assoc' must be as accessible as its enclosing type because it matches a requirement in protocol 'AssocTypeProto'}} {{none}}
+}
+internal extension AssocTypeOuterProblem3.Inner {
+  fileprivate typealias Assoc = Int // expected-note {{mark the type alias as 'internal' to satisfy the requirement}} {{3-15=}}
+}
+
+internal struct AssocTypeOuterProblem4 {
+  public struct Inner : AssocTypeProto {} // expected-error {{type alias 'Assoc' must be as accessible as its enclosing type because it matches a requirement in protocol 'AssocTypeProto'}} {{none}}
+}
+private extension AssocTypeOuterProblem4.Inner {
+  typealias Assoc = Int // expected-note {{move the type alias to another extension where it can be declared 'internal' to satisfy the requirement}} {{none}}
+}
+
 internal typealias InternalComposition = PublicClass & PublicProto // expected-note {{declared here}}
 public class DerivedFromInternalComposition : InternalComposition { // expected-error {{class cannot be declared public because its superclass is internal}}
   public func publicReq() {}
diff --git a/test/Sema/accessibility.swift b/test/Sema/accessibility.swift
@@ -57,6 +57,18 @@ private struct PrivateStruct: PublicProto, InternalProto, FilePrivateProto, Priv
   public var publicVar = 0
 }
 
+public struct PublicStructWithInternalExtension: PublicProto, InternalProto, FilePrivateProto, PrivateProto {}
+// expected-error@-1 {{method 'publicReq()' must be declared public because it matches a requirement in public protocol 'PublicProto'}} {{none}}
+// expected-error@-2 {{method 'internalReq()' must be declared internal because it matches a requirement in internal protocol 'InternalProto'}} {{none}}
+// expected-error@-3 {{method 'filePrivateReq()' must be declared fileprivate because it matches a requirement in fileprivate protocol 'FilePrivateProto'}} {{none}}
+// expected-error@-4 {{method 'privateReq()' must be declared fileprivate because it matches a requirement in private protocol 'PrivateProto'}} {{none}}
+internal extension PublicStructWithInternalExtension {
+  private func publicReq() {} // expected-note {{move the instance method to another extension where it can be declared 'public' to satisfy the requirement}} {{none}}
+  private func internalReq() {} // expected-note {{mark the instance method as 'internal' to satisfy the requirement}} {{3-11=}}
+  private func filePrivateReq() {} // expected-note {{mark the instance method as 'fileprivate' to satisfy the requirement}} {{3-10=fileprivate}}
+  private func privateReq() {} // expected-note {{mark the instance method as 'fileprivate' to satisfy the requirement}} {{3-10=fileprivate}}
+}
+
 extension PublicStruct {
   public init(x: Int) { self.init() }
 }
@@ -677,6 +689,22 @@ internal struct EquatablishOuterProblem3 {
 private func ==(lhs: EquatablishOuterProblem3.Inner, rhs: EquatablishOuterProblem3.Inner) {}
 // expected-note@-1 {{mark the operator function as 'internal' to satisfy the requirement}} {{1-8=internal}}
 
+internal struct EquatablishOuterProblem4 {
+  public struct Inner : Equatablish {} // expected-error {{method '==' must be as accessible as its enclosing type because it matches a requirement in protocol 'Equatablish'}} {{none}}
+}
+internal extension EquatablishOuterProblem4.Inner {
+  fileprivate static func ==(lhs: EquatablishOuterProblem4.Inner, rhs: EquatablishOuterProblem4.Inner) {}
+  // expected-note@-1 {{mark the operator function as 'internal' to satisfy the requirement}} {{3-15=}}
+}
+
+internal struct EquatablishOuterProblem5 {
+  public struct Inner : Equatablish {} // expected-error {{method '==' must be as accessible as its enclosing type because it matches a requirement in protocol 'Equatablish'}} {{none}}
+}
+private extension EquatablishOuterProblem5.Inner {
+  static func ==(lhs: EquatablishOuterProblem5.Inner, rhs: EquatablishOuterProblem5.Inner) {}
+  // expected-note@-1 {{move the operator function to another extension where it can be declared 'internal' to satisfy the requirement}} {{none}}
+}
+
 
 public protocol AssocTypeProto {
   associatedtype Assoc
@@ -700,6 +728,20 @@ internal struct AssocTypeOuterProblem2 {
   }
 }
 
+internal struct AssocTypeOuterProblem3 {
+  public struct Inner : AssocTypeProto {} // expected-error {{type alias 'Assoc' must be as accessible as its enclosing type because it matches a requirement in protocol 'AssocTypeProto'}} {{none}}
+}
+internal extension AssocTypeOuterProblem3.Inner {
+  fileprivate typealias Assoc = Int // expected-note {{mark the type alias as 'internal' to satisfy the requirement}} {{3-15=}}
+}
+
+internal struct AssocTypeOuterProblem4 {
+  public struct Inner : AssocTypeProto {} // expected-error {{type alias 'Assoc' must be as accessible as its enclosing type because it matches a requirement in protocol 'AssocTypeProto'}} {{none}}
+}
+private extension AssocTypeOuterProblem4.Inner {
+  typealias Assoc = Int // expected-note {{move the type alias to another extension where it can be declared 'internal' to satisfy the requirement}} {{none}}
+}
+
 internal typealias InternalComposition = PublicClass & PublicProto // expected-note {{declared here}}
 public class DerivedFromInternalComposition : InternalComposition { // expected-error {{class cannot be declared public because its superclass is internal}}
   public func publicReq() {}
diff --git a/test/attr/accessibility_proto.swift b/test/attr/accessibility_proto.swift
@@ -16,6 +16,26 @@ extension Adopter {
   // expected-note@-1 {{mark the instance method as 'public' to satisfy the requirement}} {{3-3=public }}
 }
 
+public struct Adopter2<T> : ProtoWithReqs {}
+// expected-error@-1 {{method 'foo()' must be declared public because it matches a requirement in public protocol 'ProtoWithReqs'}} {{none}}
+// expected-error@-2 {{type alias 'Assoc' must be declared public because it matches a requirement in public protocol 'ProtoWithReqs'}} {{none}}
+public extension Adopter2 {
+  internal typealias Assoc = Int
+  // expected-note@-1 {{mark the type alias as 'public' to satisfy the requirement}} {{3-12=}}
+  fileprivate func foo() {}
+  // expected-note@-1 {{mark the instance method as 'public' to satisfy the requirement}} {{3-15=}}
+}
+
+public struct Adopter3<T> : ProtoWithReqs {}
+// expected-error@-1 {{method 'foo()' must be declared public because it matches a requirement in public protocol 'ProtoWithReqs'}} {{none}}
+// expected-error@-2 {{type alias 'Assoc' must be declared public because it matches a requirement in public protocol 'ProtoWithReqs'}} {{none}}
+internal extension Adopter3 {
+  typealias Assoc = Int
+  // expected-note@-1 {{move the type alias to another extension where it can be declared 'public' to satisfy the requirement}} {{none}}
+  func foo() {}
+  // expected-note@-1 {{move the instance method to another extension where it can be declared 'public' to satisfy the requirement}} {{none}}
+}
+
 public class AnotherAdopterBase {
   typealias Assoc = Int
   // expected-note@-1 {{mark the type alias as 'public' to satisfy the requirement}} {{3-3=public }}
@@ -26,6 +46,28 @@ public class AnotherAdopterSub : AnotherAdopterBase, ProtoWithReqs {}
 // expected-error@-1 {{method 'foo()' must be declared public because it matches a requirement in public protocol 'ProtoWithReqs'}} {{none}}
 // expected-error@-2 {{type alias 'Assoc' must be declared public because it matches a requirement in public protocol 'ProtoWithReqs'}} {{none}}
 
+public class AnotherAdopterBase2 {}
+public extension AnotherAdopterBase2 {
+  internal typealias Assoc = Int
+  // expected-note@-1 {{mark the type alias as 'public' to satisfy the requirement}} {{3-12=}}
+  fileprivate func foo() {}
+  // expected-note@-1 {{mark the instance method as 'public' to satisfy the requirement}} {{3-15=}}
+}
+public class AnotherAdopterSub2 : AnotherAdopterBase2, ProtoWithReqs {}
+// expected-error@-1 {{method 'foo()' must be declared public because it matches a requirement in public protocol 'ProtoWithReqs'}} {{none}}
+// expected-error@-2 {{type alias 'Assoc' must be declared public because it matches a requirement in public protocol 'ProtoWithReqs'}} {{none}}
+
+public class AnotherAdopterBase3 {}
+internal extension AnotherAdopterBase3 {
+  typealias Assoc = Int
+  // expected-note@-1 {{move the type alias to another extension where it can be declared 'public' to satisfy the requirement}} {{none}}
+  func foo() {}
+  // expected-note@-1 {{move the instance method to another extension where it can be declared 'public' to satisfy the requirement}} {{none}}
+}
+public class AnotherAdopterSub3 : AnotherAdopterBase3, ProtoWithReqs {}
+// expected-error@-1 {{method 'foo()' must be declared public because it matches a requirement in public protocol 'ProtoWithReqs'}} {{none}}
+// expected-error@-2 {{type alias 'Assoc' must be declared public because it matches a requirement in public protocol 'ProtoWithReqs'}} {{none}}
+
 public protocol ReqProvider {}
 extension ReqProvider {
   func foo() {}
