Skip to content

Commit da4d996

Browse files
jrose-applejrose-apple
authored
Merge pull request #4610 from jrose-apple/swift-3-override-access-scopes
Switch override checking over to access scopes. rdar://problem/27820665
2 parents 9722df4 + e77cff9 commit da4d996

File tree

5 files changed

+197
-46
lines changed

5 files changed

+197
-46
lines changed

‎lib/Sema/TypeCheckDecl.cpp

Lines changed: 75 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -5532,59 +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-
5548-
bool shouldDiagnose = false;
5549-
bool shouldDiagnoseSetter = false;
5550-
if (requiredAccess > Accessibility::Private &&
5551-
!isa<ConstructorDecl>(decl)) {
5552-
shouldDiagnose = (decl->getFormalAccess() < requiredAccess);
5553-
5554-
if (!shouldDiagnose && matchDecl->isSettable(classDecl)) {
5555-
auto matchASD = cast<AbstractStorageDecl>(matchDecl);
5556-
if (matchASD->isSetterAccessibleFrom(classDecl)) {
5557-
auto ASD = cast<AbstractStorageDecl>(decl);
5558-
const DeclContext *accessDC = nullptr;
5559-
if (requiredAccess == Accessibility::Internal)
5560-
accessDC = classDecl->getParentModule();
5561-
shouldDiagnoseSetter = ASD->isSettable(accessDC) &&
5562-
!ASD->isSetterAccessibleFrom(accessDC);
5563-
}
5564-
}
5565-
}
5566-
if (shouldDiagnose || shouldDiagnoseSetter) {
5567-
bool overriddenForcesAccess = (requiredAccess == overriddenAccess);
5568-
{
5569-
auto diag = TC.diagnose(decl, diag::override_not_accessible,
5570-
shouldDiagnoseSetter,
5571-
decl->getDescriptiveKind(),
5572-
overriddenForcesAccess);
5573-
fixItAccessibility(diag, decl, requiredAccess, shouldDiagnoseSetter);
5574-
}
5575-
TC.diagnose(matchDecl, diag::overridden_here);
5576-
}
5577-
5578-
// Diagnose attempts to override a non-open method from outside its
5540+
// Also diagnose attempts to override a non-open method from outside its
55795541
// defining module. This is not required for constructors, which are
55805542
// never really "overridden" in the intended sense here, because of
55815543
// course derived classes will change how the class is initialized.
5582-
if (matchDecl->getFormalAccess(decl->getDeclContext())
5583-
< Accessibility::Open &&
5544+
Accessibility matchAccess =
5545+
matchDecl->getFormalAccess(decl->getDeclContext());
5546+
if (matchAccess < Accessibility::Open &&
55845547
matchDecl->getModuleContext() != decl->getModuleContext() &&
55855548
!isa<ConstructorDecl>(decl)) {
55865549
TC.diagnose(decl, diag::override_of_non_open,
55875550
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+
}
55885616
}
55895617

55905618
bool mayHaveMismatchedOptionals =
@@ -6498,11 +6526,14 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
64986526

64996527
if (CD->isRequired() && ContextTy) {
65006528
if (auto nominal = ContextTy->getAnyNominal()) {
6501-
if (CD->getFormalAccess() <
6502-
std::min(nominal->getFormalAccess(), Accessibility::Public)) {
6529+
auto requiredAccess = std::min(nominal->getFormalAccess(),
6530+
Accessibility::Public);
6531+
if (requiredAccess == Accessibility::Private)
6532+
requiredAccess = Accessibility::FilePrivate;
6533+
if (CD->getFormalAccess() < requiredAccess) {
65036534
auto diag = TC.diagnose(CD,
65046535
diag::required_initializer_not_accessible);
6505-
fixItAccessibility(diag, CD, nominal->getFormalAccess());
6536+
fixItAccessibility(diag, CD, requiredAccess);
65066537
}
65076538
}
65086539
}

‎test/SILGen/Inputs/mangling_private_helper.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,5 +5,5 @@ open class Base {
55
// Demonstrate the need for a vtable entry for privateMethod().
66
// This isn't strictly necessary.
77
private class Subclass : Base {
8-
override private func privateMethod() {}
8+
override fileprivate func privateMethod() {}
99
}

‎test/SILOptimizer/Inputs/devirt_access_other_module.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,5 +14,5 @@ public func getExternalClass() -> ExternalClass {
1414
}
1515

1616
private class PrivateSubclass : ExternalClass {
17-
override private func foo() {}
17+
override fileprivate func foo() {}
1818
}

‎test/Sema/accessibility.swift

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,80 @@ 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+
243+
private class PrivateSub: Base {
244+
required private init() {} // expected-error {{'required' initializer must be as accessible as its enclosing type}} {{12-19=fileprivate}}
245+
private override func foo() {} // expected-error {{overriding instance method must be as accessible as its enclosing type}} {{3-10=fileprivate}}
246+
private override var bar: Int { // expected-error {{overriding var must be as accessible as its enclosing type}} {{3-10=fileprivate}}
247+
get { return 0 }
248+
set {}
249+
}
250+
private override subscript () -> () { return () } // expected-error {{overriding subscript must be as accessible as its enclosing type}} {{3-10=fileprivate}}
251+
}
252+
253+
private class PrivateSubGood: Base {
254+
required fileprivate init() {}
255+
fileprivate override func foo() {}
256+
fileprivate override var bar: Int {
257+
get { return 0 }
258+
set {}
259+
}
260+
fileprivate override subscript () -> () { return () }
261+
}
262+
263+
private class PrivateSubPrivateSet: Base {
264+
required fileprivate init() {}
265+
fileprivate override func foo() {}
266+
private(set) override var bar: Int { // expected-error {{setter of overriding var must be as accessible as its enclosing type}}
267+
get { return 0 }
268+
set {}
269+
}
270+
private(set) override subscript () -> () { // okay; read-only in base class
271+
get { return () }
272+
set {}
273+
}
274+
}
201275

202276
public typealias PublicTA1 = PublicStruct
203277
public typealias PublicTA2 = InternalStruct // expected-error {{type alias cannot be declared public because its underlying type uses an internal type}}

‎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)