Switch override checking over to access scopes. (#4508)

jrose-apple · web-flow · commit 8b1ce7d1e74f · 2016-08-26T09:07:20.000-07:00
The class version of f65ad81. Fixes some incorrect diagnostics. We really do need https://bugs.swift.org/browse/SR-2209 to make this all cleaner. rdar://problem/27820665
diff --git a/lib/Sema/TypeCheckDecl.cpp b/lib/Sema/TypeCheckDecl.cpp
@@ -5538,62 +5538,87 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
         }
       }
 
-      auto overriddenAccess = matchDecl->getFormalAccess();
-
       // Check that the override has the required accessibility.
       // Overrides have to be at least as accessible as what they
       // override, except:
       //   - they don't have to be more accessible than their class and
       //   - a final method may be public instead of open.
-      // FIXME: Copied from TypeCheckProtocol.cpp.
-      Accessibility requiredAccess =
-        std::min(classDecl->getFormalAccess(), overriddenAccess);
-      if (requiredAccess == Accessibility::Open && decl->isFinal())
-        requiredAccess = Accessibility::Public;
-      else if (requiredAccess == Accessibility::Private)
-        requiredAccess = Accessibility::FilePrivate;
-
-      bool shouldDiagnose = false;
-      bool shouldDiagnoseSetter = false;
-      if (!isa<ConstructorDecl>(decl)) {
-        shouldDiagnose = (decl->getFormalAccess() < requiredAccess);
-
-        if (!shouldDiagnose && matchDecl->isSettable(classDecl)) {
-          auto matchASD = cast<AbstractStorageDecl>(matchDecl);
-          if (matchASD->isSetterAccessibleFrom(classDecl)) {
-            auto ASD = cast<AbstractStorageDecl>(decl);
-            const DeclContext *accessDC = nullptr;
-            if (requiredAccess == Accessibility::Internal)
-              accessDC = classDecl->getParentModule();
-            else if (requiredAccess == Accessibility::FilePrivate)
-              accessDC = classDecl->getDeclContext();
-            shouldDiagnoseSetter = ASD->isSettable(accessDC) &&
-                                   !ASD->isSetterAccessibleFrom(accessDC);
-          }
-        }
-      }
-      if (shouldDiagnose || shouldDiagnoseSetter) {
-        bool overriddenForcesAccess = (requiredAccess == overriddenAccess);
-        {
-          auto diag = TC.diagnose(decl, diag::override_not_accessible,
-                                  shouldDiagnoseSetter,
-                                  decl->getDescriptiveKind(),
-                                  overriddenForcesAccess);
-          fixItAccessibility(diag, decl, requiredAccess, shouldDiagnoseSetter);
-        }
-        TC.diagnose(matchDecl, diag::overridden_here);
-      }
-
-      // Diagnose attempts to override a non-open method from outside its
+      // Also diagnose attempts to override a non-open method from outside its
       // defining module.  This is not required for constructors, which are
       // never really "overridden" in the intended sense here, because of
       // course derived classes will change how the class is initialized.
-      if (matchDecl->getFormalAccess(decl->getDeclContext())
-            < Accessibility::Open &&
+      Accessibility matchAccess =
+          matchDecl->getFormalAccess(decl->getDeclContext());
+      if (matchAccess < Accessibility::Open &&
           matchDecl->getModuleContext() != decl->getModuleContext() &&
           !isa<ConstructorDecl>(decl)) {
         TC.diagnose(decl, diag::override_of_non_open,
                     decl->getDescriptiveKind());
+
+      } else if (matchAccess == Accessibility::Open &&
+                 classDecl->getFormalAccess(decl->getDeclContext()) ==
+                   Accessibility::Open &&
+                 decl->getFormalAccess() != Accessibility::Open &&
+                 !decl->isFinal()) {
+        {
+          auto diag = TC.diagnose(decl, diag::override_not_accessible,
+                                  /*setter*/false,
+                                  decl->getDescriptiveKind(),
+                                  /*fromOverridden*/true);
+          fixItAccessibility(diag, decl, Accessibility::Open);
+        }
+        TC.diagnose(matchDecl, diag::overridden_here);
+
+      } else {
+        auto matchAccessScope =
+            matchDecl->getFormalAccessScope(decl->getDeclContext());
+        const DeclContext *requiredAccessScope =
+            classDecl->getFormalAccessScope(decl->getDeclContext());
+
+        // FIXME: This is the same operation as
+        // TypeAccessScopeChecker::intersectAccess.
+        if (!requiredAccessScope) {
+          requiredAccessScope = matchAccessScope;
+        } else if (matchAccessScope) {
+          if (matchAccessScope->isChildContextOf(requiredAccessScope)) {
+            requiredAccessScope = matchAccessScope;
+          } else {
+            assert(requiredAccessScope == matchAccessScope ||
+                   requiredAccessScope->isChildContextOf(matchAccessScope));
+          }
+        }
+
+        bool shouldDiagnose = false;
+        bool shouldDiagnoseSetter = false;
+        if (!isa<ConstructorDecl>(decl)) {
+          shouldDiagnose = !decl->isAccessibleFrom(requiredAccessScope);
+
+          if (!shouldDiagnose && matchDecl->isSettable(decl->getDeclContext())){
+            auto matchASD = cast<AbstractStorageDecl>(matchDecl);
+            if (matchASD->isSetterAccessibleFrom(decl->getDeclContext())) {
+              const auto *ASD = cast<AbstractStorageDecl>(decl);
+              shouldDiagnoseSetter =
+                  ASD->isSettable(requiredAccessScope) &&
+                  !ASD->isSetterAccessibleFrom(requiredAccessScope);
+            }
+          }
+        }
+        if (shouldDiagnose || shouldDiagnoseSetter) {
+          bool overriddenForcesAccess =
+              (requiredAccessScope == matchAccessScope &&
+               matchAccess != Accessibility::Open);
+          Accessibility requiredAccess =
+              accessibilityFromScopeForDiagnostics(requiredAccessScope);
+          {
+            auto diag = TC.diagnose(decl, diag::override_not_accessible,
+                                    shouldDiagnoseSetter,
+                                    decl->getDescriptiveKind(),
+                                    overriddenForcesAccess);
+            fixItAccessibility(diag, decl, requiredAccess,
+                               shouldDiagnoseSetter);
+          }
+          TC.diagnose(matchDecl, diag::overridden_here);
+        }
       }
 
       bool mayHaveMismatchedOptionals =
diff --git a/test/Sema/accessibility.swift b/test/Sema/accessibility.swift
@@ -198,6 +198,48 @@ internal class InternalSubPrivateSet: Base {
   }
 }
 
+fileprivate class FilePrivateSub: Base {
+  required private init() {} // expected-error {{'required' initializer must be as accessible as its enclosing type}} {{12-19=fileprivate}}
+  private override func foo() {} // expected-error {{overriding instance method must be as accessible as its enclosing type}} {{3-10=fileprivate}}
+  private override var bar: Int { // expected-error {{overriding var must be as accessible as its enclosing type}} {{3-10=fileprivate}}
+    get { return 0 }
+    set {}
+  }
+  private override subscript () -> () { return () } // expected-error {{overriding subscript must be as accessible as its enclosing type}} {{3-10=fileprivate}}
+}
+
+fileprivate class FilePrivateSubGood: Base {
+  required init() {} // no-warning
+  override func foo() {}
+  override var bar: Int {
+    get { return 0 }
+    set {}
+  }
+  override subscript () -> () { return () }
+}
+
+fileprivate class FilePrivateSubGood2: Base {
+  fileprivate required init() {} // no-warning
+  fileprivate override func foo() {}
+  fileprivate override var bar: Int {
+    get { return 0 }
+    set {}
+  }
+  fileprivate override subscript () -> () { return () }
+}
+
+fileprivate class FilePrivateSubPrivateSet: Base {
+  required init() {}
+  private(set) override var bar: Int { // expected-error {{overriding var must be as accessible as its enclosing type}} {{3-10=fileprivate}}
+    get { return 0 }
+    set {}
+  }
+  private(set) override subscript () -> () { // okay; read-only in base class
+    get { return () }
+    set {}
+  }
+}
+
 private class PrivateSub: Base {
   required private init() {} // expected-error {{'required' initializer must be as accessible as its enclosing type}} {{12-19=fileprivate}}
   private override func foo() {} // expected-error {{overriding instance method must be as accessible as its enclosing type}} {{3-10=fileprivate}}
diff --git a/test/attr/open.swift b/test/attr/open.swift
@@ -77,6 +77,52 @@ class SubClass : ExternalOpenClass {
   }
 }
 
+open class InvalidOpenSubClass : ExternalOpenClass {
+  public override func openMethod() {} // expected-error {{overriding instance method must be as accessible as the declaration it overrides}} {{3-9=open}}
+  public override var openProperty: Int { get{return 0} set{} } // expected-error {{overriding var must be as accessible as the declaration it overrides}} {{3-9=open}}
+  public override subscript(index: MarkerForOpenSubscripts) -> Int { // expected-error {{overriding subscript must be as accessible as the declaration it overrides}} {{3-9=open}}
+    get { return 0 }
+    set {}
+  }
+}
+
+open class InvalidOpenSubClass2 : ExternalOpenClass {
+  internal override func openMethod() {} // expected-error {{overriding instance method must be as accessible as the declaration it overrides}} {{3-11=open}}
+  internal override var openProperty: Int { get{return 0} set{} } // expected-error {{overriding var must be as accessible as the declaration it overrides}} {{3-11=open}}
+  internal override subscript(index: MarkerForOpenSubscripts) -> Int { // expected-error {{overriding subscript must be as accessible as the declaration it overrides}} {{3-11=open}}
+    get { return 0 }
+    set {}
+  }
+}
+
+open class OpenSubClassFinalMembers : ExternalOpenClass {
+  final public override func openMethod() {}
+  final public override var openProperty: Int { get{return 0} set{} } 
+  final public override subscript(index: MarkerForOpenSubscripts) -> Int { 
+    get { return 0 }
+    set {}
+  }
+}
+
+open class InvalidOpenSubClassFinalMembers : ExternalOpenClass {
+  final internal override func openMethod() {} // expected-error {{overriding instance method must be as accessible as its enclosing type}} {{9-17=public}}
+  final internal override var openProperty: Int { get{return 0} set{} } // expected-error {{overriding var must be as accessible as its enclosing type}} {{9-17=public}}
+  final internal override subscript(index: MarkerForOpenSubscripts) -> Int { // expected-error {{overriding subscript must be as accessible as its enclosing type}} {{9-17=public}}
+    get { return 0 }
+    set {}
+  }
+}
+
+public class PublicSubClass : ExternalOpenClass {
+  public override func openMethod() {}
+  public override var openProperty: Int { get{return 0} set{} }
+  public override subscript(index: MarkerForOpenSubscripts) -> Int {
+    get { return 0 }
+    set {}
+  }
+}
+
+
 // The proposal originally made these invalid, but we changed our minds.
 open class OpenSuperClass {
   public func publicMethod() {}
