godotengine / godotengine/godot
[Core] `Array` and `Dictionary` read only behavior is unsafe
- Dominant language
- C++
- Stars
- 117k
- Forks
- 26.8k
- PR merge metrics
- PR metrics pending
Description
### Tested versions
N/A
### System information
N/A
### Issue description
For example:
* https://github.com/godotengine/godot/pull/93636
You also have that, in C++, the following is broken, for example:
```cpp
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:
```cpp
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
Contributor guide
Research direction
Start by tracing the C++ implementations of Array and Dictionary subscript access and Array::get, then compare their const-access behavior with LocalVector. Reproduce the read-only Array examples from the issue and establish a design in which accesses do not share a mutable internal value or unexpectedly change an existing reference.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 30/100