nicklockwood / SwiftFormat

A command-line tool and Xcode Extension for formatting Swift code
MIT License
7.67k stars 623 forks source link

redundantReturn misses return with multiple Where clauses in Switch #1554

Closed bpdabrowski closed 8 months ago

bpdabrowski commented 9 months ago

Hello! My team and I were seeing the below issue in our codebase when using multiple where clauses in a switch statement. The redundantReturn rule seems to skip over and not remove the return statements when there are multiple where clauses like the failing example below. When there is only one where clause, the return statement is removed properly as seen in the succeeding example.

I added a couple of tests below based off of the tests in RulesTests+Redundancy to hopefully illustrate the issue a bit better. Thanks for your help 😃

The below test is an example showing the failing where clause that we seeing in our codebase.

func testRedundantSwitchStatementReturnInFunction_Fails() {
        let input = """
        func foo(cases: FooCases, count: Int) -> String? {
            switch cases {
            case .fooCase1 where count == 0:
                return "foo"
            case .fooCase2 where count < 100,
                 .fooCase3 where count < 100, // 🛑 the double where clauses seem to be causing this return to be skipped by the redundantReturn rule.
                 .fooCase4:
                return "bar"
            default:
                return nil
            }
        }
        """
        let output = """
        func foo(cases: FooCases, count: Int) -> String? {
            switch cases {
            case .fooCase1 where count == 0:
                "foo"
            case .fooCase2 where count < 100,
                 .fooCase3 where count < 100,
                 .fooCase4:
                "bar"
            default:
                nil
            }
        }
        """
        let options = FormatOptions(swiftVersion: "5.9")
        testFormatting(for: input, output, rule: FormatRules.redundantReturn, options: options)
    }

Failing test output:

XCTAssertEqual failed: ("func foo(cases: FooCases, count: Int) -> String? {
    switch cases {
    case .fooCase1 where count == 0:
        "foo"
    case .fooCase2 where count < 100,
         .fooCase3 where count < 100,
         .fooCase4:
        return "bar" // 🛑 Missed this return.
    default:
        nil
    }
}") is not equal to ("func foo(cases: FooCases, count: Int) -> String? {
    switch cases {
    case .fooCase1 where count == 0:
        "foo"
    case .fooCase2 where count < 100,
         .fooCase3 where count < 100,
         .fooCase4:
        "bar"
    default:
        nil
    }
}")

The below test shows a succeeding test with a single where clause where it properly removes the return statement from all cases. Wondering if its possible to have this functionality for multiple where clauses.

    func testRedundantSwitchStatementReturnInFunction_Succeeds() {
        let input = """
        func anotherFoo(cases: FooCases, count: Int) -> String? {
            switch cases {
            case .fooCase1 where count == 0:
                return "foo"
            case .fooCase2 where count < 100, // 👈 Only a single where clause here.
                 .fooCase4:
                return "bar"
            default:
                return nil
            }
        }
        """
        let output = """
        func anotherFoo(cases: FooCases, count: Int) -> String? {
            switch cases {
            case .fooCase1 where count == 0:
                "foo"
            case .fooCase2 where count < 100,
                 .fooCase4:
                "bar" // ✅ Removed the return here.
            default:
                nil
            }
        }
        """
        let options = FormatOptions(swiftVersion: "5.9")
        testFormatting(for: input, output, rule: FormatRules.redundantReturn, options: options)
    }
nicklockwood commented 8 months ago

@calda if you could take a look at this one it would be appreciated! (Don't worry if not - I've no reason to think it's your code that's at fault, I just can't face it right now 😅)

calda commented 8 months ago

This is a bug in the conditionalBranches(at:) / switchStatementBranches(at:) code I had written. Will post a fix, thanks for tagging me.

calda commented 8 months ago

Here's a fix: https://github.com/nicklockwood/SwiftFormat/pull/1577

This actually turned out to be an issue in the tokenizer rather than in switchStatementBranches itself.

nicklockwood commented 8 months ago

@bpdabrowski fixed in 0.52.9