Skip to content

Commit e77cff9

Browse files
jrose-applejrose-apple
committed
Switch override checking over to access scopes. (#4508)
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 (cherry picked from commit 8b1ce7d)
1 parent 66268d0 commit e77cff9

File tree

3 files changed

+157
-44
lines changed

3 files changed

+157
-44
lines changed

‎lib/Sema/TypeCheckDecl.cpp

Lines changed: 69 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -5532,62 +5532,87 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
55325532
}
55335533
}
55345534

5535-
auto overriddenAccess = matchDecl->getFormalAccess();
5536-
55375535
// Check that the override has the required accessibility.
55385536
// Overrides have to be at least as accessible as what they
55395537
// override, except:
55405538
// - they don't have to be more accessible than their class and
55415539
// - a final method may be public instead of open.
5542-
// FIXME: Copied from TypeCheckProtocol.cpp.
5543-
Accessibility requiredAccess =
5544-
std::min(classDecl->getFormalAccess(), overriddenAccess);
5545-
if (requiredAccess == Accessibility::Open && decl->isFinal())
5546-
requiredAccess = Accessibility::Public;
5547-
else if (requiredAccess == Accessibility::Private)
5548-
requiredAccess = Accessibility::FilePrivate;
5549-
5550-
bool shouldDiagnose = false;
5551-
bool shouldDiagnoseSetter = false;
5552-
if (!isa<ConstructorDecl>(decl)) {
5553-
shouldDiagnose = (decl->getFormalAccess() < requiredAccess);
5554-
5555-
if (!shouldDiagnose && matchDecl->isSettable(classDecl)) {
5556-
auto matchASD = cast<AbstractStorageDecl>(matchDecl);
5557-
if (matchASD->isSetterAccessibleFrom(classDecl)) {
5558-
auto ASD = cast<AbstractStorageDecl>(decl);
5559-
const DeclContext *accessDC = nullptr;
5560-
if (requiredAccess == Accessibility::Internal)
5561-
accessDC = classDecl->getParentModule();
5562-
else if (requiredAccess == Accessibility::FilePrivate)
5563-
accessDC = classDecl->getDeclContext();
5564-
shouldDiagnoseSetter = ASD->isSettable(accessDC) &&
5565-
!ASD->isSetterAccessibleFrom(accessDC);
5566-
}
5567-
}
5568-
}
5569-
if (shouldDiagnose || shouldDiagnoseSetter) {
5570-
bool overriddenForcesAccess = (requiredAccess == overriddenAccess);
5571-
{
5572-
auto diag = TC.diagnose(decl, diag::override_not_accessible,
5573-
shouldDiagnoseSetter,
5574-
decl->getDescriptiveKind(),
5575-
overriddenForcesAccess);
5576-
fixItAccessibility(diag, decl, requiredAccess, shouldDiagnoseSetter);
5577-
}
5578-
TC.diagnose(matchDecl, diag::overridden_here);
5579-
}
5580-
5581-
// Diagnose attempts to override a non-open method from outside its
5540+
// Also diagnose attempts to override a non-open method from outside its
55825541
// defining module. This is not required for constructors, which are
55835542
// never really "overridden" in the intended sense here, because of
55845543
// course derived classes will change how the class is initialized.
5585-
if (matchDecl->getFormalAccess(decl->getDeclContext())
5586-
< Accessibility::Open &&
5544+
Accessibility matchAccess =
5545+
matchDecl->getFormalAccess(decl->getDeclContext());
5546+
if (matchAccess < Accessibility::Open &&
55875547
matchDecl->getModuleContext() != decl->getModuleContext() &&
55885548
!isa<ConstructorDecl>(decl)) {
55895549
TC.diagnose(decl, diag::override_of_non_open,
55905550
decl->getDescriptiveKind());
5551+
5552+
} else if (matchAccess == Accessibility::Open &&
5553+
classDecl->getFormalAccess(decl->getDeclContext()) ==
5554+
Accessibility::Open &&
5555+
decl->getFormalAccess() != Accessibility::Open &&
5556+
!decl->isFinal()) {
5557+
{
5558+
auto diag = TC.diagnose(decl, diag::override_not_accessible,
5559+
/*setter*/false,
5560+
decl->getDescriptiveKind(),
5561+
/*fromOverridden*/true);
5562+
fixItAccessibility(diag, decl, Accessibility::Open);
5563+
}
5564+
TC.diagnose(matchDecl, diag::overridden_here);
5565+
5566+
} else {
5567+
auto matchAccessScope =
5568+
matchDecl->getFormalAccessScope(decl->getDeclContext());
5569+
const DeclContext *requiredAccessScope =
5570+
classDecl->getFormalAccessScope(decl->getDeclContext());
5571+
5572+
// FIXME: This is the same operation as
5573+
// TypeAccessScopeChecker::intersectAccess.
5574+
if (!requiredAccessScope) {
5575+
requiredAccessScope = matchAccessScope;
5576+
} else if (matchAccessScope) {
5577+
if (matchAccessScope->isChildContextOf(requiredAccessScope)) {
5578+
requiredAccessScope = matchAccessScope;
5579+
} else {
5580+
assert(requiredAccessScope == matchAccessScope ||
5581+
requiredAccessScope->isChildContextOf(matchAccessScope));
5582+
}
5583+
}
5584+
5585+
bool shouldDiagnose = false;
5586+
bool shouldDiagnoseSetter = false;
5587+
if (!isa<ConstructorDecl>(decl)) {
5588+
shouldDiagnose = !decl->isAccessibleFrom(requiredAccessScope);
5589+
5590+
if (!shouldDiagnose && matchDecl->isSettable(decl->getDeclContext())){
5591+
auto matchASD = cast<AbstractStorageDecl>(matchDecl);
5592+
if (matchASD->isSetterAccessibleFrom(decl->getDeclContext())) {
5593+
const auto *ASD = cast<AbstractStorageDecl>(decl);
5594+
shouldDiagnoseSetter =
5595+
ASD->isSettable(requiredAccessScope) &&
5596+
!ASD->isSetterAccessibleFrom(requiredAccessScope);
5597+
}
5598+
}
5599+
}
5600+
if (shouldDiagnose || shouldDiagnoseSetter) {
5601+
bool overriddenForcesAccess =
5602+
(requiredAccessScope == matchAccessScope &&
5603+
matchAccess != Accessibility::Open);
5604+
Accessibility requiredAccess =
5605+
accessibilityFromScopeForDiagnostics(requiredAccessScope);
5606+
{
5607+
auto diag = TC.diagnose(decl, diag::override_not_accessible,
5608+
shouldDiagnoseSetter,
5609+
decl->getDescriptiveKind(),
5610+
overriddenForcesAccess);
5611+
fixItAccessibility(diag, decl, requiredAccess,
5612+
shouldDiagnoseSetter);
5613+
}
5614+
TC.diagnose(matchDecl, diag::overridden_here);
5615+
}
55915616
}
55925617

55935618
bool mayHaveMismatchedOptionals =

‎test/Sema/accessibility.swift

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,48 @@ internal class InternalSubPrivateSet: Base {
198198
}
199199
}
200200

201+
fileprivate class FilePrivateSub: Base {
202+
required private init() {} // expected-error {{'required' initializer must be as accessible as its enclosing type}} {{12-19=fileprivate}}
203+
private override func foo() {} // expected-error {{overriding instance method must be as accessible as its enclosing type}} {{3-10=fileprivate}}
204+
private override var bar: Int { // expected-error {{overriding var must be as accessible as its enclosing type}} {{3-10=fileprivate}}
205+
get { return 0 }
206+
set {}
207+
}
208+
private override subscript () -> () { return () } // expected-error {{overriding subscript must be as accessible as its enclosing type}} {{3-10=fileprivate}}
209+
}
210+
211+
fileprivate class FilePrivateSubGood: Base {
212+
required init() {} // no-warning
213+
override func foo() {}
214+
override var bar: Int {
215+
get { return 0 }
216+
set {}
217+
}
218+
override subscript () -> () { return () }
219+
}
220+
221+
fileprivate class FilePrivateSubGood2: Base {
222+
fileprivate required init() {} // no-warning
223+
fileprivate override func foo() {}
224+
fileprivate override var bar: Int {
225+
get { return 0 }
226+
set {}
227+
}
228+
fileprivate override subscript () -> () { return () }
229+
}
230+
231+
fileprivate class FilePrivateSubPrivateSet: Base {
232+
required init() {}
233+
private(set) override var bar: Int { // expected-error {{overriding var must be as accessible as its enclosing type}} {{3-10=fileprivate}}
234+
get { return 0 }
235+
set {}
236+
}
237+
private(set) override subscript () -> () { // okay; read-only in base class
238+
get { return () }
239+
set {}
240+
}
241+
}
242+
201243
private class PrivateSub: Base {
202244
required private init() {} // expected-error {{'required' initializer must be as accessible as its enclosing type}} {{12-19=fileprivate}}
203245
private override func foo() {} // expected-error {{overriding instance method must be as accessible as its enclosing type}} {{3-10=fileprivate}}

‎test/attr/open.swift

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,52 @@ class SubClass : ExternalOpenClass {
7777
}
7878
}
7979

80+
open class InvalidOpenSubClass : ExternalOpenClass {
81+
public override func openMethod() {} // expected-error {{overriding instance method must be as accessible as the declaration it overrides}} {{3-9=open}}
82+
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}}
83+
public override subscript(index: MarkerForOpenSubscripts) -> Int { // expected-error {{overriding subscript must be as accessible as the declaration it overrides}} {{3-9=open}}
84+
get { return 0 }
85+
set {}
86+
}
87+
}
88+
89+
open class InvalidOpenSubClass2 : ExternalOpenClass {
90+
internal override func openMethod() {} // expected-error {{overriding instance method must be as accessible as the declaration it overrides}} {{3-11=open}}
91+
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}}
92+
internal override subscript(index: MarkerForOpenSubscripts) -> Int { // expected-error {{overriding subscript must be as accessible as the declaration it overrides}} {{3-11=open}}
93+
get { return 0 }
94+
set {}
95+
}
96+
}
97+
98+
open class OpenSubClassFinalMembers : ExternalOpenClass {
99+
final public override func openMethod() {}
100+
final public override var openProperty: Int { get{return 0} set{} }
101+
final public override subscript(index: MarkerForOpenSubscripts) -> Int {
102+
get { return 0 }
103+
set {}
104+
}
105+
}
106+
107+
open class InvalidOpenSubClassFinalMembers : ExternalOpenClass {
108+
final internal override func openMethod() {} // expected-error {{overriding instance method must be as accessible as its enclosing type}} {{9-17=public}}
109+
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}}
110+
final internal override subscript(index: MarkerForOpenSubscripts) -> Int { // expected-error {{overriding subscript must be as accessible as its enclosing type}} {{9-17=public}}
111+
get { return 0 }
112+
set {}
113+
}
114+
}
115+
116+
public class PublicSubClass : ExternalOpenClass {
117+
public override func openMethod() {}
118+
public override var openProperty: Int { get{return 0} set{} }
119+
public override subscript(index: MarkerForOpenSubscripts) -> Int {
120+
get { return 0 }
121+
set {}
122+
}
123+
}
124+
125+
80126
// The proposal originally made these invalid, but we changed our minds.
81127
open class OpenSuperClass {
82128
public func publicMethod() {}

0 commit comments

Comments
 (0)