Fix superfluous disable command for custom_rules

[mildm8nnered]

So this is a kind of scary PR - addresses #4754 - The superfluous_disable_command rule should work for individual custom_rules.

#4755 attempted to address this, but had issues, and was reverted in #5336

This PR is based on @marcelofabri 's work in #4755 with a few additional fixes.

A fair few times I thought I had this cracked, only to find a big hole. I think it's right now, but this PR definitely deserves additional scrutiny.

Consider the example below:

custom_rules:
  forbidden:
    regex: 'FORBIDDEN'
    message: "This is forbidden"
    severity: warning

The following code should trigger a superfluous_disable_command violation:

let FORBIDDEN = 1
// swiftlint:disable:next forbidden
let ALLOWED = 2

Today this would produce:

% swiftlint --config t.config t.swift --quiet
/Some/Path/To/t.swift:1:5: warning: forbidden Violation: This is forbidden (forbidden)

and after this PR is applied, we get:

% swiftlint.debug --config t.config t.swift --quiet
/Some/Path/To/t.swift:1:5: warning: forbidden Violation: This is forbidden (forbidden)
/Some/Path/To/t.swift:3:1: warning: Superfluous Disable Command Violation: SwiftLint rule 'forbidden' did not trigger a violation in the disabled region; remove the disable command (superfluous_disable_command)

It is currently also possible to disable all custom rules with:

// swiftlint:disable:next custom_rules
let FORBIDDEN = 1

That behaviour is still supported.

[SwiftLintBot]

	17 Messages
📖	Linting Aerial with this PR took 0.95s vs 0.93s on main (2% slower)
📖	Linting Alamofire with this PR took 1.27s vs 1.28s on main (0% faster)
📖	Linting Brave with this PR took 7.46s vs 7.44s on main (0% slower)
📖	Linting DuckDuckGo with this PR took 4.87s vs 4.89s on main (0% faster)
📖	Linting Firefox with this PR took 10.94s vs 10.97s on main (0% faster)
📖	Linting Kickstarter with this PR took 10.05s vs 10.08s on main (0% faster)
📖	Linting Moya with this PR took 0.52s vs 0.53s on main (1% faster)
📖	Linting NetNewsWire with this PR took 2.55s vs 2.55s on main (0% slower)
📖	Linting Nimble with this PR took 0.76s vs 0.76s on main (0% slower)
📖	Linting PocketCasts with this PR took 8.57s vs 8.7s on main (1% faster)
📖	Linting Quick with this PR took 0.43s vs 0.43s on main (0% slower)
📖	Linting Realm with this PR took 4.68s vs 4.69s on main (0% faster)
📖	Linting Sourcery with this PR took 2.34s vs 2.34s on main (0% slower)
📖	Linting Swift with this PR took 4.57s vs 4.57s on main (0% slower)
📖	Linting VLC with this PR took 1.25s vs 1.25s on main (0% slower)
📖	Linting Wire with this PR took 18.23s vs 18.25s on main (0% faster)
📖	Linting WordPress with this PR took 11.82s vs 11.79s on main (0% slower)

Generated by 🚫 Danger

[SimplyDanny]

Does this mean a custom rule custom_1 will be disabled if custom_2 is actually named in the disable command? rule.customRuleIdentifiers are all custom identifiers defined, aren't they?

[mildm8nnered]

So this specific function will return true in that case, yes, but the actual identifiers will be checked afterwards, here in the construction of identifiersWithRegions

    func superfluousDisableCommandViolations(regions: [Region],
                                             superfluousDisableCommandRule: SuperfluousDisableCommandRule?,
                                             allViolations: [StyleViolation]) -> [StyleViolation] {
        guard regions.isNotEmpty, let superfluousDisableCommandRule else {
            return []
        }

        let regionsDisablingSuperfluousDisableRule = regions.filter { region in
            region.isRuleDisabled(superfluousDisableCommandRule)
        }

        let identifiersWithRegions: [(String, [Region])] = {

The proof of the pudding 🤞 is (slightly expanded for clarity):

    func testUnviolatedSpecificCustomRulesTriggersSuperfluousDisableCommand() throws {
        let customRules =         [
            "forbidden": [
                "regex": "FORBIDDEN",
            ],
            "forbidden2": [
                "regex": "FORBIDDEN2",
            ],
        ]

        let example = Example("""
                              // swiftlint:disable:next forbidden forbidden2
                              let FORBIDDEN = 1
                              """)

        let violations = try self.violations(forExample: example, customRules: customRules)
        XCTAssertEqual(violations.count, 1)
        XCTAssertTrue(violations[0].isSuperfluousDisableCommandViolation(for: "forbidden2"))
    }

[mildm8nnered]

I'm wondering if a naming change might make this clearer, but I don't have a good candidate ...

[SimplyDanny]

I still have my concerns with the special handling of custom rules and some improvement ideas in my backlog.

Do you mind if I push some changes to your branch later?

[mildm8nnered]

I still have my concerns with the special handling of custom rules and some improvement ideas in my backlog.

Do you mind if I push some changes to your branch later?

Yes, I think there are still some oddities outside of superfluous_disable

Feel free to add more changes!

[SimplyDanny]

I managed to avoid explicit type checks at a few places. There are more in other (untouched) code, though, that we should get rid of in the long run.

Please check if my changes make sense to you, @mildm8nnered.

[SimplyDanny]

Perhaps we could get rid of this by letting Self.description.allIdentifiers contain the custom IDs in case of custom rules.

[mildm8nnered]

I couldn't see an easy way to customize allIdentifiers, as the RuleDescription currently can't see the configuration :-(

[SimplyDanny]

Yeah, also what I struggled with. We'd need to move allIdentifiers into Rule, but this has other disadvantages. So let's not bother with that in this PR. I think it's good as it is. The special handling of custom rules is a broader topic we may address in follow-ups.

[mildm8nnered]

I managed to avoid explicit type checks at a few places. There are more in other (untouched) code, though, that we should get rid of in the long run.

Please check if my changes make sense to you, @mildm8nnered.

So it may take me a while to digest those, but straight away it looks a lot shorter and cleaner.

One change request - I think you should add yourself to the authors in the changelog.

[mildm8nnered]

I managed to avoid explicit type checks at a few places. There are more in other (untouched) code, though, that we should get rid of in the long run.
Please check if my changes make sense to you, @mildm8nnered.

So it may take me a while to digest those, but straight away it looks a lot shorter and cleaner.

One change request - I think you should add yourself to the authors in the changelog.

I think your changes look great @SimplyDanny. Am I ok to hit merge on this? Hopefully we'll do better than #4755 🤞

[SimplyDanny]

Am I ok to hit merge on this?

Oh, I misread this as a statement. Just merged it. 🫣

Hopefully we'll do better than #4755 🤞

People will let us know ... 😅