The redundant_type_annotation rule does more harm than good

[regexident]

New Issue Checklist

• Updated SwiftLint to the latest version
• I searched for existing GitHub issues

Describe the bug

A clear and concise description of what the bug is.

Complete output when running SwiftLint, including the stack trace and command used

$ swiftlint lint

Environment

• SwiftLint version: 0.45.0
• Installation method used: brew install swiftlint
• Paste your configuration file:

opt_in_rules:
  - redundant_type_annotation

• Are you using nested configurations?
  No.

• Which Xcode version are you using (check xcodebuild -version)?

  Xcode 12.5.1
  Build version 12E507
  

• Do you have a sample that shows the issue? Run echo "[string here]" | swiftlint lint --no-cache --use-stdin --enable-all-rules
  to quickly test if your example is really demonstrating the issue. If your example is more
  complex, you can use swiftlint lint --path [file here] --no-cache --enable-all-rules.

struct Foo {}

struct SomeType{
    // This performs a wrong fix:
    public var foo: Foo = Foo()
    // This performs a fix, but really shouldn't:
    public var bar: Bool = true
    public var baz: Int = 42
    public var blee: String = "blee"
}

Expected result

struct Foo {}

struct SomeType {
    public var foo: Foo = .init()
    public var bar: Bool = true
    public var baz: Int = 42
    public var blee: String = "blee"
}

Actual result

struct Foo {}

struct SomeType {
    public var foo = Foo()
    public var bar = true
    public var baz: Int = 42
    public var blee: String = "blee"
}

The "fix" made the code less consistent: Some properties now have an explicit type, others don't. As a reader of that code you now have to parse each line, adapting to explicitly typed vs. implicitly inferred syntaxes on a line-by-line basis. And the selection seems arbitrary, too, given that both, 42 and "blee" could just as well have been type-inferred to Int and String. Not to mention Double, [T], Set<T>, [T: U], …

"But I think it just looks prettier" is never a good reason for picking a less correct/robust approach.

To make things worse initializing properties of a type via type inference (especially for public ones) is generally a bad practice and code smell and should be avoided. (Yes, Apple does it too in their sample code. But Apple's sample code was never been known for being any good anyway. Remember AppDelegate?)

The reason for this is that type-inference in public APIs can very easily lead to API breakage that is very hard to even detect, but can very easily break a user's code.

(Just to be clear: the exact change applied by this rule as seen above is not harmful in itself regarding API breakage, but it makes it needlessly hard/impossible to stay consistent in one's efforts of defensively providing explicit types for all instance properties. So it's not helping either.)

Why inferred property types are to be avoided (in public, if not all APIs):

This right here is an unfortunately all too common anti-pattern:

import ThirdParty

class Foo {
    public var theThing = ThirdParty.createTheThing()

    func useTheThing() {
        self.theThing.doTheThing()
    }
}

What you should be doing instead is

public var theThing: Thing = ThirdParty.createTheThing()

The argument for avoiding type inference on exported properties (apart from introducing cognitive overhead due to users having to now manually look up the value's type, which might require a proper IDE/LSP) is the following:

By inferring the type of one's property from the return type of a function the foreign function's signature implicitly becomes part of one's own API.
This is particularly problematic when assigning public properties from an external library's public function: your public API is now an implicit superset of your own public API and the referenced subset of the external library's public API (which in turn might be making use of type inference and thus be vulnerable to the same breakage).

Let's say ThirdParty was initially (at the time when you added it as a dependency) implemented like this:

public class Thing {
    // ...
    func doTheThing() { … }
}
public func createTheThing() -> Thing { … }

but then at some point changed into this:

public protocol ThingProtocol {
    func doTheThing() { … }
}
public class Thing: ThingProtocol {
    // ...
    func doTheThing() { … }
}
public func createTheThing() -> ThingProtocol { … }

At this point your own code would still work just fine and you wouldn't be suspicious of any breakage.
But your API's users might have needed to explicitly type the value of Foo.theThing:

let thing: Thing = foo.theThing

Their code now doesn't compile anymore, as theThing is now inferred as ThingProtocol.

As such one should strive to always provide explicit types on (public) properties. Even if having redundant_type_annotation remove such explicit types did not actually introduce such a stability vulnerability it would at the very least introduce syntactic inconsistencies and further contribute to the spread of this anti-pattern.

[jaredgrubb]

FWIW, I think my PR will allow what the Requester wants:
#5839