[NFC] Simplify some value bindings

[WowbaggersLiquidLunch]

This makes the bindings a little easier to read imo.

Also this might pave a little way for implementing the pitched is case, if we decide to ban

foo is case is Bar

for being too silly.

[WowbaggersLiquidLunch]

@swift-ci please test

[ahoppen]

I understand that code style is always a little subjective but I disagree with you here. I think at the old version is more readable because it has one fewer nesting level. Which part of the old implementation did you find hard to read? Maybe we can find some style that works for both of us.

[WowbaggersLiquidLunch]

Which part of the old implementation did you find hard to read?

I suppose it wasn't hard to read. It just felt a little weird to me that handle is bound to 3 times in the case label. Trading one more nesting level for a "single-source-of-truth" binding to handle and fewer ? feels better to me. This also puts the 3 cases on the same line without anything else in between, so it's clear (to me) from a single glance what the case matches. It is indeed subjective which side of the trade-off one prefers.

Maybe we can find some style that works for both of us.

What if the 3 patterns are indented to align vertically? That probably would improve the "from a single glance" aspect imo.

---

Another reason for this PR is that I wanted to land in some small preparations for implementing is case, so only the most relevant changes would be parked in a PR pending for evolution approval. I'm partially considering banning (and sort of expecting during the review that some might want to ban) foo is case is Bar in the implementation. If we do ban it, then it might be better to diagnose it like this:

switch self.at(...) {
case .some(patternStart, handle):
  if context == .matching {
    addDiagnostics(...)
  }
  switch patternStart {...}
case .nil:
  ...
}

than this:

switch self.at(...) {
case .varKeyword, .letKeyword, .inoutKeyword:
  if context == .matching {
    addDiagnostics(...)
  }
  ...
case .isKeyword:
  if context == .matching {
    addDiagnostics(...)
  }
  ...
case .nil:
  ...
}

But I might be jumping way too ahead here.

[ahoppen]

I suppose it wasn't hard to read. It just felt a little weird to me that handle is bound to 3 times in the case label. Trading one more nesting level for a "single-source-of-truth" binding to handle and fewer ? feels better to me. This also puts the 3 cases on the same line without anything else in between, so it's clear (to me) from a single glance what the case matches. It is indeed subjective which side of the trade-off one prefers.

I don’t really have a problem with binding handle three times because the compiler will complain if you forget to bind it in one of the cases.

What if the 3 patterns are indented to align vertically? That probably would improve the "from a single glance" aspect imo.

I agree that that would help. I’m wondering whether we should fix that in swift-format. CC @allevato

Another reason for this PR is that I wanted to land in some small preparations for implementing is case, so only the most relevant changes would be parked in a PR pending for evolution approval. I'm partially considering banning (and sort of expecting during the review that some might want to ban) foo is case is Bar in the implementation. If we do ban it, then it might be better to diagnose it like this:

I don’t fully understand what you’re trying to say here. Also where are these examples coming from?

[allevato]

I agree that that would help. I’m wondering whether we should fix that in swift-format. CC @allevato

case clauses in general have been tricky to format well since they terminate with a : instead of a braced structure, but I suppose that's not the problem here. I guess there are two options to have the desired effect:

1. Force Allow a break immediately after case:

    case
      (.varKeyword, let handle)?,
      (.letKeyword, let handle)?,
      (.inoutKeyword, let handle)?:
   

2. Use a keyword-length-based indentation:

    case (.varKeyword, let handle)?,
         (.letKeyword, let handle)?,
         (.inoutKeyword, let handle)?:
   

We've resisted #2 in the past in favor of having a uniform indentation step throughout. This isn't really much different than the issue you have with if/guard statements that have multiple conditions:

if someLongCondition,
  someOtherConditionThatDoesntQuiteLineUp
{

where breaking after the if feels particularly egregious because it's just two characters. But for case and guard, it feels slightly more reasonable.

The least invasive change to make would be to change this space to break, which would allow you to insert a newline after case and the formatter wouldn't remove it as it does today. If someone wants to make that change, I'm happy to review it.

EDIT: Actually, it's more complicated than that because of the grouping around the case; a newline between any of the clauses would also fire the first one. This would need more care when implementing.

Longer-term, it might be worth revisiting the rigid uniform indentation step, but I don't want to do that quite yet since it would have much broader implications to how the formatting algorithm works.

[WowbaggersLiquidLunch]

I don’t fully understand what you’re trying to say here. Also where are these examples coming from?

I think I can explain my rationale better when I have the PR for is case ready (probably in a few hours or a longest maybe 2 days).

Though I admit that the more I think about it, the less merit I see in pushing for this PR.

[ahoppen]

OK, let’s just put this PR on hold then for now and re-evaluate it when you’ve got your other PR up.

[WowbaggersLiquidLunch]

It turned out I misunderstood how diagnostics is produced, so my second reason for this PR doesn't stand. I don't really want to change the code for minor aesthetic reasons alone, so I'm closing this PR. Thanks for the review!

[ahoppen]

Aesthetic changes are always welcome, even if they are minor, so don't feel like you shouldn't create PRs like this in the future. We just need to agree that the change is for the better 😉