diff --git a/CHANGELOG.md b/CHANGELOG.md index c790f52bea..e1a195344f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Dangerfile b/Dangerfile index b4a492f8cf..f04da8f81c 100644 --- a/Dangerfile +++ b/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 diff --git a/Rules.md b/Rules.md index a1537f6210..6fea9b94b2 100644 --- a/Rules.md +++ b/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 +Shorthand syntactic sugar should be used, i.e. [Int] instead of Array. ### Examples diff --git a/Source/SwiftLintFramework/Rules/SyntacticSugarRule.swift b/Source/SwiftLintFramework/Rules/SyntacticSugarRule.swift index 9c1671a043..c6653716d6 100644 --- a/Source/SwiftLintFramework/Rules/SyntacticSugarRule.swift +++ b/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", + description: "Shorthand syntactic sugar should be used, i.e. [Int] instead of Array.", 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 = "(?:(?" - 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" + sugaredType = "Int?" + case "ImplicitlyUnwrappedOptional": + type = "ImplicitlyUnwrappedOptional" + sugaredType = "Int!" + case "Array": + type = "Array" + sugaredType = "[Int]" + case "Dictionary": + type = "Dictionary" + sugaredType = "[String: Int]" + default: + return type(of: self).description.description + } + + return "Shorthand syntactic sugar should be used, i.e. \(sugaredType) instead of \(type)." + } } diff --git a/script/oss-check b/script/oss-check index fadeabd90d..b90ebdd341 100755 --- a/script/oss-check +++ b/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