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

Update preferInferredTypes rule to also support converting from inferred types to explicit types, fix bugs #1658

Merged

Conversation

calda
Copy link
Collaborator

@calda calda commented Mar 28, 2024

This PR renames the preferInferredTypes rule to propertyType, and makes the rule support both directions of conversions. It now fully respects the --redundantType inferred/explicit/infer-locals-only option, and can convert freely between

let foo: Foo = .init()

and

let foo = Foo()
  // --redundantType inferred
- let foo: Foo = .init()
+ let foo = Foo()

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

  // --redundantType explicit
- let foo = Foo()
+ let foo: Foo = .init()

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

  // --redundantType prefer-locals-only
  class Foo {
-     let foo = Foo()
+     let foo: Foo = .init()

      func bar() {
-         let bar: Bar = .defaultValue
+         let bar = Bar.defaultValue
      }
  }

That being said each option has various edge cases that may require manual fixes. When I ran these on Airbnb's app codebase:

  • inferred had about 15-20 cases that I had to fix manually (not so bad considering we have like 2M+ LOC!)
  • infer-locals-only mostly only had issues with cases where we had functions that look like types (e.g. func Translation(...)) or static members that don't return the same type. I added a --preservesymbols option that lets you exclude any sort of type or property that has this issue, which solved most of these issues. After this it was only miscellaneous issues that were individually easy to work around.
  • I didn't try to use explicit, but it seems reasonable to support it if somebody wants to use it. I expect this would be somewhat difficult, but it's definitely possible.

I'd be fine with leaving the rule as-is (preferInferredTypes only supporting inferred rather than explicit or infer-locals-only) if you think the infer-locals-only and explicit options are too unreliable. I find inferred and infer-locals-only definitely reliably enough for use in Airbnb's codebase / style guide.


This PR also fixes other issues where the existing code would cause build failures:

For example, this compiles:

extension String {
  static let foo = "Foo"
}

let foo: String? = .foo

but this doesn't:

extension String {
  static let foo = "Foo"
}

// error: type 'String?' has no member 'foo'
let foo = String?.foo

I would have posted the two changes as separate PRs, but the propertyType rewrite depended on the bugfixes.

@calda calda force-pushed the cal--preferInferredTypes-fixes branch from 4d7aaeb to 6a9444a Compare March 28, 2024 17:57
// If the RHS starts with a leading dot, then we know its accessing some static member on this type.
if formatter.tokens[rhsStartIndex].isOperator(".") {
// Update the . token from a prefix operator to an infix operator.
formatter.replaceToken(at: rhsStartIndex, with: .operator(".", .infix))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The redundantInit rule didn't work correctly when run after the preferInferredType rule. At first I fixed this by changing the redundantInit rule to not check the operator type, but thinking about it more it seems better to have the preferInferredType rule change the "." operator from .prefix to .infix.

@@ -991,7 +991,7 @@ struct _Descriptors {
argumentName: "doccomments",
displayName: "Doc comments",
help: "Preserve doc comments: \"default\" or \"preserve\".",
keyPath: \.preserveSingleLineForEach,
keyPath: \.preserveDocComments,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unrelated bugfix, this OptionDescriptor key path was wrong

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

Attention: Patch coverage is 98.30508% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 95.17%. Comparing base (ca70962) to head (532eab8).

Files Patch % Lines
Sources/Rules.swift 98.31% 3 Missing ⚠️
Sources/ParsingHelpers.swift 97.95% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1658      +/-   ##
===========================================
+ Coverage    95.13%   95.17%   +0.03%     
===========================================
  Files           20       20              
  Lines        22707    22880     +173     
===========================================
+ Hits         21603    21775     +172     
- Misses        1104     1105       +1     

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

let myShape1: any ShapeStyle = .myShape

// This would fail with "error: static member 'myShape' cannot be used on protocol metatype '(any ShapeStyle).Type'"
// let myShape2 = (any ShapeStyle).myShape
Copy link
Collaborator Author

@calda calda Mar 28, 2024

Choose a reason for hiding this comment

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

We can't detect this unless the code uses the optional existential any syntax, so I added a note about it in the "known issues" section. Within the Airbnb codebase all existing examples of this pattern do use the any syntax, so this is ok for us.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the latest changes, you could add --preservesymbols ShapeStyle to avoid this problem when without the existential any syntax.

…e property's type is an existential, or if the RHS value has an infix operator
@calda calda force-pushed the cal--preferInferredTypes-fixes branch from 9934dff to 2188dea Compare March 28, 2024 20:41
@calda
Copy link
Collaborator Author

calda commented Mar 29, 2024

@nicklockwood, I updated the rule implementation to support both prefer-locals-only and explicit rather than just inferred like it did before. This PR also includes a bunch of bug fixes. I added some context and commentary to the PR description, let me know what you think.

@calda
Copy link
Collaborator Author

calda commented Mar 29, 2024

We may also want to rename the --redundantType option to --propertyType and then have both the redundantType and propertyType rule respect it. It's slightly weird for the propertyType rule to use the --redundantType option, but I agree your thinking that we do want them to be configured using the same option rather than two separate options (so there's never a conflict). I'm also fine with leaving it as-is.

@calda calda changed the title Fix issue where preferInferredTypes could cause build failure if property has optional type Update preferInferredTypes rule to also support converting from inferred types to explicit types, fix bugs Mar 29, 2024
@nicklockwood nicklockwood merged commit da7e2b7 into nicklockwood:develop Mar 30, 2024
7 checks passed
@nicklockwood
Copy link
Owner

@calda thanks so much for these features and fixes, I really appreciate it. Sorry I've not had very much bandwidth to discuss the design. I'm not certain yet what the best approach is with respect to naming. I'll try to collate my thoughts and get back to you in the next few days.

@calda
Copy link
Collaborator Author

calda commented Mar 30, 2024

Thanks, no worries :) appreciate the merges, we can iterate on develop

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.

None yet

2 participants