-
-
Notifications
You must be signed in to change notification settings - Fork 22k
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 operator[]
for typed dictionaries
#96797
Core: Fix operator[]
for typed dictionaries
#96797
Conversation
modules/gdscript/tests/scripts/analyzer/errors/typed_dictionary_assign_differently_typed_key.gd
Outdated
Show resolved
Hide resolved
3e52612
to
3d79d8b
Compare
...les/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_value.gd
Outdated
Show resolved
Hide resolved
modules/gdscript/tests/scripts/analyzer/errors/typed_dictionary_assign_differently_typed_key.gd
Outdated
Show resolved
Hide resolved
e2a9e04
to
57e3d4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but it would be nice to get a second review.
783c8c0
to
962f02d
Compare
core/variant/dictionary.cpp
Outdated
/// @warning This operator does not validate the value type. For scripting/extensions this is done | ||
/// in `variant_setget.cpp`. Consider using `set()` if the data might be invalid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Just a note) Perhaps we should stick to one style for comment keywords.
Occurrences
core/io/packet_peer.cpp: 1
302: 2: //warning may lose packets
core/math/transform_2d.h: 2
42: 2: // Warning #1: basis of Transform2D is stored differently from Basis. In terms of columns array, the basis matrix looks like "on paper":
50: 2: // Warning #2: 2D be aware that unlike 3D code, 2D code uses a left-handed coordinate system: Y-axis points down,
editor/plugins/tiles/tile_set_atlas_source_editor.h: 1
180: 3: // Warning: keep in this order.
modules/gdscript/gdscript_compiler.cpp: 1
2634: 1: // Warning: this function cannot initiate compilation of other classes, or it will result in cyclic dependency issues.
modules/gdscript/gdscript_parser.cpp: 1
4319: 3: // WARNING: Do not merge with the previous `if` because there `!=`, not `==`!
modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs: 1
29: 9: /// Warning: Deserialized object can contain code which gets executed. Do not use this
platform/windows/display_server_windows.cpp: 1
3965: 2: // WARNING: we get called with events before the window is registered in our collection
platform/windows/gl_manager_windows_native.cpp: 1
455: 2: // WARNING: p_window_id is an eternally growing integer since popup windows keep coming and going
servers/physics_2d/godot_body_pair_2d.cpp: 1
165: 1: // Warning: the way velocity is adjusted down to cause a collision means the momentum will be weaker than it should for a bounce!
servers/physics_2d/godot_step_2d.cpp: 2
254: 2: // Warning: This doesn't run on threads, because it involves thread-unsafe processing.
261: 2: // Warning: _solve_island modifies the constraint islands for optimization purpose,
servers/physics_3d/godot_body_pair_3d.cpp: 1
165: 1: // Warning: the way velocity is adjusted down to cause a collision means the momentum will be weaker than it should for a bounce!
servers/physics_3d/godot_step_3d.cpp: 2
358: 2: // Warning: This doesn't run on threads, because it involves thread-unsafe processing.
365: 2: // Warning: _solve_island modifies the constraint islands for optimization purpose,
servers/rendering/renderer_rd/shaders/effects/ssao.glsl: 2
53: 54: #define SSAO_DEPTH_MIPS_ENABLE_AT_QUALITY_PRESET (2) // !!warning!! the MIP generation on the C++ side will be enabled on quality preset 2 regardless of this value, so if changing here, change
56: 1: // !!warning!! the edge handling is hard-coded to 'disabled' on quality level 0, and enabled above, on the C++ side; while toggling it here will work for
servers/rendering/renderer_rd/shaders/effects/ssil.glsl: 1
52: 1: // !!warning!! the edge handling is hard-coded to 'disabled' on quality level 0, and enabled above, on the C++ side; while toggling it here will work for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm personally a big fan of the "proper" syntax implemented in gdextension_interface.h
, though that only works in a docstrings & is primarily suited for exposed/binding contexts. I'm down for a universal WARNING:
preface, akin to TODO:
& FIXME:
, though that'll need to be specified in the documentation.
962f02d
to
e8017e5
Compare
e8017e5
to
b3d7960
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Thanks! |
operator[]
#96772Caused by an erroneous regression when rebasing the typed dictionary PR. Reimplements type checking and passing
_p->typed_fallback
to erroneous keys when usingoperator[]
on dictionaries.This is technically a partial fix, because it seems the untyped value assignment via
operator[]
is something that can happen regardless. However, fixing that will probably be a much more involved change, as the passed value isn't linked to the typed dictionary in isolation.