Merge pull request #1804 from marcelofabri/fix-1803

Improve syntactic_sugar message to be type-specific

CHANGELOG.md

@@ -68,6 +68,10 @@
   [Mazyod](https://github.com/mazyod)
   [#1767](https://github.com/realm/SwiftLint/issues/1767)
 
+* Improve `syntactic_sugar` violation message to be type-specific.  
+  [Marcelo Fabri](https://github.com/marcelofabri)
+  [#1803](https://github.com/realm/SwiftLint/issues/1803)
+
 ##### Bug Fixes
 
 * Fix false positive on `force_unwrapping` rule when declaring

Dangerfile

@@ -16,7 +16,7 @@ has_app_changes = !modified_files.grep(/Source/).empty?
 has_test_changes = !modified_files.grep(/Tests/).empty?
 has_danger_changes = !modified_files.grep(/Dangerfile|script\/oss-check|Gemfile/).empty?
 has_rules_changes = !modified_files.grep(/Source\/SwiftLintFramework\/Rules/).empty?
-has_rules_docs_changes = !modified_files.include?('Rules.md')
+has_rules_docs_changes = modified_files.include?('Rules.md')
 
 # Add a CHANGELOG entry for app changes
 if !modified_files.include?('CHANGELOG.md') && has_app_changes

Rules.md

@@ -9682,7 +9682,7 @@ Identifier | Enabled by default | Supports autocorrection | Kind
 --- | --- | --- | ---
 `syntactic_sugar` | Enabled | No | idiomatic
 
-Shorthand syntactic sugar should be used, i.e. [Int] instead of Array<Int>
+Shorthand syntactic sugar should be used, i.e. [Int] instead of Array<Int>.
 
 ### Examples
 

Source/SwiftLintFramework/Rules/SyntacticSugarRule.swift

@@ -17,7 +17,7 @@ public struct SyntacticSugarRule: ConfigurationProviderRule {
     public static let description = RuleDescription(
         identifier: "syntactic_sugar",
         name: "Syntactic Sugar",
-        description: "Shorthand syntactic sugar should be used, i.e. [Int] instead of Array<Int>",
+        description: "Shorthand syntactic sugar should be used, i.e. [Int] instead of Array<Int>.",
         kind: .idiomatic,
         nonTriggeringExamples: [
             "let x: [Int]",
@@ -50,38 +50,81 @@ public struct SyntacticSugarRule: ConfigurationProviderRule {
     public func validate(file: File) -> [StyleViolation] {
         let types = ["Optional", "ImplicitlyUnwrappedOptional", "Array", "Dictionary"]
         let negativeLookBehind = "(?:(?<!\\.)|Swift\\.)"
-        let pattern = negativeLookBehind + "\\b(?:" + types.joined(separator: "|") + ")\\s*<.*?>"
-        let kinds = SyntaxKind.commentAndStringKinds()
+        let pattern = negativeLookBehind + "\\b(" + types.joined(separator: "|") + ")\\s*<.*?>"
+        let excludingKinds = Set(SyntaxKind.commentAndStringKinds())
         let contents = file.contents.bridge()
+        let range = NSRange(location: 0, length: contents.length)
 
-        return file.match(pattern: pattern, excludingSyntaxKinds: kinds).flatMap { range in
-
-            // avoid triggering when referring to an associatedtype
-            let start = range.location + range.length
-            let restOfFileRange = NSRange(location: start, length: contents.length - start)
-            if regex("\\s*\\.").firstMatch(in: file.contents, options: [],
-                                           range: restOfFileRange)?.range.location == start {
-                guard let byteOffset = contents.NSRangeToByteRange(start: range.location,
-                                                                   length: range.length)?.location else {
-                    return nil
-                }
-
-                let kinds = file.structure.kinds(forByteOffset: byteOffset)
-                    .flatMap { SwiftExpressionKind(rawValue: $0.kind) }
-                guard kinds.contains(.call) else {
-                    return nil
-                }
+        return regex(pattern).matches(in: file.contents, options: [], range: range).flatMap { result in
+            let range = result.range
+            guard let byteRange = contents.NSRangeToByteRange(start: range.location, length: range.length) else {
+                return nil
+            }
 
-                if let (range, kinds) = file.match(pattern: "\\s*\\.(?:self|Type)", range: restOfFileRange).first,
-                    range.location == start, kinds == [.keyword] || kinds == [.identifier] {
+            let kinds = file.syntaxMap.kinds(inByteRange: byteRange)
+            guard excludingKinds.isDisjoint(with: kinds),
+                isValidViolation(range: range, file: file) else {
                     return nil
-                }
             }
 
+            let type = contents.substring(with: result.range(at: 1))
             return StyleViolation(ruleDescription: type(of: self).description,
                                   severity: configuration.severity,
-                                  location: Location(file: file, characterOffset: range.location))
+                                  location: Location(file: file, characterOffset: range.location),
+                                  reason: message(for: type))
+
         }
     }
 
+    private func isValidViolation(range: NSRange, file: File) -> Bool {
+        let contents = file.contents.bridge()
+
+        // avoid triggering when referring to an associatedtype
+        let start = range.location + range.length
+        let restOfFileRange = NSRange(location: start, length: contents.length - start)
+        if regex("\\s*\\.").firstMatch(in: file.contents, options: [],
+                                       range: restOfFileRange)?.range.location == start {
+            guard let byteOffset = contents.NSRangeToByteRange(start: range.location,
+                                                               length: range.length)?.location else {
+                return false
+            }
+
+            let kinds = file.structure.kinds(forByteOffset: byteOffset)
+                .flatMap { SwiftExpressionKind(rawValue: $0.kind) }
+            guard kinds.contains(.call) else {
+                return false
+            }
+
+            if let (range, kinds) = file.match(pattern: "\\s*\\.(?:self|Type)", range: restOfFileRange).first,
+                range.location == start, kinds == [.keyword] || kinds == [.identifier] {
+                return false
+            }
+        }
+
+        return true
+    }
+
+    private func message(for originalType: String) -> String {
+        let type: String
+        let sugaredType: String
+
+        switch originalType {
+        case "Optional":
+            type = "Optional<Int>"
+            sugaredType = "Int?"
+        case "ImplicitlyUnwrappedOptional":
+            type = "ImplicitlyUnwrappedOptional<Int>"
+            sugaredType = "Int!"
+        case "Array":
+            type = "Array<Int>"
+            sugaredType = "[Int]"
+        case "Dictionary":
+            type = "Dictionary<String, Int>"
+            sugaredType = "[String: Int]"
+        default:
+            return type(of: self).description.description
+        }
+
+        return "Shorthand syntactic sugar should be used, i.e. \(sugaredType) instead of \(type)."
+    }
 }

script/oss-check

@@ -7,6 +7,7 @@
 require 'fileutils'
 require 'open3'
 require 'optparse'
+require 'erb'
 
 ################################
 # Options
@@ -229,10 +230,12 @@ def diff_and_report_changes_to_danger
     master = non_empty_lines("#{@working_dir}/master_reports/#{repo.name}.txt")
 
     (master - branch).each do |fixed|
-      message "This PR fixed a violation in #{repo.name}: [#{fixed}](#{convert_to_link(repo, fixed)})"
+      escaped_message = ERB::Util.html_escape fixed
+      message "This PR fixed a violation in #{repo.name}: [#{escaped_message}](#{convert_to_link(repo, fixed)})"
     end
     (branch - master).each do |violation|
-      warn "This PR introduced a violation in #{repo.name}: [#{violation}](#{convert_to_link(repo, violation)})"
+      escaped_message = ERB::Util.html_escape violation
+      warn "This PR introduced a violation in #{repo.name}: [#{escaped_message}](#{convert_to_link(repo, violation)})"
     end
   end
 end