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: Add static typing for for loop variable #80247

Merged

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Aug 4, 2023

One of the inconvenient things in 3.x for users who prefer static typing is the for loop variable. Things have improved a lot in 4.x as we have added typed arrays and generally improved type inference when possible.

However, dictionaries are still untyped (see also PR #78656), plus other untyped scenarios are also possible. There is a workaround with declaring a new local variable, but it is less convenient and nice:

var dict := {a = 1, b = 2, c = 3}
for _key in dict:
    var key: String = _key
    # ...

This PR adds the ability to declare static type of for loop variable, saving you from the problem:

var dict := {a = 1, b = 2, c = 3}
for key: String in dict:
    # Type inference and autocompletion work for `key`.
    var length := key.length()
    print(length)

If the runtime type is different, this will result in an runtime error:

var dict := {1: 2}
for key: String in dict:
    print(key)
# Runtime error: Trying to assign value of type 'int' to a variable of type 'String'.

Also added a warning if the specified type is a supertype of the inferred type.

@dalexeev dalexeev added this to the 4.x milestone Aug 4, 2023
@dalexeev dalexeev requested a review from a team as a code owner August 4, 2023 09:47
@dalexeev dalexeev force-pushed the gds-for-loop-var-static-typing branch from c6938e1 to a5be2af Compare August 4, 2023 09:54
@dalexeev dalexeev requested a review from a team as a code owner August 4, 2023 09:54
@dalexeev dalexeev force-pushed the gds-for-loop-var-static-typing branch from a5be2af to ab64675 Compare August 4, 2023 10:02
@Mickeon
Copy link
Contributor

Mickeon commented Aug 8, 2023

In this PR, what happens with the following line of code (which could be a common use case to save some lines):

# I want to iterate through all of the child lights.
for light: PointLight2D in get_children():
    print(light)

Because it's not always guaranteed that all nodes are PointLight2D, if this is run, is null sometimes printed (akin to using the as keyword), or does it lead to a run-time error?

@dalexeev
Copy link
Member Author

dalexeev commented Aug 9, 2023

if this is run, is null sometimes printed (akin to using the as keyword), or does it lead to a run-time error

There will be a runtime error. This code is similar to this

for _light in get_children():
    var light: PointLight2D = _light
    print(light)

not this

for _light in get_children():
    var light: PointLight2D = _light as PointLight2D
    print(light)

A static type is for the situation where you expect all elements to satisfy the type. If this is not true and you want to skip elements of the wrong type, then you should use

for child in get_children():
    if child is PointLight2D:
        var light: PointLight2D = child
        print(light)

or

for child in get_children():
    var light := child as PointLight2D
    if light:
        print(light)

@geekley
Copy link

geekley commented Aug 9, 2023

Cool, I'm just not sold on the double : in the syntax.
On one hand it's consistent with type declarations on variables and parameters. But on the other it's inconsistent (and quite ugly) to mix 2 : that have different purposes on the same line. I think it may increase visual clutter, considering it could slightly confuse with technically-valid syntax like one-liners if x: if y: f() (yes, I know this is discouraged, but my point is that a : followed by an expression can make you think of that).

Personally I would prefer:

for String key in dict:
    print(key)

specially if in the future we could have type narrowing with syntax x is MyType y like in C#, which I really like.

Other alternatives:

for key as String in dict:
    print(key)

or even just

for key String in dict:
    print(key)

is much cleaner IMO.

@dalexeev
Copy link
Member Author

dalexeev commented Aug 9, 2023

@geekley See godotengine/godot-proposals#632 (comment). The as has different semantics, people might think it works like the as keyword (cast to null or skip the iteration in case of type mismatch), but it doesn't. Also C-style (Type name) is not used anywhere in GDScript, I don't think it is a good idea to use it here.

As for the double colon, we already have this with variable properties (set and get):

var a: int:
    set(value):
        a = value

In my opinion, the double colon in for looks much better than above (since there is in expression), so I don't think we should worry about it.

@oparisy
Copy link

oparisy commented Aug 17, 2023

Also added a warning if the specified type is a supertype of the inferred type.

@dalexeev What is the intent of this warning? I can't really think of an equivalent in other languages/compilers, and as I described in godotengine/godot-proposals#632 OO-wise there are situations where declaring a type upper in the inheritance hierarchy is desirable (to work on a simple interface instead of a complex concrete class for example).

@dalexeev
Copy link
Member Author

@oparisy In my opinion, this is unusual behavior. If we can infer the iterator type, it's weird to specify a supertype. Only if you want to assign other value to the variable in this iteration, which is also weird in my opinion, programming languages usually recommend declaring new variables rather than using the same variable for different values.

Note that specifying the iterator type is somewhat different from other cases. The iterator already has a type inferred from the iterated value. You can specify a more specific type for convenience, although this is unsafe. I would not recommend specifying the iterator type if the inferred type suits your needs. Unlike regular variables where we recommend specifying the type explicitly and using := only the inferred type is obvious. Specifying type in each for would be too verbose.

That's why I added the warning. But this is just my opinion, let me know if you disagree and why. That's a good question for a future GDScript team meeting, thanks.

@oparisy
Copy link

oparisy commented Aug 17, 2023

That's why I added the warning. But this is just my opinion, let me know if you disagree and why.

WRT use cases for deliberately specifying a supertype of the inferred type, I added those that came to my mind on the linked issue already, so I'm not sure what to add.

But do you mean that you mostly added this warning to help people keep their code readable, by avoiding the explicit type specification in for loops when it's redundant with inferred type? If so it makes perfect sense!

if (list_resolved) {
variable_type.type_source = GDScriptParser::DataType::ANNOTATED_INFERRED;
variable_type.kind = GDScriptParser::DataType::BUILTIN;
variable_type.builtin_type = Variant::INT;
list_visible_type = "Array[int]"; // NOTE: `range()` has `Array` return type.
Copy link
Member

Choose a reason for hiding this comment

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

Does this bring issues?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is just a note as the error message can be confusing. There is a suggestion to actually make the return type Array[int], see #67025.

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

This is awesome, and it looks good! Thanks @dalexeev for this PR.

@akien-mga akien-mga requested a review from vnen August 17, 2023 14:09
@dalexeev dalexeev force-pushed the gds-for-loop-var-static-typing branch from ab64675 to 6c59ed9 Compare August 17, 2023 17:54
@Loregret
Copy link

Does this PR improve the performance of loops?

@dalexeev
Copy link
Member Author

Does this PR improve the performance of loops?

It depends on the code. By itself, this change adds an opcode to runtime (if a static type is specified and it is not redundant), which slightly reduces performance. But in general, if the iterator variable is used several times in the loop body, then it can increase performance.

@dalexeev dalexeev modified the milestones: 4.x, 4.2 Aug 20, 2023
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Discussed in the GDScript PR meeting, we approve this PR! Thanks @dalexeev !

@akien-mga akien-mga merged commit 7d3bee7 into godotengine:master Aug 21, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@dalexeev dalexeev deleted the gds-for-loop-var-static-typing branch August 21, 2023 17:36
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 static typing for loop variables in for loops
7 participants