Merge pull request swiftlang#6 from allevato/sr-11123

Fix an index-out-of-bounds error in DontRepeatTypeInStaticProperties.

Author: allevato

Sources/SwiftFormatRules/DontRepeatTypeInStaticProperties.swift

@@ -32,69 +32,87 @@ public struct DontRepeatTypeInStaticProperties: SyntaxLintRule {
   }
 
   public func visit(_ node: ClassDeclSyntax) -> SyntaxVisitorContinueKind {
-    determinePropertyNameViolations(members: node.members.members, nodeId: node.identifier.text)
+    diagnoseStaticMembers(node.members.members, endingWith: node.identifier.text)
     return .skipChildren
   }
 
   public func visit(_ node: EnumDeclSyntax) -> SyntaxVisitorContinueKind {
-    determinePropertyNameViolations(members: node.members.members, nodeId: node.identifier.text)
+    diagnoseStaticMembers(node.members.members, endingWith: node.identifier.text)
     return .skipChildren
   }
 
   public func visit(_ node: ProtocolDeclSyntax) -> SyntaxVisitorContinueKind {
-    determinePropertyNameViolations(members: node.members.members, nodeId: node.identifier.text)
+    diagnoseStaticMembers(node.members.members, endingWith: node.identifier.text)
     return .skipChildren
   }
 
   public func visit(_ node: StructDeclSyntax) -> SyntaxVisitorContinueKind {
-    determinePropertyNameViolations(members: node.members.members, nodeId: node.identifier.text)
+    diagnoseStaticMembers(node.members.members, endingWith: node.identifier.text)
     return .skipChildren
   }
 
   public func visit(_ node: ExtensionDeclSyntax) -> SyntaxVisitorContinueKind {
-    determinePropertyNameViolations(
-      members: node.members.members,
-      nodeId: node.extendedType.description)
+    let members = node.members.members
+
+    switch node.extendedType {
+    case let simpleType as SimpleTypeIdentifierSyntax:
+      diagnoseStaticMembers(members, endingWith: simpleType.name.text)
+    case let memberType as MemberTypeIdentifierSyntax:
+      // 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
+    }
+
     return .skipChildren
   }
 
-  func determinePropertyNameViolations(members: MemberDeclListSyntax, nodeId: String) {
+  /// 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: MemberDeclListSyntax, endingWith typeName: String) {
     for member in members {
-      guard let decl = member.decl as? VariableDeclSyntax else { continue }
-      guard let modifiers = decl.modifiers else { continue }
-      guard modifiers.has(modifier: "static") || modifiers.has(modifier: "class") else { continue }
+      guard
+        let varDecl = member.decl as? VariableDeclSyntax,
+        let modifiers = varDecl.modifiers,
+        modifiers.has(modifier: "static") || modifiers.has(modifier: "class")
+      else { continue }
 
-      let typeName = withoutPrefix(name: nodeId)
+      let bareTypeName = removingPossibleNamespacePrefix(from: typeName)
 
-      for id in decl.identifiers {
-        let varName = id.identifier.text
-        guard varName.contains(typeName) else { continue }
-        diagnose(.removeTypeFromName(name: varName, type: typeName), on: decl)
+      for pattern in varDecl.identifiers {
+        let varName = pattern.identifier.text
+        if varName.contains(bareTypeName) {
+          diagnose(.removeTypeFromName(name: varName, type: bareTypeName), on: varDecl)
+        }
       }
     }
   }
 
-  // Returns the given string without capitalized prefix in the beginning
-  func withoutPrefix(name: String) -> String {
-    let formattedName = name.trimmingCharacters(in: CharacterSet.whitespaces)
-    let upperCase = Array(formattedName.uppercased())
-    let original = Array(formattedName)
-    guard original[0] == upperCase[0] else { return name }
-
-    var prefixEndsAt = 0
-    var idx = 0
-    while idx <= name.count - 2 {
-      if original[idx] == upperCase[idx] && original[idx + 1] != upperCase[idx + 1] {
-        prefixEndsAt = idx
+  /// Returns the portion of the given string that excludes a possible Objective-C-style capitalized
+  /// namespace prefix (a leading sequence of more than one uppercase letter).
+  ///
+  /// If the name has zero or one leading uppercase letters, the entire name is returned.
+  private func removingPossibleNamespacePrefix(from name: String) -> Substring {
+    guard let first = name.first, first.isUppercase else { return name[...] }
+
+    for index in name.indices.dropLast() {
+      let nextIndex = name.index(after: index)
+      if name[index].isUppercase && !name[nextIndex].isUppercase {
+        return name[index...]
       }
-      idx += 1
     }
-    return String(formattedName.dropFirst(prefixEndsAt))
+
+    return name[...]
   }
 }
 
 extension Diagnostic.Message {
-  static func removeTypeFromName(name: String, type: String) -> Diagnostic.Message {
+  static func removeTypeFromName(name: String, type: Substring) -> Diagnostic.Message {
     return .init(.warning, "remove '\(type)' from '\(name)'")
   }
 }

Tests/SwiftFormatRulesTests/DontRepeatTypeInStaticPropertiesTests.swift

@@ -7,33 +7,34 @@ import XCTest
 public class DontRepeatTypeInStaticPropertiesTests: DiagnosingTestCase {
   public func testRepetitiveProperties() {
     let input =
-    """
-    public class UIColor {
-      static let redColor: UIColor
-      public class var blueColor: UIColor
-      var yellowColor: UIColor
-      static let green: UIColor
-      public class var purple: UIColor
-    }
-    enum Sandwich {
-      static let bolognaSandwich: Sandwich
-      static var hamSandwich: Sandwich
-      static var turkey: Sandwich
-    }
-    protocol RANDPerson {
-      var oldPerson: Person
-      static let youngPerson: Person
-    }
-    struct TVGame {
-      static var basketballGame: TVGame
-      static var baseballGame: TVGame
-      static let soccer: TVGame
-      let hockey: TVGame
-    }
-    extension URLSession {
-      class var sharedSession: URLSession
-    }
-    """
+      """
+      public class UIColor {
+        static let redColor: UIColor
+        public class var blueColor: UIColor
+        var yellowColor: UIColor
+        static let green: UIColor
+        public class var purple: UIColor
+      }
+      enum Sandwich {
+        static let bolognaSandwich: Sandwich
+        static var hamSandwich: Sandwich
+        static var turkey: Sandwich
+      }
+      protocol RANDPerson {
+        var oldPerson: Person
+        static let youngPerson: Person
+      }
+      struct TVGame {
+        static var basketballGame: TVGame
+        static var baseballGame: TVGame
+        static let soccer: TVGame
+        let hockey: TVGame
+      }
+      extension URLSession {
+        class var sharedSession: URLSession
+      }
+      """
+
     performLint(DontRepeatTypeInStaticProperties.self, input: input)
     XCTAssertDiagnosed(.removeTypeFromName(name: "redColor", type: "Color"))
     XCTAssertDiagnosed(.removeTypeFromName(name: "blueColor", type: "Color"))
@@ -55,4 +56,28 @@ public class DontRepeatTypeInStaticPropertiesTests: DiagnosingTestCase {
 
     XCTAssertDiagnosed(.removeTypeFromName(name: "sharedSession", type: "Session"))
   }
+
+  public func testSR11123() {
+    let input =
+      """
+      extension A {
+        static let b = C()
+      }
+      """
+
+    performLint(DontRepeatTypeInStaticProperties.self, input: input)
+    XCTAssertNotDiagnosed(.removeTypeFromName(name: "b", type: "A"))
+  }
+
+  public func testDottedExtendedType() {
+    let input =
+      """
+      extension Dotted.Thing {
+        static let defaultThing: Dotted.Thing
+      }
+      """
+
+    performLint(DontRepeatTypeInStaticProperties.self, input: input)
+    XCTAssertDiagnosed(.removeTypeFromName(name: "defaultThing", type: "Thing"))
+  }
 }

Tests/SwiftFormatRulesTests/XCTestManifests.swift

@@ -76,7 +76,9 @@ extension DontRepeatTypeInStaticPropertiesTests {
     //   `swift test --generate-linuxmain`
     // to regenerate.
     static let __allTests__DontRepeatTypeInStaticPropertiesTests = [
+        ("testDottedExtendedType", testDottedExtendedType),
         ("testRepetitiveProperties", testRepetitiveProperties),
+        ("testSR11123", testSR11123),
     ]
 }