Skip to content

Commit 38ee56c

Browse files
allevatoallevato
committed
Fix NoAccessLevelOnExtensionDeclaration to update members inside #if blocks.
Fixes #966.
1 parent 8e60b2e commit 38ee56c

File tree

2 files changed

+217
-130
lines changed

2 files changed

+217
-130
lines changed

‎Sources/SwiftFormat/Rules/NoAccessLevelOnExtensionDeclaration.swift

+148-130
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,13 @@ import SwiftSyntax
2121
/// `internal`, as that is the default access level) have the explicit access level removed.
2222
@_spi(Rules)
2323
public final class NoAccessLevelOnExtensionDeclaration: SyntaxFormatRule {
24+
/// The access level keyword that was attached to the current extension, or nil if we are not
25+
/// inside an extension.
26+
private var accessKeyword: Keyword? = nil
27+
28+
/// Findings propagated up to the extension visitor from any members that were rewritten.
29+
private var notesFromRewrittenMembers: [Finding.Note] = []
30+
2431
public override func visit(_ node: ExtensionDeclSyntax) -> DeclSyntax {
2532
guard
2633
let accessKeyword = node.modifiers.accessLevelModifier,
@@ -29,80 +36,169 @@ public final class NoAccessLevelOnExtensionDeclaration: SyntaxFormatRule {
2936
return DeclSyntax(node)
3037
}
3138

32-
var result = node
39+
let keywordToAdd: Keyword?
40+
let message: Finding.Message
3341

3442
switch keyword {
35-
// Public, private, fileprivate, or package keywords need to be moved to members
3643
case .public, .private, .fileprivate, .package:
37-
// The effective access level of the members of a `private` extension is `fileprivate`, so
38-
// we have to update the keyword to ensure that the result is correct.
39-
var accessKeywordToAdd = accessKeyword
40-
let message: Finding.Message
44+
// These access level modifiers need to be moved to members. Additionally, `private` is a
45+
// special case, because the *effective* access level for a top-level private extension is
46+
// `fileprivate`, so we need to preserve that when we apply it to the members.
4147
if keyword == .private {
42-
accessKeywordToAdd.name.tokenKind = .keyword(.fileprivate)
48+
keywordToAdd = .fileprivate
4349
message = .moveAccessKeywordAndMakeFileprivate(keyword: accessKeyword.name.text)
4450
} else {
51+
keywordToAdd = keyword
4552
message = .moveAccessKeyword(keyword: accessKeyword.name.text)
4653
}
4754

48-
let (newMembers, notes) =
49-
addMemberAccessKeyword(accessKeywordToAdd, toMembersIn: node.memberBlock)
50-
diagnose(message, on: accessKeyword, notes: notes)
51-
52-
result.modifiers.remove(anyOf: [keyword])
53-
result.extensionKeyword.leadingTrivia = accessKeyword.leadingTrivia
54-
result.memberBlock.members = newMembers
55-
return DeclSyntax(result)
56-
57-
// Internal keyword redundant, delete
5855
case .internal:
59-
diagnose(.removeRedundantAccessKeyword, on: accessKeyword)
60-
61-
result.modifiers.remove(anyOf: [keyword])
62-
result.extensionKeyword.leadingTrivia = accessKeyword.leadingTrivia
63-
return DeclSyntax(result)
56+
// If the access level keyword was `internal`, then it's redundant and we can just remove it.
57+
// We don't need to modify the members at all in this case.
58+
message = .removeRedundantAccessKeyword
59+
keywordToAdd = nil
6460

6561
default:
66-
break
62+
// For anything else, just return the extension and its members unchanged.
63+
return DeclSyntax(node)
64+
}
65+
66+
// Set the keyword that should be applied to nested decls when we visit them, if any. Since
67+
// extensions can't nest, we don't have to worry about maintaining a stack here.
68+
self.accessKeyword = keywordToAdd
69+
self.notesFromRewrittenMembers = []
70+
defer { self.accessKeyword = nil }
71+
72+
var result: ExtensionDeclSyntax
73+
if keywordToAdd != nil {
74+
// Visit the children if we need to add a keyword to the extension members.
75+
result = super.visit(node).as(ExtensionDeclSyntax.self)!
76+
} else {
77+
// We don't need to visit the children in this case.
78+
result = node
6779
}
6880

81+
// Finally, emit the finding (which includes notes from any rewritten members) and remove the
82+
// access level keyword from the extension itself.
83+
diagnose(message, on: accessKeyword, notes: self.notesFromRewrittenMembers)
84+
result.modifiers.remove(anyOf: [keyword])
85+
result.extensionKeyword.leadingTrivia = accessKeyword.leadingTrivia
6986
return DeclSyntax(result)
7087
}
7188

72-
// Adds given keyword to all members in declaration block
73-
private func addMemberAccessKeyword(
74-
_ modifier: DeclModifierSyntax,
75-
toMembersIn memberBlock: MemberBlockSyntax
76-
) -> (MemberBlockItemListSyntax, [Finding.Note]) {
77-
var newMembers: [MemberBlockItemSyntax] = []
78-
var notes: [Finding.Note] = []
79-
80-
for memberItem in memberBlock.members {
81-
let decl = memberItem.decl
82-
guard
83-
let modifiers = decl.asProtocol(WithModifiersSyntax.self)?.modifiers,
84-
modifiers.accessLevelModifier == nil
85-
else {
86-
newMembers.append(memberItem)
87-
continue
88-
}
89+
public override func visit(_ node: ActorDeclSyntax) -> DeclSyntax {
90+
return applyingAccessModifierIfNone(to: node)
91+
}
92+
93+
public override func visit(_ node: ClassDeclSyntax) -> DeclSyntax {
94+
return applyingAccessModifierIfNone(to: node)
95+
}
96+
97+
public override func visit(_ node: EnumDeclSyntax) -> DeclSyntax {
98+
return applyingAccessModifierIfNone(to: node)
99+
}
100+
101+
public override func visit(_ node: FunctionDeclSyntax) -> DeclSyntax {
102+
return applyingAccessModifierIfNone(to: node)
103+
}
104+
105+
public override func visit(_ node: InitializerDeclSyntax) -> DeclSyntax {
106+
return applyingAccessModifierIfNone(to: node)
107+
}
108+
109+
public override func visit(_ node: StructDeclSyntax) -> DeclSyntax {
110+
return applyingAccessModifierIfNone(to: node)
111+
}
112+
113+
public override func visit(_ node: SubscriptDeclSyntax) -> DeclSyntax {
114+
return applyingAccessModifierIfNone(to: node)
115+
}
116+
117+
public override func visit(_ node: TypeAliasDeclSyntax) -> DeclSyntax {
118+
return applyingAccessModifierIfNone(to: node)
119+
}
89120

90-
// Create a note associated with each declaration that needs to have an access level modifier
91-
// added to it.
92-
notes.append(
93-
Finding.Note(
94-
message: .addModifierToExtensionMember(keyword: modifier.name.text),
95-
location:
96-
Finding.Location(decl.startLocation(converter: context.sourceLocationConverter))
97-
)
121+
public override func visit(_ node: VariableDeclSyntax) -> DeclSyntax {
122+
return applyingAccessModifierIfNone(to: node)
123+
}
124+
125+
/// Adds `modifier` to `decl` if it doesn't already have an explicit access level modifier and
126+
/// returns the new declaration.
127+
///
128+
/// If `decl` already has an access level modifier, it is returned unchanged.
129+
private func applyingAccessModifierIfNone(to decl: some DeclSyntaxProtocol) -> DeclSyntax {
130+
// Only go further if we are applying an access level keyword and if the decl is one that
131+
// allows modifiers but doesn't already have an access level modifier.
132+
guard
133+
let accessKeyword,
134+
let modifiers = decl.asProtocol(WithModifiersSyntax.self)?.modifiers,
135+
modifiers.accessLevelModifier == nil
136+
else {
137+
return DeclSyntax(decl)
138+
}
139+
140+
// Create a note associated with each declaration that needs to have an access level modifier
141+
// added to it.
142+
self.notesFromRewrittenMembers.append(
143+
Finding.Note(
144+
message: .addModifierToExtensionMember(keyword: TokenSyntax.keyword(accessKeyword).text),
145+
location:
146+
Finding.Location(decl.startLocation(converter: context.sourceLocationConverter))
98147
)
148+
)
99149

100-
var newItem = memberItem
101-
newItem.decl = applyingAccessModifierIfNone(modifier, to: decl)
102-
newMembers.append(newItem)
150+
switch Syntax(decl).as(SyntaxEnum.self) {
151+
case .actorDecl(let actorDecl):
152+
return applyingAccessModifierIfNone(accessKeyword, to: actorDecl, declKeywordKeyPath: \.actorKeyword)
153+
case .classDecl(let classDecl):
154+
return applyingAccessModifierIfNone(accessKeyword, to: classDecl, declKeywordKeyPath: \.classKeyword)
155+
case .enumDecl(let enumDecl):
156+
return applyingAccessModifierIfNone(accessKeyword, to: enumDecl, declKeywordKeyPath: \.enumKeyword)
157+
case .initializerDecl(let initDecl):
158+
return applyingAccessModifierIfNone(accessKeyword, to: initDecl, declKeywordKeyPath: \.initKeyword)
159+
case .functionDecl(let funcDecl):
160+
return applyingAccessModifierIfNone(accessKeyword, to: funcDecl, declKeywordKeyPath: \.funcKeyword)
161+
case .structDecl(let structDecl):
162+
return applyingAccessModifierIfNone(accessKeyword, to: structDecl, declKeywordKeyPath: \.structKeyword)
163+
case .subscriptDecl(let subscriptDecl):
164+
return applyingAccessModifierIfNone(accessKeyword, to: subscriptDecl, declKeywordKeyPath: \.subscriptKeyword)
165+
case .typeAliasDecl(let typeAliasDecl):
166+
return applyingAccessModifierIfNone(accessKeyword, to: typeAliasDecl, declKeywordKeyPath: \.typealiasKeyword)
167+
case .variableDecl(let varDecl):
168+
return applyingAccessModifierIfNone(accessKeyword, to: varDecl, declKeywordKeyPath: \.bindingSpecifier)
169+
default:
170+
return DeclSyntax(decl)
171+
}
172+
}
173+
174+
private func applyingAccessModifierIfNone<Decl: DeclSyntaxProtocol & WithModifiersSyntax>(
175+
_ modifier: Keyword,
176+
to decl: Decl,
177+
declKeywordKeyPath: WritableKeyPath<Decl, TokenSyntax>
178+
) -> DeclSyntax {
179+
// If there's already an access modifier among the modifier list, bail out.
180+
guard decl.modifiers.accessLevelModifier == nil else { return DeclSyntax(decl) }
181+
182+
var result = decl
183+
var modifier = DeclModifierSyntax(name: .keyword(modifier))
184+
modifier.trailingTrivia = [.spaces(1)]
185+
186+
guard var firstModifier = decl.modifiers.first else {
187+
// If there are no modifiers at all, add the one being requested, moving the leading trivia
188+
// from the decl keyword to that modifier (to preserve leading comments, newlines, etc.).
189+
modifier.leadingTrivia = decl[keyPath: declKeywordKeyPath].leadingTrivia
190+
result[keyPath: declKeywordKeyPath].leadingTrivia = []
191+
result.modifiers = .init([modifier])
192+
return DeclSyntax(result)
103193
}
104194

105-
return (MemberBlockItemListSyntax(newMembers), notes)
195+
// Otherwise, insert the modifier at the front of the modifier list, moving the (original) first
196+
// modifier's leading trivia to the new one (to preserve leading comments, newlines, etc.).
197+
modifier.leadingTrivia = firstModifier.leadingTrivia
198+
firstModifier.leadingTrivia = []
199+
result.modifiers[result.modifiers.startIndex] = firstModifier
200+
result.modifiers.insert(modifier, at: result.modifiers.startIndex)
201+
return DeclSyntax(result)
106202
}
107203
}
108204

@@ -122,81 +218,3 @@ extension Finding.Message {
122218
"add '\(keyword)' access modifier to this declaration"
123219
}
124220
}
125-
126-
/// Adds `modifier` to `decl` if it doesn't already have an explicit access level modifier and
127-
/// returns the new declaration.
128-
///
129-
/// If `decl` already has an access level modifier, it is returned unchanged.
130-
private func applyingAccessModifierIfNone(
131-
_ modifier: DeclModifierSyntax,
132-
to decl: DeclSyntax
133-
) -> DeclSyntax {
134-
switch Syntax(decl).as(SyntaxEnum.self) {
135-
case .actorDecl(let actorDecl):
136-
return applyingAccessModifierIfNone(modifier, to: actorDecl, declKeywordKeyPath: \.actorKeyword)
137-
case .classDecl(let classDecl):
138-
return applyingAccessModifierIfNone(modifier, to: classDecl, declKeywordKeyPath: \.classKeyword)
139-
case .enumDecl(let enumDecl):
140-
return applyingAccessModifierIfNone(modifier, to: enumDecl, declKeywordKeyPath: \.enumKeyword)
141-
case .initializerDecl(let initDecl):
142-
return applyingAccessModifierIfNone(modifier, to: initDecl, declKeywordKeyPath: \.initKeyword)
143-
case .functionDecl(let funcDecl):
144-
return applyingAccessModifierIfNone(modifier, to: funcDecl, declKeywordKeyPath: \.funcKeyword)
145-
case .structDecl(let structDecl):
146-
return applyingAccessModifierIfNone(
147-
modifier,
148-
to: structDecl,
149-
declKeywordKeyPath: \.structKeyword
150-
)
151-
case .subscriptDecl(let subscriptDecl):
152-
return applyingAccessModifierIfNone(
153-
modifier,
154-
to: subscriptDecl,
155-
declKeywordKeyPath: \.subscriptKeyword
156-
)
157-
case .typeAliasDecl(let typeAliasDecl):
158-
return applyingAccessModifierIfNone(
159-
modifier,
160-
to: typeAliasDecl,
161-
declKeywordKeyPath: \.typealiasKeyword
162-
)
163-
case .variableDecl(let varDecl):
164-
return applyingAccessModifierIfNone(
165-
modifier,
166-
to: varDecl,
167-
declKeywordKeyPath: \.bindingSpecifier
168-
)
169-
default:
170-
return decl
171-
}
172-
}
173-
174-
private func applyingAccessModifierIfNone<Decl: DeclSyntaxProtocol & WithModifiersSyntax>(
175-
_ modifier: DeclModifierSyntax,
176-
to decl: Decl,
177-
declKeywordKeyPath: WritableKeyPath<Decl, TokenSyntax>
178-
) -> DeclSyntax {
179-
// If there's already an access modifier among the modifier list, bail out.
180-
guard decl.modifiers.accessLevelModifier == nil else { return DeclSyntax(decl) }
181-
182-
var result = decl
183-
var modifier = modifier
184-
modifier.trailingTrivia = [.spaces(1)]
185-
186-
guard var firstModifier = decl.modifiers.first else {
187-
// If there are no modifiers at all, add the one being requested, moving the leading trivia
188-
// from the decl keyword to that modifier (to preserve leading comments, newlines, etc.).
189-
modifier.leadingTrivia = decl[keyPath: declKeywordKeyPath].leadingTrivia
190-
result[keyPath: declKeywordKeyPath].leadingTrivia = []
191-
result.modifiers = .init([modifier])
192-
return DeclSyntax(result)
193-
}
194-
195-
// Otherwise, insert the modifier at the front of the modifier list, moving the (original) first
196-
// modifier's leading trivia to the new one (to preserve leading comments, newlines, etc.).
197-
modifier.leadingTrivia = firstModifier.leadingTrivia
198-
firstModifier.leadingTrivia = []
199-
result.modifiers[result.modifiers.startIndex] = firstModifier
200-
result.modifiers.insert(modifier, at: result.modifiers.startIndex)
201-
return DeclSyntax(result)
202-
}

‎Tests/SwiftFormatTests/Rules/NoAccessLevelOnExtensionDeclarationTests.swift

+69
Original file line numberDiff line numberDiff line change
@@ -356,4 +356,73 @@ final class NoAccessLevelOnExtensionDeclarationTests: LintOrFormatRuleTestCase {
356356
]
357357
)
358358
}
359+
360+
func testIfConfigMembers() {
361+
assertFormatting(
362+
NoAccessLevelOnExtensionDeclaration.self,
363+
input: """
364+
1️⃣public extension Foo {
365+
#if os(macOS)
366+
2️⃣var x: Bool
367+
#else
368+
3️⃣var y: String
369+
#endif
370+
}
371+
""",
372+
expected: """
373+
extension Foo {
374+
#if os(macOS)
375+
public var x: Bool
376+
#else
377+
public var y: String
378+
#endif
379+
}
380+
""",
381+
findings: [
382+
FindingSpec(
383+
"1️⃣",
384+
message: "move this 'public' access modifier to precede each member inside this extension",
385+
notes: [
386+
NoteSpec("2️⃣", message: "add 'public' access modifier to this declaration"),
387+
NoteSpec("3️⃣", message: "add 'public' access modifier to this declaration"),
388+
]
389+
)
390+
]
391+
)
392+
}
393+
394+
func testStateDoesNotLeakBetweenExtensions() {
395+
assertFormatting(
396+
NoAccessLevelOnExtensionDeclaration.self,
397+
input: """
398+
1️⃣public extension Foo {
399+
2️⃣var x: Bool
400+
}
401+
3️⃣internal extension Foo {
402+
var y: String
403+
}
404+
""",
405+
expected: """
406+
extension Foo {
407+
public var x: Bool
408+
}
409+
extension Foo {
410+
var y: String
411+
}
412+
""",
413+
findings: [
414+
FindingSpec(
415+
"1️⃣",
416+
message: "move this 'public' access modifier to precede each member inside this extension",
417+
notes: [
418+
NoteSpec("2️⃣", message: "add 'public' access modifier to this declaration")
419+
]
420+
),
421+
FindingSpec(
422+
"3️⃣",
423+
message: "remove this redundant 'internal' access modifier from this extension"
424+
),
425+
]
426+
)
427+
}
359428
}

0 commit comments

Comments
 (0)