godotengine / godot-proposals

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

Add support for typed WeakRefs to GDScript #9174

Open AThousandShips opened 9 months ago

AThousandShips commented 9 months ago

Describe the project you are working on

N/A

Describe the problem or limitation you are having in your project

WeakRef is great in a lot of ways and saves a lot of work, however they are a bit cryptic in their usage and can be hard to use, and they are especially bad for code readability and conveying intent.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

A new syntactic sugar feature for GDScript which would be WeakRef[T], this would both be a symbolic feature, indicating intent to someone reading the code, and also be enforced on assignment, at least by the analyzer or in the editor/debug builds. It would also provide static type hinting when using the get_ref method, if desired we could also make the weakref method able to hint at the result type as well if the argument is typed, so var my_ref := weakref(foo) would be typed WeakRef[Foo] if foo was typed as Foo.

See also #7363 for discussion on exporting WeakRef and related, where this would also be useful for various aspects of this. With the possibility also of syntactic sugar for a weak export, see https://github.com/godotengine/godot-proposals/issues/7363#issuecomment-1935879066 for details.

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

This would be a parsing and compilation feature of GDScript, some form of metadata on the associated variables, and analyzer code. It could also be enforced more strongly on other levels with the VM and similar.

Some question marks I have are how we would handle assignment from other WeakRefs, how null would be treated, and similar, but those are aspects I think would be part of working out how to implement it and use it.

The above suggestion for syntactic sugar for exports would also be an optional alternative solution to this, as WeakRef is more relevant IMO in exports than using them as local variables, and in that case the loss of readability and intent is less significant.

It could also be a higher level feature in C++ as well, with some template <T> TypedWeakRef : public WeakRef with associated typing management, if needed, that would make the GDScript side of things less critical and would use methods similar to how TypedArray works, but since WeakRef is used much less on the C++ side of the engine this would be less critical, though it would be more powerful, and again provide the same benefits. Though this isn't possible in the current engine architecture due to class bindings with templates.

The solution might be to add typing features to WeakRef like we have for Array, I suspect that would be the most convenient method in the end.

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

It can be worked around by adding code comments, but that isn't very clear or clean.

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

It is a core feature of GDScript.

AThousandShips commented 9 months ago

Exploring some possible implementations with the typed array style solution, will see if it leads anywhere as a start

dalexeev commented 9 months ago

See also:

I think to implement this properly we would at least need some refactoring of GDScriptParser::DataType (and GDScriptDataType). But I'm not sure that we can provide runtime guarantees without affecting WeakRef (Array uses runtime checks).

I think that for a proper implementation we would need a unified type system with first class types (the type whose values represent types). This will allow nested types (like Array[Array[int]]), generic methods (like Array[T].back() returns T, not Variant), and potentially things like PackedScene[T], WeakRef[T]. I plan to write a proposal about this later. We've discussed this several times on the #gdscript channel in RocketChat.

AThousandShips commented 9 months ago

Managed some basic ideas for this on the C++ side but got bogged down trying to work with some other ideas, I still consider it relevant but I found my original use case less critical when looking at implementations, so this much broader generics and deeper systems for it feels more appropriate as I feel the original need for the weak references in this fashion to be less urgent as such, and looking forward to future improvements of the generics, with a unified type system!

Thank you for your feedback, and hopefully we can find a more unified proposal idea for this, I'd love for this idea to be part of a wider system, even if it does mean delaying it

Since exporting WeakRef isn't currently something supported well (did some work on serializing WeakRef but decided it wasn't worth it) the alternative of specifically an @export_weak annotation wouldn't be much of a feature in the meantime either

Mickeon commented 8 months ago

Out of curiosity I went around and checked what other Object classifies as a container, and could possibly be typed in the API to justify something more feature-complete in... some future. There isn't actually much. PackedScene and WeakRef are outliers. So if something is done now it could be unified later more nicely. But it does not sound simple at all, so it really may have to wait.

For your interest, it really is a stretch. PackedDataContainer, TileSetScenesCollectionSource, MeshLibrary...

Mickeon commented 8 months ago

How could I have been so stupid!? InstancePlaceholder would GREATLY benefit from this, too.