Skip to content

Upgrade the DontRepeatTypeInStaticProperties rule. #777

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 1, 2024
Merged

Upgrade the DontRepeatTypeInStaticProperties rule. #777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Documentation/RuleDocumentation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

Use the rules below in the `rules` block of your `.swift-format`
configuration file, as described in
[Configuration](Configuration.md). All of these rules can be
[Configuration](Documentation/Configuration.md). All of these rules can be
applied in the linter, but only some of them can format your source code
automatically.

Expand Down
12 changes: 2 additions & 10 deletions Sources/SwiftFormat/Core/Pipelines+Generated.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ class LintPipeline: SyntaxVisitor {
visitIfEnabled(AllPublicDeclarationsHaveDocumentation.visit, for: node)
visitIfEnabled(AlwaysUseLowerCamelCase.visit, for: node)
visitIfEnabled(BeginDocumentationCommentWithOneLineSummary.visit, for: node)
visitIfEnabled(DontRepeatTypeInStaticProperties.visit, for: node)
visitIfEnabled(NoLeadingUnderscores.visit, for: node)
visitIfEnabled(TypeNamesShouldBeCapitalized.visit, for: node)
visitIfEnabled(UseTripleSlashForDocumentationComments.visit, for: node)
Expand All @@ -86,7 +85,6 @@ class LintPipeline: SyntaxVisitor {
onVisitPost(rule: AllPublicDeclarationsHaveDocumentation.self, for: node)
onVisitPost(rule: AlwaysUseLowerCamelCase.self, for: node)
onVisitPost(rule: BeginDocumentationCommentWithOneLineSummary.self, for: node)
onVisitPost(rule: DontRepeatTypeInStaticProperties.self, for: node)
onVisitPost(rule: NoLeadingUnderscores.self, for: node)
onVisitPost(rule: TypeNamesShouldBeCapitalized.self, for: node)
onVisitPost(rule: UseTripleSlashForDocumentationComments.self, for: node)
Expand Down Expand Up @@ -182,7 +180,6 @@ class LintPipeline: SyntaxVisitor {

override func visit(_ node: EnumDeclSyntax) -> SyntaxVisitorContinueKind {
visitIfEnabled(BeginDocumentationCommentWithOneLineSummary.visit, for: node)
visitIfEnabled(DontRepeatTypeInStaticProperties.visit, for: node)
visitIfEnabled(FullyIndirectEnum.visit, for: node)
visitIfEnabled(NoLeadingUnderscores.visit, for: node)
visitIfEnabled(OneCasePerLine.visit, for: node)
Expand All @@ -192,7 +189,6 @@ class LintPipeline: SyntaxVisitor {
}
override func visitPost(_ node: EnumDeclSyntax) {
onVisitPost(rule: BeginDocumentationCommentWithOneLineSummary.self, for: node)
onVisitPost(rule: DontRepeatTypeInStaticProperties.self, for: node)
onVisitPost(rule: FullyIndirectEnum.self, for: node)
onVisitPost(rule: NoLeadingUnderscores.self, for: node)
onVisitPost(rule: OneCasePerLine.self, for: node)
Expand All @@ -202,14 +198,12 @@ class LintPipeline: SyntaxVisitor {

override func visit(_ node: ExtensionDeclSyntax) -> SyntaxVisitorContinueKind {
visitIfEnabled(AvoidRetroactiveConformances.visit, for: node)
visitIfEnabled(DontRepeatTypeInStaticProperties.visit, for: node)
visitIfEnabled(NoAccessLevelOnExtensionDeclaration.visit, for: node)
visitIfEnabled(UseTripleSlashForDocumentationComments.visit, for: node)
return .visitChildren
}
override func visitPost(_ node: ExtensionDeclSyntax) {
onVisitPost(rule: AvoidRetroactiveConformances.self, for: node)
onVisitPost(rule: DontRepeatTypeInStaticProperties.self, for: node)
onVisitPost(rule: NoAccessLevelOnExtensionDeclaration.self, for: node)
onVisitPost(rule: UseTripleSlashForDocumentationComments.self, for: node)
}
Expand Down Expand Up @@ -423,7 +417,6 @@ class LintPipeline: SyntaxVisitor {
override func visit(_ node: ProtocolDeclSyntax) -> SyntaxVisitorContinueKind {
visitIfEnabled(AllPublicDeclarationsHaveDocumentation.visit, for: node)
visitIfEnabled(BeginDocumentationCommentWithOneLineSummary.visit, for: node)
visitIfEnabled(DontRepeatTypeInStaticProperties.visit, for: node)
visitIfEnabled(NoLeadingUnderscores.visit, for: node)
visitIfEnabled(TypeNamesShouldBeCapitalized.visit, for: node)
visitIfEnabled(UseTripleSlashForDocumentationComments.visit, for: node)
Expand All @@ -432,7 +425,6 @@ class LintPipeline: SyntaxVisitor {
override func visitPost(_ node: ProtocolDeclSyntax) {
onVisitPost(rule: AllPublicDeclarationsHaveDocumentation.self, for: node)
onVisitPost(rule: BeginDocumentationCommentWithOneLineSummary.self, for: node)
onVisitPost(rule: DontRepeatTypeInStaticProperties.self, for: node)
onVisitPost(rule: NoLeadingUnderscores.self, for: node)
onVisitPost(rule: TypeNamesShouldBeCapitalized.self, for: node)
onVisitPost(rule: UseTripleSlashForDocumentationComments.self, for: node)
Expand Down Expand Up @@ -469,7 +461,6 @@ class LintPipeline: SyntaxVisitor {
override func visit(_ node: StructDeclSyntax) -> SyntaxVisitorContinueKind {
visitIfEnabled(AllPublicDeclarationsHaveDocumentation.visit, for: node)
visitIfEnabled(BeginDocumentationCommentWithOneLineSummary.visit, for: node)
visitIfEnabled(DontRepeatTypeInStaticProperties.visit, for: node)
visitIfEnabled(NoLeadingUnderscores.visit, for: node)
visitIfEnabled(TypeNamesShouldBeCapitalized.visit, for: node)
visitIfEnabled(UseSynthesizedInitializer.visit, for: node)
Expand All @@ -479,7 +470,6 @@ class LintPipeline: SyntaxVisitor {
override func visitPost(_ node: StructDeclSyntax) {
onVisitPost(rule: AllPublicDeclarationsHaveDocumentation.self, for: node)
onVisitPost(rule: BeginDocumentationCommentWithOneLineSummary.self, for: node)
onVisitPost(rule: DontRepeatTypeInStaticProperties.self, for: node)
onVisitPost(rule: NoLeadingUnderscores.self, for: node)
onVisitPost(rule: TypeNamesShouldBeCapitalized.self, for: node)
onVisitPost(rule: UseSynthesizedInitializer.self, for: node)
Expand Down Expand Up @@ -568,6 +558,7 @@ class LintPipeline: SyntaxVisitor {
visitIfEnabled(AllPublicDeclarationsHaveDocumentation.visit, for: node)
visitIfEnabled(AlwaysUseLowerCamelCase.visit, for: node)
visitIfEnabled(BeginDocumentationCommentWithOneLineSummary.visit, for: node)
visitIfEnabled(DontRepeatTypeInStaticProperties.visit, for: node)
visitIfEnabled(NeverUseImplicitlyUnwrappedOptionals.visit, for: node)
visitIfEnabled(UseTripleSlashForDocumentationComments.visit, for: node)
return .visitChildren
Expand All @@ -576,6 +567,7 @@ class LintPipeline: SyntaxVisitor {
onVisitPost(rule: AllPublicDeclarationsHaveDocumentation.self, for: node)
onVisitPost(rule: AlwaysUseLowerCamelCase.self, for: node)
onVisitPost(rule: BeginDocumentationCommentWithOneLineSummary.self, for: node)
onVisitPost(rule: DontRepeatTypeInStaticProperties.self, for: node)
onVisitPost(rule: NeverUseImplicitlyUnwrappedOptionals.self, for: node)
onVisitPost(rule: UseTripleSlashForDocumentationComments.self, for: node)
}
Expand Down
1 change: 1 addition & 0 deletions Sources/SwiftFormat/Rules/AlwaysUseLowerCamelCase.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import SwiftSyntax
///
/// This rule does not apply to test code, defined as code which:
/// * Contains the line `import XCTest`
/// * The function is marked with `@Test` attribute
///
/// Lint: If an identifier contains underscores or begins with a capital letter, a lint error is
/// raised.
Expand Down
112 changes: 54 additions & 58 deletions Sources/SwiftFormat/Rules/DontRepeatTypeInStaticProperties.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,69 +22,28 @@ import SwiftSyntax
@_spi(Rules)
public final class DontRepeatTypeInStaticProperties: SyntaxLintRule {

public override func visit(_ node: ClassDeclSyntax) -> SyntaxVisitorContinueKind {
diagnoseStaticMembers(node.memberBlock.members, endingWith: node.name.text)
return .skipChildren
}

public override func visit(_ node: EnumDeclSyntax) -> SyntaxVisitorContinueKind {
diagnoseStaticMembers(node.memberBlock.members, endingWith: node.name.text)
return .skipChildren
}

public override func visit(_ node: ProtocolDeclSyntax) -> SyntaxVisitorContinueKind {
diagnoseStaticMembers(node.memberBlock.members, endingWith: node.name.text)
return .skipChildren
}

public override func visit(_ node: StructDeclSyntax) -> SyntaxVisitorContinueKind {
diagnoseStaticMembers(node.memberBlock.members, endingWith: node.name.text)
return .skipChildren
}

public override func visit(_ node: ExtensionDeclSyntax) -> SyntaxVisitorContinueKind {
let members = node.memberBlock.members

switch Syntax(node.extendedType).as(SyntaxEnum.self) {
case .identifierType(let simpleType):
diagnoseStaticMembers(members, endingWith: simpleType.name.text)
case .memberType(let memberType):
// We don't need to drill recursively into this structure because types with more than two
// components are constructed left-heavy; that is, `A.B.C.D` is structured as `((A.B).C).D`,
// and the final component of the top type is what we want.
diagnoseStaticMembers(members, endingWith: memberType.name.text)
default:
// Do nothing for non-nominal types. If Swift adds support for extensions on non-nominals,
// we'll need to update this if we need to support some subset of those.
break
/// Visits the static/class properties and diagnoses any where the name has the containing
/// type name (excluding possible namespace prefixes, like `NS` or `UI`) as a suffix.
public override func visit(_ node: VariableDeclSyntax) -> SyntaxVisitorContinueKind {
guard node.modifiers.contains(anyOf: [.class, .static]),
let typeName = Syntax(node).containingDeclName
else {
return .visitChildren
}

return .skipChildren
}

/// Iterates over the static/class properties in the given member list and diagnoses any where the
/// name has the containing type name (excluding possible namespace prefixes, like `NS` or `UI`)
/// as a suffix.
private func diagnoseStaticMembers(_ members: MemberBlockItemListSyntax, endingWith typeName: String) {
for member in members {
guard
let varDecl = member.decl.as(VariableDeclSyntax.self),
varDecl.modifiers.contains(anyOf: [.class, .static])
else { continue }

let bareTypeName = removingPossibleNamespacePrefix(from: typeName)

for binding in varDecl.bindings {
guard let identifierPattern = binding.pattern.as(IdentifierPatternSyntax.self) else {
continue
}
let bareTypeName = removingPossibleNamespacePrefix(from: typeName)
for binding in node.bindings {
guard let identifierPattern = binding.pattern.as(IdentifierPatternSyntax.self) else {
continue
}

let varName = identifierPattern.identifier.text
if varName.contains(bareTypeName) {
diagnose(.removeTypeFromName(name: varName, type: bareTypeName), on: identifierPattern)
}
let varName = identifierPattern.identifier.text
if varName.contains(bareTypeName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I'm getting another look at this, I wonder if we should update this to be hasSuffix, since the diagnostic explicitly talks about suffixes.

Copy link
Contributor Author

@shawnhyam shawnhyam Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question... we wouldn't catch plurals (which we don't mention or have tests for), but there are likely false positives with the current logic too.

public actor Cookie {
  // cases that are caught with .contains but not with .hasSuffix
  static let chocolateChipCookies: Int
  static let rawCookieDough: Int
}

diagnose(.removeTypeFromName(name: varName, type: bareTypeName), on: identifierPattern)
}
}

return .visitChildren
}

/// Returns the portion of the given string that excludes a possible Objective-C-style capitalized
Expand All @@ -110,3 +69,40 @@ extension Finding.Message {
"remove the suffix '\(type)' from the name of the variable '\(name)'"
}
}

extension Syntax {
/// Returns the name of the immediately enclosing type of this decl if there is one,
/// otherwise nil.
fileprivate var containingDeclName: String? {
switch Syntax(self).as(SyntaxEnum.self) {
case .actorDecl(let node):
return node.name.text
case .classDecl(let node):
return node.name.text
case .enumDecl(let node):
return node.name.text
case .protocolDecl(let node):
return node.name.text
case .structDecl(let node):
return node.name.text
case .extensionDecl(let node):
switch Syntax(node.extendedType).as(SyntaxEnum.self) {
case .identifierType(let simpleType):
return simpleType.name.text
case .memberType(let memberType):
// the final component of the top type `A.B.C.D` is what we want `D`.
return memberType.name.text
default:
// Do nothing for non-nominal types. If Swift adds support for extensions on non-nominals,
// we'll need to update this if we need to support some subset of those.
return nil
}
default:
if let parent = self.parent {
return parent.containingDeclName
}

return nil
}
}
}
1 change: 1 addition & 0 deletions Sources/SwiftFormat/Rules/NeverForceUnwrap.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import SwiftSyntax
///
/// This rule does not apply to test code, defined as code which:
/// * Contains the line `import XCTest`
/// * The function is marked with `@Test` attribute
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this noise from another PR that was merged in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this noise from another PR that was merged in?

This was because I ran generate-swift-format, and there have been some manual edits to the generated files. So I made the code edits necessary to keep the generated file the same.

///
/// Lint: If a force unwrap is used, a lint warning is raised.
@_spi(Rules)
Expand Down
1 change: 1 addition & 0 deletions Sources/SwiftFormat/Rules/NeverUseForceTry.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import SwiftSyntax
///
/// This rule does not apply to test code, defined as code which:
/// * Contains the line `import XCTest`
/// * The function is marked with `@Test` attribute
///
/// Lint: Using `try!` results in a lint error.
///
Expand Down
1 change: 1 addition & 0 deletions Sources/SwiftFormat/Rules/NeverUseImplicitlyUnwrappedOptionals.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import SwiftSyntax
///
/// This rule does not apply to test code, defined as code which:
/// * Contains the line `import XCTest`
/// * The function is marked with `@Test` attribute
///
/// TODO: Create exceptions for other UI elements (ex: viewDidLoad)
///
Expand Down
1 change: 1 addition & 0 deletions Sources/generate-swift-format/PipelineGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ final class PipelineGenerator: FileGenerator {
handle.write(
"""
override func visitPost(_ node: \(nodeType)) {

"""
)
for ruleName in lintRules.sorted() {
Expand Down
4 changes: 4 additions & 0 deletions Tests/SwiftFormatTests/Rules/DontRepeatTypeInStaticPropertiesTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ final class DontRepeatTypeInStaticPropertiesTests: LintOrFormatRuleTestCase {
extension URLSession {
class var 8️⃣sharedSession: URLSession
}
public actor Cookie {
static let 9️⃣chocolateChipCookie: Cookie
}
""",
findings: [
FindingSpec("1️⃣", message: "remove the suffix 'Color' from the name of the variable 'redColor'"),
Expand All @@ -42,6 +45,7 @@ final class DontRepeatTypeInStaticPropertiesTests: LintOrFormatRuleTestCase {
FindingSpec("6️⃣", message: "remove the suffix 'Game' from the name of the variable 'basketballGame'"),
FindingSpec("7️⃣", message: "remove the suffix 'Game' from the name of the variable 'baseballGame'"),
FindingSpec("8️⃣", message: "remove the suffix 'Session' from the name of the variable 'sharedSession'"),
FindingSpec("9️⃣", message: "remove the suffix 'Cookie' from the name of the variable 'chocolateChipCookie'"),
]
)
}
Expand Down