[Core] `Array` and `Dictionary` read only behavior is unsafe

[AThousandShips]

Tested versions

N/A

System information

N/A

Issue description

For example:

• GDScript: Add tests for calling with wrong arguments in Callable.callv() when passing a readonly (const) Array #93636

You also have that, in C++, the following is broken, for example:

Array tmp;
tmp.push_back(1);
tmp.push_back(2);
tmp.make_read_only();
assert(tmp[0] != tmp[1]);

All due to the fact that the return from the subscript operator and Array::get return a reference to a single internal read_only value

This would also cause problems in cases like:

const Variant &first_value = arr[0];

for (int i = 1; i < arr.size(); ++i) {
    // Do something involving access to `arr`, if the array is read-only it will change the value in `first_value`.
}

The main problem is that the non-const subscript operator has to return a non-const reference, if this wasn't necessary we could safely return const Variant & for the const operator and just trust that no one messes with it with casting, just like the const operator for LocalVector, but since we might be working with a non-const but read-only Array reference we can't reasonably error or crash when accessing the non-const operator.

---

A possible temporary improvement would be to remove the use of the read-only data in the const access cases (this is already the case in Dictionary), that would reduce the impact somewhat, but the non-const case is more tricky.

Steps to reproduce

N/A

Minimal reproduction project (MRP)

N/A

[AThousandShips]

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

[AThousandShips]

CC @reduz @RandomShaper

[Nolkaloid]

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.

[AThousandShips]

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

[Nolkaloid]

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.

[AThousandShips]

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

[Nolkaloid]

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...