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: Stringy StringNames #75170

Closed
wants to merge 1 commit into from

Conversation

rune-scape
Copy link
Contributor

another follow-up to #68747

assert(&"abc" is String)

but not the other way around (eg. "abc" is not StringName)

  • StringNames are iterable and indexable like Strings:
var sn = &"abc"

for c in sn:
    print(c)

assert(sn[0] == "a")
sn[0] = "b" # Error: Cannot assign a new value to a constant.

@rune-scape rune-scape requested review from a team as code owners March 21, 2023 06:31
@YuriSizov YuriSizov added this to the 4.1 milestone Mar 21, 2023
@rune-scape rune-scape force-pushed the rune-stringlike branch 2 times, most recently from cd18fee to 20dea82 Compare March 21, 2023 21:39
@rune-scape rune-scape requested a review from a team as a code owner March 21, 2023 21:39
@dalexeev
Copy link
Member

dalexeev commented Mar 22, 2023

Literal StringNames aren't allowed in match statements, as it wouldn't makes sense now

I don't see a good reason for this restriction.

type checking (is keyword) now counts StringNames as Strings

I do not think it's a good idea. In some places, String and StringName are still different things, so adding an exception to such a basic operator as a type checking seems very controversial to me. Otherwise we will have to add something like strict_is in the future. This change would make sense if StringName were completely hidden in GDScript, but it's not.

but not the other way around (eg. "abc" is not StringName)

We try to avoid inconsistent and non-symmetric behavior (for "equal" objects - in math, the equivalence relation is reflexive, symmetric, and transitive). But as I said, I don't think the previous point is a good idea.

sn[0] = "b" # Error: Cannot assign a new value to a constant.

For String, this behavior is not changed, is it? Do you see that two "equal" values have different behavior? This is another proof why changing the behavior of is is not a good idea.

In general, I'm not opposed to add the exception for match if the behavior is the same for literals, constants, and variable expressions. But I'm definitely against most of the other changes in this PR. Personally, I still think it is better not add the exception, match already works differently from the == operator (including for int vs float, String vs NodePath, arrays). Even though it may be initially confusing for users who are not familiar with GDScript. We can solve existing problems with match in a different way (improve the documentation, add a less strict match syntax, etc.).

var x = NodePath("abc")
match x:
    "abc":
        print("match")
    _:
        print("not match")
# not match

It works this way in both 3.x and 4.0, your PR doesn't fix it either, many users use String and NodePath interchangeably. The addition of StringName in 4.0 just exposed this problem. The match design doesn't always match what we want in GDScript, but that's no reason to break that design. We might add in the future an alternative less strict form of match that uses exactly the same comparisons as the == operator.

@YuriSizov
Copy link
Contributor

I don't think that "confusing by default, but with a less strict and more predictable alternative" is a good trait for a programming language feature. As @vnen mentioned, match being different is not really a required part of the design, but mostly a legacy thing. So that's not something we have to preserve, unless you can provide a good reason why match needs to be different going forward.

I agree, though, that this PR seems rather excessive in its changes, and goes way beyond the original issue.

@dalexeev
Copy link
Member

as @vnen mentioned, match being different is not really a required part of the design, but mostly a legacy thing.

In my understanding, this design (strong type checking unlike the == operator) is indeed chosen for historical reasons, you're right. As well as the fact that we didn't change this behavior before the 4.0 release, we just noticed it too late.

But what I'm trying to point out is that going against this design can be even worse (whether the design is good or bad, unintentional or deliberately chosen). Worse than exceptions can only be exceptions to exceptions. To me, this looks like an ad hoc patch that doesn't fix a problem, just one of its symptoms. But the real problem is on another level.

Perhaps in the future we will decide that 1 not matches 1.0 (Vector2i() not matches Vector2(), "abc" not matches NodePath("abc"), etc.) is also bad, and we need a less strict match syntax (the same as the == operator), say switch. If we don't break the consistency of match now, then in the future we will have match and switch with clear designs. And if we add this exception now, then in the future we will have match with the exception, mixing its behavior with switch.

(By "design" I mean not necessarily a conscious choice when making a decision, but rather the self-consistency of a feature.)

@YuriSizov
Copy link
Contributor

YuriSizov commented Mar 22, 2023

Worse than exceptions can only be exceptions to exceptions.

If match behaves exactly like ==, which seems to be a reasonable and default expectation of GDScript users, then we won't have exceptions to exceptions. It will be one, consistent behavior across all conditional statements.

@dalexeev
Copy link
Member

If match behaves exactly like ==, which seems to be a reasonable and default expectation of GDScript users, then we won't have exceptions to exceptions. It will be one, consistent behavior across all conditional statements.

I guess it's too late to change match behavior for int vs float and so on?

@YuriSizov
Copy link
Contributor

I guess it's too late to change match behavior for int vs float and so on?

I don't know, we are changing the behavior here either way. By the time we release this, in 4.1, some code will be written with the existing behavior in mind, and will be broken in some ways. And personally, I'm not sure there is a lot of code out there that depends on 1 and 1.0 being different in a match statement, or depends on nodepaths not matching strings.

The way I understood George, all of this kind of exists for legacy reasons. So IMO all of this can be subject to change if we want to improve expected behavior.

I appreciate strict type checks, but as a user I'd expect the change between if else and match to be structural, not logical. I expect the same from switch in other programming languages. Perhaps match should not be seen the same way, but we don't have switch and that's what the users lean to for a similar syntax when they need one.

@dalexeev
Copy link
Member

I have a guess why it was done this way. The == operator does not allow comparing values of any type, in some cases it can cause a runtime error (for example 1 == "str"). This is one of the reasons why we added is_same() in 4.0.

match will never result in a comparison error. Perhaps the type pre-check was used to prevent the error in the bud.

But even if we implement it differently (if this comparison with the == operator is allowed, then the behavior is the same as ==, otherwise this match branch is skipped without error), then we still get that if-elif-else is not equivalent to match. This is only true in the forward direction, if you replace if with match. If you replace match with if, the behavior still may change (comparison error). match causing the error doesn't make sense to me.

@adamscott
Copy link
Member

The PR has been reviewed by the GDScript team. We agreed that match statements should both match String on StringName, and vice-versa. But we're not sure about the idea to make StringName a "subclass" or matching with is for String.

So, @rune-scape, can you split your PR to make the match part standalone?

Thank you for your PR, very appreciated.

@dalexeev
Copy link
Member

dalexeev commented Jun 19, 2023

I think this PR should be closed as a new one has been opened (#78389). There is a consensus on what the behavior of match should be, but the rest of the changes were rejected, as stated above.

Or at least this PR should be removed from 4.1 milestone.

@adamscott
Copy link
Member

Closing the PR for the reason given by @dalexeev. @rune-scape, if there's anything, feel free to reopen it.

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.

5 participants