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

Fix mismatch between String and StringName in dictionary keys #48139

Merged
merged 2 commits into from
Apr 24, 2021

Conversation

vnen
Copy link
Member

@vnen vnen commented Apr 23, 2021

Fixes the problems mixing String and StringName types when used as dictionary keys. This means users will have to be more careful of what they use as keys.

For reference, this is the expected behavior:

var dict = {"a": 1, &"b": 2}
print(dict["a"]) # should work
print(dict[&"a"]) # should NOT work
print(dict["b"]) # should NOT work
print(dict[&"b"]) # should work
print(dict.a) # should NOT work
print(dict.b) # should work

Note that shorthand notation (dict.a) assumes the key is a StringName). Also the new StringName literal notation: &"name".

This PR also makes sure the Lua-style dictionary literal uses StringName as keys, so the shorthand works too:

var dict = {a = 1, b = 2}
print(dict["a"]) # should NOt work
print(dict[&"a"]) # should work
print(dict["b"]) # should NOT work
print(dict[&"b"]) # should work
print(dict.a) # should work
print(dict.b) # should work

Fixes #44241
Supersedes #47899

vnen added 2 commits April 23, 2021 15:42
There was a mixup between String and StringName keys. Now they're
clearly separated. This also means you have to consider which type
you're using for the dictionary keys and how you are accessing them.
@akien-mga
Copy link
Member

Also supersedes #44310?

@vnen
Copy link
Member Author

vnen commented Apr 24, 2021

Also supersedes #44310?

That one does a different thing, which allows String and StringName to be the same. I believe we want to keep the idea that different types are always different, so that one probably shouldn't be merged, but it can be evaluated in its own merit (though I don't think it's the proper fix for this).

@akien-mga akien-mga merged commit d1dc28e into godotengine:master Apr 24, 2021
@akien-mga
Copy link
Member

Thanks!

@AndreaCatania
Copy link
Contributor

Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dictionary element are not available by index
3 participants