Revert "Merge pull request #3473 from realm/feature/3472-fix-custom-r…

…ules-merging" (#3503) This reverts commit 537e53f, reversing changes made to ba49f7d.
realm · Jan 22, 2021 · 6a38b15 · 6a38b15
1 parent 6de5771
commit 6a38b15
Show file tree

Hide file tree

Showing 7 changed files with 45 additions and 154 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -27,21 +27,6 @@
 
 #### Bug Fixes
 
-* 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)
-
-* Fix misleading warnings about rules defined in the `custom_rules` not
-  being available (when using multiple configurations).  
-  [Frederick Pietschmann](https://github.com/fredpi)
-  [#3472](https://github.com/realm/SwiftLint/issues/3472)
-
-* Fix bug that prevented the reconfiguration of a custom rule in a child
-  config.  
-  [Frederick Pietschmann](https://github.com/fredpi)
-  [#3477](https://github.com/realm/SwiftLint/issues/3477)
-
 * Fix typos in configuration options for `file_name` rule.  
   [advantis](https://github.com/advantis)
 

diff --git a/...tensions/Configuration+RulesWrapper.swift → ...work/Extensions/Configuration+Rules.swift b/...tensions/Configuration+RulesWrapper.swift → ...work/Extensions/Configuration+Rules.swift
@@ -32,22 +32,18 @@ internal extension Configuration {
             if let cachedResultingRules = cachedResultingRules { return cachedResultingRules }
 
             // Calculate value
-            let customRulesFilter: (RegexConfiguration) -> (Bool)
             var resultingRules = [Rule]()
             switch mode {
             case .allEnabled:
-                customRulesFilter = { _ in true }
                 resultingRules = allRulesWrapped.map { $0.rule }
 
             case var .only(onlyRulesRuleIdentifiers):
-                customRulesFilter = { onlyRulesRuleIdentifiers.contains($0.identifier) }
                 onlyRulesRuleIdentifiers = validate(ruleIds: onlyRulesRuleIdentifiers, valid: validRuleIdentifiers)
                 resultingRules = allRulesWrapped.filter { tuple in
                     onlyRulesRuleIdentifiers.contains(type(of: tuple.rule).description.identifier)
                 }.map { $0.rule }
 
             case var .default(disabledRuleIdentifiers, optInRuleIdentifiers):
-                customRulesFilter = { !disabledRuleIdentifiers.contains($0.identifier) }
                 disabledRuleIdentifiers = validate(ruleIds: disabledRuleIdentifiers, valid: validRuleIdentifiers)
                 optInRuleIdentifiers = validate(ruleIds: optInRuleIdentifiers, valid: validRuleIdentifiers)
                 resultingRules = allRulesWrapped.filter { tuple in
@@ -57,13 +53,6 @@ internal extension Configuration {
                 }.map { $0.rule }
             }
 
-            // Filter custom rules
-            if var customRulesRule = (resultingRules.first { $0 is CustomRules }) as? CustomRules {
-                customRulesRule.configuration.customRuleConfigurations =
-                    customRulesRule.configuration.customRuleConfigurations.filter(customRulesFilter)
-                resultingRules = resultingRules.filter { !($0 is CustomRules) } + [customRulesRule]
-            }
-
             // Sort by name
             resultingRules = resultingRules.sorted {
                 type(of: $0).description.identifier < type(of: $1).description.identifier
@@ -142,22 +131,10 @@ internal extension Configuration {
                     newAllRulesWrapped: newAllRulesWrapped,
                     child: child,
                     childDisabled: childDisabled,
-                    childOptIn: childOptIn,
-                    validRuleIdentifiers: validRuleIdentifiers
+                    childOptIn: childOptIn
                 )
 
             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
@@ -168,17 +145,21 @@ internal extension Configuration {
                 newMode = .allEnabled
             }
 
-            // Assemble & return merged rules
+            // Assemble & return merged Rules
             return RulesWrapper(
                 mode: newMode,
-                allRulesWrapped: mergedCustomRules(newAllRulesWrapped: newAllRulesWrapped, with: child),
+                allRulesWrapped: merged(
+                    customRules: newAllRulesWrapped,
+                    mode: newMode,
+                    with: child
+                ),
                 aliasResolver: { child.aliasResolver(self.aliasResolver($0)) }
             )
         }
 
-        private func mergedAllRulesWrapped(with child: RulesWrapper) -> [ConfigurationRuleWrapper] {
+        private func mergedAllRulesWrapped(with sub: RulesWrapper) -> [ConfigurationRuleWrapper] {
             let mainConfigSet = Set(allRulesWrapped.map(HashableConfigurationRuleWrapperWrapper.init))
-            let childConfigSet = Set(child.allRulesWrapped.map(HashableConfigurationRuleWrapperWrapper.init))
+            let childConfigSet = Set(sub.allRulesWrapped.map(HashableConfigurationRuleWrapperWrapper.init))
             let childConfigRulesWithConfig = childConfigSet.filter {
                 $0.configurationRuleWrapper.initializedWithNonEmptyConfiguration
             }
@@ -190,44 +171,54 @@ internal extension Configuration {
                 .map { $0.configurationRuleWrapper }
         }
 
-        private func mergedCustomRules(
-            newAllRulesWrapped: [ConfigurationRuleWrapper], with child: RulesWrapper
+        private func merged(
+            customRules rules: [ConfigurationRuleWrapper], mode: RulesMode, with child: RulesWrapper
         ) -> [ConfigurationRuleWrapper] {
             guard
-                let parentCustomRulesRule = (allRulesWrapped.first { $0.rule is CustomRules })?.rule
-                    as? CustomRules,
-                let childCustomRulesRule = (child.allRulesWrapped.first { $0.rule is CustomRules })?.rule
-                    as? CustomRules
+                let customRulesRule = (allRulesWrapped.first {
+                    $0.rule is CustomRules
+                })?.rule as? CustomRules,
+                let childCustomRulesRule = (child.allRulesWrapped.first {
+                    $0.rule is CustomRules
+                })?.rule as? CustomRules
             else {
                 // Merging is only needed if both parent & child have a custom rules rule
-                return newAllRulesWrapped
+                return rules
+            }
+
+            let customRulesFilter: (RegexConfiguration) -> (Bool)
+            switch mode {
+            case .allEnabled:
+                customRulesFilter = { _ in true }
+
+            case let .only(onlyRules):
+                customRulesFilter = { onlyRules.contains($0.identifier) }
+
+            case let .default(disabledRules, _):
+                customRulesFilter = { !disabledRules.contains($0.identifier) }
             }
 
-            // Create new custom rules rule, prioritizing child custom rules
             var configuration = CustomRulesConfiguration()
-            configuration.customRuleConfigurations = childCustomRulesRule.configuration.customRuleConfigurations
-                + parentCustomRulesRule.configuration.customRuleConfigurations.filter { parentConfig in
-                    !childCustomRulesRule.configuration.customRuleConfigurations.contains { childConfig in
-                        childConfig.identifier == parentConfig.identifier
-                    }
-                }
-            var newCustomRulesRule = CustomRules()
-            newCustomRulesRule.configuration = configuration
+            configuration.customRuleConfigurations = Set(customRulesRule.configuration.customRuleConfigurations)
+                .union(Set(childCustomRulesRule.configuration.customRuleConfigurations))
+                .filter(customRulesFilter)
+
+            var customRules = CustomRules()
+            customRules.configuration = configuration
 
-            return newAllRulesWrapped.filter { !($0.rule is CustomRules) } + [(newCustomRulesRule, true)]
+            return rules.filter { !($0.rule is CustomRules) } + [(customRules, true)]
         }
 
         private func mergeDefaultMode(
             newAllRulesWrapped: [ConfigurationRuleWrapper],
             child: RulesWrapper,
             childDisabled: Set<String>,
-            childOptIn: Set<String>,
-            validRuleIdentifiers: Set<String>
+            childOptIn: Set<String>
         ) -> RulesMode {
             let childDisabled = child.validate(ruleIds: childDisabled, valid: validRuleIdentifiers)
             let childOptIn = child.validate(ruleIds: childOptIn, valid: validRuleIdentifiers)
 
-            switch mode { // Switch parent's mode. Child is in default mode.
+            switch mode {
             case var .default(disabled, optIn):
                 disabled = validate(ruleIds: disabled, valid: validRuleIdentifiers)
                 optIn = child.validate(ruleIds: optIn, valid: validRuleIdentifiers)
@@ -247,27 +238,6 @@ 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,16 +38,11 @@ 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")
             }
-            static var _2CustomRulesReconfig: String {
-                Dir.level2.stringByAppendingPathComponent("custom_rules_reconfig.yml")
-            }
             static var _3: String { Dir.level3.stringByAppendingPathComponent(Configuration.defaultFileName) }
             static var nested: String { Dir.nested.stringByAppendingPathComponent(Configuration.defaultFileName) }
         }
@@ -66,16 +61,11 @@ 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])
             }
-            static var _2CustomRulesReconfig: Configuration {
-                Configuration(configurationFiles: [Yml._2CustomRulesReconfig])
-            }
             static var _3: Configuration { Configuration(configurationFiles: [Yml._3]) }
             static var nested: Configuration { Configuration(configurationFiles: [Yml.nested]) }
         }

diff --git a/Tests/SwiftLintFrameworkTests/ConfigurationTests+MultipleConfigs.swift b/Tests/SwiftLintFrameworkTests/ConfigurationTests+MultipleConfigs.swift
@@ -76,13 +76,13 @@ extension ConfigurationTests {
         )
         guard let mergedCustomRules = mergedConfiguration.rules.first(where: { $0 is CustomRules }) as? CustomRules
             else {
-            return XCTFail("Custom rules are expected to be present")
+            return XCTFail("Custom rule are expected to be present")
         }
         XCTAssertTrue(
-            mergedCustomRules.configuration.customRuleConfigurations.contains { $0.identifier == "no_abc" }
+            mergedCustomRules.configuration.customRuleConfigurations.contains(where: { $0.identifier == "no_abc" })
         )
         XCTAssertTrue(
-            mergedCustomRules.configuration.customRuleConfigurations.contains { $0.identifier == "no_abcd" }
+            mergedCustomRules.configuration.customRuleConfigurations.contains(where: { $0.identifier == "no_abcd" })
         )
     }
 
@@ -93,54 +93,16 @@ extension ConfigurationTests {
         )
         guard let mergedCustomRules = mergedConfiguration.rules.first(where: { $0 is CustomRules }) as? CustomRules
             else {
-            return XCTFail("Custom rules are expected to be present")
+            return XCTFail("Custom rule are expected to be present")
         }
         XCTAssertFalse(
-            mergedCustomRules.configuration.customRuleConfigurations.contains { $0.identifier == "no_abc" }
+            mergedCustomRules.configuration.customRuleConfigurations.contains(where: { $0.identifier == "no_abc" })
         )
         XCTAssertTrue(
-            mergedCustomRules.configuration.customRuleConfigurations.contains { $0.identifier == "no_abcd" }
+            mergedCustomRules.configuration.customRuleConfigurations.contains(where: { $0.identifier == "no_abcd" })
         )
     }
 
-    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 rules are expected to be present")
-        }
-        XCTAssertTrue(
-            mergedCustomRules.configuration.customRuleConfigurations.contains { $0.identifier == "no_abc" }
-        )
-        XCTAssertTrue(
-            mergedCustomRules.configuration.customRuleConfigurations.contains { $0.identifier == "no_abcd" }
-        )
-    }
-
-    func testCustomRulesReconfiguration() {
-        // Custom Rule severity gets reconfigured to "error"
-        let mergedConfiguration = Mock.Config._0CustomRulesOnly.merged(
-            withChild: Mock.Config._2CustomRulesReconfig,
-            rootDirectory: ""
-        )
-        guard let mergedCustomRules = mergedConfiguration.rules.first(where: { $0 is CustomRules }) as? CustomRules
-            else {
-            return XCTFail("Custom rules are expected to be present")
-        }
-        XCTAssertEqual(
-            mergedCustomRules.configuration.customRuleConfigurations.filter { $0.identifier == "no_abc" }.count, 1
-        )
-        guard let customRule = (mergedCustomRules.configuration.customRuleConfigurations.first {
-            $0.identifier == "no_abc"
-        }) else {
-            return XCTFail("Custom rule is expected to be present")
-        }
-        XCTAssertEqual(customRule.severityConfiguration.severity, .error)
-    }
-
     // 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
diff --git a/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/Level1/Level2/custom_rules_reconfig.yml b/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/Level1/Level2/custom_rules_reconfig.yml
diff --git a/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/custom_rules_only.yml b/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/custom_rules_only.yml