Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Fix property access on read-only Dictionary #89647

Merged
merged 1 commit into from
May 1, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Mar 18, 2024

Read-only dictionaries can be modified with property access, like so:

var my_dict := { "a": 0 }
my_dict.make_read_only()
my_dict.a = 1

This seems to be an oversight (and can't find any reports on it), but this makes it aligned with how it works for const

@AThousandShips AThousandShips added bug topic:core cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 18, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Mar 18, 2024
@AThousandShips AThousandShips requested a review from a team as a code owner March 18, 2024 14:14
@AThousandShips AThousandShips requested a review from a team as a code owner March 18, 2024 14:21
if (v) {
*v = p_value;
r_valid = true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would just silently fail before as it uses the read-only temporary

@dalexeev
Copy link
Member

I'm not familiar with VariantInternal, but shouldn't the [] operator check for readonly? I think that the check should work not only in GDScript (variant_setget.cpp), but also in the core.

Variant &Dictionary::operator[](const Variant &p_key) {
if (unlikely(_p->read_only)) {

@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 18, 2024

As explained above, it uses the temporary, it doesn't throw any errors for the operator version and there isn't a distinct set method (which there is for Array, and that handles the check), we can't really have an error here as we return a reference (nor is it reliable as such, as we could be accessing it by reading only, the operator[] can't check for assignment without some proxy)

It works in core as well, with the valid return, just like for any other checks like that, when using set on a Variant, but unfortunately it can't be enforced in any other way

@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 18, 2024

What we might want to do is add a check to get_or_add as it does mutate like that, will see about that separately

Edit: Writing up an improvement, adding a set method to Dictionary for safe usage in c++, and adding a check to get_or_add, will push soon

@akien-mga akien-mga merged commit 273a643 into godotengine:master May 1, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the read_only_dict branch May 1, 2024 10:27
@AThousandShips
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants