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

continue in match makes the control flow fallthrough implemented #37684

Conversation

ThakeeNathees
Copy link
Contributor

@ThakeeNathees ThakeeNathees commented Apr 8, 2020

@Calinou
Copy link
Member

Calinou commented Apr 8, 2020

Wasn't this implemented in Godot 3.0 already? Or did the feature break over time?

@ThakeeNathees
Copy link
Contributor Author

@Calinou in 3.0 it doesn't fall through, continue to search for another match instead

@akien-mga akien-mga added this to the 4.0 milestone Apr 10, 2020
@vnen
Copy link
Member

vnen commented Apr 21, 2020

This changes the meaning of continue in match statements. It's probably more useful like this (see godotengine/godot-proposals#160) but it's worth it pointing out, as it will also need a documentation change.

@ThakeeNathees
Copy link
Contributor Author

ThakeeNathees commented May 9, 2020

documentation already states that continue will fall through the execution. (https://docs.godotengine.org/en/stable/getting_started/scripting/gdscript/gdscript_basics.html#match)

Crash-course for people who are familiar with switch statements:

  1. Replace switch with match.
  2. Remove case.
  3. Remove any breaks. If you don’t want to break by default, you can use continue for a fallthrough.
  4. Change default to a single underscore.

@vnen

@swarnimarun
Copy link
Contributor

WeW I was just writing a state machine and this tripped me up, bad... :3
(Trust me pure coincidence)

Would someone mind marking it a bug-fix pls? (At least it inconsistent with the docs)
@ThakeeNathees is absolutely correct.

Or should I fix it in the docs for now for older versions?

@vnen
Copy link
Member

vnen commented May 22, 2020

Would someone mind marking it a bug-fix pls? (At least it inconsistent with the docs)

Or should I fix it in the docs for now for older versions?

That's the thing, I don't know if this is a bug. It's been there for so long now that I'm afraid some people might be relying on this behavior. Even though potentially "relying on a bug" is not safe, changes in behavior in a game engine can completely break the game. so it's usually avoided in a patch release. Maybe if we ever do a 3.3... (definitely will change in 4.0 though).

The reason I don't think it's a bug is because the original pull request mentions this as the implemented behavior:

A continue stops execution in the current block and checks the next pattern instead.

However, the same person wrote the docs and stated that it was the same as a fallthrough in a switch statement, maybe missed at the time that a switch doesn't check the next case but just run the code in the block. It was probably an afterthought (which is evidenced by the original discussion #6845). Which is natural given that all the languages that provides pattern matching don't have any fallthrough mechanism. The discussion also mentions the implemented behavior as expected (to replace pattern guards in a way).

I would say that the documentation is wrong. When docs don't match code, usually the issue is in the docs. I was surprised by this too (since I've never used the "fallthrough" in a match statement), but that's how it was originally meant to be.

@swarnimarun
Copy link
Contributor

@vnen oh didn't know about the discussion but that's the same conclusion I drew from it. The only reason it was a little weird was, that, I was expecting it to work like Python.

I changed my code to follow a more pattern matching notion. Pretty sure it's something that was inspired from Rust.
My entire code just got a major facelift. :)

@vnen
Also if we go by pattern matching notion I don't think this change is valid. Or even possible, afaik fall through and pattern matching is incompatible.

For example say,

var val = [120, 34]
match val:
    [var x, var y]:
        continue
    var z:
        # do something with z here
        pass

@ThakeeNathees How will it be handled by fallthrough.

Without fallthrough it will go from first case to second.

@ThakeeNathees
Copy link
Contributor Author

1. match val:
2.    [var x, var y]:
3.       continue
4.    var z:
5.        # do something with z here
6.        pass

once the execution met continue it'll not check the pattern again. in the above code after the the line 3 continue the execution just jumps to line 5 and since line 6 don't have continue it'll jump out of the statement
@swarnimarun

@swarnimarun
Copy link
Contributor

@ThakeeNathees exactly my point it is not handled..... :3

There is no definition of z which means it will error.

@groud
Copy link
Member

groud commented May 23, 2020

I believe you misunderstood what fall through means in the documentation, and that the current behavior is perfectly expected.

The initial reseasonning behind the continue statement in match operations was first to avoid the need, as in a lot of other languages, to have a break in every statement. So instead of having this break, you would only have to specify when you want your code to check the next statement too.

So in GDscript, you never need breaks, but if you want the next statement to run too, you can use a continue.

Honesty the documentation has been really clear to me, I understand it' s not the behavior you have in several other languages with the continue keyword, but IMHO, it completely makes sense.

That being said, to avoid confusion with other languages we could maybe rename the "continue" keyword in a context of a match statement, but I don't have any idea of a good alternative.

@swarnimarun
Copy link
Contributor

Maybe even worse. (I read the commit code too before mentioning the issue, but I have no idea how runtime issues like these are handled in GDScript but I very much assume the best case would be a perf hit and runtime error or worst case a segfault)...

Also from a pattern matching standpoint, this PR and proposal just don't seem useful.

A rematching case is much more powerful than a fallthrough.

Heck there are causes for concern that go much deeper,

match val:
   [var x, var y]:
       print(x, ", ", y)
       continue
   var x:
       print(x)
       continue

@ThakeeNathees
What about this... \o\
\o/ This is one fire I don't want to touch.... :3

@swarnimarun
Copy link
Contributor

@groud That seems like a sensible solution.

Also, I was only confused because it wasn't clear to me that there was "pattern matching" with match statements.
I think rematch is a good candidate for the change. LoL

On a serious note, I think some change to the docs to mention this behaviour for the older versions of the doc is necessary, and a rename for the 4.0 maybe. Because like me almost everyone, even ones who are used to pattern matching might just assume things about GDScript because of Pythonesque-ness of the language.
Docs being vague doesn't help.

@ThakeeNathees
Copy link
Contributor Author

match val:
   [var x, var y]:
       print(x, ", ", y)
       continue
   var x:
       print(x)
       continue

@swarnimarun if gdscript is an interpreted language we just can't jump from continue to print(x) because x is not defined. but here var x is already compiled so in the var x: pattern block x is already defined.

but the problem is that since the capture val to x line doesn't execute the value of x will be the same as val when just before match block started to execute, I'll fix it if we decided to go with this however since the new parser will implement this and people might be relying on this behavior I'm agree with @vnen

@swarnimarun
Copy link
Contributor

Oooof it was sarcasm both the times...
(But I guess it's nice if someone else is reading it)

I know what the issue is and print is an example when it will cause a bug silently, using a function that requires certain type would mean that it will show the error at entirely wrong place. This is something that just can't be fixed.

Because you can't just fall-through to next step here, you will have to match on pattern to get the next value. Which is exactly what it does at the moment...

So I am not sure what you plan to do to fix it but it is already fixed.
If you continue it starts matching with the next case pattern.

Only difference is if the match fails it skips the case.

This means it falls through but just doesn't do it without matching.

I am assuming you need a case like,

match month:
    Jan:
    Dec:
    Mar:
    May:
    Jul:
    Aug:
    Oct:
          return 31

You can do a clean "or" separated multi pattern match instead which is much more sane.
Which will be much better syntax.

Now if you want conditional fall through, that's IMHO what I like to call code smell.

You have array based matches like,

[var x, ..]
[_, _, var x]

If you want more pattern scenarios can be added which wouldn't be bad but I don't see how changing the usage of continue would be useful.

On the contrary now I find that it would make it rather confusing to use, as you won't be able to use any of the patterns with var bindings in the subsequent cases the same way as ones without them if you used continue based non-matching fall through.

Features that require adding exceptional cases without much useful functional enhancement are not something I would like.

At the very least I am against replacing the current functionality.

@vnen
Copy link
Member

vnen commented May 23, 2020

I believe you misunderstood what fall through means in the documentation,

Honestly the way it's written begs for this misunderstanding. It's a series of steps on how to replace switch with match and states that if you need a fallthrough, you add continue. In a switch-case statement, "fallthrough" means just running the next block without checking for the value. Everyone would assume that's how it works. In fact, the text on how to replace a switch is wrong.

It only mentions continue once more:

If you want to have a fallthrough, you can use continue to stop execution in the current block and check the ones below it.

Which again misuses what a "fallthrough" means in this context. The text is now changed to better note it checks the next patterns. But I still think "fallthrough" should be removed (though this should be discussed in docs).


@swarnimarun

Also if we go by pattern matching notion I don't think this change is valid. Or even possible, afaik fall through and pattern matching is incompatible.

For example say,

var val = [120, 34]
match val:
    [var x, var y]:
        continue
    var z:
        # do something with z here
        pass

This is the reason it's not available in other languages as well. For example in Rust:

match x {
    None => None,
    Some(i) => Some(i + 1),
}

If you go from the first branch to the next, what would i stand for?

So the current behavior is the best we can do without getting in this conundrum.

However, it's only there to replace guards, so I wonder if we should remove continue and add guards... But that's another discussion anyway.

@swarnimarun
Copy link
Contributor

swarnimarun commented May 23, 2020

@vnen I would like guards as well maybe we can discuss it in a thread or proposal for it. But for now continue is enough.

For 4.0 for sure though guards would be a better solution. But may as well discuss it further at a proper place.

@akien-mga
Copy link
Member

Given that this is a fairly old PR and that the discussion is mostly about the feature proposal and not the implementation, I suggest we close it and keep the discussion focused in godotengine/godot-proposals#160.

See also godotengine/godot-proposals#4775 for a recent related proposal.

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

Successfully merging this pull request may close these issues.

Add fallthrough for match cases
6 participants