nicklockwood / SwiftFormat

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

Add preferInferredTypes rule #1640

Closed calda closed 3 months ago

calda commented 3 months ago

This PR adds a new preferInferredTypes rule that converts property declarations with an explicit type (e.g. let foo: Foo = .init()) to instead use an inferred type (let foo = Foo()).

- let foo: Foo = .init()
+ let foo = Foo()

- let bar: Bar = .defaultValue
+ let bar = Bar.defaultValue

- let baaz: Baaz = .buildBaaz(foo: foo, bar: bar)
+ let baaz = Baaz.buildBaaz(foo: foo, bar: bar)

  let float: CGFloat = 10.0
  let array: [String] = []
  let anyFoo: AnyFoo = foo

Another approach I considered, but didn't implement, was to have a more general propertyDeclarationTypes rule that can also do the inverse transformation (convert let foo = Foo() to let foo: Foo = .init()).

That conversion is more error-prone, since we don't exactly know what the name of the type is. For example, Foo() could be actually be referring to a func Foo() -> Bar. A real-world example that comes to mind is let rect = CGRectMake(0, 0, 0, 0).

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.28%. Comparing base (5d0c8ce) to head (674460c). Report is 4 commits behind head on develop.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1640 +/- ## =========================================== + Coverage 95.11% 95.28% +0.17% =========================================== Files 20 20 Lines 22339 22553 +214 =========================================== + Hits 21247 21490 +243 + Misses 1092 1063 -29 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nicklockwood commented 3 months ago

@calda I'm in two minds about this rule because it overlaps a bit with the redundantType rule. I wonder if it would make more sense to add it as a configuration option for that rule instead of creating a new one?

calda commented 3 months ago

@nicklockwood I had some similar thoughts at first, but I think it feels a bit too weird to put this in behavior in the redundantType rule since in these examples the type isn't redundant. The rule name doesn't matter toooo much of course, but end users do see the rule names and I could see it being confusing if users see a redundantType rule violation error when there isn't a redundant type on the property, or if they have to use // swiftformat:disable:next redundantType to disable this functionality etc.

I'm of course fine with either if you have a preference. A new option like --inferTypes ifRedundant / --inferTypes always seems reasonable enough to me as well.

nicklockwood commented 3 months ago

@calda I see your point. I'm OK with it being a separate rule but the conflict I see is that if you've specified --redundanttype explicit or --redundanttype infer-locals-only then this:

struct {
  let view: UIView = UIView()
}

Would be changed to this by the redundantType rule:

struct Foo {
  let view: UIView = .init()
}

And then the preferInferredTypes rule would convert it to this:

struct Foo {
  let foo = Foo()
}

Which is unlikely to be what you wanted. I guess preferInferredTypes should read and respect the --redundanttype option as a shared option? We might also want the redundantType to detect if preferInferredTypes rule is enabled and modulate its behavior to avoid edit churn.

calda commented 3 months ago

We could easily update the preferInferredTypes rule to check and respect --redundanttype infer-locals-only. It seems like a good idea to support that, so I can make that change.

For the case where the user has both --redundantType explicit and the preferInferredTypes rule enabled, I think it makes sense for preferInferredTypes to take precedent. preferInferredTypes is broader since it applies to all properties, not just properties that currently have a redundant type. So, I think that aspect of the current behavior is reasonable.

What do you think?

calda commented 3 months ago

updated @nicklockwood: https://github.com/nicklockwood/SwiftFormat/pull/1640/commits/674460c6af086cfb259940856783bcf125d3e4c9