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

[GDScript 2.0] Constant array/dictionary inconsistency #62680

Closed
AThousandShips opened this issue Jul 3, 2022 · 4 comments · Fixed by #71051
Closed

[GDScript 2.0] Constant array/dictionary inconsistency #62680

AThousandShips opened this issue Jul 3, 2022 · 4 comments · Fixed by #71051

Comments

@AThousandShips
Copy link
Member

AThousandShips commented Jul 3, 2022

Godot version

90c33ed

System information

Windows 10

Issue description

Accessing constant arrays and dictionaries with index/key from a variable does not trigger an error:

const dict : Dictionary = { "A":[0] }
const ind_c = "A"
dict["A"][0] = 1 # Causes error
dict[ind_c][0] = 1 # Causes error
var ind_v = "A"
dict[ind][0] = 1 # Does not

It appears to be caused by a failure to set the type of such a subscript to constant, where as the case with a literal is folded into a constant expression (which isn't possible for a variable as the index/key is not known at compile time)

Steps to reproduce

See above

Minimal reproduction project

No response

@AThousandShips AThousandShips changed the title Constant array/dictionary inconsistency [GDScript 2.0] Constant array/dictionary inconsistency Jul 3, 2022
@akien-mga akien-mga added this to the 4.0 milestone Jul 3, 2022
@akien-mga akien-mga moved this to To Assess in 4.x Priority Issues Jul 3, 2022
@AThousandShips
Copy link
Member Author

From what I can tell and some minor testing it would be here:

if (p_subscript->base->is_constant && p_subscript->index->is_constant) {

With the first clause making a constant base with constant index/key a constant expression (thus folding it to the actual value) but the subsequent clause ignores any constness of the base type

@AThousandShips
Copy link
Member Author

I believe adding a simple fix solves this but I don't know if I'm well versed enough in the inner workings of GDScript to handle such a PR

@cdemirer
Copy link
Contributor

cdemirer commented Jul 4, 2022

I think it wouldn't be easy to do this currently, but since read-only Array/Dictionary was introduced recently (#61127) it'll probably be possible in GDScript soon.

Actually, checking usage like above can be done before that, but currently const doesn't imply read-only (for some reason), and I assume that would change soon.

@AThousandShips
Copy link
Member Author

This is true! But I think this check needs to be kept in mind when/if implementing read-only arrays/dictionaries in GDScript

And read-only isn't necessarily required for this, nor would it necessarily solve it from my reading of the read-only code in dict/array as it doesn't enforce anything, it only seems to fail silently (i.e. allow writes to the value but not update the original value), so the error would be needed to indicate this and prevent compilation I feel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants