apple / swift-metrics

Metrics API for Swift
https://apple.github.io/swift-metrics/
Apache License 2.0
651 stars 61 forks source link

Sanity script should check for missing `return` statements #73

Open MrLotU opened 4 years ago

MrLotU commented 4 years ago

Since Swift 5.2 (IIRC) you're allowed to omit the return statement in single statement functions and getters as such:

func foo() -> String {
    "bar"
}

Since we also support versions pre 5.2, the sanity script should check for this and enforce the use of return statements everywhere.

Expected behavior

The sanity script should check and correct missing return statements.

Actual behavior

Sanity script does not correct this

If possible, minimal yet complete reproducer code (or URL to code)

See #61 (Does not use return in a couple of places, did pass sanity script)

ktoso commented 4 years ago

Hmm... if it's possible to configure swift-format in some way which can catch this that we can enable I think. Do you know if that's possible?

Otherwise I'm not sure about the value/effort ratio; we build on 5.1 as well, so this build would fail on compilation if there's a missing return right?

Lukasa commented 4 years ago

I know that swift-format can remove the return statement on 5.2, so one imagines it can add it in 5.1 mode.

MrLotU commented 4 years ago

Yeah, not at all sure if this is possible, but something that would be helpful if it was. If not, than indeed CI failing on older Swift versions will catch it, but pre-catching things like this is always preferred IMO 😄

tomerd commented 4 years ago

+1 to deal with this in swift-format config, this will then be naturally integrated into the sanity check