Allow consistent brace formatting checks (OptionalWhenBraces triggered)

[TWiStErRob]

Expected Behavior of the rule

In many cases consistency is more important than minimalistic code. So I think it would be beneficial to add a flag (and enable by default) to treat curly braces as allowed (dare I say "mandatory"?). This would elevate all when cases to the same level of visibility and structure giving the code more consistency. I'm all for removing all braces, iff all cases in the when let me remove it, but as soon as there's one with complex logic, the others should be treated equally too.

Context

Compare these two functions:

private fun OutgoingContent.asString(): String? =
	when (this) {
		is OutgoingContent.NoContent -> ""
		is OutgoingContent.WriteChannelContent -> {
			// Need to consume the asynchronicity to satisfy the contract.
			runBlocking {
				val channel = ByteChannel(true)
				this@asString.writeTo(channel)
				channel.tryReadText(Charsets.UTF_8)
			}
		}
		is TextContent ->
			this.text
		else -> {
			logger.error("Unknown response body content: ${this::class}")
			null
		}
	}

private fun OutgoingContent.asString(): String? =
	when (this) {
		is OutgoingContent.NoContent -> {
			""
		}

		is OutgoingContent.WriteChannelContent -> {
			// Need to consume the asynchronicity to satisfy the contract.
			runBlocking {
				val channel = ByteChannel(true)
				this@asString.writeTo(channel)
				channel.tryReadText(Charsets.UTF_8)
			}
		}

		is TextContent -> {
			this.text
		}

		else -> {
			logger.error("Unknown response body content: ${this::class}")
			null
		}
	}

and notice how in the first one TextContent handling is hidden away because the "braces are optional".

[BraisGabin]

100%. I think that when should be treated in a similar way as if is. The problem is that if is an old known pattern that the mandatory braces are "obvious" if you can't fit the if/else in the same line.

I would remove OptionalWhenBraces from detekt. And I would create a new rule that enforces something similar to what you proposed:

• If all the cases are one line: it's fine to don't have braces.
• If at least one case doesn't fit in one line (because it contains multiple staments or because it's long) the braces should be mandatory for all the cases.
• If, even if all the cases are one liners, if one of them uses braces, all of the cases should use braces (or remove the braces from the one that have braces)

What do you think?

---

Opinion: the braces on the when cases are really missleding. I'm never sure if I'm just opening a block or creating a lambda. I feel that this is an issue that Kotlin, as a language, should fix.

[TWiStErRob]

Glad you agree. Sounds like ConsistentWhenBraces would be a good add, but I would keep OptionalWhenBraces too, because lot of people naturally like to golf their code preferring concise over consistent. Long term, if the Detekt team agrees it would be nice to get to the below state to guide the community code style:

• ConsistentWhenBraces (enabled by default)
• OptionalWhenBraces (disabled by default)
  Of course this can be flipped with 4 lines of config.

Alternatively:

OptionalWhenBraces:
  braceStyle: consistent|always|neverForSingleExpressions

---

I'm never sure if I'm just opening a block or creating a lambda. I feel that this is an issue that Kotlin, as a language, should fix.

Does it matter? Ideally they'll optimize them away and therefore they're just syntax for grouping "statements". Unless you meant block=Unit, lambda=returns silently, in which case I totally agree and always need to parse a lot of context and infer in my head to actually see if the expression is used or not. I think a simple inlay hint would go a long way (simulated by a /**/ comments):

[segunfamisa]

Heya folks!

Is this issue up for grabs? If it is, I'd love to have a go at it, if that's okay.

[cortinico]

Is this issue up for grabs? If it is, I'd love to have a go at it, if that's okay.

Yes it is 👍 I've assigned it to you @segunfamisa

[segunfamisa]

Quick question:

TL;DR

I'm wondering if OptionalWhenBraces and ConsistentWhenBraces can co-exist. I think it could get contradictory.

Context

I noticed that per the current implementation the code snippet below is non compliant for OptionalWhenBraces , while at the same time, would be compliant with the proposed ConsistentWhenBraces rule.

fun x() {
  when (1) {
    1 -> {
        print(1)
    }
    2 -> {
        print(2)
        print(2)
    }
  }
}

To be clear on what is expected in terms of how the two new rules will work together, does it mean that the implementation will look like:

1. Relax behaviour of OptionalWhenBraces to raise errors only when none of the branches needs braces and yet there are braces? This means that the snippet above is going to be compliant with OptionalWhenBraces in this new behaviour. Is this correct?
2. Implement ConsistentWhenBrances to raise errors when there is at least one branch that has braces, while others don't have (or vice versa)

Or, we could go with the alternative suggestion from @TWiStErRob in #5133 (comment)

[TWiStErRob]

I would like to come back to this, this rule is very inconsistent with MandatoryBracesIfStatements, for a trivial if-else, we can require braces, but for very complex when the only thing we can do is remove the braces. Here's an example:

when {
	AGPVersions.CLASSPATH >= AGPVersions.v70x -> { // OptionalWhenBraces
		// AGP 7.4 compatibility: calling onVariants$default somehow changed, being explicit about params helps.
		project.androidComponents.onVariants(project.androidComponents.selector().all()) {
			if (version.isRenameAPK) // MandatoryBracesIfStatements
				renameAPKPost7(it as ApplicationVariant)
		}
	}
	else -> { // OptionalWhenBraces
		project.beforeAndroidTasksCreated {
			android.applicationVariants.all {
				if (version.isRenameAPK) // MandatoryBracesIfStatements
					renameAPKPre7(it, it)
			}
		}
	}
}

notice that the when code is way more complicated than the if.

[TWiStErRob]

I'm wondering if OptionalWhenBraces and ConsistentWhenBraces can co-exist. I think it could get contradictory. @segunfamisa in #5133 (comment)

I think they would coexist for one release while deprecation happens, but not sure how rules are deprecated.

Re alternative solution, I thought more and braceStyle is way too complicated to cover all cases... there must be a simpler way.

I think we should deprecate/remove OptionalWhenBraces in favor of a new ConsistentWhenBraces with a braceStyle config option would be more generic and hence provide overlapping functionality.

• optional: exactly the same as OptionalWhenBraces
• always: stronger than MandatoryBracesIfStatements, no exceptions, even single line
• consistent: my proposal in OP (doesn't matter which one, but be the same inside one when)
• requiredForMultiline: equivalent to MandatoryBracesIfStatements, and kind of the opposite of OptionalWhenBraces (if there's any code on a different line than ->, report)
• onlyMandatory: kind of never, but has to allow it when multiline code.

I can see these are the possible ways to write cases:

when {
    cond -> sameLine
    cond -> { sameLine }
    cond -> same
        .butWraps()
    cond ->
        wrapped
    cond -> {
        wrapped
    }
    cond -> {
        multi
        statement
    }
    multi,
    cond -> ...
}

MandatoryBracesWhenStatements:
   singleLine: always|never|consistent
   multiLine: always|never|consistent # (wrapped in the above code)
   #multiStatement: always # There's no other option.

so

• MandatoryBracesIfStatements equivalent: singleLine=never, multiLine=always
• OptionalWhenBraces equivalent: singleLine=never, multiLine=never
• ConsistentWhenBraces equivalent: singleLine=consistent, multiLine=consistent
• RequiredWhenBraces eqvuivalent: singleLine=always, multiLine=always
• + any the other combinations if user chooses.
• default TBD

[BraisGabin]

I really like this idea! We could create a rule set called braces and add there the rules:

• BracesOnIfStatments
• BracesOnForStatments
• BracesOnWhileStatments
• BracesOnWhenStatments

(as far as I know you can't write a do-while without braces)

And give to each of them those options! (for and while wouldn't need consistent, I think).

[VitalyVPinchuk]

Hello, I'd like to work on the issue if it's available.

[atulgpt]

Hi, @VitalyVPinchuk thanks for taking this up. I am super interested in this PR 😍

[TWiStErRob]

@BraisGabin I would re-open this, there's 3 more rules to write :)

[BraisGabin]

Sure! It was closed automatically by Github.

[TWiStErRob]

For the record: we've decided to keep the styles ruleset for BracesOn rules, because their naming already groups them somewhat.