Skip to content

Commit 63e1937

Browse files
dingobyedingobye
committed
Make it more comprehensive to warn when redundant access
modifier is used in an extension. In addition, add warnings for access modifier redundancy on property setters; and address comments from Jordan Rose.
1 parent f34020b commit 63e1937

File tree

9 files changed

+230
-49
lines changed

9 files changed

+230
-49
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1187,14 +1187,19 @@ ERROR(access_control_setter_more,none,
11871187
"%select{variable|property|subscript}1 cannot have "
11881188
"%select{%error|a fileprivate|an internal|a public|an open}2 setter",
11891189
(AccessLevel, unsigned, AccessLevel))
1190+
WARNING(access_control_setter_redundant,none,
1191+
"'%select{private|fileprivate|internal|public|open}0(set)' modifier is "
1192+
"redundant for %select{a private|a fileprivate|an internal|a public|an open}2 "
1193+
"%1",
1194+
(AccessLevel, DescriptiveDeclKind, AccessLevel))
11901195
WARNING(access_control_ext_member_more,none,
11911196
"declaring %select{%error|a fileprivate|an internal|a public|open}0 %1 in "
11921197
"%select{a private|a fileprivate|an internal|a public|%error}2 extension",
11931198
(AccessLevel, DescriptiveDeclKind, AccessLevel))
11941199
WARNING(access_control_ext_member_redundant,none,
11951200
"'%select{%error|fileprivate|internal|public|%error}0' modifier is redundant "
1196-
"for %1 declared in %select{%error|a fileprivate|an internal|a public|%error}2 "
1197-
"extension",
1201+
"for %1 declared in %select{a private (equivalent to fileprivate)|a fileprivate"
1202+
"|an internal|a public|%error}2 extension",
11981203
(AccessLevel, DescriptiveDeclKind, AccessLevel))
11991204
ERROR(access_control_ext_requirement_member_more,none,
12001205
"cannot declare %select{%error|a fileprivate|an internal|a public|an open}0 %1 "

‎lib/Sema/MiscDiagnostics.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3952,7 +3952,7 @@ void swift::performStmtDiagnostics(TypeChecker &TC, const Stmt *S) {
39523952

39533953
void swift::fixItAccess(InFlightDiagnostic &diag, ValueDecl *VD,
39543954
AccessLevel desiredAccess, bool isForSetter,
3955-
bool shouldNotReplace) {
3955+
bool shouldUseDefaultAccess) {
39563956
StringRef fixItString;
39573957
switch (desiredAccess) {
39583958
case AccessLevel::Private: fixItString = "private "; break;
@@ -3998,7 +3998,7 @@ void swift::fixItAccess(InFlightDiagnostic &diag, ValueDecl *VD,
39983998
// this function is sometimes called to make access narrower, so assuming
39993999
// that a broader scope is acceptable breaks some diagnostics.
40004000
if (attr->getAccess() != desiredAccess) {
4001-
if (shouldNotReplace) {
4001+
if (shouldUseDefaultAccess) {
40024002
// Remove the attribute if replacement is not preferred.
40034003
diag.fixItRemove(attr->getRange());
40044004
} else {

‎lib/Sema/MiscDiagnostics.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ void fixItAccess(InFlightDiagnostic &diag,
5555
ValueDecl *VD,
5656
AccessLevel desiredAccess,
5757
bool isForSetter = false,
58-
bool shouldNotReplace = false);
58+
bool shouldUseDefaultAccess = false);
5959

6060
/// Emit fix-its to correct the argument labels in \p expr, which is the
6161
/// argument tuple or single argument of a call.

‎lib/Sema/TypeCheckAttr.cpp

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1459,13 +1459,6 @@ static bool hasThrowingFunctionParameter(CanType type) {
14591459
return false;
14601460
}
14611461

1462-
static inline bool isRedundantAttrInExtension(AccessLevel access,
1463-
AccessControlAttr *extAttr) {
1464-
return access == extAttr->getAccess() &&
1465-
access != AccessLevel::Private &&
1466-
access != AccessLevel::Open;
1467-
}
1468-
14691462
void AttributeChecker::visitRethrowsAttr(RethrowsAttr *attr) {
14701463
// 'rethrows' only applies to functions that take throwing functions
14711464
// as parameters.
@@ -1519,22 +1512,17 @@ void AttributeChecker::visitAccessControlAttr(AccessControlAttr *attr) {
15191512

15201513
if (auto extAttr =
15211514
extension->getAttrs().getAttribute<AccessControlAttr>()) {
1522-
// Extensions are top level declarations, for which the literally lowest
1523-
// access level `private` is equivalent to `fileprivate`.
1524-
AccessLevel extAccess = std::max(extAttr->getAccess(),
1525-
AccessLevel::FilePrivate);
1526-
if (attr->getAccess() > extAccess) {
1515+
AccessLevel defaultAccess = extension->getDefaultAccessLevel();
1516+
if (attr->getAccess() > defaultAccess) {
15271517
auto diag = TC.diagnose(attr->getLocation(),
15281518
diag::access_control_ext_member_more,
15291519
attr->getAccess(),
15301520
D->getDescriptiveKind(),
15311521
extAttr->getAccess());
1532-
bool shouldNotReplace = isRedundantAttrInExtension(extAccess, extAttr);
1533-
swift::fixItAccess(diag, cast<ValueDecl>(D), extAccess, false,
1534-
shouldNotReplace);
1522+
swift::fixItAccess(diag, cast<ValueDecl>(D), defaultAccess, false,
1523+
true);
15351524
return;
1536-
}
1537-
if (isRedundantAttrInExtension(attr->getAccess(), extAttr)) {
1525+
} else if (attr->getAccess() == defaultAccess) {
15381526
TC.diagnose(attr->getLocation(),
15391527
diag::access_control_ext_member_redundant,
15401528
attr->getAccess(),
@@ -1576,6 +1564,15 @@ AttributeChecker::visitSetterAccessAttr(SetterAccessAttr *attr) {
15761564
getterAccess, storageKind, attr->getAccess());
15771565
attr->setInvalid();
15781566
return;
1567+
1568+
} else if (attr->getAccess() == getterAccess) {
1569+
TC.diagnose(attr->getLocation(),
1570+
diag::access_control_setter_redundant,
1571+
attr->getAccess(),
1572+
D->getDescriptiveKind(),
1573+
getterAccess)
1574+
.fixItRemove(attr->getRange());
1575+
return;
15791576
}
15801577
}
15811578

‎test/Compatibility/accessibility.swift

Lines changed: 95 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,8 @@ fileprivate extension PublicStruct {
8989
private func extImplFilePrivate() {}
9090
}
9191
private extension PublicStruct {
92-
public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-9=fileprivate}}
93-
fileprivate func extFuncPrivate() {}
92+
public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-10=}}
93+
fileprivate func extFuncPrivate() {} // expected-warning {{'fileprivate' modifier is redundant for instance method declared in a private (equivalent to fileprivate) extension}} {{3-15=}}
9494
private func extImplPrivate() {}
9595
}
9696
public extension InternalStruct { // expected-error {{extension of internal struct cannot be declared public}} {{1-8=}}
@@ -109,8 +109,8 @@ fileprivate extension InternalStruct {
109109
private func extImplFilePrivate() {}
110110
}
111111
private extension InternalStruct {
112-
public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-9=fileprivate}}
113-
fileprivate func extFuncPrivate() {}
112+
public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-10=}}
113+
fileprivate func extFuncPrivate() {} // expected-warning {{'fileprivate' modifier is redundant for instance method declared in a private (equivalent to fileprivate) extension}} {{3-15=}}
114114
private func extImplPrivate() {}
115115
}
116116
public extension FilePrivateStruct { // expected-error {{extension of fileprivate struct cannot be declared public}} {{1-8=}}
@@ -129,8 +129,8 @@ fileprivate extension FilePrivateStruct {
129129
private func extImplFilePrivate() {}
130130
}
131131
private extension FilePrivateStruct {
132-
public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-9=fileprivate}}
133-
fileprivate func extFuncPrivate() {}
132+
public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-10=}}
133+
fileprivate func extFuncPrivate() {} // expected-warning {{'fileprivate' modifier is redundant for instance method declared in a private (equivalent to fileprivate) extension}} {{3-15=}}
134134
private func extImplPrivate() {}
135135
}
136136
public extension PrivateStruct { // expected-error {{extension of private struct cannot be declared public}} {{1-8=}}
@@ -149,8 +149,8 @@ fileprivate extension PrivateStruct { // expected-error {{extension of private s
149149
private func extImplFilePrivate() {}
150150
}
151151
private extension PrivateStruct {
152-
public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-9=fileprivate}}
153-
fileprivate func extFuncPrivate() {}
152+
public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-10=}}
153+
fileprivate func extFuncPrivate() {} // expected-warning {{'fileprivate' modifier is redundant for instance method declared in a private (equivalent to fileprivate) extension}} {{3-15=}}
154154
private func extImplPrivate() {}
155155
}
156156

@@ -716,3 +716,90 @@ public typealias BadPublicComposition2 = PublicClass & InternalProto // expected
716716
public typealias BadPublicComposition3<T> = InternalGenericClass<T> & PublicProto // expected-error {{type alias cannot be declared public because its underlying type uses an internal type}}
717717
public typealias BadPublicComposition4 = InternalGenericClass<Int> & PublicProto // expected-error {{type alias cannot be declared public because its underlying type uses an internal type}}
718718
public typealias BadPublicComposition5 = PublicGenericClass<InternalStruct> & PublicProto // expected-error {{type alias cannot be declared public because its underlying type uses an internal type}}
719+
720+
721+
open class ClassWithProperties {
722+
open open(set) var openProp = 0 // expected-warning {{'open(set)' modifier is redundant for an open property}} {{8-18=}}
723+
public public(set) var publicProp = 0 // expected-warning {{'public(set)' modifier is redundant for a public property}} {{10-22=}}
724+
internal internal(set) var internalProp = 0 // expected-warning {{'internal(set)' modifier is redundant for an internal property}} {{12-26=}}
725+
fileprivate fileprivate(set) var fileprivateProp = 0 // expected-warning {{'fileprivate(set)' modifier is redundant for a fileprivate property}} {{15-32=}}
726+
private private(set) var privateProp = 0 // expected-warning {{'private(set)' modifier is redundant for a private property}} {{11-24=}}
727+
internal(set) var defaultProp = 0 // expected-warning {{'internal(set)' modifier is redundant for an internal property}} {{3-17=}}
728+
}
729+
730+
extension ClassWithProperties {
731+
// expected-warning@+1 {{'internal(set)' modifier is redundant for an internal property}} {{12-26=}}
732+
internal internal(set) var defaultExtProp: Int {
733+
get { return 42 }
734+
set {}
735+
}
736+
// expected-warning@+1 {{'internal(set)' modifier is redundant for an internal property}} {{3-17=}}
737+
internal(set) var defaultExtProp2: Int {
738+
get { return 42 }
739+
set {}
740+
}
741+
}
742+
743+
public extension ClassWithProperties {
744+
// expected-warning@+2 {{'public' modifier is redundant for property declared in a public extension}} {{3-10=}}
745+
// expected-warning@+1 {{'public(set)' modifier is redundant for a public property}} {{10-22=}}
746+
public public(set) var publicExtProp: Int {
747+
get { return 42 }
748+
set {}
749+
}
750+
// expected-warning@+1 {{'public(set)' modifier is redundant for a public property}} {{3-15=}}
751+
public(set) var publicExtProp2: Int {
752+
get { return 42 }
753+
set {}
754+
}
755+
}
756+
757+
internal extension ClassWithProperties {
758+
// expected-warning@+2 {{'internal' modifier is redundant for property declared in an internal extension}} {{3-12=}}
759+
// expected-warning@+1 {{'internal(set)' modifier is redundant for an internal property}} {{12-26=}}
760+
internal internal(set) var internalExtProp: Int {
761+
get { return 42 }
762+
set {}
763+
}
764+
// expected-warning@+1 {{'internal(set)' modifier is redundant for an internal property}} {{3-17=}}
765+
internal(set) var internalExtProp2: Int {
766+
get { return 42 }
767+
set {}
768+
}
769+
}
770+
771+
fileprivate extension ClassWithProperties {
772+
// expected-warning@+2 {{'fileprivate' modifier is redundant for property declared in a fileprivate extension}} {{3-15=}}
773+
// expected-warning@+1 {{'fileprivate(set)' modifier is redundant for a fileprivate property}} {{15-32=}}
774+
fileprivate fileprivate(set) var fileprivateExtProp: Int {
775+
get { return 42 }
776+
set {}
777+
}
778+
// expected-warning@+1 {{'fileprivate(set)' modifier is redundant for a fileprivate property}} {{3-20=}}
779+
fileprivate(set) var fileprivateExtProp2: Int {
780+
get { return 42 }
781+
set {}
782+
}
783+
private(set) var fileprivateExtProp3: Int {
784+
get { return 42 }
785+
set {}
786+
}
787+
}
788+
789+
private extension ClassWithProperties {
790+
// expected-warning@+2 {{'fileprivate' modifier is redundant for property declared in a private (equivalent to fileprivate) extension}} {{3-15=}}
791+
// expected-warning@+1 {{'fileprivate(set)' modifier is redundant for a fileprivate property}} {{15-32=}}
792+
fileprivate fileprivate(set) var privateExtProp: Int {
793+
get { return 42 }
794+
set {}
795+
}
796+
// expected-warning@+1 {{'fileprivate(set)' modifier is redundant for a fileprivate property}} {{3-20=}}
797+
fileprivate(set) var privateExtProp2: Int {
798+
get { return 42 }
799+
set {}
800+
}
801+
private(set) var privateExtProp3: Int {
802+
get { return 42 }
803+
set {}
804+
}
805+
}

‎test/Compatibility/tuple_arguments_4.swift

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1631,15 +1631,13 @@ do {
16311631

16321632
// https://bugs.swift.org/browse/SR-6509
16331633
public extension Optional {
1634-
// expected-warning@+1 {{'public' modifier is redundant for instance method declared in a public extension}}
1635-
public func apply<Result>(_ transform: ((Wrapped) -> Result)?) -> Result? {
1634+
func apply<Result>(_ transform: ((Wrapped) -> Result)?) -> Result? {
16361635
return self.flatMap { value in
16371636
transform.map { $0(value) }
16381637
}
16391638
}
16401639

1641-
// expected-warning@+1 {{'public' modifier is redundant for instance method declared in a public extension}}
1642-
public func apply<Value, Result>(_ value: Value?) -> Result?
1640+
func apply<Value, Result>(_ value: Value?) -> Result?
16431641
where Wrapped == (Value) -> Result {
16441642
return value.apply(self)
16451643
}

‎test/Constraints/tuple_arguments.swift

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1632,15 +1632,13 @@ do {
16321632

16331633
// https://bugs.swift.org/browse/SR-6509
16341634
public extension Optional {
1635-
// expected-warning@+1 {{'public' modifier is redundant for instance method declared in a public extension}}
1636-
public func apply<Result>(_ transform: ((Wrapped) -> Result)?) -> Result? {
1635+
func apply<Result>(_ transform: ((Wrapped) -> Result)?) -> Result? {
16371636
return self.flatMap { value in
16381637
transform.map { $0(value) }
16391638
}
16401639
}
16411640

1642-
// expected-warning@+1 {{'public' modifier is redundant for instance method declared in a public extension}}
1643-
public func apply<Value, Result>(_ value: Value?) -> Result?
1641+
func apply<Value, Result>(_ value: Value?) -> Result?
16441642
where Wrapped == (Value) -> Result {
16451643
return value.apply(self)
16461644
}

‎test/SILGen/accessibility_warnings.swift

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ internal struct InternalStruct {
1515

1616
public private(set) var publicVarPrivateSet = 0
1717

18+
// expected-warning@+1 {{'public(set)' modifier is redundant for a public property}} {{10-22=}}
1819
public public(set) var publicVarPublicSet = 0
1920

2021
// CHECK-DAG: sil hidden @$S22accessibility_warnings14InternalStructV16publicVarGetOnlySivg
@@ -56,6 +57,13 @@ extension PrivateStruct {
5657
public var publicVarExtension: Int { get { return 0 } set {} }
5758
}
5859

60+
public extension PublicStruct {
61+
// CHECK-DAG: sil @$S22accessibility_warnings12PublicStructV09extMemberC0yyF
62+
public func extMemberPublic() {} // expected-warning {{'public' modifier is redundant for instance method declared in a public extension}} {{3-10=}}
63+
// CHECK-DAG: sil private @$S22accessibility_warnings12PublicStructV07extImplC033_5D2F2E026754A901C0FF90C404896D02LLyyF
64+
private func extImplPublic() {}
65+
}
66+
5967
internal extension PublicStruct {
6068
// CHECK-DAG: sil hidden @$S22accessibility_warnings12PublicStructV17extMemberInternalyyF
6169
public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-10=}}
@@ -64,7 +72,7 @@ internal extension PublicStruct {
6472
}
6573
private extension PublicStruct {
6674
// CHECK-DAG: sil private @$S22accessibility_warnings12PublicStructV16extMemberPrivate33_5D2F2E026754A901C0FF90C404896D02LLyyF
67-
public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-9=fileprivate}}
75+
public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-10=}}
6876
// CHECK-DAG: sil private @$S22accessibility_warnings12PublicStructV14extImplPrivate33_5D2F2E026754A901C0FF90C404896D02LLyyF
6977
private func extImplPrivate() {}
7078
}
@@ -77,15 +85,15 @@ internal extension InternalStruct {
7785
}
7886
private extension InternalStruct {
7987
// CHECK-DAG: sil private @$S22accessibility_warnings14InternalStructV16extMemberPrivate33_5D2F2E026754A901C0FF90C404896D02LLyyF
80-
public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-9=fileprivate}}
88+
public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-10=}}
8189
// CHECK-DAG: sil private @$S22accessibility_warnings14InternalStructV14extImplPrivate33_5D2F2E026754A901C0FF90C404896D02LLyyF
8290
private func extImplPrivate() {}
8391
}
8492

8593

8694
private extension PrivateStruct {
8795
// CHECK-DAG: sil private @$S22accessibility_warnings13PrivateStruct33_5D2F2E026754A901C0FF90C404896D02LLV09extMemberC0yyF
88-
public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-9=fileprivate}}
96+
public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-10=}}
8997
// CHECK-DAG: sil private @$S22accessibility_warnings13PrivateStruct33_5D2F2E026754A901C0FF90C404896D02LLV07extImplC0yyF
9098
private func extImplPrivate() {}
9199
}
@@ -120,6 +128,7 @@ internal class InternalClass {
120128
// CHECK-DAG: sil hidden [transparent] @$S22accessibility_warnings13InternalClassC19publicVarPrivateSetSivg
121129
public private(set) var publicVarPrivateSet = 0
122130

131+
// expected-warning@+1 {{'public(set)' modifier is redundant for a public property}} {{10-22=}}
123132
public public(set) var publicVarPublicSet = 0
124133

125134
// CHECK-DAG: sil hidden @$S22accessibility_warnings13InternalClassC16publicVarGetOnlySivg

0 commit comments

Comments
 (0)