Kotlin / KEEP

Kotlin Evolution and Enhancement Process
Apache License 2.0
3.29k stars 357 forks source link

Guards #371

Closed serras closed 2 days ago

serras commented 3 months ago

This is an issue to discuss guards. The current full text of the proposal can be found here.

This KEEP proposes an extension of branches in when expressions with subject, which unlock the ability to perform multiple checks in the condition of the branch.

kyay10 commented 3 months ago

Instead of the else if syntax, I'd much prefer just allowing arbitrary Boolean expressions in when with subject. It's rather unambiguous because only equality, is-checks, etc are allowed right now. One issue might be when over a Boolean, but perhaps the compiler can prohibit that entirely.

serras commented 3 months ago

Instead of the else if syntax, I'd much prefer just allowing arbitrary Boolean expressions in when with subject.

Unfortunately, this is not as easy. For example, if I write:

fun foo(n: Int) = when (n) {
  0, 1 -> "a"
  otherThing() -> "b"
}

it's not clear whether otherThing() should be a function returning a boolean, or returning an integer to be compared against. Even more so, if there are two otherThing() in scope, each of them returning the two different types, there would be no way to specify what we want to call.

It could be possible to make a "better" transformation by checking the types, but I think that in this case having this translation hold before the type checking phase is a good thing to have.

kyay10 commented 3 months ago

I would expect otherThing to evaluate exactly as if it was standalone. I see the elegance in transforming before type checking. Perhaps transforming into a fictitious matches function could be better though? Here's what I'm thinking:

fun Any?.matches(condition: Boolean) = condition
fun Any?.matches(value: Any?) = this == value

Hence, any main condition in a when-with-subject that isn't preceded by in or is would get transformed to a call to this fictitious matches, and then overload resolution would choose the right one and report if there's ambiguity or anything. I'm not proposing that matches exist at all, I'm using it as a proxy for the compiler to delay the decision for what type of check we're doing until we have the needed types.

roomscape commented 3 months ago

While the idea behind this proposal makes sense to me, I don't think it translates into reality well. It ends up being shackled by some confusing rules that could just make the resulting code less clear rather than more.

In the absence of exhaustiveness checking, the only real remaining motivation for the change is the argument that when-with-subject is more expressive and concise.

It is possible to switch to a when expression without subject, but that obscures the fact that our control flow depends on the Status value.

Actually I disagree with that argument. The proposed syntax:

when (status) {
  is Status.Ok && status.info.isEmpty() -> "no data"
  ...
}

seems to me to be more confusing than just using a when with no subject. Why can status be omitted from the first part of the predicate and not from the second? For that matter, what manner of beast is this guard/predicate/thing? It's evidently not an ordinary expression, since it has its own syntax rules and restrictions. That makes it significantly less appealing in my mind than the existing ability to use a full-fat predicate expression, with all of its smart-casting potential, on the left-hand-side of an ordinary when without a subject.

when {
  status is Status.Ok && status.info.isEmpty() -> "no data"
  ...
}

This is only a few characters longer than the proposed modification, behaves exactly the same, and requires zero additions to the language. If I want to make it clear that "status" is the subject, I can do so with let.

status.let {
  when {
    it is Status.Ok && it.info.isEmpty() -> "no data"
    ...
  }
}

I understand the concerns that have led to the restricted version of the guard syntax in the proposal. But those restrictions are new rules to be learned, and to be honest, I already find the left hand side of a when-with-subject to be a confusing deviation from the ordinary rules of the language. The additional restricted guard syntax makes it more confusing. Without the benefits of exhaustiveness checking, I don't think this proposal can meet the minus 100 points bar. Even if exhaustiveness checks could be added, I worry they would become complex and hard to reason about.

serras commented 3 months ago

without the benefits of exhaustiveness checking

Although this proposal doesn't concern with exhaustiveness checking, we are pretty confident that if we implement this in the Kotlin compiler, the exhaustiveness would be correspondingly upgraded.

roomscape commented 3 months ago

Thanks, that's good to know!

From the linked ticket, I gather the proposed improvements also extend to when statements with no subject. In that case, my strong preference would be to always use when with no subject, and to drop when-with-subject from the language entirely.

There's no need for two ways of doing the same thing. Once you add exhaustiveness checks, when with no subject is strictly more capable. It's also simpler to learn and understand, since it just uses regular predicates instead of needing its own special syntax.

kyay10 commented 3 months ago

@roomscape I personally think when-with-subject has great potential to be a powerful pattern-matching construct. In its current form, it isn't yet, but I think it can get there, so I don't think it's a good idea to drop it entirely.

LordRaydenMK commented 3 months ago

without the benefits of exhaustiveness checking

Although this proposal doesn't concern with exhaustiveness checking, we are pretty confident that if we implement this in the Kotlin compiler, the exhaustiveness would be correspondingly upgraded.

I think this proposal makes sense only with exhaustiveness checking. As an android dev I write code similar to the provided examples quite often. And the reason I do sealed classes for states that are mutually exclusive is because I don't have to worry about adding a new subtype and forgetting about updating the when statement.

hannomalie commented 3 months ago

Heavily agree with @roomscape 's first reply. In addition, I have three things

  1. The proposal should be updated to include a snippet how the code would look like currently, with those additional condition checks, but without guards. I gave it a try and especially with making use of extension receivers it really doesn't look like code that needs to be enhanced from my pov. (Snippet is below)
  2. With the current state and using when with subject (first example), the entries of a sealed class gave that nice and clean 1:1 mapping and made all the states pretty clear. adding an additional nesting doesn't destroy that clean thing, but i see guards destroy it. I needed to actually take a look why there is suddenly an else clause with "unknown problem". Was easier to understand before.
  3. I would say the else clause used in the first example with guards is actually a really bad idea - when the Problem enum is extended, the function will end up in the "unknown problem" branch. Might only be a problem of the given example, but then again, we should not do examples with that else branch at all.
fun Status.renderNested(): String = when(this) {
    is Status.Loading -> "loading"
    is Status.Ok -> if(info.isEmpty()) "no data" else info.joinToString()
    is Status.Error -> when(problem) {
        Problem.CONNECTION -> "problems with connection"
        Problem.AUTHENTICATION -> "could not be authenticated"
        Problem.UNKNOWN -> "unknown problem"
    }
}
serras commented 3 months ago

Indeed, in smaller examples it's arguable that the change is minimal, and as argued in the KEEP, the translation is also quite direct to non-subject version. However, this has the potential of steering the people towards a style where nested control flow is made clearer.

Some use cases where I think guards shine:

private fun FirExpression.tryToSetSourceForImplicitReceiver(): FirExpression = when (this) {
    is FirSmartCastExpression ->
        this.apply { replaceOriginalExpression(this.originalExpression.tryToSetSourceForImplicitReceiver()) }
    is FirThisReceiverExpression && isImplicit ->
        buildThisReceiverExpressionCopy(this) {
            source = callInfo.callSite.source?.fakeElement(KtFakeSourceElementKind.ImplicitReceiver)
        }
    else -> this
}
Peanuuutz commented 3 months ago

If else if is a thing, I'd prefer if over &&, because it plays better with else if and doesn't make || awkward. The cons part suggested in the proposal isn't vital enough to not consider this option as it's totally fine to regard them as nested if checks being inlined.

udalov commented 3 months ago

I'm concerned about using the existing token && because it is not at all the same as the usage in a conjunction inside a normal expression. It will make the code more difficult to read both to humans and to compilers. For example, besides the || issue mentioned in the proposal, I cannot swap the arguments of && if that is possible logically (i.e. no side effects, no type information from the left is used on the right, etc.), which breaks my mental model of what && is.

Using any other unambiguous token, including a new keyword or a new symbol combination, would be much more preferable. It will have to be learned by users, but that is a good thing.

rnett commented 3 months ago

For exhaustiveness checking, I think a current hole would be classes like Result that have boolean methods for checking the type. Would the compiler be smart enough to handle this as-is? If not, it might be a good use-case for a new contract that defines that a class is "covered" by the two boolean functions, i.e. so that the compiler can know that checking isSuccess() and isFailure() is indeed exhaustive.

kyay10 commented 3 months ago

@rnett I think something like https://github.com/Kotlin/KEEP/pull/312 would likely be more apt to fix that

serras commented 3 months ago

I cannot swap the arguments of && if that is possible logically

Unfortunately this is also the case currently with Kotlin, as data flow analysis works left-to-right. For example, person != null && person.name == "alex" does not behave similarly to person.name == "alex" && person != null.

tKe commented 3 months ago

Uniformly using if provides a nicer syntax and less surprising for those coming from similar pattern guard constructs (i.e. rust or scala), and is potentially semantically clearer (and has no confusion with disjunctions). It also lends itself to less confusion if/when more complete pattern matching reaches the language.

udalov commented 3 months ago

Unfortunately this is also the case currently with Kotlin, as data flow analysis works left-to-right. For example, person != null && person.name == "alex" does not behave similarly to person.name == "alex" && person != null.

Yes. This is why I mentioned "no side effects, no type information from the left is used on the right, etc".

quickstep24 commented 3 months ago

else if is a well-known concept

That is the problem. else if is a well-known concept, but for something else (no pun intended). if-syntax indicates the start of a statement (or expression), but here it is used as a condition. Keywords are a visual clue to the structure of the program. It bites my eyes to have when and if in one expression.

The natural syntax would be

fun foo(n: Int) = when (n) {
  0, 1 -> "a"
  is Any && otherThing() -> "b"
}

which is just two letters more (or three if the subject is nullable) and does not need explanation.

quickstep24 commented 3 months ago

Or, for the lazy, it could be

fun foo(n: Int) = when (n) {
  0, 1 -> "a"
  _ && otherThing() -> "b"
}

But maybe that is too compact.

vlsi commented 3 months ago

Should the proposal ensure that less specific conditions should go after more specific ones?

In other words, the following should better fail at compile time:

when (str) {
    is String -> println("String")
    is String && str.isNotEmpty() -> println("non-empty String") // it is never reached
}
Laxystem commented 3 months ago

Would like to note,

when (charSequence) {
    is String && charSequence.length > 5 -> ...
    "MEOW" -> ...
    else "meow" in charSequence -> ...
    else -> null
}

For elses, we can just not put a guard keyword at all.

Oh, and another thing: IntelliJ should have the option of coloring the && guard (if it is chosen) differently than the operator, e.g. as a keyword or bold.

Peanuuutz commented 3 months ago

else (&&) "meow" in charSequence

...coloring the && guard (if it is chosen) differently than the operator, e.g. as a keyword or bold.

This is one of the reasons why I dislike &&. It looks obvious and familar but has these small itches and breaks the consistency, while if fits in more appropriately.

serras commented 3 months ago

The KEEP has been updated with sections on why else is needed in some places and a description of exhaustiveness checking.

serras commented 3 months ago

Should the proposal ensure that less specific conditions should go after more specific ones?

This should be covered in principle by the reachability analysis in the compiler.

serras commented 3 months ago

We have preliminary results from our survey (which cannot be shared directly, as it contains some personal information). The comments, and the discussion that they've spawned within the Kotlin team, have pointed some weaknesses of the original proposed syntax with &&. According to the results, if is also preferred to when as the keyword leading to the guard.

As a result, a new version of the proposal has been published, using if as the syntax for guards in every when entry.

Amejonah1200 commented 3 months ago

While reading the initial proposal, I found that it might collide with future pattern matching features. As guards with if have been added, this does now, in fact, not pose any problems to introducing patterns later on.

The syntax ("else" | PATTERN) ("if" <expr>)? is much appreciated for when(value) {}, where as when {} can just have ("else" | <expr>) (as it is only plain if branches).

serras commented 2 days ago

The KEEP has been merged. Thanks everybody for their comments and suggestions!