Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add preferInferredTypes rule #1640

Merged
merged 2 commits into from
Mar 16, 2024

Conversation

calda
Copy link
Collaborator

@calda calda commented Mar 13, 2024

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).

let prevToken = formatter.token(at: prevIndex), case let .identifier(name) = prevToken,
let firstChar = name.first, firstChar != "$",
let prevToken = formatter.token(at: prevIndex),
formatter.isValidEndOfType(at: prevIndex),
Copy link
Collaborator Author

@calda calda Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed some cases that were previously unsupported by the redundantInit rule, like [String].init(), Array<String>.init(), and String?.init()

Copy link

codecov bot commented Mar 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@             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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nicklockwood
Copy link
Owner

@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
Copy link
Collaborator Author

calda commented Mar 15, 2024

@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
Copy link
Owner

nicklockwood commented Mar 15, 2024

@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
Copy link
Collaborator Author

calda commented Mar 15, 2024

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
Copy link
Collaborator Author

calda commented Mar 15, 2024

updated @nicklockwood: 674460c

@nicklockwood nicklockwood merged commit 78c2340 into nicklockwood:develop Mar 16, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants