Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#2435 - Make modifier_order rule rely on an explicit set of rules #2458

Merged
merged 6 commits into from
Nov 28, 2018
Merged

#2435 - Make modifier_order rule rely on an explicit set of rules #2458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1690bec
#2435 - Adjust modifier_order rule to require explicit modifier order…
abdulowork Nov 14, 2018
826caab
#2435 - Move modifier order rule examples to a separate file
abdulowork Nov 14, 2018
b98d76b
#2435 - Add modifier interference tests
abdulowork Nov 15, 2018
03f8af6
#2435 - Fix whitespaces
abdulowork Nov 15, 2018
e29f1c6
Minor edits
jpsim Nov 28, 2018
0deadd1
Add changelog entry
jpsim Nov 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
252 changes: 75 additions & 177 deletions Source/SwiftLintFramework/Rules/Style/ModifierOrderRule.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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: [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this?

Copy link
Contributor Author

@abdulowork abdulowork Nov 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving the order as is would fix all of the violations where some other modifier preceded or was in-between the [.override, .acl] modifiers. Matching the prior behavior of the rule with this version was impossible so I just used the oss-check as a guideline to establish the modifier order. The order should definitely be made less strict if this change is desired.

.override,
.acl,
.setterACL,
.dynamic,
.mutators,
.lazy,
.final,
.required,
.convenience,
.typeMethods,
.owned
]
)

public init() {}

Expand All @@ -14,163 +25,8 @@ public struct ModifierOrderRule: ASTRule, OptInRule, ConfigurationProviderRule {
description: "Modifier order should be consistent.",
kind: .style,
minSwiftVersion: .fourDotOne ,
nonTriggeringExamples: [
Copy link
Contributor

@masters3d masters3d Nov 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be its. own commit? hard to see if any of the test cases changed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, moved to a separate commit. None of the test cases were changed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some non triggering test cases like the ones you described above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the examples

"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,
Expand All @@ -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)]
}
}
}

Expand All @@ -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
}
Expand All @@ -240,3 +133,8 @@ private extension String {
return components(separatedBy: charachter).last ?? ""
}
}

private struct ModifierDescription: Equatable {
let keyword: String
let group: SwiftDeclarationAttributeKind.ModifierGroup
}
Loading