Fix custom_rules merging when a configuration is based on only_rules

DwayneCoussement · Jan 14, 2021 · 5bffc77 · 5bffc77
1 parent e3e169b
commit 5bffc77
Show file tree

Hide file tree

Showing 6 changed files with 78 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -30,6 +30,11 @@
   being available (when using multiple configurations).  
   [Frederick Pietschmann](https://github.com/fredpi)
   [#3472](https://github.com/realm/SwiftLint/issues/3472)
+
+* Fix `custom_rules` merging when the parent configuration is based on `only_rules`.  
+  [Frederick Pietschmann](https://github.com/fredpi)
+  [#3468](https://github.com/realm/SwiftLint/issues/3468)
+
 
 ## 0.42.0: He Chutes, He Scores
 

diff --git a/...work/Extensions/Configuration+Rules.swift → ...tensions/Configuration+RulesWrapper.swift b/...work/Extensions/Configuration+Rules.swift → ...tensions/Configuration+RulesWrapper.swift
@@ -142,10 +142,24 @@ internal extension Configuration {
                     newAllRulesWrapped: newAllRulesWrapped,
                     child: child,
                     childDisabled: childDisabled,
-                    childOptIn: childOptIn
+                    childOptIn: childOptIn,
+                    validRuleIdentifiers: validRuleIdentifiers
                 )
 
             case var .only(childOnlyRules):
+                // If the custom_rules rule is enabled, add all rules defined by the child's custom_rules rule
+                if
+                    (childOnlyRules.contains { $0 == CustomRules.description.identifier }),
+                    let childCustomRulesRule = (child.allRulesWrapped.first { $0.rule is CustomRules })?.rule
+                        as? CustomRules
+                {
+                    childOnlyRules = childOnlyRules.union(
+                        Set(
+                            childCustomRulesRule.configuration.customRuleConfigurations.map { $0.identifier }
+                        )
+                    )
+                }
+
                 childOnlyRules = child.validate(ruleIds: childOnlyRules, valid: validRuleIdentifiers)
 
                 // Always use the child only rules
@@ -159,11 +173,7 @@ internal extension Configuration {
             // Assemble & return merged rules
             return RulesWrapper(
                 mode: newMode,
-                allRulesWrapped: mergedCustomRules(
-                    newAllRulesWrapped: newAllRulesWrapped,
-                    mode: newMode,
-                    with: child
-                ),
+                allRulesWrapped: mergedCustomRules(newAllRulesWrapped: newAllRulesWrapped, with: child),
                 aliasResolver: { child.aliasResolver(self.aliasResolver($0)) }
             )
         }
@@ -183,7 +193,7 @@ internal extension Configuration {
         }
 
         private func mergedCustomRules(
-            newAllRulesWrapped: [ConfigurationRuleWrapper], mode: RulesMode, with child: RulesWrapper
+            newAllRulesWrapped: [ConfigurationRuleWrapper], with child: RulesWrapper
         ) -> [ConfigurationRuleWrapper] {
             guard
                 let parentCustomRulesRule = (allRulesWrapped.first { $0.rule is CustomRules })?.rule
@@ -211,12 +221,13 @@ internal extension Configuration {
             newAllRulesWrapped: [ConfigurationRuleWrapper],
             child: RulesWrapper,
             childDisabled: Set<String>,
-            childOptIn: Set<String>
+            childOptIn: Set<String>,
+            validRuleIdentifiers: Set<String>
         ) -> RulesMode {
             let childDisabled = child.validate(ruleIds: childDisabled, valid: validRuleIdentifiers)
             let childOptIn = child.validate(ruleIds: childOptIn, valid: validRuleIdentifiers)
 
-            switch mode {
+            switch mode { // Switch parent's mode. Child is in default mode.
             case var .default(disabled, optIn):
                 disabled = validate(ruleIds: disabled, valid: validRuleIdentifiers)
                 optIn = child.validate(ruleIds: optIn, valid: validRuleIdentifiers)
@@ -236,6 +247,27 @@ internal extension Configuration {
                 )
 
             case var .only(onlyRules):
+                // Add identifiers of custom rules iff the custom_rules rule is enabled
+                if (onlyRules.contains { $0 == CustomRules.description.identifier }) {
+                    if let childCustomRulesRule = (child.allRulesWrapped.first { $0.rule is CustomRules })?.rule
+                        as? CustomRules {
+                        onlyRules = onlyRules.union(
+                            Set(
+                                childCustomRulesRule.configuration.customRuleConfigurations.map { $0.identifier }
+                            )
+                        )
+                    }
+
+                    if let parentCustomRulesRule = (self.allRulesWrapped.first { $0.rule is CustomRules })?.rule
+                        as? CustomRules {
+                        onlyRules = onlyRules.union(
+                            Set(
+                                parentCustomRulesRule.configuration.customRuleConfigurations.map { $0.identifier }
+                            )
+                        )
+                    }
+                }
+
                 onlyRules = validate(ruleIds: onlyRules, valid: validRuleIdentifiers)
 
                 // Allow parent only rules that weren't disabled via the child config

diff --git a/Tests/SwiftLintFrameworkTests/ConfigurationTests+Mock.swift b/Tests/SwiftLintFrameworkTests/ConfigurationTests+Mock.swift
@@ -38,8 +38,10 @@ internal extension ConfigurationTests {
             static var _0: String { Dir.level0.stringByAppendingPathComponent(Configuration.defaultFileName) }
             static var _0CustomPath: String { Dir.level0.stringByAppendingPathComponent("custom.yml") }
             static var _0CustomRules: String { Dir.level0.stringByAppendingPathComponent("custom_rules.yml") }
+            static var _0CustomRulesOnly: String { Dir.level0.stringByAppendingPathComponent("custom_rules_only.yml") }
             static var _2: String { Dir.level2.stringByAppendingPathComponent(Configuration.defaultFileName) }
             static var _2CustomRules: String { Dir.level2.stringByAppendingPathComponent("custom_rules.yml") }
+            static var _2CustomRulesOnly: String { Dir.level2.stringByAppendingPathComponent("custom_rules_only.yml") }
             static var _2CustomRulesDisabled: String {
                 Dir.level2.stringByAppendingPathComponent("custom_rules_disabled.yml")
             }
@@ -61,8 +63,10 @@ internal extension ConfigurationTests {
             static var _0: Configuration { Configuration(configurationFiles: []) }
             static var _0CustomPath: Configuration { Configuration(configurationFiles: [Yml._0CustomPath]) }
             static var _0CustomRules: Configuration { Configuration(configurationFiles: [Yml._0CustomRules]) }
+            static var _0CustomRulesOnly: Configuration { Configuration(configurationFiles: [Yml._0CustomRulesOnly]) }
             static var _2: Configuration { Configuration(configurationFiles: [Yml._2]) }
             static var _2CustomRules: Configuration { Configuration(configurationFiles: [Yml._2CustomRules]) }
+            static var _2CustomRulesOnly: Configuration { Configuration(configurationFiles: [Yml._2CustomRulesOnly]) }
             static var _2CustomRulesDisabled: Configuration {
                 Configuration(configurationFiles: [Yml._2CustomRulesDisabled])
             }

diff --git a/Tests/SwiftLintFrameworkTests/ConfigurationTests+MultipleConfigs.swift b/Tests/SwiftLintFrameworkTests/ConfigurationTests+MultipleConfigs.swift
@@ -103,6 +103,23 @@ extension ConfigurationTests {
         )
     }
 
+    func testCustomRulesMergingWithOnlyRules() {
+        let mergedConfiguration = Mock.Config._0CustomRulesOnly.merged(
+            withChild: Mock.Config._2CustomRulesOnly,
+            rootDirectory: ""
+        )
+        guard let mergedCustomRules = mergedConfiguration.rules.first(where: { $0 is CustomRules }) as? CustomRules
+            else {
+            return XCTFail("Custom rule are expected to be present")
+        }
+        XCTAssertTrue(
+            mergedCustomRules.configuration.customRuleConfigurations.contains(where: { $0.identifier == "no_abc" })
+        )
+        XCTAssertTrue(
+            mergedCustomRules.configuration.customRuleConfigurations.contains(where: { $0.identifier == "no_abcd" })
+        )
+    }
+
     // MARK: - Nested Configurations
     func testLevel0() {
         XCTAssertEqual(Mock.Config._0.configuration(for: SwiftLintFile(path: Mock.Swift._0)!),

diff --git a/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/Level1/Level2/custom_rules_only.yml b/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/Level1/Level2/custom_rules_only.yml
@@ -0,0 +1,4 @@
+custom_rules:
+  no_abcd:
+    name: "Don't use abcd"
+    regex: 'abcd'
diff --git a/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/custom_rules_only.yml b/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/custom_rules_only.yml
@@ -0,0 +1,7 @@
+only_rules:
+  - custom_rules
+
+custom_rules:
+  no_abc:
+    name: "Don't use abc"
+    regex: 'abc'