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

Guards #371

Closed
serras opened this issue Feb 7, 2024 · 55 comments
Closed

Guards #371

serras opened this issue Feb 7, 2024 · 55 comments

Comments

@serras
Copy link
Contributor

serras commented Feb 7, 2024

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.

@serras serras mentioned this issue Feb 7, 2024
@kyay10
Copy link

kyay10 commented Feb 7, 2024

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
Copy link
Contributor Author

serras commented Feb 7, 2024

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
Copy link

kyay10 commented Feb 7, 2024

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
Copy link

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
Copy link
Contributor Author

serras commented Feb 7, 2024

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
Copy link

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
Copy link

kyay10 commented Feb 7, 2024

@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
Copy link

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
Copy link

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
Copy link
Contributor Author

serras commented Feb 7, 2024

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:

  • Having to add a side condition to one of the branches: if you are using when with subject, this means refactoring your code to inline the subject. With guards you can just add && sideCondition.
  • Having a when expression where you handle a few complex cases, and then default for the rest. For example, take this code from the compiler, it could be rewritten as:
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
Copy link

Peanuuutz commented Feb 7, 2024

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
Copy link
Member

udalov commented Feb 7, 2024

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
Copy link

rnett commented Feb 7, 2024

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
Copy link

kyay10 commented Feb 7, 2024

@rnett I think something like #312 would likely be more apt to fix that

@serras
Copy link
Contributor Author

serras commented Feb 7, 2024

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
Copy link

tKe commented Feb 8, 2024

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
Copy link
Member

udalov commented Feb 8, 2024

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
Copy link

quickstep24 commented Feb 8, 2024

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
Copy link

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
Copy link

vlsi commented Feb 10, 2024

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
Copy link

Laxystem commented Feb 11, 2024

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
Copy link

Peanuuutz commented Feb 11, 2024

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
Copy link
Contributor Author

serras commented Feb 12, 2024

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

@serras
Copy link
Contributor Author

serras commented Feb 12, 2024

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
Copy link
Contributor Author

serras commented Feb 14, 2024

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
Copy link

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
Copy link
Contributor Author

serras commented May 16, 2024

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

@serras serras closed this as completed May 16, 2024
@daniel-rusu
Copy link

@serras , most people became aware of this feature after this issue was closed. Is this feature still open to feedback or has it been set in stone?

Kotlin has been my primary language since 2017 and I trained several teams on transitioning to Kotlin and coached countless individuals on Kotliln. The idea of guarded conditions is nice with some clean implementations in other languages. However, the current syntax and extra capabilities in the KEEP are a bad fit when taken together with existing nuances and mental model of Kotlin. I think that guarded conditions are nice to add when aligning with the pre-existing mental model but in the current form, they degrade the overall quality of the language.

Am I too late to this discussion or should I elaborate more?

@Laxystem
Copy link

Laxystem commented Jun 4, 2024

Am I too late to this discussion or should I elaborate more?

I'd say the downsides of this feature are important even if implemented as-is -- even if just for Kotlin 3.0's sake.

@serras
Copy link
Contributor Author

serras commented Jun 4, 2024

@daniel-rusu the current design is going to be implemented as described here for 2.1. This is going to come in Beta, so there's still room for additional improvements, so feel free to elaborate here; but bear in mind that at this point we may not take big U-turns in the design.

@edrd-f
Copy link

edrd-f commented Jun 5, 2024

I too feel this was a bit rushed. KEEP was opened in February and merged in May. I presume this was due to KotlinConf. The problem is that there are still important issues to be discussed, such as @vlsi's comment or the exhaustiveness topic. Both cases introduce pitfalls to the language, and on the other hand, the gains doesn't look to compensate such issues. Any chance this can be implemented as experimental, just as with context receivers?

@Laxystem
Copy link

Laxystem commented Jun 5, 2024

Yeah, this was very rushed. JetBrains have never sacrificed language quality for marketing, and I hope this is an exception and not the rule.

Languages, after all, are FOSS, and can be forked. And if JetBrains starts Microsoft-ing/Oracle-izing Kotlin, it would just be hard forked by the community. This is not a threat (who am I to maintain a language), but am just pointing out facts - it's not worth it for JetBrains to prioritize marketing over language. It'll result in tech debt and a dissatisfied userbase, which a large part of it cares enough to stop using JetBrains tooling completely (considering Kotlin's entire selling point is "Java/JS but good and without oracle/MS").

So, back to a more on topic topic - question: why guards at all, actually?

when (any) {
    is String if length == 7 -> { /*...*/ }
    is String -> { /*...*/ }
    else -> { /*...*/ }
}
// Completely equals to
when (any) {
    is String -> if (length == 7) {
        // ...
    } else { /*...*/ }
    else -> { /*...*/ }
}

This is much more readable, and is a matter of modifying the official code style, instead of adding a controversial language feature. The new K2 compiler is able to do smart casting and other flow checking even without new language features. And a short if, with no braces, would be even more readable. Nested whens, too, are even more readable - could implement a warning to use a when { } instead of an if to prevent confusion, and we're fine.

The only bonus of guards is the ability to skip; i.e. only have is String if length == 7 and is CharSequence; but why can't we just:

when (any) {
    is String -> if (length == 7) /*...*/ else continue
    is CharSequence -> { /*...*/ }
    else -> { /*...*/ }
}

It feels much more Kotlin-y for me to support continue in switches - and it's still syntax sugar for the same bytecode.

@daniel-rusu
Copy link

daniel-rusu commented Jun 5, 2024

I'll explain my concerns and sorry in advance for the length. I'm not trying to stop the concept of this feature but instead hoping to nudge the direction into a form that achieves the same goals but with fewer sacrifices. I'll take an analytical data-based approach to steer the conversation away from personal preferences.

From the Kotlin specification, one of the main goals of Kotlin is to be pragmatic for developers focusing on getting work done essentially making developers more productive:

https://kotlinlang.org/spec/introduction.html

It's well-accepted in the industry that the ratio of time spent reading versus writing code is about 10:1. This suggests we should gladly trade a significant increase in writing effort for a small reduction in reading effort. If a change increases the overall writing time by 100% but cuts the overall reading time by just 20%, productivity improves (eg. 10 hrs reading + 1 hr writing turns into 8 hrs reading + 2 hrs writing thus saving 1 hour). DSLs are a great example of this tradeoff where they can be a bit tricky to define but make the intention of the code that uses them obvious.

The effort in reading code is due to the cognitive complexity of understanding the meaning rather than the time it takes to read the text. We usually skim through code very quickly before deciding where to slow down and focus so we quickly move past 95% of code making quick assumptions along the way. When our assumptions are incorrect, we can spend lots of time going in the wrong direction. Therefore the code shouldn't just be understandable when read carefully, instead, it should be intuitively obvious, especially at the language level so we can skim through it quickly. Given that Kotlin has already reached a stable state, to test whether a new feature is readable, the majority of people who have never heard of this feature should be able to briefly look at a snippet of code that uses most of the abilities of the new feature and intuitively assume the correct meaning without having to take time to think about it or read any explanations of how this feature works. If this bar cannot be met then the cognitive effort is increased and readability is reduced.

We might think that new capabilities that reduce defect rates save us from the reading time it would take to work on those defects thereby improving productivity. This is true when preventing common categories of defects like null pointer exceptions. Considering that NPEs are 1 category of defects, several years ago I counted 50 categories of defects that were possible in Java but were prevented by Kotlin language features. In Java, we spent lots of time hunting down programming errors and carefully analyzing the possibility of mistakes. Kotlin features guarantee that most of those can't happen so we don't have to think about them. In Kotlin, we're usually hunting down misunderstandings or scenarios that we haven't considered, so the code is usually behaving as intended but our intentions were incorrect. Since Kotlin already prevents so many categories of defects, most of the low-hanging fruit has been picked. There are some promising candidates like name-based destructoring, but for the most part further defect reduction would be targetting a tiny portion of the overall defects that Kotlin developers commonly encounter. Reducing defect rates is welcome but this by itself would likely have a minimal impact on our overall productivity given the rarity of these specific defects in the current high-quality state of Kotlin.

Therefore, given that readability is the main driving force behind productivity, sacrificing readability would be going against one of the main principles of Kotlin to be pragmatic in helping developers get work done quicker. New language features that target defect rates should do so without sacrificing readability. Reducing readability also negatively affects defect rates as that increases the chance of making incorrect assumptions.

Since this turned into a book (sorry), I'll come back when I have more time and analyze this feature along with minor tweaks that should make it more closely aligned with the main philosophy of Kotlin.

@roomscape
Copy link

I agree, this discussion feels incomplete. There was some back-and-forth on the what and how, but I'm still missing the why.

What is the purpose of the proposed change?

  • It doesn't add new functionality. Exhaustiveness checks were mentioned in the proposal, but my understanding is that those are a separate feature which would work just fine without this change.
  • It doesn't make code easier to read or write. When I read this thread, I see a consensus that the existing syntax is actually more clear and even sometimes more concise than the new proposal.

I've always appreciated the rigorous discussion and lively debate that precedes any changes in Kotlin, even for standard library functions. New features mean more complexity, and so a change needs to bring a lot to the table in order to outweigh that cost and justify its inclusion. Language design changes need to meet an extraordinarily high bar, proportionate to the scale of their impact.

What problem is this proposal trying to solve, and why does that problem justify a change to an established programming language?

@francescotescari
Copy link

francescotescari commented Jun 5, 2024

+1 on the fact that this features doesn't meet the minus 100 points rule IMHO, as the inherent complexity of adding a new language construct is not justified by a large amount of use cases that would actually greatly benefit from this feature, or lack of valid alternatives (as previously mentioned in this thread).

Starting from the top, the first comment that comes to my mind when I see such piece of code in a review is "Can we refactor it?" From

fun render(status: Status): String = when (status) {
    Status.Loading -> "loading"
    is Status.Ok if status.info.isEmpty() -> "no data"
    is Status.Ok -> status.info.joinToString()
    is Status.Error if status.problem == Problem.CONNECTION ->
      "problems with connection"
    is Status.Error if status.problem == Problem.AUTHENTICATION ->
      "could not be authenticated"
    else -> "unknown problem"
}

to

fun render(status: Status): String = when (status) {
    Status.Loading -> "loading"
    is Status.Ok -> if status.info.isEmpty() -> "no data" else status.info.joinToString()
    is Status.Error -> when(status.problem) {
        Problem.CONNECTION -> problems with connection"
        Problem.AUTHENTICATION -> "could not be authenticated"
        else -> "unknown problem"
    }
}

which is much clearer IMHO (or even moving the nested when to an val Status.Error.cause: String extension if the nesting hurts your eyes)

Why would we prefer to have a flat view that mixes conditions, checking multiple variables, over a structured approach that follows more closely how a programmer reasons?

Indeed in the example below it becomes harder for me to understand when "problem try again" is the result:

fun render(status: Status): String = when (status) {
    Status.Loading -> "loading"
    is Status.Ok if status.info.isNotEmpty() -> status.info.joinToString()
    is Status.Error if status.isCritical -> "critical problem"
    else -> "problem, try again"
}

Here I have to skim through all the mixed conditions in all the branches to understand when "problem, try again" can happen. While in the "classical" example I can simply traverse the tree of conditions that brings me to the "problem, try again" leaves, which is simpler to do.

fun render(status: Status): String = when (status) {
    Status.Loading -> "loading"
    is Status.Ok ->
      if (status.info.isNotEmpty()) status.info.joinToString()
      else "problem, try again"
    is Status.Error ->
      if (status.isCritical) "critical problem"
      else "problem, try again"
}

The fact that there is duplication in this approach is not harmful IMHO as it clearly indicates that we return the same result in two different branches of this exhaustive decision tree, compare to the proposed guards solution, which is a non-exhaustive tree + catch-them-all else statement that hides the number of paths to the "problem, try again" result.

Then the following example shows how the limitations in guards actually end up making you write code that goes against the reasons why guards are added in the first place anyway:

fun testString(response: String?) {
    when (response) {
        null -> { }
        "y", "Y" -> println("You got it")
        "n", "N" -> println("Shame")
        else if response.equals("YES", ignoreCase = true) ->
          println("You got it")
        else if response.equals("NO", ignoreCase = true) ->
          println("Shame")
        else -> println("Sorry")
    }
}

Here we violate both the

  • 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.: using the else if response.equals effectively hides the fact that that branch is still depending on the response variable, but not matched directly. We might as well use a plain when without subject.
  • we need to repeat some code: here we have to repeat the println("Shame") and println("You got it") twice.

If this code was in review, I would suggest to have isYes and isNo String extensions and rewrite it as:

fun testString(response: String?) {
    when {
        response == null -> Unit
        response.isYes() -> println("You got it")
        response.isNo() -> println("Shame")
        else -> println("Sorry")
    }
}

// Or maybe we could work on this feature instead to remove the duplication, for example
/*
fun testString(response: String?) {
    when(response) {
        null -> Unit
        .isYes() -> println("You got it")
        .isNo() -> println("Shame")
        else -> println("Sorry")
    }
}
*/

Anyway, guards are not really the best option in this case as well.

Continuing,

One little step
Whereas other languages have introduced full pattern matching (like Java in JEP-440 and JEP-441), we find guards to be a much smaller extension to the language, but covering many of those use cases. This is possible because we benefit from the powerful data- and control-flow analysis in the language. If we perform a type test on the subject, then we can access all the fields corresponding to that type in the guard.

I think this is not a good replacement for comparisons with other languages. Which other languages have guards as in the proposed Kotlin version? What are the differences? Why are we mentioning full pattern matching and how do the proposed guards work similarly/replace it? Why would we want guards to cover the full pattern matching use cases with this solution, instead of adding full patter matching?

De-duplication of heads

Two challenges make this de-duplication harder than it seems at first sight. First of all, you need to ensure that the program produces exactly the same result regardless of evaluating the condition once or more than once. While knowing this is trivial for a subset of expressions (for example, type checks), in its most general form a condition may contain function calls or side effects.

I don't fully get this point. Can we get a real world use case? Also, from reading this, generally we want to perform the side effect calls only once, suggesting that the deduplicated (without guards) solution would be the preferred one.

@gildor
Copy link
Contributor

gildor commented Jun 5, 2024

I absolutely agree with all recent comments.
The question and example showed by @francescotescari, with the render function is exactly what I wanted to discuss when I first time saw this proposal, this why my downvote is on this KEEP

Unfortunately, I never found time to actually discuss it, and I am very glad that the community returned to it. I agree that KEEP was rushed and is not convincing, at least with current examples.

I would like to use render example from Keep as a contr-argument, as to why this feature shouldn't be implemented, it actually makes code worse, it would never be written this way in our real code, because exhaustiveness is an extremely powerful tool and we ready to increase the amount of code to have exhaustive checks from the compile. But this example even do not require more code, it's safer without guards, doesn't require more code and actually more readable and structured.

@Laxystem
Copy link

Laxystem commented Jun 5, 2024

A truly useful guards feature would be able to make the below function actually look like Kotlin. Otherwise, this feature simply has no use case.
    @OptIn(ExperimentalContracts::class)
    @ExperimentalSerializationApi
    private inline fun <T> determineEffective(
        original: T,
        descriptor: SerialDescriptor,
        inheritWrapping: (T) -> T,
        wrapper: Wrapper<T>
    ): T {
        contract {
            callsInPlace(inheritWrapping, InvocationKind.AT_MOST_ONCE)
            callsInPlace(wrapper)
        }

        when (descriptor.annotations.singleOrNull<NonLinkedJson>()?.wrapWithValueObject) {
            true -> return primitive(wrapper, original)
            false -> return original
            null -> { /* do nothing */ }
        }

        if (descriptor.annotations.containsAny<NonLinkedJson>()) return primitive(wrapper, original)

        var effectiveDeserializer = original // also, get rid of the var. Mutability in Kotlin, fr?

        if (descriptor.kind is StructureKind.LIST) {
            if (descriptor.annotations.containsAny<Container>()) effectiveDeserializer = wrapper.invoke(
                effectiveDeserializer,
                "quest.laxla.khuzdul.Container",
                JsonLdList,
                StructureKind.CLASS
            )
        } else {
            if (descriptor.kind is PrimitiveKind) effectiveDeserializer = primitive(wrapper, effectiveDeserializer)

            effectiveDeserializer = wrapper.invoke(
                effectiveDeserializer,
                "quest.laxla.khuzdul.Functional",
                JsonLdValue,
                StructureKind.LIST
            )
        }

        return inheritWrapping(effectiveDeserializer)
    }

    private inline fun <T> primitive(wrap: Wrapper<T>, value: T) = wrap(
        value,
        "quest.laxla.khuzdul.Primitive",
        JsonLdValue,
        StructureKind.CLASS
    )
Edit: it is possible in Kotlin today. My point worked better than I thought it would. Took an hour to write, but is incredibly faster to read than first version.
    @OptIn(ExperimentalContracts::class)
    @ExperimentalSerializationApi
    private inline fun <T> determineEffective(
        original: T,
        descriptor: SerialDescriptor,
        inheritWrapping: (T) -> T,
        wrapper: Wrapper<T>
    ): T {
        contract {
            callsInPlace(inheritWrapping, InvocationKind.AT_MOST_ONCE)
            callsInPlace(wrapper)
        }

        return when (descriptor.annotations.singleOrNull<NonLinkedJson>()?.wrapWithValueObject) {
            true -> primitive(wrapper, original)
            false -> original
            null -> inheritWrapping(
                if (descriptor.kind !is StructureKind.LIST) wrapper.invoke(
                    if (descriptor.kind is PrimitiveKind) primitive(
                        wrapper,
                        original
                    ) else original, // wrapped original 2
                    "quest.laxla.khuzdul.Functional",
                    JsonLdValue,
                    StructureKind.LIST
                ) else if (descriptor.annotations.containsAny<Container>()) wrapper.invoke(
                    original,
                    "quest.laxla.khuzdul.Container",
                    JsonLdList,
                    StructureKind.CLASS
                ) else original // wrapped original 1
            )
        }
    }

    private inline fun <T> primitive(wrap: Wrapper<T>, value: T) = wrap(
        value,
        "quest.laxla.khuzdul.Primitive",
        JsonLdValue,
        StructureKind.CLASS
    )

@Peanuuutz
Copy link

Peanuuutz commented Jun 6, 2024

The purpose of guards and part of pattern matching is to flatten. Let's reconsider the render example: (Don't mind if I simplify other parts)

sealed class Status {
    Loading,
    Error(val problem: Problem, val isCritical: Boolean),
    Ok(val info: List<String>);
}

enum class Problem { Connection, Authentication, Unknown }

fun render(status: Status): String {
    return when (status) {
        Loading -> "loading"
        is Ok { val info } -> if (info.isEmpty()) "no data" else info.joinToString()
        is Error { val problem } -> when (problem) {
            Connection -> "problems with connection"
            Authentication -> "problems with authentication"
            else -> "unknown problem"
        }
    }
}

Have you noticed that, in nowadays' Kotlin there's no way to express these structured cases in a more straight way? We have to nest over and over again to match the situations, and this is an anti-pattern of data-oriented programming, which we've been striving for back when we introduced data classes and sealed hierarchy.
Now, how about we flatten this?

fun render(status: Status): String {
    return when (status) {
        Loading -> "loading"
        is Ok { val info } if info.isEmpty() -> "no data"
        is Ok { val info } -> info.joinToString()
        is Error { problem = Connection } -> "problems with connection"
        is Error { problem = Authentication } -> "problems with authentication"
        is Error { problem = Unknown } -> "unknown problem"
    }
}

The difference is now obvious (at least more than what the proposal says). We can see how these compound cases are matched against, one for each line and each line gives us a value, without having to nest all the way into the deepest structure. Even if we don't cover all the cases like this:

fun render(status: Status): String {
    return when (status) {
        Loading -> "loading"
        is Ok { val info } if info.isNotEmpty() -> info.joinToString()
        is Error { val isCritical } if isCritical -> "critical problem"
        else -> "problem, try again"
    }
}

We still keep this "checklist" style, with a huge visual improvement from traditional if-else chain:

fun render(status: Status): String {
    return if (status is Loading) {
        "loading"
    } else if (status is Ok { val info } && info.isNotEmpty()) {
        info.joinToString()
    } else if (status is Error { val isCritical } && isCritical) {
        "critical problem"
    } else {
        "problem, try again"
    }
}

The mental burden caused by not being exhausted is yet another question - should we enforce exhaustion? Well, how about we do that:

fun render(status: Status): String {
    return when (status) {
        Loading -> "loading"
        is Ok { val info } if info.isNotEmpty() -> info.joinToString()
        is Ok -> "problem, try again"
        is Error { val isCritical } if isCritical -> "critical problem"
        is Error -> "problem, try again"
    }
}

If I were to exhaust the cases, is it clear enough? And look, everything is still aligned perfectly. We don't have jagged cases anywhere.

How about we rewrite the example above me?

SerialDescriptor { val annotations, val kind } = descriptor

val wrapWithValueObject = annotations.singleOrNull<NonLinkedJson>()?.wrapWithValueObject
val hasNonLinkedJson = annotations.containsAny<NonLinkedJson>()

when {
    wrapWithValueObject == true -> return primitive(wrapper, original)
    wrapWithValueObject == false -> return original
    hasNonLinkedJson -> return primitive(wrapper, original)
}

val effectiveDeserializer = when (kind) {
    StructureKind.List if annotations.containsAny<Container>() -> wrapper.invoke(
        original,
        "quest.laxla.khuzdul.Container",
        JsonLdList,
        StructureKind.CLASS
    )
    StructureKind.List -> original
    is PrimitiveKind -> wrapper.invoke(
        primitive(wrapper, original),
        "quest.laxla.khuzdul.Functional",
        JsonLdValue,
        StructureKind.LIST
    )
    else -> wrapper.invoke(
        original,
        "quest.laxla.khuzdul.Functional",
        JsonLdValue,
        StructureKind.LIST
    )
}

return inheritWrapping(effectiveDeserializer)

@daniel-rusu
Copy link

daniel-rusu commented Jun 8, 2024

@serras , I finally got a chance to get back to this so I'll focus on the readability problems and mental model issues and provide suggestions to address them.

As a brief refresher, my previous comment showed that reducing readability effectively goes against one of Kotlin's main principles of being pragmatic as readability is by far the largest contributor to developer productivity due to the 10:1 ratio of reading to writing code. Reducing readability also increases defect rates.

Readability Test

To test whether a change is readable, developers who haven't heard of this feature should:

  1. Be able to quickly read code that uses most of the new capabilities without needing to slow down
  2. Be able to easily infer the correct behavior without having read about how this feature works
  3. Be able to understand the meaning without increasing the cognitive overhead (no extra mental gymnastics)

I should note that any changes that break the existing mental model automatically reduce readability and thereby productivity. Lastly, we should think twice before trying to replicate the syntax of languages that have a steeper learning curve than Kotlin (like Scala, Rust, etc.) as those would usually take Kotlin into a less-readable direction.

Readability Problems

1. Breaks the mental model of wrapping if-conditions in parenthesis

The KEEP allows trivial lines like this in a when which are very difficult to read:

is Int if x > 0 -> x % 2

This is a trivial example so typical conditions are even more complex. The easy fix to this problem is to align with the existing mental model for if-statements and always require parenthesis around the condition:

is Int if (x > 0) -> x % 2

This rule should always apply even if the condition is based on a single boolean variable. Readability is much more important than saving 2 keystrokes.

2. Breaks the mental model that else-if is always connected to an if/else-if chain

Consider this simple example:

when (person.maritalStatus) {
    MaritalStatus.DIVORCED -> if (person.children.size > 0) { payAlimony(person) }
    else if (person.isEmployed) -> // employed person without children due to the previous if-statement???
    ...
}

The same confusion can happen if the previous branch had a guard because we might only use the guard condition in our mental model when quickly skimming the code instead of also accounting for the value before the guard:

when (person.maritalStatus) {
    MaritalStatus.DIVORCED if (person.children.size > 0) -> payAlimony(person)
    else if (person.isEmployed) -> // employed person without children due to the previous if-statement???
    ...
}

There is also an inconsistency because the first branch uses if for the guard whereas the second branch with an implied match-all-values uses else-if for the guard.

It's also strange for a when to morph into an else-if chain. This would get even stranger if the else-if chain can have other regular value-based branches in between.

The simplest improvement to address all this confusion would be to make it clear that each condition stands alone without implying any sort of if / else-if chain and use if every time since it's always assumed that all previous branches failed anyway:

when (person.maritalStatus) {
    MaritalStatus.MARRIED -> handleMarriedPerson(person)
    MaritalStatus.DIVORCED if (person.children.size > 0) -> payAlimony(person)
    if (person.isEmployed) -> // less confusion as the conditions are independent
    if (otherCondition) -> ...
    else -> ...
}

3. Breaks the mental model that if is either a statement or expression

When looking at a guard like:

when (person.maritalStatus) {
    MaritalStatus.DIVORCED if (person.children.size > 0) -> payAlimony(person)
    ...
}

if is no longer a statement since it doesn't have a body and it's also not an expression because we can't concatenate it with the branch value without a binary operator. So we essentially have a 3rd type of if that I would call an if-condition.

For a solution, see the next problem as that will include solutions that also address this problem.

4. Breaks the mental model of logic

This is the largest readability problem with the current design.

Branches in a when either match on a value (or type of value), or match a value and a condition (guard). However that implicit and is obscured and we get a strange syntax that we're supposed to mentally interpret as an implied and. This is a red flag from a readability perspective. It gets much worse when using functions for the value.

when (x) {
    transform(person) if (shouldTransform) -> handlePerson(person)
}

This seems like the guard is controlling whether the transform function should be called. While this example might seem contrived, note that most code looks alien to us when jumping in an unfamiliar codebase and skimming through trying to make sense of it. We need to be able to quickly skim through code and make assumptions as we're glancing at code taking just a split second to make assumptions. It needs to be intuitive to avoid increasing the cognitive load.

The initial proposal wanted to use && as I'm assuming that felt natural and intuitive but it ran into several issues. Instead of switching to a less readable form, let's tweak it to address those shortcomings while maintaining readability. Here are some proposals that fix the issues that I've outlined. Note how intuitive and readable they are as you can pretty much infer the correct meaning without extra training:

Use braces around the guard:

when (person.gender) {
    Gender.MALE && { person.age < 18 } -> handleBoy(person)
    { person.age > 65 } -> handleSenior(person)
    else -> ...
}

This essentially trades the subtle rules around braces to avoid breaking the much larger mental models around if-statements and boolean logic.

As an alternative option, we could use a special new equals function (or some other name):

when (person.gender) {
    equals(Gender.MALE) && person.age < 18 -> handleBoy(person)
    person.age > 18  -> handleAdult(person)
    else -> ...
}

We can read this as when person.gender.equals(Gender.MALE) && person.age < 18 almost as if the value of the when is the receiver. If guards get improved to support multiple values then that special equals function could be updated to accept variable arguments.

Out of these 2 options, the first option with braces would probably be better as it's more consistent with existing regular branches that don't have guards.

Feature status

The KEEP in its current state is not ready for a beta release as it introduces too many sacrifices degrading the quality of the language and reducing our productivity essentially going against the main principle of Kotlin of being pragmatic. The beta label also has a strong implied meaning that the design is essentially ironed out so only bug-fixes would be expected. At best, this seems like an experimental feature in its current state.

Here are some of the impacts of the current KEEP (mostly related to readability):

  • Increased learning curve
  • Increased syntax complexity
  • Inconsistent with existing constructs
  • Increased cognitive load
  • While guards can simplify some control flows, overuse can obscure the underlying control flow
  • Increased duplication when wanting to use the same guard & result with multiple values (or fall back to a frankenstien mix of approaches)
  • Reduced exhaustiveness checking increases defect rates

I haven't touched on performance but understanding how this code will be executed is important from a mental model perspective. Currently, we can mostly say that a when without a value compiles to an if / else-if chain and a when with a value branching on values can compile to a switch statement when it's beneficial to do so. Guards introduce a mental model gap of how the code gets compiled. This gap might be addressable with improved documentation so I wanted to touch on this since this affects our mental model which is important for readability.

Some side notes:

  • Code with lots of guards seems difficult to refactor as there's a high chance of unintended consequences due to the way multiple branches are unified.
  • Adds further friction to Java-to-Kotlin migrations. Java developers will be frustrated when reading code with this unintuitive syntax.
  • Community adoption. In its current form, I plan on updating our style guide to prevent using this feature as it adds more negative than good (we also banned position-based destructuring of classes that aren't part of the standard library)
  • Hopefully this won't negatively impact future plans for full pattern matching since they'll need to play nicely together

I hope this didn't sound too negative as I tried to be transparent and cover the key topics without turning this into a book. Readability should be a minimum requirement that should be reached for all new features. After meeting readability standards, it can be evaluated to ensure it meets other standard language requirements like the minus 100 points bar etc.

@Amejonah1200
Copy link

Increased learning curve
Increased syntax complexity
Inconsistent with existing constructs
Increased cognitive load

As a Rust enjoyer myself, I can say that guards in match/when patterns have, imo, 0 impact on those points. Guards are just a condition on whether that branch is going to be executed if the pattern does indeed match. Would that help if you knew from the start that when does pattern matching?

@daniel-rusu
Copy link

Increased learning curve
Increased syntax complexity
Inconsistent with existing constructs
Increased cognitive load

As a Rust enjoyer myself, I can say that guards in match/when patterns have, imo, 0 impact on those points. Guards are just a condition on whether that branch is going to be executed if the pattern does indeed match. Would that help if you knew from the start that when does pattern matching?

Saying that the changes in the KEEP have 0 impact on those points makes me think that personal preferences are at play rather than looking at it objectively. Or it could be one of those things where once you've gone through the effort of internalizing a concept, you forget about the effort it took to get there. For example, I met an expert skier that told me skiing is as easy as riding a bicycle so there's nothing to it. While an expert skier might feel like there is nothing to it, the truth is that they need to be much more alert and responsive while skiing compared to riding a bicycle.

You might feel very comfortable in Rust with guards but that doesn't mean there isn't an easier and more intuitive way in which the language can add guard conditions. I'm not intending to start a language war here as Rust would be my first choice for systems programming. However, while the productivity of this KEEP might seem on par with guards in Rust, Kotlin is a more productive language than Rust so matching Rust would effectively reduce the productivity of Kotlin.

I would love to see full pattern-matching support in Kotlin. I also see some value in guard conditions in the current state of Kotlin so I'm not opposed to the concept. My main concern is that the KEEP in its current form introduces too many sacrifices which break the existing mental model and significantly degrade readability effectively reducing productivity. My previous comment provided suggestions that would address my readability concerns.

While Kotlin is significantly more readable than Java in general, the KEEP in its current form makes guard conditions less readable than guards in Java:
https://blogs.oracle.com/javamagazine/post/java-pattern-matching-guards-coverage-dominance

I would like to see this KEEP enhanced to meet minimum readability standards based on the current Kotlin mental model and then continue to the next step of the language enhancement process such as ensuring soundness etc.

@Peanuuutz
Copy link

Peanuuutz commented Jun 9, 2024

Would that help if I noted that Java/C# uses when in place of guards, Swift uses where and Rust/Python uses if? The other parts are exactly the same (Let's forget about case and parentheses). It's basically just changing to a different keyword. People in these languages are accepting this concept. I don't see why it's not the case here.

@Laxystem
Copy link

Laxystem commented Jun 9, 2024

Would that help if I noted that Java/C# uses when in place of guards, Swift uses where and Rust/Python uses if? The other parts are exactly the same (Let's forget about case and parentheses). It's basically just changing to a different keyword. People in these languages are accepting this concept. I don't see why it's not the case here.

Because as mentioned above, Kotlin is a more productive language than Rust, Java, Swift, etc., so it is held to a higher standard.

Plus, Kotlin is a different language with different design choices.

Your argument is Ad Populum; the fact that others do it doesn't make it good generally, nor does it make it good for Kotlin specifically. If you're saying Rust designers know what they're doing, that's Ad Verecundiam; everyone can make mistakes.

So please, let's ignore what others do here, and talk about what Kotlin should do.

@Peanuuutz
Copy link

Peanuuutz commented Jun 9, 2024

As I said, guards and pattern matching are essential to data oriented programming. Without guards, we can only nest; without pattern matching, we can only nest. Please take a look at my comment above, the ability of easily composing conditions does make code more readable.

We can choose another keyword like where, or add parenthese around guards, but please, please don't return to nested checking, it doesn't make code better.

when (status) {
    is Ok -> when {
        status.info.isEmpty() -> "No data"
        else -> status.info.joinToString()
    }
    is Err -> when (status.problem) {
        Connection -> "Connection fails"
        Authentication -> when {
            status.level > Level.Fatal -> "Fatal error: Authentication fails"
            else -> "Authentication fails"
        }
        else -> "Unknown problem"
    }
}

when (status) {
    is Ok { val info } if (info.isEmpty()) -> "No data"
    is Ok { val info } -> info.joinToString()
    is Err { problem = Connection } -> "Connection fails"
    is Err { problem = Authentication, val level } if (level > Level.Fatal) -> "Fatal error: Authentication fails"
    is Err { problem = Authentication } -> "Authentication fails"
    is Err -> "Unknown problem"
}

@roomscape
Copy link

roomscape commented Jun 9, 2024

Pattern-matching covers a few different things. In its most basic form, like Java's instanceof patterns, it's solving the same problem as smart casting. Kotlin already does this well.

When pattern matching was extended to switch statements in Java, the goal was exhaustive type checks—in other words, to replicate Kotlin's when-with-subject.

When guards were added to switch patterns in Java, the goal was to give access to some more conditional logic within the confines of the switch. Since the switch block is what provides the exhaustiveness, any features that want to benefit from exhaustiveness have to be added to the syntax of the switch. Adding new syntax is not a decision to take likely, but in this situation it might have been the best way to achieve the goal.

But in Kotlin, things are very different. With any luck, we'll soon have exhaustiveness checks everywhere. If we want exhaustiveness plus extra branching logic, we can do it by adding exhaustiveness to our existing conditionals, instead of adding new conditionals to our existing exhaustiveness! That's awesome, because it means that—unlike Java—we're not going to be forced into adding new syntax.

We need to build on Kotlin's existing concepts and features, and solve the problems we actually have, not the ones we see people solving in other languages. Guards are designed to solve a problem that does not exist in Kotlin. Smart casting is not the same as pattern matching, and Kotlin is not the same as Java.

@Peanuuutz
Copy link

Peanuuutz commented Jun 9, 2024

There is a problem with current Kotlin - we can't early return in when-with-subject (to yield a value from it) without nesting, and guard is the solution I find the most readable and achievable.

@roomscape
Copy link

I agree about the limitations of when-with-subject 👍. And I can see that guards could be a solution. I just think we also have some other, better, more Kotlin-y options.

@francescotescari
Copy link

@Peanuuutz The examples you bring is a proposal for full pattern matching syntax (which is not yet in the language and not part of this keep). With guards only, the code will look much different:

when (status) {
    is Ok -> when {
        status.info.isEmpty() -> "No data"
        else -> status.info.joinToString()
    }
    is Err -> when (status.problem) {
        Connection -> "Connection fails"
        Authentication -> when {
            status.level > Level.Fatal -> "Fatal error: Authentication fails"
            else -> "Authentication fails"
        }
        else -> "Unknown problem"
    }
}

// With guards
when (status) {
    is Ok -> if status.info.isEmpty() -> "No data"
    is Ok -> status.info.joinToString()
    is Err if status.problem == Connection -> "Connection fails"
    is Err if status.problem == Authentication && status.level > Level.Fatal -> "Fatal error: Authentication fails"
    is Err if status.problem == Authentication -> "Authentication fails"
    is Err -> "Unknown problem"
}

/* Full pattern matching. Not a thing (yet?)
when (status) {
    is Ok { val info } if (info.isEmpty()) -> "No data"
    is Ok { val info } -> info.joinToString()
    is Err { problem = Connection } -> "Connection fails"
    is Err { problem = Authentication, val level } if (level > Level.Fatal) -> "Fatal error: Authentication fails"
    is Err { problem = Authentication } -> "Authentication fails"
    is Err -> "Unknown problem"
}
*/

Indeed, I don't get why we want to add guards before adding pattern matching, it will just cause the language to temporarily push/allow people to use the second version of the example above, and when (hopefully) we will have a better pattern matching syntax, we should switch again to the third version, causing the code written with the second version to become non-idiomatic, and have two new learning curves instead of one for the same goal. Most likely having guards with this syntax will also become a limitation of the possible syntaxes used for pattern matching. See comment here (not mine but I agree).

If you want to use the guards version, you can do it already in kotlin with minor differences:

when {
    status is Ok && if (status.info.isEmpty()) -> "No data"
    status is Ok -> status.info.joinToString()
    status is Err && status.problem == Connection -> "Connection fails"
    status is Err && status.problem == Authentication && status.level > Level.Fatal -> "Fatal error: Authentication fails"
    status is Err && status.problem == Authentication -> "Authentication fails"
    status is Err -> "Unknown problem"
}

which looks much more similar to the guards version than the "patter matching/destructured one", and even looks more coherent since status is repeated everywhere and not just for the first level of matching, while with guards it has to be repeated each time for nested variables matching (ugly).

The main focus of the discussion for me here is not about the fact that we need something more for data oriented programming (youtrack is the place for that kind of discussion), but about the proposed solution and the impact it has on the language. Guards proposed like in the merged KEEP do not replace pattern matching nicely (like in other modern languages) at all, and do not solve the exhaustiveness issue (for which the proposed solution suggests using the current standard and nesting), so when used with ADTs the proposed solution reduces the inherent safety of the language.
In your example, if we add a status.problem entry (e.g. Storage), the code will compile just fine because we have the catch-all-problem-types branch. Maybe this is fine in your example where it seems it's not that critical (just for logging?), but for real business logic I would never give up the safety of exhaustiveness with ADTs in favour of flattening a when for readibility. (Comment on the examples of the keep too: all the use examples seem to just return a logging string, which is a use case where lack of exhaustiveness and lack of safety is tolerable, while in practice 95% of the times I use a when statement I use it with ADT and I really want exhaustiveness).

This is how I would write it if I really wanted to avoid nesting (which would probably not be the case for this use case), even with guards available in the language:

when (status) {
    is Ok -> if (status.info.isEmpty()) "No data" else status.info.joinToString()
    is Err -> status.explained 
}

val Err.explained: String get() = when (this) {
  Connection -> "Connection fails"
  Authentication -> if (level > Level.Fatal) "Fatal error: Authentication fails" else "Authentication fails"
  Storage,
  OtherError -> "Unknown problem"
}

I'm not saying there is no room for improvement or that nesting is the only way forward, but just that the proposed solution has many drawbacks, doesn't add much power of expressiveness to the language, and should be reconsidered IMHO when thinking about the design of complex patter matching.

@Peanuuutz
Copy link

Peanuuutz commented Jun 10, 2024

Actually I'm also more fond of pushing off guards and waiting for full pattern matching. I'm here only to elaborate that guards do serve a purpose and are not redundant in the evolution of Kotlin language. We can make guards be experimental until that point. I'm just scared to see it being completely dropped.

@zarechenskiy
Copy link
Contributor

zarechenskiy commented Jun 14, 2024

Let me provide a bit more rationale and motivation behind our decisions.

Guards and parenthesis

This is something we argued about in the early stages of KEEP. Note how this problem arises only because if was chosen as a guard. Readability problems disappear when we replace if with &&, where additional parentheses seem unnecessary.

More importantly, we see that guards are mostly used for simple checks like isEmpty/isError/OK, followed by the main check against the subject. For such simple checks, we believe parentheses are unnecessary. However, more complex checks will require parentheses according to code style guidelines.

But why did we pick “if”?

First of all, we can't use any new keyword like given because it would introduce an incompatible change. Such an identifier could already be used as a name for an infix function (e.g., infix fun given(...)...). Introducing a new keyword would require a long migration period and potentially break existing code, which we are not ready to do.

Next, if we examine the use of potential if or where keywords, if works better because it is already used in a similar context after -> in when expressions. This allows us to make non-disruptive changes by positioning if before and after ->, and use a more familiar concept.

Lastly, if works well with else if branches. Neither && nor a new keyword fits here. Regarding the argument of 'simplest improvements' by dropping else before if, KEEP addresses this idea, you can find more details and the rationale behind the need for else in the proposal.

There was an argument about the combination of if and else if breaking the mental model. It’s an interesting observation, but consider the current combination of if and else in when expressions, which doesn't disrupt the mental model. In most cases, else and else if are clearly separated due to formatting: if you have a multiline if in your branch, there should be a new line before the next condition or else branch.


It might lead to the impression that everything is decided and there is no room for change. Please know that this is not the case. We shared this KEEP three months before KotlinConf and one of the intentions was to discuss new upcoming features in person, which was great! Does it mean we rushed with decisions? Oh, no, the idea of introducing guards as an incremental improvement for pattern matching facilities appeared long time ago, but now we finally write our conversations down and ready to move with it further.

The feature is going to be shipped in its first iteration and we’re preparing a bunch of activities to validate some of our assumptions. One of them is about the necessity of parentheses and “feature abuse”, and this is why we release features in Betas and enable them for our internal projects, to actually see how people use new features. We conducted a research before KEEP and we’re going to do it again after actual on-hands experience. So, thanks for your concerns. That’s something we’re going to recheck with more data!

Finally, I see a lot of comments thinking that just because we add guards, this is the end of the game for more data-oriented programming features. It is not. We will provide a more detailed response to address comments about pattern matching and to highlight Kotlin-specific variables involved in this design equation. We plan to show how these variables can be integrated into Kotlin's evolution, ensuring that guards is just one part of a broader strategy for enhancing data-oriented programming features.

@edrd-f
Copy link

edrd-f commented Jun 14, 2024

@zarechenskiy thanks for the follow-up. It's great that this is going through another cycle of feedback based on actual uses of the feature, however, I wonder if it shouldn't be released in a prototype/experimental/alpha stage first. Beta usually means "We are committed to the design, but it still needs bug fixes and optimizations". What's the reasoning for jumping straight to Beta in this case?

@zarechenskiy
Copy link
Contributor

however, I wonder if it shouldn't be released in a prototype/experimental/alpha stage first

@edrd-f Valid point!

I used the word 'iteration' in my answer for a reason: terms like alpha and beta vaguely describe the status of features. We’re going to shift to 'iterations' to avoid the assumption that a feature in Beta means it will be released in the next version no matter what. We will provide proper IDE and compiler support for features in any iteration, but we also want to give our language the time it needs

@kyay10
Copy link

kyay10 commented Jun 14, 2024

@zarechenskiy So does that mean that Beta means "proper IDE and compiler support" while alpha/prototype/etc means "some compiler support, minimal IDE support" or something along those lines?

@zarechenskiy
Copy link
Contributor

zarechenskiy commented Jun 14, 2024

So does that mean that Beta means "proper IDE and compiler support" while alpha/prototype/etc means "some compiler support, minimal IDE support"

@kyay10 Right

@gildor
Copy link
Contributor

gildor commented Jun 20, 2024

I may be missing some important use cases, but I still don't see any good example on Keep, where guards are improved code. Examples from KEEP about error messages and Yes/No parsing are already questionable enough and many examples in this thread show that without this feature they could be rewritten at least not worse, but arguably better than the proposed Guard solution
Breaking exhaustiveness of the when expression a critical issue, which I don't see would be acceptable in our code base.

Mihail mentioned it as a partial solution. Maybe I don't see the whole picture, but I agree with some of the commenters that in the current state, it doesn't follow the -100 points principle, creates hard-to-read code, and even promotes a bad style of programming (so it goes against what is considered good style with exhaustive checks as much as possible now).

If it is just a step, and potential improvements in the future require this, I can see it, but I just don't see that we ever allowed to use this style in our code base, maybe I just missing examples where I would say "yes, now it's more clear/readable"

Considering that this feature will be added as experimental any case, I think we should just wait and see, hope that we will get a new KEEP to discuss evolution and feedaback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests