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 rule to infer property types from the right-hand-side value rather than writing the type explicitly on the left-hand side #263

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ let package = Package(

.binaryTarget(
name: "SwiftFormat",
url: "https://github.com/calda/SwiftFormat/releases/download/0.54-beta-4/SwiftFormat.artifactbundle.zip",
checksum: "65335d1e059714d570ee6dbe76d3738fbae3a404dafb109371a6a55670b5bcd7"),
url: "https://github.com/calda/SwiftFormat/releases/download/0.54-beta-5/SwiftFormat.artifactbundle.zip",
checksum: "7447986db45a51164d23672c07f971406a4c0589b0c423fcb85e95ed8f8e7e48"),

.binaryTarget(
name: "SwiftLintBinary",
Expand Down
103 changes: 92 additions & 11 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -333,25 +333,106 @@ _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)
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

let earth: Planet = Planet.earth

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

// WRONG: Most literals provide a default type that can be inferred.
let enableGravity: Bool = true
let numberOfPlanets: Int = 8
Copy link
Contributor

Choose a reason for hiding this comment

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

@calda how do we handle/guide on the case where you want to tell the language that a literal should be interpreted in a special way? For example, one way to get a floating-point value of 100 is to write

let myFloatingPointValue: Double = 100

which will produce a different result than

let myFloatingPointValue = 100 // implicitly of type `Int`

In this example, another option is to write

let myFloatingPointValue = 100.0 // implicitly of type `Double`

but even then it's necessary to write an explicit type in some cases, like if you explicitly wanted a Float instead of Double

let myFloatingPointValue: Float = 100

Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious how our autocorrection handles this case where removing the type can change the meaning of the code, and I also think that situations like this may warrant some discussion in the prose of this rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my testing I see that the linter is smart enough to correct let numberOfPlanets: Int = 8 to let numberOfPlanets = 8 but not correct let numberOfPlanets: Float = 8. Wow! I wonder if you happen to have a code pointer to how this is implemented.

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw I also saw as I continued to review that this is covered in more detail in the next rule, though I wonder if we should move some of the content from the next rule up to this one since this rule is the one focused on removing types.

Copy link
Member Author

@calda calda Apr 5, 2024

Choose a reason for hiding this comment

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

I implemented this back in nicklockwood/SwiftFormat#921! Here's the code: https://github.com/nicklockwood/SwiftFormat/pull/921/files#diff-1f5cbb52e333eb61c8d6fef2bd765e3e8e6fa8f3101f91f98a9dfce9e1ef480dR1363 It special-cases support for Bool, String, Int, and Double literals.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also saw as I continued to review that this is covered in more detail in the next rule, though I wonder if we should move some of the content from the next rule up to this one since this rule is the one focused on removing types.

Good idea. I think the next rule probably goes in to too much detail about literals, and that content would be a better fit for the discussion about the redundantType rule.

let sunMass: Double = 1.989e30

// RIGHT
let enableGravity = true
let numberOfPlanets = 8
let sunMass = 1.989e30

// WRONG: Types can be inferred from if/switch expressions as well if each branch has the same explicit type.
let smallestPlanet: Planet =
if treatPlutoAsPlanet {
Planet.pluto
} else {
Planet.mercury
}

// RIGHT
let smallestPlanet =
if treatPlutoAsPlanet {
Planet.pluto
} else {
Planet.mercury
}
```

</details>

* <a id='infer-property-types'></a>(<a href='#infer-property-types'>link</a>) **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.** [![SwiftFormat: preferInferredTypes](https://img.shields.io/badge/SwiftFormat-preferInferredTypes-7B0051.svg)](https://github.com/nicklockwood/SwiftFormat/blob/master/Rules.md#preferInferredTypes)
calda marked this conversation as resolved.
Show resolved Hide resolved

<details>

```swift
enum Direction {
case left
case right
}
// WRONG
let sun: Star = .init(mass: 1.989e30)
let earth: Planet = .earth

func someDirection() -> Direction {
// WRONG
return Direction.left
// RIGHT
let sun = Star(mass: 1.989e30)
let earth = Planet.earth

// RIGHT
return .left
Copy link
Member 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.

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

// ALSO RIGHT: Explicit types are required when there is no right-hand-side value.
let sun: Star
let earth: Planet

// ALSO RIGHT: Explicit types can be necessary when the right-hand side has
// a different type from the one written explicitly on the left-hand side.
let naturalSatellite: PlanetaryBody? = Moon(mass: 7.347e22)
let moon: PlanetaryBody? = nil // nil literals are typeless
let numberOfPlanets: UInt = 8 // integer literals default to `Int`
let sunMass: CGFloat = 1.989e30 // floating-point literals default to `Double`
calda marked this conversation as resolved.
Show resolved Hide resolved
let planets: [Planet] = [] // empty collection literals are typeless

// ALSO RIGHT: Some of the examples above can also be written idiomatically without an explicit type on
// the left-hand-side, by instead giving the right-hand side value an explicit type. Either style is fine.
let numberOfPlanets = UInt(8)
calda marked this conversation as resolved.
Show resolved Hide resolved
let sunMass = CGFloat(1.989e30)
let planets = [Planet]()
```

There are some rarer cases where the inferred type syntax has a different meaning than the explicit type syntax. In these cases, the explicit type syntax is still permitted:

```swift
extension String {
static let earth = "Earth"
}

// RIGHT: If the property's type is optional, moving the optional type
// to the right-hand side may result in invalid code.
let planetName: String? = .earth

// WRONG: fails with "error: type 'String?' has no member 'foo'"
let planetName = String?.earth
```

```swift
struct SaturnOutline: ShapeStyle { ... }

extension ShapeStyle where Self == SaturnOutline {
static var saturnOutline: SaturnOutline {
SaturnOutline()
}
}

// RIGHT: If the property's type is an existential / protocol type, moving the type
// 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

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

@calda calda Apr 4, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@bachand bachand Apr 5, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

```

</details>
Expand Down
3 changes: 2 additions & 1 deletion Sources/AirbnbSwiftFormatTool/airbnb.swiftformat
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,5 @@
--rules wrapMultilineConditionalAssignment
--rules blankLineAfterMultilineSwitchCase
--rules consistentSwitchStatementSpacing
--rules semicolons
--rules semicolons
calda marked this conversation as resolved.
Show resolved Hide resolved
--rules preferInferredTypes