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

GDScript: Make match behavior consistent (String vs StringName) #74872

Closed

Conversation

dalexeev
Copy link
Member

This was introduced in #68747, but I don't think we really wanted it for match. Unlike the == operator, match in GDScript has always been type strict, even 1 and 1.0 are considered different for match. #60145 wasn't really a bug, it's a documentation issue, see also #74462.

I also noticed the following bug in 4.0.stable, so we have a choice. 🙂

CC @rune-scape

@rune-scape
Copy link
Contributor

rune-scape commented Mar 14, 2023

I also noticed the following bug in 4.0.stable, so we have a choice. 🙂
image

this was intentional, the second is the normal behavior, that i didn't touch. the former (as in a StringName literal matching when given a String value) could be removed, because the intention was to make specifically a String Literal match when given a StringName Value. nothing more

from godot 3.x to 4 some return values changed from String to StringName, making some projects break with a very hard to catch bug. it can look like the match statement is simply broken.
match is very much type strict, but theres a good reason for this exception, it weeds out 3.x to 4 project conversion bugs, and makes match statements simpler to use without really changing how they work

i also personally think that String and StringName should have (more) exceptions to some type rules, to make using them more intuitive, that was what #68747 was for
i would be For making String and StringName match each other in all cases (in GDScript contexts) because the type difference between the 2 is near nothing compared to the difference between an int and a float.

@dalexeev
Copy link
Member Author

match is very much type strict, but theres a good reason for this exception, it weeds out 3.x to 4 project conversion bugs, and makes match statements simpler to use without really changing how they work

I think overall consistency is more important in the long run than the converter issue, which we could solve like this:

match expr:
# -->
var c3to4_tmp1 = expr
match str(c3to4_tmp1) if c3to4_tmp1 is StringName else c3to4_tmp1:

this was intentional, the second is the normal behavior, that i didn't touch. the former (as in a StringName literal matching when given a String value) could be removed, because the intention was to make specifically a String Literal match when given a StringName Value. nothing more

To me this looks like a bug. If we decide to keep this exception in match, then I would prefer it to work the same way for literals, constants, and dynamic expressions.

@vnen
Copy link
Member

vnen commented Mar 14, 2023

match has always been type strict, so I would keep it consistent for String vs. StringName. I know many people tend to complain about this behavior. It probably warrants some discussion (although I couldn't find any existing proposal about this) but for now let's keep the way it already is.

If we don't want it to be type strict, then it should apply to all types.

Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Chaosus Chaosus added this to the 4.1 milestone Mar 14, 2023
@YuriSizov
Copy link
Contributor

I think merging this will open the Pandora's box again. Users were very frustrated throughout the 4.0 preview builds before String and StringName got unified. This is because many engine APIs would return a StringName, where the difference from the scripting perspective is very superficial. There is no clear indication that when comparing you should prepend your literals with an ampersand to work with those APIs, other than a return type which many miss the meaning or purpose of.

While the changes from this PR can be worked around with an if-else statement, it brings more questions and confusion as, once again, it is not immediately clear why these strings and those other strings returned by the engine APIs work slightly differently. Having the float vs int precedent is a good argument, but I feel like those two are a bit more accepted to be different than String and StringName (which is also an entirely "Godot concept"). And even then we have issues with float vs int and some users.

I understand the desire for consistency, but I'd propose we arrive to a consensus that wouldn't inconvenience a lot of users.

@rune-scape
Copy link
Contributor

rune-scape commented Mar 16, 2023

I would prefer it to work the same way for literals, constants, and dynamic expressions.

i agree, and i think String and StringName should match each other in a match statement, i maybe should have made that change in #68747, im going to see about fixing that and a few other things i missed about String and StringName

the type difference between the 2 is near nothing compared to the difference between an int and a float.

i wanted to clarify what i meant by this is that String and StringName can be converted between each other without losing any information (unlike int and float). not knowing which of those types a value is (should) make No practical difference to a gdscript user, just a performance difference, this is why i think StringName should be the exception to strict typing in a match statement

@adamscott
Copy link
Member

The GDScript team pondered on this PR and the decision was made to accept match-es of String and StringName as equal.
So that PR cannot unfortunately be merged.

Thanks by the way for the PR, very appreciated.

@dalexeev dalexeev closed this Apr 14, 2023
@dalexeev dalexeev deleted the gds-match-string-vs-string-name branch April 14, 2023 16:51
@vnen
Copy link
Member

vnen commented Apr 14, 2023

BTW, I did bring this up first, which lead up to this PR. I should have gathered some more opinions before match should be changed.

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.

6 participants