Add child and parent config configuration features

[fredpi]

This PR closes #1352 by introducing a new sub_config option that can be used in any configuration file. The sub_config must reference a file on the same level so that the option isn't confused with nested configurations. The sub_config feature also allows a sub_config to itself specify a sub_config. There's also a reference cycle detection.

---

Scroll down for an updated version of what this PR implements.

---

I'd love to see this merged as soon as possible, as I'm eagerly waiting to use this feature.

[SwiftLintBot]

	1 Warning
⚠️	Big PR

	15 Messages
📖	Linting Aerial with this PR took 1.17s vs 1.19s on master (1% faster)
📖	Linting Alamofire with this PR took 2.19s vs 2.2s on master (0% faster)
📖	Linting Firefox with this PR took 8.9s vs 8.96s on master (0% faster)
📖	Linting Kickstarter with this PR took 14.38s vs 14.65s on master (1% faster)
📖	Linting Moya with this PR took 1.12s vs 1.18s on master (5% faster)
📖	Linting Nimble with this PR took 1.29s vs 1.35s on master (4% faster)
📖	Linting Quick with this PR took 0.53s vs 0.53s on master (0% slower)
📖	Linting Realm with this PR took 2.32s vs 2.34s on master (0% faster)
📖	Linting SourceKitten with this PR took 0.98s vs 1.06s on master (7% faster)
📖	Linting Sourcery with this PR took 6.59s vs 6.71s on master (1% faster)
📖	Linting Swift with this PR took 12.17s vs 12.28s on master (0% faster)
📖	Linting WordPress with this PR took 14.24s vs 14.67s on master (2% faster)
📖	This PR fixed a violation in Moya: /usr/local/var/buildkite-agent/builds/coastalelectricgaserver-com-1/swiftlint/swiftlint/osscheck/Moya/Tests/Observable+MoyaSpec.swift:598:1: warning: File Line Length Violation: File should contain 400 lines or less: currently contains 598 (file_length)
📖	This PR fixed a violation in Moya: /usr/local/var/buildkite-agent/builds/coastalelectricgaserver-com-1/swiftlint/swiftlint/osscheck/Moya/Tests/SignalProducer+MoyaSpec.swift:575:1: warning: File Line Length Violation: File should contain 400 lines or less: currently contains 575 (file_length)
📖	This PR fixed a violation in Moya: /usr/local/var/buildkite-agent/builds/coastalelectricgaserver-com-1/swiftlint/swiftlint/osscheck/Moya/Tests/Single+MoyaSpec.swift:585:1: warning: File Line Length Violation: File should contain 400 lines or less: currently contains 585 (file_length)

Generated by 🚫 Danger

[fredpi]

Just a note regarding the changes / fixes to rule configuration: As SwiftLintBot noted, with this new implementation, 3 warnings in Moya are now fixed. In fact, previously those have been false positives, as in the nested .swiftlint.yml covering these files there's the following configuration for the file_length rule:

file_length:
  warning: 1000

As the parent rule configuration was used for merging and there was no smart mechanism to decide which configuration to choose, the configuration originating from the nested configuration was ignored. With this PR, that's now fixed.

[jpsim]

Thanks for kicking this off @fredpi! This is a very commonly requested feature. I haven't taken a close look just yet, but I can comment on the two failing CI jobs.

The first is because the Foundation import in RulesStorage.swift is unused. You can just remove that import.

The second failing job is TSan detecting a data race because RulesStorage.resultingRules is being mutated and read from different threads without any coordination:

WARNING: ThreadSanitizer: data race (pid=3478)
  Write of size 8 at 0x7b18000582a8 by thread T6:
    #0 RulesStorage.resultingRules.getter <compiler-generated> (SwiftLintPackageTests:x86_64+0x23437a)
    #1 Configuration.rules.getter Configuration.swift:26 (SwiftLintPackageTests:x86_64+0x1d65b2)
    #2 Linter.init(file:configuration:cache:compilerArguments:) Linter.swift:126 (SwiftLintPackageTests:x86_64+0x1f0490)
    #3 closure #1 in IntegrationTests.testSwiftLintAutoCorrects() IntegrationTests.swift:36 (SwiftLintPackageTests:x86_64+0x80e41f)
    #4 partial apply for closure #1 in IntegrationTests.testSwiftLintAutoCorrects() <compiler-generated> (SwiftLintPackageTests:x86_64+0x80e4e8)
    #5 thunk for @callee_guaranteed (@guaranteed File) -> (@owned [Correction]) <compiler-generated> (SwiftLintPackageTests:x86_64+0x80e55b)
    #6 partial apply for thunk for @callee_guaranteed (@guaranteed File) -> (@owned [Correction]) <compiler-generated> (SwiftLintPackageTests:x86_64+0x80e600)
    #7 thunk for @callee_guaranteed (@in_guaranteed A) -> (@owned [A1]) <compiler-generated> (SwiftLintPackageTests:x86_64+0x16d563)
    #8 partial apply for thunk for @callee_guaranteed (@in_guaranteed A) -> (@owned [A1]) <compiler-generated> (SwiftLintPackageTests:x86_64+0x16d655)
    #9 closure #1 in closure #1 in Array.parallelMap<A>(transform:) Array+SwiftLint.swift:60 (SwiftLintPackageTests:x86_64+0x16ea1d)
    #10 partial apply for closure #1 in closure #1 in Array.parallelMap<A>(transform:) <compiler-generated> (SwiftLintPackageTests:x86_64+0x16f2ab)
    #11 partial apply for thunk for @callee_guaranteed (@unowned Int) -> () <null>:2670432 (libswiftDispatch.dylib:x86_64+0xeaf0)
    #12 _dispatch_client_callout2 <null>:2670432 (libdispatch.dylib:x86_64+0x3671)

You can run this yourself locally with make test_tsan.

[fredpi]

@jpsim Thanks for giving me a direction how to make the tests work - they are all passing now 👍

[jpsim]

Thinking through this feature a bit, I wonder if we should consider how this could live alongside #2729.

Can we align these to use the same mechanism for pulling in external (sub)configurations?

Seems like we should be able to align them, and maybe even come up with a single unified solution.

[acecilia]

Would it be attainable in this PR to pass multiple swiftlint configuration files through the command line?

[fredpi]

@jpsim @acecilia Sorry for my late reply ;)

Your suggestions to combine this feature with a parent_config configuration and multiple configuration files via the command line sound reasonable and I will try to implement them.

Graph

However, this may require more refactoring, as all configuration files used to build the final configuration and their relationship must be stored, resulting in a graph that has some requirements to represent a valid configuration:

• The graph must be cycle-free. This is what this PR already implemented: It's impossible to create cycles in sub_config references. This behavior must now be extended to the new configuration options.
• Each vertex must be the origin of either zero edges or one edge, and the target of either zero edges or one edge, meaning it's not possible to have multiple sub_config references. Also, a file that specifies a parent_config shall not be referenced as a sub_config from somewhere else, as the vertex representing the config would then be the target of two edges. One could circumvent this restriction by defining a precedence (e. g. considering parent_config earlier than sub_config), but I guess that's misleading.

Configuration Interferences

How would you handle interferences between the sub_config / parent_config / command line configurations: If a user passes config1.yml config2.yml config3.yml via the command line, should we ignore sub_config / parent_config references within these files? I'm rather in favor of combining all configurations, checking for validity and breaking without any recovering mechanism if the resulting graph is invalid. At least, this is what the current implementation does when it detects cycles...

Nested Configurations

Another issue comes with nested configurations: Consider a setup with the two configurations A, B on the root file level and configuration C one level deeper. A has B as its sub_config, C has B as its parent_config. Now, with the current implementation, a nested sub config is different to a sub_config: It's merged after the top-level-config has been fully merged already (instead of collecting everything and merging thereafter). Still, the original configs used to merge the top-level-config must be referenced, so that we can check a nested config doesn't reference a config used to build the top-level-config.

Previously, this could be ensured by only allowing sub_config references in the same directory, but I guess it's also reasonable to allow references in different directories and then check that the nested configuration has not been used to build the top-level config already.

[jpsim]

I'm assuming you're still working through the CI failures, let me know if you'd like some help.

[fredpi]

@jpsim Thanks for offering help – I'm still in the process of completely rearranging this PR to also allow for parent configs, multiple configs via the command line and remote configs. This will still take some time and I will drop a notice when I think I'm done ;)

[fredpi]

@jpsim @acecilia

This PR is approaching its completion – only tests (+ bugfixes?) are missing as of now

Change Overview

• Using multiple configurations is now documented extensively in the README
• You can now specify child_config / parent_config configurations. This even works recursively, as long as there are no cycles and no ambiguities.
• Nested configurations got a speed bump (via caching) and of course now ignore configs that were already used to build the primary config (using child_config / parent_config references)
• You can pass multiple configurations via the command line that are interpreted as a hierarchy
• You can specify a remote child_config / parent_config! This is just easy as dropping in a url that starts with http://or https:// where normally there would be the path to a local file. Remote configs get cached (in a manner that git doesn't see it). There are timeouts to loading remote configs, that can also be customized using remote_timeout / remote_timeout_if_cached
• Rule Configurations now get merged properly (Previously, the rule configurations from the parent configuration were always used). This change results in the following behavior:
  • When a sub config doesn't configure a rule, yet the parent config does, the parent config will be used
  • When a sub config configures a rule, and the parent doesn't, the sub config will be used
  • When a sub config configures a rule, and the parent does, too, the sub config will be used
• Warnings about duplicate and invalid identifiers now appear as actual warnings in Xcode
• Internally, the whole Configuration clutter has been cleaned up in a way that I hope everyone is happy with. I strongly felt like the current merging implementation that relied on a rules array and a rulesMode didn't express the very foundation of what a configuration is: A mode that is applied to an array of all available rules with configurations, resulting in an array of rules that should be linted. To express this very relation, I created the new RulesWrapper type. While offering a resultingRules variable that is lazily computed, it still retains what caused these rules and therefore eases understanding of the merging algorithm.

[fredpi]

Closing this, as I created a new PR for these changes: #3058