Add rule to infer property types from the right-hand-side value rather than writing the type explicitly on the left-hand side #263

calda · 2024-03-13T23:28:48Z

Please react with 👍/👎 below if you agree or disagree with this proposal.

Summary

This PR proposes a new rule to prefer letting the type of a property be inferred from the right-hand-side value rather than writing the type explicitly on the left-hand side:

// WRONG
let sun: Star = .init(mass: 1.989e30)
let earth: Planet = .earth

// RIGHT
let sun = Star(mass: 1.989e30)
let earth = Planet.earth

Autocorrect support for this rule is implemented in nicklockwood/SwiftFormat#1640.

Reasoning

We already have a rule to omit redundant types. It converts let sun: Star = Star() to let sun = Star(), rather than to let sun: Star = .init().

The syntax with inferred types is more idiomatic, and is much more common, than the syntax with explicit types.

I briefly checked the frequency of these two styles in our codebase with a regex search:

Syntax with implicit types (let sun = Star() or let earth = Planet.earth, etc):
- ~20,000 matches using the regex ((let)|(var)) \w+ = [A-Z]+\w+(\.|\()
Syntax with explicit types (let sun: Star = .init(), etc)
- ~5,000 matches using the regex ((let)|(var)) \w+: \w+ = \..

This rule only applies to simple cases where the right-hand side starts with a leading dot (meaning we know that the RHS value refers to a static member of the type written on the LHS). For other cases, we don't state a preference.

calda · 2024-03-13T23:30:04Z

README.md

@@ -333,25 +333,72 @@ _You can enable the following settings in Xcode by running [this script](resourc

  ```swift
  // WRONG
-  let host: Host = Host()
+  let sun: Star = Star(mass: 1.989e30)


While I was here I expanded the list of examples used by the "Don't include types where they can be easily inferred." rule. These updates reflect the current behavior of the SwiftFormat redundantType rule.

calda · 2024-03-13T23:31:52Z

README.md


-    // RIGHT
-    return .left


This is a nice example and is good advice in general, but doesn't reflect the behavior of the SwiftFormat redundantType rule that we use. I don't believe we have autocorrect support for this specific example.

I added an asana task to our internal board for us to look at adding autocorrect for this sort of case

Sources/AirbnbSwiftFormatTool/airbnb.swiftformat

…r than writing the type explicitly on the left-hand side

calda · 2024-04-04T19:24:29Z

README.md

+  let myShape1: any ShapeStyle = .saturnOutline
+
+  // WRONG: fails with "error: static member 'saturnOutline' cannot be used on protocol metatype '(any ShapeStyle).Type'"
+  let myShape2 = (any ShapeStyle).myShape


By the way -- for this we have the ability to tell SwiftFormat a list of symbols to exclude from this rule (--preservesymbols ShapeStyle,OtherType,etc). Inside the apps repo we could exclude any type or property name where we frequently use this syntax. Two relevant examples are:

UnresolvedShapeStyle

all existing callsites use any UnresolvedShapeStyle so this is already handled properly

canonicalFake

there are about a dozen examples of let experimentsService: ExperimentsService = .canonicalFake (etc). It seems like a good idea to exclude this using --preservesymbols canonicalFake. Updating the callsites to let experimentsService: any ExperimentsService = .canonicalFake also works.

Thanks for flagging this @calda . I was wondering about this... My recollection is that eventually it's going to be required that we use any for any existential/protocol types but that this requirement is being slowly rolled out. Is your understanding also that eventually all existential/protocol type will need to be preceded by any? If so, I think it's less problematic if we need to work around certain situations like the above ones you highlighted for the time being.

I am trying to understand if there would be cases when we still need --preservesymbols if/when the language requires that all existential/protocol types are preceded by any.

I saw on the Swift Evolution forums that they decided to not make any mandatory in Swift 6 mode after all. Although after a quick search on the forums I can't actually find where they said that.

This doesn't necessarily confirm it, but if you build code like this using a nightly Swift 6 build it doesn't require any.

I am trying to understand if there would be cases when we still need --preservesymbols if/when the language requires that all existential/protocol types are preceded by any.

If all call sites used the existential any syntax, we wouldn't need to use --preservesymbols

Found the quote: https://forums.swift.org/t/progress-toward-the-swift-6-language-mode/68315

The Language Steering Group has decided that one previously-accepted upcoming feature, ExistentialAny, will not be enabled by default in Swift 6. SE-0335: Introduce existential any introduces the any keyword to identify existential types. The proposal also specifies that "bare" protocol names will no longer be permitted as types---they must either use any or some, as appropriate---under the upcoming feature flag ExistentialAny. Given the concerns about migration to consistent use of existential any in the language, and the expectation that additional language improvements will be coming that might affect the end result of that migration, the Language Steering Group is deferring the source-incompatible changes in SE-0335 to a future language revision.

It sounds like they didn't rule out this direction, but rather just deferred it for longer (probably a very long time, if we have to wait for Swift 7!)

In the apps repo PR that I merged today, I included --preservesymbols canonicalFake. This is a common and reasonable pattern that I think we should continue supporting. It's not common to use the any ExperimentService spelling today, so it felt too weird to require it in this specific case due to this limitation of the linter.

bachand

@calda this is looking great! I was hoping to talk through some comments before approving if that's OK but I feel like we are very close 👍

README.md

bachand · 2024-04-05T01:56:21Z

README.md

+  let myShape1: any ShapeStyle = .saturnOutline
+
+  // WRONG: fails with "error: static member 'saturnOutline' cannot be used on protocol metatype '(any ShapeStyle).Type'"
+  let myShape2 = (any ShapeStyle).myShape


Thanks for flagging this @calda . I was wondering about this... My recollection is that eventually it's going to be required that we use any for any existential/protocol types but that this requirement is being slowly rolled out. Is your understanding also that eventually all existential/protocol type will need to be preceded by any? If so, I think it's less problematic if we need to work around certain situations like the above ones you highlighted for the time being.

bachand · 2024-04-05T01:59:56Z

README.md

+  let myShape1: any ShapeStyle = .saturnOutline
+
+  // WRONG: fails with "error: static member 'saturnOutline' cannot be used on protocol metatype '(any ShapeStyle).Type'"
+  let myShape2 = (any ShapeStyle).myShape


I am trying to understand if there would be cases when we still need --preservesymbols if/when the language requires that all existential/protocol types are preceded by any.

calda · 2024-04-11T17:47:19Z

Thanks for the review @bachand! I updated the rule to clarify that it applies to both local variables and instance properties, improved the main example to show both cases, and simplified some of the other examples related to edge cases.

…operties, simplify examples

calda · 2024-06-08T01:36:00Z

README.md

+  // to the right-hand side will result in invalid code if the value is defined in an
+  // extension like `extension ShapeStyle where Self == SaturnOutline`.
+  // SwiftFormat autocorrect detects this case by checking for the existential `any` keyword.
+  let myShape1: any ShapeStyle = .saturnOutline


I spent a bit of time thinking about this case in more detail, that let myShape1: ShapeStyle = .saturnOutline is allowed but let myShape1: = ShapeStyle.saturnOutline isn't. I couldn't understand why this wasn't allowed.

This syntax was introduced in SE-0299. This question came up briefly in the review thread, and Pavel mentioned that there was more discussion in the pitch thread. A found the most compelling discussion here in the pitch thread.

They intentionally don't frame this as adding a static member to the protocol type, but rather a shorthand syntax that lets you omit the name of the concrete type.

Basically, the thinking is that this code:

let myShape1: ShapeStyle = .saturnOutline

is actually implicit shorthand for this code:

let myShape1: ShapeStyle = SaturnOutline.saturnOutline

not shorthand for this code:

let myShape1: ShapeStyle = ShapeStyle.saturnOutline

This is subtle but distinct, and is a reasonable explanation for why ShapeStyle.saturnOutline isn't allowed.

This problem should also go away eventually, once any is required in a future language mode (just not Swift 6).

calda commented Mar 13, 2024

View reviewed changes

Sources/AirbnbSwiftFormatTool/airbnb.swiftformat Outdated Show resolved Hide resolved

calda force-pushed the cal--infer-types branch from 7820918 to f0bbb0b Compare March 14, 2024 01:06

Add rule to infer property types from the right-hand-side value rathe…

d343fb2

…r than writing the type explicitly on the left-hand side

calda force-pushed the cal--infer-types branch from f0bbb0b to d343fb2 Compare March 22, 2024 21:25

calda requested a review from bachand March 22, 2024 21:25

calda added 2 commits March 28, 2024 14:35

Update to SwiftFormat build with bug fixes

3750e2c

Add discussion around edge cases

5a39cdc

jqsilver approved these changes Apr 4, 2024

View reviewed changes

calda commented Apr 4, 2024

View reviewed changes

bachand reviewed Apr 5, 2024

View reviewed changes

Clarify that the rule applies to both local variables and instance pr…

39f921f

…operties, simplify examples

calda force-pushed the cal--infer-types branch from dd1db39 to 39f921f Compare April 11, 2024 18:08

calda requested review from mannylopez and miguel-jimenez-0529 May 30, 2024 17:19

calda and others added 2 commits June 7, 2024 15:58

Update airbnb.swiftformat

e3c339c

Update SwiftFormat to latest build

0425be7

calda force-pushed the cal--infer-types branch from fa8cbbe to 0425be7 Compare June 7, 2024 23:21

Merge branch 'master' into cal--infer-types

0f4aed8

calda merged commit 2c1c76a into master Jun 7, 2024
5 checks passed

calda deleted the cal--infer-types branch June 7, 2024 23:51

calda commented Jun 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rule to infer property types from the right-hand-side value rather than writing the type explicitly on the left-hand side #263

Add rule to infer property types from the right-hand-side value rather than writing the type explicitly on the left-hand side #263

calda commented Mar 13, 2024 •

edited

Loading

calda Mar 13, 2024

bachand Apr 5, 2024

calda Mar 13, 2024 •

edited

Loading

calda Apr 11, 2024

calda Apr 4, 2024 •

edited

Loading

bachand Apr 5, 2024 •

edited

Loading

bachand Apr 5, 2024

calda Apr 5, 2024

calda Apr 11, 2024

calda Jun 8, 2024

bachand left a comment

bachand Apr 5, 2024 •

edited

Loading

bachand Apr 5, 2024

calda commented Apr 11, 2024

calda Jun 8, 2024 •

edited

Loading

calda Jun 8, 2024 •

edited

Loading

Add rule to infer property types from the right-hand-side value rather than writing the type explicitly on the left-hand side #263

Add rule to infer property types from the right-hand-side value rather than writing the type explicitly on the left-hand side #263

Conversation

calda commented Mar 13, 2024 • edited Loading

Summary

Reasoning

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calda Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calda Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

bachand Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachand left a comment

Choose a reason for hiding this comment

bachand Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calda commented Apr 11, 2024

calda Jun 8, 2024 • edited Loading

Choose a reason for hiding this comment

calda Jun 8, 2024 • edited Loading

Choose a reason for hiding this comment

calda commented Mar 13, 2024 •

edited

Loading

calda Mar 13, 2024 •

edited

Loading

calda Apr 4, 2024 •

edited

Loading

bachand Apr 5, 2024 •

edited

Loading

bachand Apr 5, 2024 •

edited

Loading

calda Jun 8, 2024 •

edited

Loading

calda Jun 8, 2024 •

edited

Loading