From efa68177b258804fda41bd7c0ed7e87942326634 Mon Sep 17 00:00:00 2001 From: Timofey Solonin Date: Thu, 29 Nov 2018 02:10:49 +0300 Subject: [PATCH] #2435 - Make modifier_order rule rely on an explicit set of rules (#2458) * #2435 - Adjust modifier_order rule to require explicit modifier order specified to conclude a violation * #2435 - Move modifier order rule examples to a separate file * #2435 - Add modifier interference tests * #2435 - Fix whitespaces * Minor edits * Add changelog entry --- CHANGELOG.md | 4 + .../Rules/Style/ModifierOrderRule.swift | 252 ++++++------------ .../Style/ModifierOrderRuleExamples.swift | 160 +++++++++++ SwiftLint.xcodeproj/project.pbxproj | 4 + Tests/LinuxMain.swift | 1 + .../ModifierOrderTests.swift | 59 +++- 6 files changed, 302 insertions(+), 178 deletions(-) create mode 100644 Source/SwiftLintFramework/Rules/Style/ModifierOrderRuleExamples.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 0779350670..750fc4eb54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,10 @@ [Cihat Gündüz](https://github.com/Dschee) [#2306](https://github.com/realm/SwiftLint/issues/2306) +* Ignore unspecified modifiers in `modifier_order`. + [Timofey Solonin](https://github.com/biboran) + [#2435](https://github.com/realm/SwiftLint/issues/2435) + #### Bug Fixes * Fix false positive in `nimble_operator` rule. diff --git a/Source/SwiftLintFramework/Rules/Style/ModifierOrderRule.swift b/Source/SwiftLintFramework/Rules/Style/ModifierOrderRule.swift index 83a3a62204..0dee454b1a 100644 --- a/Source/SwiftLintFramework/Rules/Style/ModifierOrderRule.swift +++ b/Source/SwiftLintFramework/Rules/Style/ModifierOrderRule.swift @@ -1,10 +1,21 @@ import SourceKittenFramework -private typealias ModifierKeyword = String -private typealias ModifierDescription = (ModifierKeyword, SwiftDeclarationAttributeKind.ModifierGroup) - public struct ModifierOrderRule: ASTRule, OptInRule, ConfigurationProviderRule { - public var configuration = ModifierOrderConfiguration(preferredModifierOrder: [.override, .acl]) + public var configuration = ModifierOrderConfiguration( + preferredModifierOrder: [ + .override, + .acl, + .setterACL, + .dynamic, + .mutators, + .lazy, + .final, + .required, + .convenience, + .typeMethods, + .owned + ] + ) public init() {} @@ -14,163 +25,8 @@ public struct ModifierOrderRule: ASTRule, OptInRule, ConfigurationProviderRule { description: "Modifier order should be consistent.", kind: .style, minSwiftVersion: .fourDotOne , - nonTriggeringExamples: [ - "public class Foo { \n" + - " public convenience required init() {} \n" + - "}", - "public class Foo { \n" + - " public static let bar = 42 \n" + - "}", - "public class Foo { \n" + - " public static var bar: Int { \n" + - " return 42" + - " }" + - "}", - "public class Foo { \n" + - " public class var bar: Int { \n" + - " return 42 \n" + - " } \n" + - "}", - "public class Bar { \n" + - " public class var foo: String { \n" + - " return \"foo\" \n" + - " } \n" + - "} \n" + - "public class Foo: Bar { \n" + - " override public final class var foo: String { \n" + - " return \"bar\" \n" + - " } \n" + - "}", - "open class Bar { \n" + - " public var foo: Int? { \n" + - " return 42 \n" + - " } \n" + - "} \n" + - "open class Foo: Bar { \n" + - " override public var foo: Int? { \n" + - " return 43 \n" + - " } \n" + - "}", - "open class Bar { \n" + - " open class func foo() -> Int { \n" + - " return 42 \n" + - " } \n" + - "} \n" + - "class Foo: Bar { \n" + - " override open class func foo() -> Int { \n" + - " return 43 \n" + - " } \n" + - "}", - "protocol Foo: class {} \n" + - "class Bar { \n" + - " public private(set) weak var foo: Foo? \n" + - "} \n", - "@objc \n" + - "public final class Foo: NSObject {} \n", - "@objcMembers \n" + - "public final class Foo: NSObject {} \n", - "@objc \n" + - "override public private(set) weak var foo: Bar? \n", - "@objc \n" + - "public final class Foo: NSObject {} \n", - "@objc \n" + - "open final class Foo: NSObject { \n" + - " open weak var weakBar: NSString? = nil \n" + - "}", - "public final class Foo {}", - "class Bar { \n" + - " func bar() {} \n" + - "}", - "internal class Foo: Bar { \n" + - " override internal func bar() {} \n" + - "}", - "public struct Foo { \n" + - " internal weak var weakBar: NSObject? = nil \n" + - "}", - "class Foo { \n" + - " internal lazy var bar: String = \"foo\" \n" + - "}" - ], - triggeringExamples: [ - "class Foo { \n" + - " convenience required public init() {} \n" + - "}", - "public class Foo { \n" + - " static public let bar = 42 \n" + - "}", - "public class Foo { \n" + - " static public var bar: Int { \n" + - " return 42 \n" + - " } \n" + - "} \n", - "public class Foo { \n" + - " class public var bar: Int { \n" + - " return 42 \n" + - " } \n" + - "}", - "public class RootFoo { \n" + - " class public var foo: String { \n" + - " return \"foo\" \n" + - " } \n" + - "} \n" + - "public class Foo: RootFoo { \n" + - " override final class public var foo: String { \n" + - " return \"bar\" \n" + - " } \n" + - "}", - "open class Bar { \n" + - " public var foo: Int? { \n" + - " return 42 \n" + - " } \n" + - "} \n" + - "open class Foo: Bar { \n" + - " public override var foo: Int? { \n" + - " return 43 \n" + - " } \n" + - "}", - "protocol Foo: class {} \n" + - "class Bar { \n" + - " private(set) public weak var foo: Foo? \n" + - "} \n", - "open class Bar { \n" + - " open class func foo() -> Int { \n" + - " return 42 \n" + - " } \n" + - "} \n" + - "class Foo: Bar { \n" + - " class open override func foo() -> Int { \n" + - " return 43 \n" + - " } \n" + - "}", - "open class Bar { \n" + - " open class func foo() -> Int { \n" + - " return 42 \n" + - " } \n" + - "} \n" + - "class Foo: Bar { \n" + - " open override class func foo() -> Int { \n" + - " return 43 \n" + - " } \n" + - "}", - "@objc \n" + - "final public class Foo: NSObject {}", - "@objcMembers \n" + - "final public class Foo: NSObject {}", - "@objc \n" + - "final open class Foo: NSObject { \n" + - " weak open var weakBar: NSString? = nil \n" + - "}", - "final public class Foo {} \n", - "internal class Foo: Bar { \n" + - " internal override func bar() {} \n" + - "}", - "public struct Foo { \n" + - " weak internal var weakBar: NSObjetc? = nil \n" + - "}", - "class Foo { \n" + - " lazy internal var bar: String = \"foo\" \n" + - "}" - ] + nonTriggeringExamples: ModifierOrderRuleExamples.nonTriggeringExamples, + triggeringExamples: ModifierOrderRuleExamples.triggeringExamples ) public func validate(file: File, @@ -181,24 +37,55 @@ public struct ModifierOrderRule: ASTRule, OptInRule, ConfigurationProviderRule { return [] } - let preferredOrder = [.atPrefixed] + configuration.preferredModifierOrder - let declaredDescriptions = dictionary.modifierDescriptions - let preferredDescriptions = preferredOrder.reduce(into: [ModifierDescription]()) { descriptions, group in - if let description = declaredDescriptions.first(where: { $0.1 == group }) { - descriptions.append(description) - } + let violatableModifiers = self.violatableModifiers(declaredModifiers: dictionary.modifierDescriptions) + let prioritizedModifiers = self.prioritizedModifiers(violatableModifiers: violatableModifiers) + let sortedByPriorityModifiers = prioritizedModifiers.sorted( + by: { lhs, rhs in lhs.priority < rhs.priority } + ).map { $0.modifier } + + let violatingModifiers = zip( + sortedByPriorityModifiers, + violatableModifiers + ).filter { sortedModifier, unsortedModifier in + return sortedModifier != unsortedModifier } - for (index, preferredDescription) in preferredDescriptions.enumerated() - where preferredDescription.1 != declaredDescriptions[index].1 { - let reason = "\(preferredDescription.0) modifier should be before \(declaredDescriptions[index].0)." - return [StyleViolation(ruleDescription: type(of: self).description, - severity: configuration.severityConfiguration.severity, - location: Location(file: file, byteOffset: offset), - reason: reason)] + if let first = violatingModifiers.first { + let preferredModifier = first.0 + let declaredModifier = first.1 + let reason = "\(preferredModifier.keyword) modifier should be before \(declaredModifier.keyword)." + return [ + StyleViolation( + ruleDescription: type(of: self).description, + severity: configuration.severityConfiguration.severity, + location: Location(file: file, byteOffset: offset), + reason: reason + ) + ] + } else { + return [] } + } - return [] + private func violatableModifiers(declaredModifiers: [ModifierDescription]) -> [ModifierDescription] { + let preferredModifierGroups = ([.atPrefixed] + configuration.preferredModifierOrder) + return declaredModifiers.filter { preferredModifierGroups.contains($0.group) } + } + + private func prioritizedModifiers( + violatableModifiers: [ModifierDescription] + ) -> [(priority: Int, modifier: ModifierDescription)] { + let prioritizedPreferredModifierGroups = ([.atPrefixed] + configuration.preferredModifierOrder).enumerated() + return violatableModifiers.reduce( + [(priority: Int, modifier: ModifierDescription)]() + ) { prioritizedModifiers, modifier in + guard let priority = prioritizedPreferredModifierGroups.first( + where: { _, group in modifier.group == group } + )?.offset else { + return prioritizedModifiers + } + return prioritizedModifiers + [(priority: priority, modifier: modifier)] + } } } @@ -216,9 +103,15 @@ private extension Dictionary where Key == String, Value == SourceKitRepresentabl .compactMap { if let attribute = $0.attribute, let modifierGroup = SwiftDeclarationAttributeKind.ModifierGroup(rawAttribute: attribute) { - return ModifierDescription(attribute.lastComponentAfter("."), modifierGroup) + return ModifierDescription( + keyword: attribute.lastComponentAfter("."), + group: modifierGroup + ) } else if let kind = $0.kind { - return ModifierDescription(kind.lastComponentAfter("."), .typeMethods) + return ModifierDescription( + keyword: kind.lastComponentAfter("."), + group: .typeMethods + ) } return nil } @@ -240,3 +133,8 @@ private extension String { return components(separatedBy: charachter).last ?? "" } } + +private struct ModifierDescription: Equatable { + let keyword: String + let group: SwiftDeclarationAttributeKind.ModifierGroup +} diff --git a/Source/SwiftLintFramework/Rules/Style/ModifierOrderRuleExamples.swift b/Source/SwiftLintFramework/Rules/Style/ModifierOrderRuleExamples.swift new file mode 100644 index 0000000000..7adbb9f871 --- /dev/null +++ b/Source/SwiftLintFramework/Rules/Style/ModifierOrderRuleExamples.swift @@ -0,0 +1,160 @@ +internal struct ModifierOrderRuleExamples { + static let nonTriggeringExamples = [ + "public class Foo { \n" + + " public convenience required init() {} \n" + + "}", + "public class Foo { \n" + + " public static let bar = 42 \n" + + "}", + "public class Foo { \n" + + " public static var bar: Int { \n" + + " return 42" + + " }" + + "}", + "public class Foo { \n" + + " public class var bar: Int { \n" + + " return 42 \n" + + " } \n" + + "}", + "public class Bar { \n" + + " public class var foo: String { \n" + + " return \"foo\" \n" + + " } \n" + + "} \n" + + "public class Foo: Bar { \n" + + " override public final class var foo: String { \n" + + " return \"bar\" \n" + + " } \n" + + "}", + "open class Bar { \n" + + " public var foo: Int? { \n" + + " return 42 \n" + + " } \n" + + "} \n" + + "open class Foo: Bar { \n" + + " override public var foo: Int? { \n" + + " return 43 \n" + + " } \n" + + "}", + "open class Bar { \n" + + " open class func foo() -> Int { \n" + + " return 42 \n" + + " } \n" + + "} \n" + + "class Foo: Bar { \n" + + " override open class func foo() -> Int { \n" + + " return 43 \n" + + " } \n" + + "}", + "protocol Foo: class {} \n" + + "class Bar { \n" + + " public private(set) weak var foo: Foo? \n" + + "} \n", + "@objc \n" + + "public final class Foo: NSObject {} \n", + "@objcMembers \n" + + "public final class Foo: NSObject {} \n", + "@objc \n" + + "override public private(set) weak var foo: Bar? \n", + "@objc \n" + + "public final class Foo: NSObject {} \n", + "@objc \n" + + "open final class Foo: NSObject { \n" + + " open weak var weakBar: NSString? = nil \n" + + "}", + "public final class Foo {}", + "class Bar { \n" + + " func bar() {} \n" + + "}", + "internal class Foo: Bar { \n" + + " override internal func bar() {} \n" + + "}", + "public struct Foo { \n" + + " internal weak var weakBar: NSObject? = nil \n" + + "}", + "class Foo { \n" + + " internal lazy var bar: String = \"foo\" \n" + + "}" + ] + + static let triggeringExamples = [ + "class Foo { \n" + + " convenience required public init() {} \n" + + "}", + "public class Foo { \n" + + " static public let bar = 42 \n" + + "}", + "public class Foo { \n" + + " static public var bar: Int { \n" + + " return 42 \n" + + " } \n" + + "} \n", + "public class Foo { \n" + + " class public var bar: Int { \n" + + " return 42 \n" + + " } \n" + + "}", + "public class RootFoo { \n" + + " class public var foo: String { \n" + + " return \"foo\" \n" + + " } \n" + + "} \n" + + "public class Foo: RootFoo { \n" + + " override final class public var foo: String { \n" + + " return \"bar\" \n" + + " } \n" + + "}", + "open class Bar { \n" + + " public var foo: Int? { \n" + + " return 42 \n" + + " } \n" + + "} \n" + + "open class Foo: Bar { \n" + + " public override var foo: Int? { \n" + + " return 43 \n" + + " } \n" + + "}", + "protocol Foo: class {} \n" + + "class Bar { \n" + + " private(set) public weak var foo: Foo? \n" + + "} \n", + "open class Bar { \n" + + " open class func foo() -> Int { \n" + + " return 42 \n" + + " } \n" + + "} \n" + + "class Foo: Bar { \n" + + " class open override func foo() -> Int { \n" + + " return 43 \n" + + " } \n" + + "}", + "open class Bar { \n" + + " open class func foo() -> Int { \n" + + " return 42 \n" + + " } \n" + + "} \n" + + "class Foo: Bar { \n" + + " open override class func foo() -> Int { \n" + + " return 43 \n" + + " } \n" + + "}", + "@objc \n" + + "final public class Foo: NSObject {}", + "@objcMembers \n" + + "final public class Foo: NSObject {}", + "@objc \n" + + "final open class Foo: NSObject { \n" + + " weak open var weakBar: NSString? = nil \n" + + "}", + "final public class Foo {} \n", + "internal class Foo: Bar { \n" + + " internal override func bar() {} \n" + + "}", + "public struct Foo { \n" + + " weak internal var weakBar: NSObjetc? = nil \n" + + "}", + "class Foo { \n" + + " lazy internal var bar: String = \"foo\" \n" + + "}" + ] +} diff --git a/SwiftLint.xcodeproj/project.pbxproj b/SwiftLint.xcodeproj/project.pbxproj index 44bf72b8ef..b580e95edb 100644 --- a/SwiftLint.xcodeproj/project.pbxproj +++ b/SwiftLint.xcodeproj/project.pbxproj @@ -187,6 +187,7 @@ B89F3BCE1FD5EE0200931E59 /* RequiredEnumCaseRuleTestCase.swift in Sources */ = {isa = PBXBuildFile; fileRef = B89F3BCB1FD5EDA900931E59 /* RequiredEnumCaseRuleTestCase.swift */; }; B89F3BCF1FD5EE1400931E59 /* RequiredEnumCaseRuleConfiguration.swift in Sources */ = {isa = PBXBuildFile; fileRef = B89F3BC71FD5ED7D00931E59 /* RequiredEnumCaseRuleConfiguration.swift */; }; BB00B4E91F5216090079869F /* MultipleClosuresWithTrailingClosureRule.swift in Sources */ = {isa = PBXBuildFile; fileRef = BB00B4E71F5216070079869F /* MultipleClosuresWithTrailingClosureRule.swift */; }; + BC87573B2195CF2A00CA7A74 /* ModifierOrderRuleExamples.swift in Sources */ = {isa = PBXBuildFile; fileRef = BC8757392195CDD500CA7A74 /* ModifierOrderRuleExamples.swift */; }; BCB68283216213130078E4C3 /* CompilerProtocolInitRuleTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = BCB68282216213130078E4C3 /* CompilerProtocolInitRuleTests.swift */; }; BFF028AE1CBCF8A500B38A9D /* TrailingWhitespaceConfiguration.swift in Sources */ = {isa = PBXBuildFile; fileRef = BF48D2D61CBCCA5F0080BDAE /* TrailingWhitespaceConfiguration.swift */; }; C25EBBDF2107884200E27603 /* PrefixedTopLevelConstantRuleTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C25EBBDD210787B200E27603 /* PrefixedTopLevelConstantRuleTests.swift */; }; @@ -613,6 +614,7 @@ B89F3BC91FD5ED9000931E59 /* RequiredEnumCaseRule.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = RequiredEnumCaseRule.swift; sourceTree = ""; }; B89F3BCB1FD5EDA900931E59 /* RequiredEnumCaseRuleTestCase.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = RequiredEnumCaseRuleTestCase.swift; sourceTree = ""; }; BB00B4E71F5216070079869F /* MultipleClosuresWithTrailingClosureRule.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = MultipleClosuresWithTrailingClosureRule.swift; sourceTree = ""; }; + BC8757392195CDD500CA7A74 /* ModifierOrderRuleExamples.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ModifierOrderRuleExamples.swift; sourceTree = ""; }; BCB68282216213130078E4C3 /* CompilerProtocolInitRuleTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CompilerProtocolInitRuleTests.swift; sourceTree = ""; }; BF48D2D61CBCCA5F0080BDAE /* TrailingWhitespaceConfiguration.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = TrailingWhitespaceConfiguration.swift; sourceTree = ""; }; C25EBBDD210787B200E27603 /* PrefixedTopLevelConstantRuleTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PrefixedTopLevelConstantRuleTests.swift; sourceTree = ""; }; @@ -1056,6 +1058,7 @@ C946FEC91EAE5E20007DD778 /* LetVarWhitespaceRule.swift */, D4EA77C91F81FACC00C315FB /* LiteralExpressionEndIdentationRule.swift */, 188B3FF1207D61040073C2D6 /* ModifierOrderRule.swift */, + BC8757392195CDD500CA7A74 /* ModifierOrderRuleExamples.swift */, 82F614F32106015100D23904 /* MultilineArgumentsBracketsRule.swift */, B25DCD071F7E9B5F0028A199 /* MultilineArgumentsRule.swift */, B25DCD091F7E9BB50028A199 /* MultilineArgumentsRuleExamples.swift */, @@ -2047,6 +2050,7 @@ A73469421FB121BA009B57C7 /* CallPairRule.swift in Sources */, D47079AF1DFE520000027086 /* VoidReturnRule.swift in Sources */, B3935EE74B1E8E14FBD65E7F /* String+XML.swift in Sources */, + BC87573B2195CF2A00CA7A74 /* ModifierOrderRuleExamples.swift in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; diff --git a/Tests/LinuxMain.swift b/Tests/LinuxMain.swift index 426e0ace9a..5a915ac392 100644 --- a/Tests/LinuxMain.swift +++ b/Tests/LinuxMain.swift @@ -710,6 +710,7 @@ extension ModifierOrderTests { ("testAttributeTypeMethod", testAttributeTypeMethod), ("testRightOrderedModifierGroups", testRightOrderedModifierGroups), ("testAtPrefixedGroup", testAtPrefixedGroup), + ("testNonSpecifiedModifiersDontInterfere", testNonSpecifiedModifiersDontInterfere), ("testViolationMessage", testViolationMessage) ] } diff --git a/Tests/SwiftLintFrameworkTests/ModifierOrderTests.swift b/Tests/SwiftLintFrameworkTests/ModifierOrderTests.swift index 95ab3b0129..489b583f63 100644 --- a/Tests/SwiftLintFrameworkTests/ModifierOrderTests.swift +++ b/Tests/SwiftLintFrameworkTests/ModifierOrderTests.swift @@ -142,7 +142,64 @@ class ModifierOrderTests: XCTestCase { ) verifyRule(descriptionOverride, - ruleConfiguration: ["preferred_modifier_order": ["override", "acl", "final"]]) + ruleConfiguration: ["preferred_modifier_order": ["override", "acl", "owned", "final"]]) + } + + func testNonSpecifiedModifiersDontInterfere() { + let descriptionOverride = RuleDescription( + identifier: "modifier_order", + name: "Modifier Order", + description: "Modifier order should be consistent.", + kind: .style, + minSwiftVersion: .fourDotOne, + nonTriggeringExamples: [ + """ + class Foo { + weak final override private var bar: UIView? + } + """, + """ + class Foo { + final weak override private var bar: UIView? + } + """, + """ + class Foo { + final override weak private var bar: UIView? + } + """, + """ + class Foo { + final override private weak var bar: UIView? + } + """ + ], + triggeringExamples: [ + """ + class Foo { + weak override final private var bar: UIView? + } + """, + """ + class Foo { + override weak final private var bar: UIView? + } + """, + """ + class Foo { + override final weak private var bar: UIView? + } + """, + """ + class Foo { + override final private weak var bar: UIView? + } + """ + ] + ) + + verifyRule(descriptionOverride, + ruleConfiguration: ["preferred_modifier_order": ["final", "override", "acl"]]) } func testViolationMessage() {