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 · 27 comments
Closed

Guards #371

serras opened this issue Feb 7, 2024 · 27 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
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