Fix CommaRule mismatch between violations and corrections

CHANGELOG.md

@@ -1,3 +1,19 @@
+## Master
+
+##### Breaking
+
+* None.
+
+##### Enhancements
+
+* None.
+
+##### Bug Fixes
+
+* Fix `CommaRule` mismatch between violations and corrections.  
+  [Norio Nomura](https://github.com/norio-nomura)
+  [#466](https://github.com/realm/SwiftLint/issues/466)
+
 ## 0.9.1: Air Duct Cleaning
 
 ##### Breaking

Source/SwiftLintFramework/Extensions/File+SwiftLint.swift

@@ -85,10 +85,7 @@ extension File {
         return regex.matchesInString(self.contents, options: [], range: range).map { match in
             let matchByteRange = contents.NSRangeToByteRange(start: match.range.location,
                 length: match.range.length) ?? match.range
-            let tokensInRange = syntax.tokens.filter { token in
-                let tokenByteRange = NSRange(location: token.offset, length: token.length)
-                return NSIntersectionRange(matchByteRange, tokenByteRange).length > 0
-            }.map({ $0 })
+            let tokensInRange = syntax.tokensIn(matchByteRange)
             return (match.range, tokensInRange)
         }
     }

Source/SwiftLintFramework/Extensions/SyntaxMap+SwiftLint.swift

@@ -0,0 +1,36 @@
+//
+//  SyntaxMap+SwiftLint.swift
+//  SwiftLint
+//
+//  Created by Norio Nomura on 2/19/16.
+//  Copyright © 2016 Realm. All rights reserved.
+//
+
+import Foundation
+import SourceKittenFramework
+
+extension SyntaxMap {
+    /// Returns array of SyntaxTokens intersecting with byte range
+    ///
+    /// - Parameter byteRange: byte based NSRange
+    internal func tokensIn(byteRange: NSRange) -> [SyntaxToken] {
+
+        func intersect(token: SyntaxToken) -> Bool {
+            return NSRange(location: token.offset, length: token.length)
+                .intersectsRange(byteRange)
+        }
+
+        func notIntersect(token: SyntaxToken) -> Bool {
+            return !intersect(token)
+        }
+
+        guard let startIndex = tokens.indexOf(intersect) else {
+            return []
+        }
+        let tokensBeginningIntersect = tokens.lazy.suffixFrom(startIndex)
+        if let endIndex = tokensBeginningIntersect.indexOf(notIntersect) {
+            return Array(tokensBeginningIntersect.prefixUpTo(endIndex))
+        }
+        return Array(tokensBeginningIntersect)
+    }
+}

Source/SwiftLintFramework/Rules/CommaRule.swift

@@ -22,7 +22,9 @@ public struct CommaRule: CorrectableRule, ConfigurationProviderRule {
         nonTriggeringExamples: [
             "func abc(a: String, b: String) { }",
             "abc(a: \"string\", b: \"string\"",
-            "enum a { case a, b, c }"
+            "enum a { case a, b, c }",
+            "func abc(\n  a: String,  // comment\n  bcd: String // comment\n) {\n}\n",
+            "func abc(\n  a: String,\n  bcd: String\n) {\n}\n",
         ],
         triggeringExamples: [
             "func abc(a: String↓ ,b: String) { }",
@@ -33,40 +35,99 @@ public struct CommaRule: CorrectableRule, ConfigurationProviderRule {
             "func abc(a: String,b: String) {}\n": "func abc(a: String, b: String) {}\n",
             "abc(a: \"string\",b: \"string\"\n": "abc(a: \"string\", b: \"string\"\n",
             "abc(a: \"string\"  ,  b: \"string\"\n": "abc(a: \"string\", b: \"string\"\n",
-            "enum a { case a  ,b }\n": "enum a { case a, b }\n"
+            "enum a { case a  ,b }\n": "enum a { case a, b }\n",
+            "let a = [1,1]\nlet b = 1\nf(1, b)\n": "let a = [1, 1]\nlet b = 1\nf(1, b)\n",
         ]
     )
 
     public func validateFile(file: File) -> [StyleViolation] {
-        let pattern = "(\\,[^\\s])|(\\s\\,)"
-        let excludingKinds = SyntaxKind.commentAndStringKinds()
-
-        return file.matchPattern(pattern, excludingSyntaxKinds: excludingKinds).map {
+        return violationRangesInFile(file).map {
             StyleViolation(ruleDescription: self.dynamicType.description,
                            severity: configuration.severity,
                            location: Location(file: file, characterOffset: $0.location))
         }
     }
 
     public func correctFile(file: File) -> [Correction] {
-        if validateFile(file).isEmpty { return [] }
-        let pattern = "\\s*\\,\\s*([^\\s])"
+        let matches = violationRangesInFile(file)
+        if matches.isEmpty { return [] }
 
+        var contents = file.contents as NSString
         let description = self.dynamicType.description
         var corrections = [Correction]()
-        var contents = file.contents
-
-        let matches = file.matchPattern(pattern, withSyntaxKinds: [.Identifier])
-
-        let regularExpression = regex(pattern)
         for range in matches.reverse() {
-            contents = regularExpression.stringByReplacingMatchesInString(contents,
-                options: [], range: range, withTemplate: ", $1")
+            contents = contents.stringByReplacingCharactersInRange(range, withString: ", ")
             let location = Location(file: file, characterOffset: range.location)
             corrections.append(Correction(ruleDescription: description, location: location))
         }
 
-        file.write(contents)
+        file.write(contents as String)
         return corrections
     }
+
+    // captures spaces and comma only
+    // http://userguide.icu-project.org/strings/regexp
+    private static let pattern =
+        "\\S" +                // not whitespace
+        "(" +                  // start first capure
+        "\\s+" +               // followed by whitespace
+        "," +                  // to the left of a comma
+        "[\\t\\p{Z}]*" +       // followed by any amount of tab or space.
+        "|" +                  // or
+        "," +                  // immediately followed by a comma
+        "(?:[\\t\\p{Z}]{0}|" + // followed by 0
+        "[\\t\\p{Z}]{2,})" +   // or 2+ tab or space characters.
+        ")" +                  // end capture
+        "(\\S)"                // second capture is not whitespace.
+
+    // swiftlint:disable:next force_try
+    private static let regularExpression = try! NSRegularExpression(pattern: pattern, options: [])
+    private static let excludingSyntaxKindsForFirstCapture = SyntaxKind.commentAndStringKinds()
+        .map { $0.rawValue }
+    private static let excludingSyntaxKindsForSecondCapture = SyntaxKind.commentKinds()
+        .map { $0.rawValue }
+
+    private func violationRangesInFile(file: File) -> [NSRange] {
+        let contents = file.contents
+        let range = NSRange(location: 0, length: contents.utf16.count)
+        let syntaxMap = file.syntaxMap
+        return CommaRule.regularExpression
+            .matchesInString(contents, options: [], range: range)
+            .flatMap { match -> NSRange? in
+                if match.numberOfRanges != 3 { return nil }
+
+                // check first captured range
+                let firstRange = match.rangeAtIndex(1)
+                guard let matchByteFirstRange = contents
+                    .NSRangeToByteRange(start: firstRange.location, length: firstRange.length)
+                    else { return nil }
+
+                // first captured range won't match tokens if it is not comment neither string
+                let tokensInFirstRange = syntaxMap.tokensIn(matchByteFirstRange)
+                    .filter { CommaRule.excludingSyntaxKindsForFirstCapture.contains($0.type) }
+
+                // If not empty, first captured range is comment or string
+                if !tokensInFirstRange.isEmpty {
+                    return nil
+                }
+
+                // check second captured range
+                let secondRange = match.rangeAtIndex(2)
+                guard let matchByteSecondRange = contents
+                    .NSRangeToByteRange(start: secondRange.location, length: secondRange.length)
+                    else { return nil }
+
+                // second captured range won't match tokens if it is not comment
+                let tokensInSecondRange = syntaxMap.tokensIn(matchByteSecondRange)
+                    .filter { CommaRule.excludingSyntaxKindsForSecondCapture.contains($0.type) }
+
+                // If not empty, second captured range is comment
+                if !tokensInSecondRange.isEmpty {
+                    return nil
+                }
+
+                // return first captured range
+                return firstRange
+        }
+    }
 }

Source/SwiftLintFrameworkTests/ConfigurationTests.swift

@@ -133,7 +133,7 @@ class ConfigurationTests: XCTestCase {
         private override func filesToLintAtPath(path: String) -> [String] {
             switch path {
             case "directory": return ["directory/File1.swift", "directory/File2.swift",
-                "directory/excluded/Excluded.swift",  "directory/ExcludedFile.swift"]
+                "directory/excluded/Excluded.swift", "directory/ExcludedFile.swift"]
             case "directory/excluded" : return ["directory/excluded/Excluded.swift"]
             case "directory/ExcludedFile.swift" : return ["directory/ExcludedFile.swift"]
             default: break
@@ -145,7 +145,7 @@ class ConfigurationTests: XCTestCase {
 
     func testExcludedPaths() {
         let configuration = Configuration(included: ["directory"],
-            excluded: ["directory/excluded",  "directory/ExcludedFile.swift"])!
+            excluded: ["directory/excluded", "directory/ExcludedFile.swift"])!
         let paths = configuration.lintablePathsForPath("", fileManager: TestFileManager())
         XCTAssertEqual(["directory/File1.swift", "directory/File2.swift"], paths)
     }

SwiftLint.xcodeproj/project.pbxproj

@@ -37,6 +37,7 @@
 		3BD9CD3D1C37175B009A5D25 /* YamlParser.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3BD9CD3C1C37175B009A5D25 /* YamlParser.swift */; };
 		3BDB224B1C345B4900473680 /* ProjectMock in Resources */ = {isa = PBXBuildFile; fileRef = 3BDB224A1C345B4900473680 /* ProjectMock */; };
 		69F88BF71BDA38A6005E7CAE /* OpeningBraceRule.swift in Sources */ = {isa = PBXBuildFile; fileRef = 692B1EB11BD7E00F00EAABFF /* OpeningBraceRule.swift */; };
+		6CC4259B1C77046200AEA885 /* SyntaxMap+SwiftLint.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6CC4259A1C77046200AEA885 /* SyntaxMap+SwiftLint.swift */; };
 		83894F221B0C928A006214E1 /* RulesCommand.swift in Sources */ = {isa = PBXBuildFile; fileRef = 83894F211B0C928A006214E1 /* RulesCommand.swift */; };
 		83D71E281B131ECE000395DE /* RuleDescription.swift in Sources */ = {isa = PBXBuildFile; fileRef = 83D71E261B131EB5000395DE /* RuleDescription.swift */; };
 		B58AEED61C492C7B00E901FD /* ForceUnwrappingRule.swift in Sources */ = {isa = PBXBuildFile; fileRef = B58AEED51C492C7B00E901FD /* ForceUnwrappingRule.swift */; };
@@ -198,6 +199,7 @@
 		692B1EB11BD7E00F00EAABFF /* OpeningBraceRule.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = OpeningBraceRule.swift; sourceTree = "<group>"; };
 		692B60AB1BD8F2E700C7AA22 /* StatementPositionRule.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = StatementPositionRule.swift; sourceTree = "<group>"; };
 		695BE9CE1BDFD92B0071E985 /* CommaRule.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = CommaRule.swift; sourceTree = "<group>"; };
+		6CC4259A1C77046200AEA885 /* SyntaxMap+SwiftLint.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "SyntaxMap+SwiftLint.swift"; sourceTree = "<group>"; };
 		6CDD62CE1C6193300094A198 /* main.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = main.swift; sourceTree = "<group>"; };
 		83894F211B0C928A006214E1 /* RulesCommand.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = RulesCommand.swift; sourceTree = "<group>"; };
 		83D71E261B131EB5000395DE /* RuleDescription.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = RuleDescription.swift; sourceTree = "<group>"; };
@@ -628,6 +630,7 @@
 				E88DEA721B0984C400A66CB0 /* String+SwiftLint.swift */,
 				E816194B1BFBF35D00946723 /* SwiftDeclarationKind+SwiftLint.swift */,
 				E87E4A081BFB9CAE00FCFE46 /* SyntaxKind+SwiftLint.swift */,
+				6CC4259A1C77046200AEA885 /* SyntaxMap+SwiftLint.swift */,
 				3B1150C91C31FC3F00D83B1E /* Yaml+SwiftLint.swift */,
 				3B5B9FE01C444DA20009AD27 /* Array+SwiftLint.swift */,
 				3BB47D841C51D80000AE6A10 /* NSRegularExpression+SwiftLint.swift */,
@@ -819,6 +822,7 @@
 			isa = PBXSourcesBuildPhase;
 			buildActionMask = 2147483647;
 			files = (
+				6CC4259B1C77046200AEA885 /* SyntaxMap+SwiftLint.swift in Sources */,
 				E881985C1BEA978500333A11 /* TrailingNewlineRule.swift in Sources */,
 				E881985E1BEA982100333A11 /* TypeBodyLengthRule.swift in Sources */,
 				69F88BF71BDA38A6005E7CAE /* OpeningBraceRule.swift in Sources */,