[Core] Array and Dictionary read only behavior is unsafe

[AThousandShips]

Tested versions

N/A

System information

N/A

Issue description

For example:

• Fix call 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...

[AThousandShips]

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

[AThousandShips]

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

[huwpascoe]

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.