Merge pull request #249 from dylansturg/synth_initializer_bug

Apply private and fileprivate rules to UseSynthesizedInitializer.

Sources/SwiftFormatRules/UseSynthesizedInitializer.swift

@@ -39,9 +39,6 @@ public final class UseSynthesizedInitializer: SyntaxLintRule {
         storedProperties.append(varDecl)
         // Collect any possible redundant initializers into a list
       } else if let initDecl = member.as(InitializerDeclSyntax.self) {
-        guard initDecl.modifiers == nil || initDecl.modifiers!.has(modifier: "internal") else {
-          continue
-        }
         guard initDecl.optionalMark == nil else { continue }
         guard initDecl.throwsOrRethrowsKeyword == nil else { continue }
         initializers.append(initDecl)
@@ -62,6 +59,8 @@ public final class UseSynthesizedInitializer: SyntaxLintRule {
           variables: storedProperties,
           initBody: initializer.body)
       else { continue }
+      guard matchesAccessLevel(modifiers: initializer.modifiers, properties: storedProperties)
+      else { continue }
       extraneousInitializers.append(initializer)
     }
 
@@ -76,6 +75,29 @@ public final class UseSynthesizedInitializer: SyntaxLintRule {
     return .skipChildren
   }
 
+  /// Compares the actual access level of an initializer with the access level of a synthesized
+  /// memberwise initializer.
+  ///
+  /// - Parameters:
+  ///   - modifiers: The modifier list from the initializer.
+  ///   - properties: The properties from the enclosing type.
+  /// - Returns: Whether the initializer has the same access level as the synthesized initializer.
+  private func matchesAccessLevel(
+    modifiers: ModifierListSyntax?, properties: [VariableDeclSyntax]
+  ) -> Bool {
+    let synthesizedAccessLevel = synthesizedInitAccessLevel(using: properties)
+    let accessLevel = modifiers?.accessLevelModifier
+    switch synthesizedAccessLevel {
+    case .internal:
+      // No explicit access level or internal are equivalent.
+      return accessLevel == nil || accessLevel!.name.tokenKind == .internalKeyword
+    case .fileprivate:
+      return accessLevel != nil && accessLevel!.name.tokenKind == .fileprivateKeyword
+    case .private:
+      return accessLevel != nil && accessLevel!.name.tokenKind == .privateKeyword
+    }
+  }
+
   // Compares initializer parameters to stored properties of the struct
   private func matchesPropertyList(
     parameters: FunctionParameterListSyntax,
@@ -160,3 +182,36 @@ extension Diagnostic.Message {
     .warning,
     "remove initializer and use the synthesized initializer")
 }
+
+/// Defines the access levels which may be assigned to a synthesized memberwise initializer.
+fileprivate enum AccessLevel {
+  case `internal`
+  case `fileprivate`
+  case `private`
+}
+
+/// Computes the access level which would be applied to the synthesized memberwise initializer of
+/// a struct that contains the given properties.
+///
+/// The rules for default memberwise initializer access levels are defined in The Swift
+/// Programming Languge:
+/// https://docs.swift.org/swift-book/LanguageGuide/AccessControl.html#ID21
+///
+/// - Parameter properties: The properties contained within the struct.
+/// - Returns: The synthesized memberwise initializer's access level.
+fileprivate func synthesizedInitAccessLevel(using properties: [VariableDeclSyntax]) -> AccessLevel {
+  var hasFileprivate = false
+  for property in properties {
+    guard let modifiers = property.modifiers else { continue }
+
+    // Private takes precedence, so finding 1 private property defines the access level.
+    if modifiers.contains(where: {$0.name.tokenKind == .privateKeyword && $0.detail == nil}) {
+      return .private
+    }
+    if modifiers.contains(where: {$0.name.tokenKind == .fileprivateKeyword && $0.detail == nil}) {
+      hasFileprivate = true
+      // Can't break here because a later property might be private.
+    }
+  }
+  return hasFileprivate ? .fileprivate : .internal
+}

Tests/SwiftFormatRulesTests/UseSynthesizedInitializerTests.swift

@@ -1,14 +1,18 @@
 import SwiftFormatRules
 
 final class UseSynthesizedInitializerTests: LintOrFormatRuleTestCase {
+  override func setUp() {
+    self.shouldCheckForUnassertedDiagnostics = true
+  }
+
   func testMemberwiseInitializerIsDiagnosed() {
     let input =
       """
       public struct Person {
 
         public var name: String
         let phoneNumber: String
-        private let address: String
+        internal let address: String
 
         init(name: String, phoneNumber: String, address: String) {
           self.name = name
@@ -19,8 +23,28 @@ final class UseSynthesizedInitializerTests: LintOrFormatRuleTestCase {
       """
 
     performLint(UseSynthesizedInitializer.self, input: input)
-    XCTAssertDiagnosed(.removeRedundantInitializer)
-    XCTAssertNotDiagnosed(.removeRedundantInitializer)
+    XCTAssertDiagnosed(.removeRedundantInitializer, line: 7)
+  }
+
+  func testInternalMemberwiseInitializerIsDiagnosed() {
+    let input =
+      """
+      public struct Person {
+
+        public var name: String
+        let phoneNumber: String
+        internal let address: String
+
+        internal init(name: String, phoneNumber: String, address: String) {
+          self.name = name
+          self.address = address
+          self.phoneNumber = phoneNumber
+        }
+      }
+      """
+
+    performLint(UseSynthesizedInitializer.self, input: input)
+    XCTAssertDiagnosed(.removeRedundantInitializer, line: 7)
   }
 
   func testMemberwiseInitializerWithDefaultArgumentIsDiagnosed() {
@@ -30,7 +54,7 @@ final class UseSynthesizedInitializerTests: LintOrFormatRuleTestCase {
 
          public var name: String = "John Doe"
          let phoneNumber: String
-         private let address: String
+         internal let address: String
 
          init(name: String = "John Doe", phoneNumber: String, address: String) {
            self.name = name
@@ -40,9 +64,8 @@ final class UseSynthesizedInitializerTests: LintOrFormatRuleTestCase {
        }
        """
 
-     performLint(UseSynthesizedInitializer.self, input: input)
-     XCTAssertDiagnosed(.removeRedundantInitializer)
-     XCTAssertNotDiagnosed(.removeRedundantInitializer)
+    performLint(UseSynthesizedInitializer.self, input: input)
+    XCTAssertDiagnosed(.removeRedundantInitializer, line: 7)
    }
 
   func testCustomInitializerVoidsSynthesizedInitializerWarning() {
@@ -81,7 +104,7 @@ final class UseSynthesizedInitializerTests: LintOrFormatRuleTestCase {
 
          public var name: String
          let phoneNumber: String
-         private let address: String
+         let address: String
 
          init(name: String = "Jane Doe", phoneNumber: String, address: String) {
            self.name = name
@@ -102,7 +125,7 @@ final class UseSynthesizedInitializerTests: LintOrFormatRuleTestCase {
 
          public var name: String = "John Doe"
          let phoneNumber: String
-         private let address: String
+         let address: String
 
          init(name: String = "Jane Doe", phoneNumber: String, address: String) {
            self.name = name
@@ -125,7 +148,7 @@ final class UseSynthesizedInitializerTests: LintOrFormatRuleTestCase {
 
         public var name: String
         var phoneNumber: String = "+15555550101"
-        private let address: String
+        let address: String
 
         init(name: String, phoneNumber: String, address: String) {
           self.name = name
@@ -146,7 +169,7 @@ final class UseSynthesizedInitializerTests: LintOrFormatRuleTestCase {
 
         public var name: String
         var phoneNumber: String?
-        private let address: String
+        let address: String
 
         init(name: String, phoneNumber: String, address: String) {
           self.name = name
@@ -167,7 +190,7 @@ final class UseSynthesizedInitializerTests: LintOrFormatRuleTestCase {
 
         public var name: String
         var phoneNumber: String?
-        private let address: String
+        let address: String
 
         init(name: String, phoneNumber: String?, address: String, anotherArg: Int) {
           self.name = name
@@ -188,7 +211,7 @@ final class UseSynthesizedInitializerTests: LintOrFormatRuleTestCase {
 
         public var name: String
         var phoneNumber: String?
-        private let address: String
+        let address: String
 
         init(name: String, phoneNumber: String?, address: String) {
           self.name = name
@@ -211,7 +234,7 @@ final class UseSynthesizedInitializerTests: LintOrFormatRuleTestCase {
 
         public var name: String
         let phoneNumber: String
-        private let address: String
+        let address: String
 
         init?(name: String, phoneNumber: String, address: String) {
           self.name = name
@@ -232,7 +255,7 @@ final class UseSynthesizedInitializerTests: LintOrFormatRuleTestCase {
 
         public var name: String
         let phoneNumber: String
-        private let address: String
+        let address: String
 
         init(name: String, phoneNumber: String, address: String) throws {
           self.name = name
@@ -253,7 +276,7 @@ final class UseSynthesizedInitializerTests: LintOrFormatRuleTestCase {
 
         public var name: String
         let phoneNumber: String
-        private let address: String
+        let address: String
 
         public init(name: String, phoneNumber: String, address: String) {
           self.name = name
@@ -266,4 +289,119 @@ final class UseSynthesizedInitializerTests: LintOrFormatRuleTestCase {
     performLint(UseSynthesizedInitializer.self, input: input)
     XCTAssertNotDiagnosed(.removeRedundantInitializer)
   }
+
+  func testDefaultMemberwiseInitializerIsNotDiagnosed() {
+    // The synthesized initializer is private when any member is private, so an initializer with
+    // default access control (i.e. internal) is not equivalent to the synthesized initializer.
+    let input =
+      """
+      public struct Person {
+
+        let phoneNumber: String
+        private let address: String
+
+        init(phoneNumber: String, address: String) {
+          self.address = address
+          self.phoneNumber = phoneNumber
+        }
+      }
+      """
+
+    performLint(UseSynthesizedInitializer.self, input: input)
+    XCTAssertNotDiagnosed(.removeRedundantInitializer)
+  }
+
+  func testPrivateMemberwiseInitializerWithPrivateMemberIsDiagnosed() {
+    // The synthesized initializer is private when any member is private, so a private initializer
+    // is equivalent to the synthesized initializer.
+    let input =
+      """
+      public struct Person {
+
+        let phoneNumber: String
+        private let address: String
+
+        private init(phoneNumber: String, address: String) {
+          self.address = address
+          self.phoneNumber = phoneNumber
+        }
+      }
+      """
+
+    performLint(UseSynthesizedInitializer.self, input: input)
+    XCTAssertDiagnosed(.removeRedundantInitializer, line: 6)
+  }
+
+  func testFileprivateMemberwiseInitializerWithFileprivateMemberIsDiagnosed() {
+    // The synthesized initializer is fileprivate when any member is fileprivate, so a fileprivate
+    // initializer is equivalent to the synthesized initializer.
+    let input =
+      """
+      public struct Person {
+
+        let phoneNumber: String
+        fileprivate let address: String
+
+        fileprivate init(phoneNumber: String, address: String) {
+          self.address = address
+          self.phoneNumber = phoneNumber
+        }
+      }
+      """
+
+    performLint(UseSynthesizedInitializer.self, input: input)
+    XCTAssertDiagnosed(.removeRedundantInitializer, line: 6)
+  }
+
+  func testCustomSetterAccessLevel() {
+    // When a property has a different access level for its setter, the setter's access level
+    // doesn't change the access level of the synthesized initializer.
+    let input =
+      """
+      public struct Person {
+        let phoneNumber: String
+        private(set) let address: String
+
+        init(phoneNumber: String, address: String) {
+          self.address = address
+          self.phoneNumber = phoneNumber
+        }
+      }
+
+      public struct Person2 {
+        fileprivate let phoneNumber: String
+        private(set) let address: String
+
+        fileprivate init(phoneNumber: String, address: String) {
+          self.address = address
+          self.phoneNumber = phoneNumber
+        }
+      }
+
+      public struct Person3 {
+        fileprivate(set) let phoneNumber: String
+        private(set) let address: String
+
+        init(phoneNumber: String, address: String) {
+          self.address = address
+          self.phoneNumber = phoneNumber
+        }
+      }
+
+      public struct Person4 {
+        private fileprivate(set) let phoneNumber: String
+        private(set) let address: String
+
+        init(phoneNumber: String, address: String) {
+          self.address = address
+          self.phoneNumber = phoneNumber
+        }
+      }
+      """
+
+    performLint(UseSynthesizedInitializer.self, input: input)
+    XCTAssertDiagnosed(.removeRedundantInitializer, line: 5)
+    XCTAssertDiagnosed(.removeRedundantInitializer, line: 15)
+    XCTAssertDiagnosed(.removeRedundantInitializer, line: 25)
+  }
 }