Skip to content

Commit f34020b

Browse files
dingobyedingobye
committed
[Sema] Warn when redundant access-level modifier is added in an extension.
This patch adds warning for redundant access-level modifiers used in an extension. It also refines the diagnostics of access_control_ext_member_more issues, in case the fixit could suggest redundant modifiers. Resolves: SR-8453.
1 parent d30aa32 commit f34020b

File tree

11 files changed

+86
-55
lines changed

11 files changed

+86
-55
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1189,7 +1189,12 @@ ERROR(access_control_setter_more,none,
11891189
(AccessLevel, unsigned, AccessLevel))
11901190
WARNING(access_control_ext_member_more,none,
11911191
"declaring %select{%error|a fileprivate|an internal|a public|open}0 %1 in "
1192-
"%select{a private|a fileprivate|an internal|public|%error}2 extension",
1192+
"%select{a private|a fileprivate|an internal|a public|%error}2 extension",
1193+
(AccessLevel, DescriptiveDeclKind, AccessLevel))
1194+
WARNING(access_control_ext_member_redundant,none,
1195+
"'%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",
11931198
(AccessLevel, DescriptiveDeclKind, AccessLevel))
11941199
ERROR(access_control_ext_requirement_member_more,none,
11951200
"cannot declare %select{%error|a fileprivate|an internal|a public|an open}0 %1 "

‎lib/Sema/MiscDiagnostics.cpp

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3951,7 +3951,8 @@ void swift::performStmtDiagnostics(TypeChecker &TC, const Stmt *S) {
39513951
//===----------------------------------------------------------------------===//
39523952

39533953
void swift::fixItAccess(InFlightDiagnostic &diag, ValueDecl *VD,
3954-
AccessLevel desiredAccess, bool isForSetter) {
3954+
AccessLevel desiredAccess, bool isForSetter,
3955+
bool shouldNotReplace) {
39553956
StringRef fixItString;
39563957
switch (desiredAccess) {
39573958
case AccessLevel::Private: fixItString = "private "; break;
@@ -3997,9 +3998,14 @@ void swift::fixItAccess(InFlightDiagnostic &diag, ValueDecl *VD,
39973998
// this function is sometimes called to make access narrower, so assuming
39983999
// that a broader scope is acceptable breaks some diagnostics.
39994000
if (attr->getAccess() != desiredAccess) {
4000-
// This uses getLocation() instead of getRange() because we don't want to
4001-
// replace the "(set)" part of a setter attribute.
4002-
diag.fixItReplace(attr->getLocation(), fixItString.drop_back());
4001+
if (shouldNotReplace) {
4002+
// Remove the attribute if replacement is not preferred.
4003+
diag.fixItRemove(attr->getRange());
4004+
} else {
4005+
// This uses getLocation() instead of getRange() because we don't want to
4006+
// replace the "(set)" part of a setter attribute.
4007+
diag.fixItReplace(attr->getLocation(), fixItString.drop_back());
4008+
}
40034009
attr->setInvalid();
40044010
}
40054011

‎lib/Sema/MiscDiagnostics.h

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,11 @@ void performTopLevelDeclDiagnostics(TypeChecker &TC, TopLevelCodeDecl *TLCD);
5151
/// Emit a fix-it to set the access of \p VD to \p desiredAccess.
5252
///
5353
/// This actually updates \p VD as well.
54-
void fixItAccess(InFlightDiagnostic &diag, ValueDecl *VD,
55-
AccessLevel desiredAccess, bool isForSetter = false);
54+
void fixItAccess(InFlightDiagnostic &diag,
55+
ValueDecl *VD,
56+
AccessLevel desiredAccess,
57+
bool isForSetter = false,
58+
bool shouldNotReplace = false);
5659

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

‎lib/Sema/TypeCheckAttr.cpp

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1459,6 +1459,13 @@ 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+
14621469
void AttributeChecker::visitRethrowsAttr(RethrowsAttr *attr) {
14631470
// 'rethrows' only applies to functions that take throwing functions
14641471
// as parameters.
@@ -1522,7 +1529,18 @@ void AttributeChecker::visitAccessControlAttr(AccessControlAttr *attr) {
15221529
attr->getAccess(),
15231530
D->getDescriptiveKind(),
15241531
extAttr->getAccess());
1525-
swift::fixItAccess(diag, cast<ValueDecl>(D), extAccess);
1532+
bool shouldNotReplace = isRedundantAttrInExtension(extAccess, extAttr);
1533+
swift::fixItAccess(diag, cast<ValueDecl>(D), extAccess, false,
1534+
shouldNotReplace);
1535+
return;
1536+
}
1537+
if (isRedundantAttrInExtension(attr->getAccess(), extAttr)) {
1538+
TC.diagnose(attr->getLocation(),
1539+
diag::access_control_ext_member_redundant,
1540+
attr->getAccess(),
1541+
D->getDescriptiveKind(),
1542+
extAttr->getAccess())
1543+
.fixItRemove(attr->getRange());
15261544
return;
15271545
}
15281546
}

‎test/Compatibility/accessibility.swift

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -74,18 +74,18 @@ extension PrivateStruct {
7474
}
7575

7676
public extension PublicStruct {
77-
public func extMemberPublic() {}
77+
public func extMemberPublic() {} // expected-warning {{'public' modifier is redundant for instance method declared in a public extension}} {{3-10=}}
7878
fileprivate func extFuncPublic() {}
7979
private func extImplPublic() {}
8080
}
8181
internal extension PublicStruct {
82-
public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-9=internal}}
82+
public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-10=}}
8383
fileprivate func extFuncInternal() {}
8484
private func extImplInternal() {}
8585
}
8686
fileprivate extension PublicStruct {
87-
public func extMemberFilePrivate() {} // expected-warning {{declaring a public instance method in a fileprivate extension}} {{3-9=fileprivate}}
88-
fileprivate func extFuncFilePrivate() {}
87+
public func extMemberFilePrivate() {} // expected-warning {{declaring a public instance method in a fileprivate extension}} {{3-10=}}
88+
fileprivate func extFuncFilePrivate() {} // expected-warning {{'fileprivate' modifier is redundant for instance method declared in a fileprivate extension}} {{3-15=}}
8989
private func extImplFilePrivate() {}
9090
}
9191
private extension PublicStruct {
@@ -94,18 +94,18 @@ private extension PublicStruct {
9494
private func extImplPrivate() {}
9595
}
9696
public extension InternalStruct { // expected-error {{extension of internal struct cannot be declared public}} {{1-8=}}
97-
public func extMemberPublic() {}
97+
public func extMemberPublic() {} // expected-warning {{'public' modifier is redundant for instance method declared in a public extension}} {{3-10=}}
9898
fileprivate func extFuncPublic() {}
9999
private func extImplPublic() {}
100100
}
101101
internal extension InternalStruct {
102-
public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-9=internal}}
102+
public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-10=}}
103103
fileprivate func extFuncInternal() {}
104104
private func extImplInternal() {}
105105
}
106106
fileprivate extension InternalStruct {
107-
public func extMemberFilePrivate() {} // expected-warning {{declaring a public instance method in a fileprivate extension}} {{3-9=fileprivate}}
108-
fileprivate func extFuncFilePrivate() {}
107+
public func extMemberFilePrivate() {} // expected-warning {{declaring a public instance method in a fileprivate extension}} {{3-10=}}
108+
fileprivate func extFuncFilePrivate() {} // expected-warning {{'fileprivate' modifier is redundant for instance method declared in a fileprivate extension}} {{3-15=}}
109109
private func extImplFilePrivate() {}
110110
}
111111
private extension InternalStruct {
@@ -114,18 +114,18 @@ private extension InternalStruct {
114114
private func extImplPrivate() {}
115115
}
116116
public extension FilePrivateStruct { // expected-error {{extension of fileprivate struct cannot be declared public}} {{1-8=}}
117-
public func extMemberPublic() {}
117+
public func extMemberPublic() {} // expected-warning {{'public' modifier is redundant for instance method declared in a public extension}} {{3-10=}}
118118
fileprivate func extFuncPublic() {}
119119
private func extImplPublic() {}
120120
}
121121
internal extension FilePrivateStruct { // expected-error {{extension of fileprivate struct cannot be declared internal}} {{1-10=}}
122-
public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-9=internal}}
122+
public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-10=}}
123123
fileprivate func extFuncInternal() {}
124124
private func extImplInternal() {}
125125
}
126126
fileprivate extension FilePrivateStruct {
127-
public func extMemberFilePrivate() {} // expected-warning {{declaring a public instance method in a fileprivate extension}} {{3-9=fileprivate}}
128-
fileprivate func extFuncFilePrivate() {}
127+
public func extMemberFilePrivate() {} // expected-warning {{declaring a public instance method in a fileprivate extension}} {{3-10=}}
128+
fileprivate func extFuncFilePrivate() {} // expected-warning {{'fileprivate' modifier is redundant for instance method declared in a fileprivate extension}} {{3-15=}}
129129
private func extImplFilePrivate() {}
130130
}
131131
private extension FilePrivateStruct {
@@ -134,18 +134,18 @@ private extension FilePrivateStruct {
134134
private func extImplPrivate() {}
135135
}
136136
public extension PrivateStruct { // expected-error {{extension of private struct cannot be declared public}} {{1-8=}}
137-
public func extMemberPublic() {}
137+
public func extMemberPublic() {} // expected-warning {{'public' modifier is redundant for instance method declared in a public extension}} {{3-10=}}
138138
fileprivate func extFuncPublic() {}
139139
private func extImplPublic() {}
140140
}
141141
internal extension PrivateStruct { // expected-error {{extension of private struct cannot be declared internal}} {{1-10=}}
142-
public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-9=internal}}
142+
public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-10=}}
143143
fileprivate func extFuncInternal() {}
144144
private func extImplInternal() {}
145145
}
146146
fileprivate extension PrivateStruct { // expected-error {{extension of private struct cannot be declared fileprivate}} {{1-13=}}
147-
public func extMemberFilePrivate() {} // expected-warning {{declaring a public instance method in a fileprivate extension}} {{3-9=fileprivate}}
148-
fileprivate func extFuncFilePrivate() {}
147+
public func extMemberFilePrivate() {} // expected-warning {{declaring a public instance method in a fileprivate extension}} {{3-10=}}
148+
fileprivate func extFuncFilePrivate() {} // expected-warning {{'fileprivate' modifier is redundant for instance method declared in a fileprivate extension}} {{3-15=}}
149149
private func extImplFilePrivate() {}
150150
}
151151
private extension PrivateStruct {
@@ -174,7 +174,7 @@ public class Base {
174174

175175

176176
public extension Base {
177-
open func extMemberPublic() {} // expected-warning {{declaring open instance method in public extension}}
177+
open func extMemberPublic() {} // expected-warning {{declaring open instance method in a public extension}}
178178
}
179179
internal extension Base {
180180
open func extMemberInternal() {} // expected-warning {{declaring open instance method in an internal extension}}

‎test/Compatibility/tuple_arguments_4.swift

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1631,12 +1631,14 @@ 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}}
16341635
public func apply<Result>(_ transform: ((Wrapped) -> Result)?) -> Result? {
16351636
return self.flatMap { value in
16361637
transform.map { $0(value) }
16371638
}
16381639
}
16391640

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

‎test/Constraints/tuple_arguments.swift

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1632,12 +1632,14 @@ 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}}
16351636
public func apply<Result>(_ transform: ((Wrapped) -> Result)?) -> Result? {
16361637
return self.flatMap { value in
16371638
transform.map { $0(value) }
16381639
}
16391640
}
16401641

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

‎test/Inputs/clang-importer-sdk/swift-modules/Foundation.swift

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -312,7 +312,7 @@ public protocol _ErrorCodeProtocol {
312312
}
313313

314314
public extension _BridgedStoredNSError {
315-
public init?(_bridgedNSError error: NSError) {
315+
init?(_bridgedNSError error: NSError) {
316316
self.init(_nsError: error)
317317
}
318318
}
@@ -321,13 +321,13 @@ public extension _BridgedStoredNSError {
321321
public extension _BridgedStoredNSError
322322
where Code: RawRepresentable, Code.RawValue: SignedInteger {
323323
// FIXME: Generalize to Integer.
324-
public var code: Code {
324+
var code: Code {
325325
return Code(rawValue: numericCast(_nsError.code))!
326326
}
327327

328328
/// Initialize an error within this domain with the given ``code``
329329
/// and ``userInfo``.
330-
public init(_ code: Code, userInfo: [String : Any] = [:]) {
330+
init(_ code: Code, userInfo: [String : Any] = [:]) {
331331
self.init(_nsError: NSError(domain: "", code: 0, userInfo: [:]))
332332
}
333333

@@ -340,13 +340,13 @@ public extension _BridgedStoredNSError
340340
public extension _BridgedStoredNSError
341341
where Code: RawRepresentable, Code.RawValue: UnsignedInteger {
342342
// FIXME: Generalize to Integer.
343-
public var code: Code {
343+
var code: Code {
344344
return Code(rawValue: numericCast(_nsError.code))!
345345
}
346346

347347
/// Initialize an error within this domain with the given ``code``
348348
/// and ``userInfo``.
349-
public init(_ code: Code, userInfo: [String : Any] = [:]) {
349+
init(_ code: Code, userInfo: [String : Any] = [:]) {
350350
self.init(_nsError: NSError(domain: "", code: 0, userInfo: [:]))
351351
}
352352
}

‎test/SILGen/accessibility_warnings.swift

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -56,15 +56,9 @@ extension PrivateStruct {
5656
public var publicVarExtension: Int { get { return 0 } set {} }
5757
}
5858

59-
public extension PublicStruct {
60-
// CHECK-DAG: sil @$S22accessibility_warnings12PublicStructV09extMemberC0yyF
61-
public func extMemberPublic() {}
62-
// CHECK-DAG: sil private @$S22accessibility_warnings12PublicStructV07extImplC033_5D2F2E026754A901C0FF90C404896D02LLyyF
63-
private func extImplPublic() {}
64-
}
6559
internal extension PublicStruct {
6660
// CHECK-DAG: sil hidden @$S22accessibility_warnings12PublicStructV17extMemberInternalyyF
67-
public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-9=internal}}
61+
public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-10=}}
6862
// CHECK-DAG: sil private @$S22accessibility_warnings12PublicStructV15extImplInternal33_5D2F2E026754A901C0FF90C404896D02LLyyF
6963
private func extImplInternal() {}
7064
}
@@ -77,7 +71,7 @@ private extension PublicStruct {
7771

7872
internal extension InternalStruct {
7973
// CHECK-DAG: sil hidden @$S22accessibility_warnings14InternalStructV09extMemberC0yyF
80-
public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-9=internal}}
74+
public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-10=}}
8175
// CHECK-DAG: sil private @$S22accessibility_warnings14InternalStructV07extImplC033_5D2F2E026754A901C0FF90C404896D02LLyyF
8276
private func extImplInternal() {}
8377
}

0 commit comments

Comments
 (0)