[Core] `Array` and `Dictionary` read only behavior is unsafe about godot HOT 10 OPEN

I agree with you, and the proxy sounds like a good idea !

Just to be sure I understand what you meant :
Lets say the [ ] operator of Array return an ArrayItem.

• As you said it would have a conversion operator to Variant.
• And for the the & operator it would return a const Variant * or a Variant *, address of the proxied Variant.

The potential issue I see here is that if the Array is read-only the & operator should return a const Variant * and a Variant * if not. But we don't know that at compile time...

from godot.

A potentially dirty but I'd say sufficient solution might be to add a proxy for the non-const return, akin to CharProxy of strings, this would error on write and handle that correctly, and be safely cast to Variant, but unsure exactly what that would entail

Alternatively the proxy would simply work like the read-only value but individually for each copy

Edit: Doing some testing with a proxy based solution

from godot.

CC @reduz @RandomShaper

from godot.

We could also make the Variant read-only by itself, it has already a method is_read_only but that only checks for read-only arrays and dictionaries. But I like the proxy idea as well.

from godot.

The is_read_only refers to if it is a read-only Array or Dictionary, I don't think messing with the insides of Variant is useful here, the issues should be solved in Array and Dictionary IMO

from godot.

My idea was the following :
If a Variant is marked as "read-only" the underlying data cannot be modified.
In this context a read-only array contains only Variants marked as "read-only". Trying to use the = operator would fail.
Of course this doesn't serve the same purpose as the original is_read_only as you mentioned.

from godot.

Again I'd say this fix should be local, not in Variant, only of no other way to fix this can be achieved reasonably should we mess with Variant IMO

from godot.

With some testing the immediate concern is that the resulting value would need to copy a lot of behaviour from Variant if it is to be a drop-in replacement, as c++ doesn't allow access via casting etc., so with a proxy type you can't trivially do arr[0].get_type() unless those methods are all transferred

Adding some read-only state to Variant would have its own issues, like how would you transfer it to get a mutable result? It'd only make sense to retain immutability on reference, not copy, so that makes the read only state very niche

Will keep testing out some ideas for the proxy and potential adjustments across the code to account for it

The most trivial solution would be to replace the current read only data with another vector, but that would be pretty fragile I think, and wasteful, the issue is having the ability to produce a unique, discardable value for each access, but that would be difficult without leaking memory

The even more trivial but less safe solution would just be to not protect the subscript at all, this might be an acceptable temporary solution

from godot.

Some candidate solutions:

• A proxy object, see above for concerns
• No protection at all, just return a reference
• Add some kind of individual protection without a proxy (can't imagine how)
• Rewrite access to work like Vector with a write proxy that simply ignores the value if read-only, this would probably be the simplest but would require a lot of reworking to replace the access that would now use non-const references

Did some basic work but probably won't be able to focus on a direct fix on this for the time being, but some loose pointers

from godot.

That proxy return variant is cursed. 🙈

There are 3 constraints the array currently tries to fulfill:

Variant get(int p_index) const;
Variant const& get_ref(int p_index) const;
Variant& get_mutable_ref(int p_index);

Any solution needs to satisfy the above. The get_ref could be deleted.

from godot.