godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

GDScript: Capture local variables consistently #8558

Open astrale-sharp opened 11 months ago

astrale-sharp commented 11 months ago

Describe the project you are working on

Hiya !

I'm working on a 2048 clone.

I first mentioned this here https://github.com/godotengine/godot/issues/85559.

Describe the problem or limitation you are having in your project

I foot-gunned myself by capturing a local variable

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

There is a consistency issue with the capture of variable.

Currently defining a variable as local to your function or your script results in different behavior :

var i = 0
func _test():
    var f = func (): i += 1; print(i)
    for k in range(7): 
                f.call()

will print: 1 2 3 4 5 6 7

While

func _test():
        var i = 0
    var f = func (): i += 1; print(i)
    for k in range(7): 
                f.call()

will print: 1 1 1 1 1 1 1

They should both print 1 2 3 4 5 6 7.

This proposal intends to fix the current inconsistency.

If this enhancement will not be used often, can it be worked around with a few lines of script?

At the cost of having more global variable yes.

Is there a reason why this should be core and not an add-on in the asset library?

Gdscript is core

Mickeon commented 11 months ago

The problem with this is that for GDScript standards it is acting exactly as it should. Some Variant types represent pointers. But because i is an int, it is always passed by value.

From an user perspective, I do agree that passing by reference feels like the expected behavior, and how other languages would handle it. It would be quite the exception to the norm, however, and it may require changing code beyond the GDScript compiler.

astrale-sharp commented 11 months ago

Which example works exactly as it should? The first one or the second one? It's the inconsistency that is the problem, that being said, being able to always capture by reference in lambda functions seems like the ideal goal.

EDIT: But yea, I agree I was surprised that it captured integer as reference, it was a nice surprise but a surprise none the less

dalexeev commented 11 months ago

The problem is that local variables may cease to exist before the lambda callable does. For example:

func _test():
    var i = 0
    return func ():
        i += 1
        print(i)

func _ready():
    var f = _test()
    for k in range(7):
        f.call()

Therefore, captured variables in this case will either raise an error or must somehow prevent the call scope from being freed while the lambda callable exists. But since GDScript doesn't have a garbage collector, we're likely to get memory leaks due to unreachable circular references that you can't destroy because you don't have access to GDScript's internals.

However, JavaScript and PHP (if you explicitly specify capture by reference) work as you would like (but they both use GC).

JavaScript ```javascript function _test() { let i = 0; return function () { i += 1; console.log(i); }; } let f = _test(); for (let k = 0; k < 7; k++) { f(); } // Prints 1 2 3 4 5 6 7. ```
PHP ```php

As for class members, although the syntax is the same, it is a different operation. In this case i += 1 is analogous to self.i += 1 or set("i", get("i") + 1), so this works. Local variables in GDScript are captured by value (if it's not a reference type); class members are not captured at all, they just use the same access syntax as local variables.

It's funny that in the case of JavaScript and PHP such a restriction would seem less inconsistent, since in these languages the prefix this. and $this-> is required to access class members, unlike self. in GDScript. Also in the case of PHP, the variable capture happens explicitly with use, and you can specify whether to capture by value or by reference (&).

I agree that this seems inconsistent, but I'm not sure it can/should be fixed, given the way GDScript works. We probably just need to document it better.

Mickeon commented 11 months ago

It's worth nothing that you can "emulate" this behaviour by using an Array. Although depending on your needs, it may not always be ideal.

func _test():
        var arr = [5]
    var f = func (): arr[0] += 1; print(arr[0])
    for k in range(7): 
                f.call()

To my knowledge, this may be even faster than using a property, because it avoids the extra overhead behind setters and getters(?)

astrale-sharp commented 11 months ago

@dalexeev Very interesting! Thanks for the explanation!

Maybe we should consider raising a warning/error when trying to modify a captured local variable then?

arnaudgolfouse commented 10 months ago

Local variables in GDScript are captured by value

Then I'm not sure I understand the observed behaviour 🤔 After it has been captured, where does the i variable "live" ? I assume it is stored in a dedicated slot of the function, but then that slot would be increased 7 times, leading to 1 2 3 4 5 6 7 (which is not what is observed)...

In other words, on which variable is the i += 1 instruction actually acting ?

AThousandShips commented 10 months ago

It lives in local scope, so it's copied in from the original value each time AFAIK, it'd essentially be like func (capture_i = i): i = capture_i ...

dalexeev commented 10 months ago
  1. The value is captured once when the lambda is created and does not change more.
  2. The lambda local does not depend on the outer local. It's just a variable of the same name.
  3. You can change the captured variable inside the lambda, but this will not change the outer variable and will not affect subsequent calls to the lambda.
  4. Arrays, dictionaries and objects are passed by reference, so as long as both variables point to the same array, it will be modified for both variables. But you can assign a new array to the captured variable, and then the original array will not be modified.
Example ```gdscript func test(): var i = 1 var f = func (): prints("f", i) i += 1 prints("f", i) f.call() print(i) i = 10 f.call() print(i) var g = func (): prints("g", i) i += 1 prints("g", i) g.call() print(i) f.call() print(i) var a = [] var h = func (): prints("h", a) a.append(1) prints("h", a) a = [] a.append(2) prints("h", a) h.call() print(a) ``` ``` f 1 f 2 1 f 1 f 2 10 g 10 g 11 10 f 1 f 2 10 h [] h [1] h [2] [1] ```
arnaudgolfouse commented 10 months ago

So, you are saying (and from what I understand of the source code):

I think I understand, but I still find it a bit weird that the function does not act on the original copy directly. Of course, now it would be hard to implement something else, but still.

Thank you for the explanation though !

AThousandShips commented 10 months ago

The reason it doesn't is explained above, and IMO having the stored value be mutated between calls makes less sense to me, if it doesn't reference a local variable in scope

astrale-sharp commented 10 months ago

@dalexeev

My main point of confusion is, since it works with Array/Dictionary, aren't we already at risk of:

may cease to exist before the lambda callable does

likely to get memory leaks due to unreachable circular references that you can't destroy because you don't have access to GDScript's internals.

?

AThousandShips commented 10 months ago

My main point of confusion is, since it works with Array/Dictionary, aren't we already at risk of

No, they are reference counted and don't have a local scope in the same way, they persist until the last copy is freed

astrale-sharp commented 10 months ago

I mean, if that's enough, surely we could Box/ReferenceCount captured local integers/float/builtins variables to get the same behavior

AThousandShips commented 10 months ago

That'd be a lot of extra overhead, adding a whole new data type, and lots more, and how would it work? Simple types don't work like that, it'd require replacing simple types everywhere, that'd be much slower and take a lot more memory

I'd also change a lot of behavior in weird ways

astrale-sharp commented 10 months ago

Not everywhere, just if it's captured? but you're right that then everywhere it needs to expect boxed builtins which maybe bad

AThousandShips commented 10 months ago

It can't selectively use them only when needed I'd say, that'd be even more complicated and error prone

YuriSizov commented 3 months ago

So I ran into this working on an undo redo system, as I was trying to share some id between the DO and the UNDO actions.


var variation_value := -1

state.add_do_action(func() -> void:
    variation_value = Controller.clone_pattern_nocheck(pattern_idx)
)

state.add_undo_action(func() -> void:
    Controller.delete_pattern(variation_value)
)

I understand the limitation, and for the time being I can hack around it with a reference type. However, consider the code above and see if it makes sense to you that two lambdas sharing seemingly the "same" variable are not actually referencing the same variable, but instead implicitly have two new variables with the same name as the higher-order scope. It's very confusing to read, it's not obvious at all that this doesn't work as expected.

There should probably be guards against this. Maybe a counter for the number of captures of local variables and a warning if it's two or more?

dalexeev commented 3 months ago

@YuriSizov At least GDScript now warns about the behavior, see godotengine/godot#93691.

YuriSizov commented 3 months ago

@dalexeev Yes, thanks, that's a good addition! Although I feel that even code without assignments might mislead developers. But that's definitely a step in the right direction.