Add rule to prefer using if/switch expressions when initializing a new property with the result of a conditional statement

[calda]

Summary

This PR proposes a new rule to prefer using if/switch expressions when initializing a new property with the result of a conditional statement.

Support for this syntax was added in Swift 5.9, and autocorrect is implemented via the conditionalAssignment SwiftFormat rule.

Reasoning

There are several benefits to using an if/switch expression over simply performing assignment on each branch of the following conditional statement:

1. In most cases, you no longer need to explicitly write a type annotation for the property that is being assigned to.
2. The compiler will diagnose more cases where using a mutable var is unnecessary.
3. The resulting syntax is visually ligher because the property name being assigned doesn't need to be written on each branch.

// BEFORE
// 1. An explicit type annotation is required for the uninitialized property.
// 2. `var` is unnecessary here because `planetLocation` is never modified after being initialized, but the compiler doesn't diagnose.
// 3. The `planetLocation` property name is written on each branch so is redundant and visually noisy
var planetLocation: String
if let star = planet.star {
  planetLocation = "The \(star.name) system"
} else {
  planetLocation = "Rogue planet"
}

print(planetLocation)

// AFTER
// 1. No need to write an explicit `: String` type annotation.
// 2. The compiler correctly diagnoses that the `var` is unnecessary and emits a warning suggesting to use `let` instead. 
// 3. Each conditional branch is simply the value being assigned.
var planetLocation =
  if let star = planet.star {
    "The \(star.name) system"
  } else {
    "Rogue planet"
  }

print(planetLocation)

Examples

// WRONG
let planetLocation: String
if let star = planet.star {
  planetLocation = "The \(star.name) system"
} else {
  planetLocation = "Rogue planet"
}

let planetType: PlanetType
switch planet {
case .mercury, .venus, .earth, .mars:
  planetType = .terrestrial
case .jupiter, .saturn, .uranus, .neptune:
  planetType = .gasGiant
}

let canBeTerraformed: Bool 
if 
  let star = planet.star, 
  !planet.isHabitable,
  planet.isInHabitableZone(of: star) 
{
  canBeTerraformed = true
} else {
  canBeTerraformed = false
}

// RIGHT
let planetLocation =
  if let star = planet.star {
    "The \(star.name) system"
  } else {
    "Rogue planet"
  }

let planetType: PlanetType = 
  switch planet {
  case .mercury, .venus, .earth, .mars:
    .terrestrial
  case .jupiter, .saturn, .uranus, .neptune:
    .gasGiant
  }

let canBeTerraformed = 
  if 
    let star = planet.star, 
    !planet.isHabitable,
    planet.isInHabitableZone(of: star) 
  {
    true
  } else {
    false
  }

// ALSO RIGHT. This example cannot be converted to an if/switch expression
// because one of the branches is more than just a single expression.
let planetLocation: String
if let star = planet.star {
  planetLocation = "The \(star.name) system"
} else {
  let actualLocaton = galaxy.name ?? "the universe"
  planetLocation = "Rogue planet somewhere in \(actualLocation)"
}

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

[jpsoultanis]

Maybe this is just my bias here, but I'm not in favor of this change. I tend to follow the mantra of "better to be clear than clever".

In my opinion based on the examples above, the code produced in this new pattern is much harder to understand.

Maybe some developers will prefer this new style, and that is ok, but I don't think we should lint away the old approach which has merit.

[calda]

@jpsoultanis, interested to hear which aspects of the pattern you find harder to understand.

---

You may have noticed this confusing code in the PR body a few minutes ago:

var planetLocation = planet.star {
  "The \(star.name) system"
} else {
  "Rogue planet"
}

this was a typo and was not valid Swift, it was supposed to be:

var planetLocation = if let star = planet.star {
  "The \(star.name) system"
} else {
  "Rogue planet"
}

I just updated that example and it is now correct.

[jpsoultanis]

@jpsoultanis, interested to hear which aspects of the pattern you find harder to understand.

You may have noticed this confusing code in the PR body a few minutes ago:

var planetLocation = planet.star {
  "The \(star.name) system"
} else {
  "Rogue planet"
}

this was a typo and was not valid Swift, it was supposed to be:

let planetLocation = if let star = planet.star {
  "The \(star.name) system"
} else {
  "Rogue planet"
}

I just updated that example and it is now correct.

I see, that addresses my own confusion. I still think the "wrong" approach is more explicit and clear, but this may be just because I am used to it.

If I had to articulate it, the issue is that I've trained my brain to scan and when I see an if let then I "know" that there's some logic in the block that is executed once the unwrap is made. But with this change, the block actually functions as the setting of a var, which contradicts my mental model.

[calda]

I think I understand what you mean. I think there is some natural tension with how this formatting defies our existing expectations of how an if / switch statement looks:

let planetLocation = if let star = planet.star {
  "The \(star.name) system"
} else {
  "Rogue planet"
}

I'm curious to hear your thoughts about this alternative formatting, where we insert a line break before the if keyword:

let planetLocation = 
  if let star = planet.star {
    "The \(star.name) system"
  } else {
    "Rogue planet"
  }

This is the formatting used in most of the examples in the Swift Evolution proposal for this feature, and has two improvements over the other formatting:

1. The if statement starts on the left of a dedicated line of code, which is more naturally compatible with how we're used to scanning / reading if statements (we aren't used to looking for them in the middle of some other line of code).
2. The conditional branches are always to the right of, and more indented than, the if keyword, which is the shape we're used to seeing. (In the other example it seems a little funny how the "true" and "false" branches of the code are visually to the left of the condition itself, rather than below / to the right like we're used to).

Curious how your thoughts on this formatting compare to formatting used in the examples in the PR body @jpsoultanis.

Regardless of whether or not we merge this PR, it would make sense to choose a preferred formatting for if / switch expressions. Even if we don't merge this PR, engineers will be permitted to write code using this functionality (unless we specifically disallow it).

[nmkel999]

I actually like the Swift 5.9 syntax. The most convincing argument in favor of the new syntax relates to the type system:

Not only does this add the ceremony of explicit typing and assignment on each branch (perhaps tempting overly terse variable names), it is only practical when the type is easily known. It cannot be used with opaque types, and is very inconvenient and ugly if the type is a complex nested generic.

However, I'm really used to the pre-5.9 way of doing things (especially when I'm reading code). I'd prefer to gradually introduce this new syntax, rather than putting it in the style guide as the preferred way of doing things right now.

[bachand]

I am glad to see the engagement on this PR!

@calda , I am going to hold off on approving this PR given that there is ongoing discussion. If you feel like we should move ahead with this proposal, please tag me and I can review again with the aim of approving.

I am in favor of this proposal. Related to this discussion, though, I would prefer the alternate syntax for the reasons you outlined @calda .

let planetLocation = 
  if let star = planet.star {
    "The \(star.name) system"
  } else {
    "Rogue planet"
  }

I am personally in favor of the "less is more" approach to explicit types in Swift, so I like how this new Swift 5.9 syntax can avoid the need to provide an explicit type in a variable declaration. Related to what @nmkel999 quoted, I find that not specifying types in code makes it easier to perform sweeping refactors.

I also love that this new syntax causes the compiler to diagnose that let can be used instead of var.

[bachand]

I wonder if we can have a more specific/descriptive slug than conditional-assignment but I don't feel strongly that we need to make a change.

[jpsoultanis]

@calda I much prefer the syntax you mentioned with the line break:

let planetLocation = 
  if let star = planet.star {
    "The \(star.name) system"
  } else {
    "Rogue planet"
  }

Makes it much more clear at-a-glance that we are performing a variable assignment.

Thanks for your work on this 😄

[calda]

#253 adopts the formatting we discussed above, where we insert a line break after the assignment operator.

[bachand]

I am approving since I believe that there is general alignment on moving ahead with this proposal. Thanks @calda !

[bachand]

👍