structs redux - #1318
Closed
mcdubhghlas wants to merge 43 commits into
Closed
Conversation
Still chasing a massive memroy leak
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Fix unit test compilation by adding STRUCT handling to: - Type name validation switch (line 284-285) - Zero-validation switch (line 312)
Changed gdscript_struct_instance_serialize to return void and take Dictionary by reference as output parameter instead of returning by value. Functions with extern "C" linkage cannot return C++ classes by value due to incompatible calling conventions.
The STRUCT case did the same thing as the else block, causing -Wduplicated-branches warnings. Struct calls now fall through to the default case which handles them correctly.
… defaulting to dict.
…ds a parser, analzer, tokenizer, plus tests.
WalkthroughAdds experimental GDScript ChangesGDScript struct support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (7)
core/variant/variant_utility.cpp (1)
954-956: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
type_convert(x, TYPE_STRUCT)returns the input unchanged for any source type.Because the branch just returns
p_variant,type_convert(5, TYPE_STRUCT)yields theint5rather than a struct (or an error), which can silently mask a type mismatch downstream. It's TODO-gated, so acceptable for now — want me to open a tracking issue for struct conversion?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/variant/variant_utility.cpp` around lines 954 - 956, Track the unimplemented STRUCT branch in type_convert as a follow-up issue rather than treating the unchanged input as a valid conversion. Open a tracking issue for struct conversion and retain the current TODO-gated behavior until proper conversion or an explicit error is implemented.modules/gdscript/gdscript_parser.cpp (1)
985-1024: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe VAR/FUNC/PASS member-parsing switch here is duplicated almost verbatim in
parse_struct_file_body()(Lines 1108-1143), including the_initconstructor capture and duplicate-member error reporting. Consider extracting a sharedparse_struct_member(StructNode *)helper so the two entry points can't drift apart.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/gdscript/gdscript_parser.cpp` around lines 985 - 1024, Extract the duplicated VAR/FUNC/PASS handling into a shared parse_struct_member(StructNode *) helper, including variable and function parsing, duplicate-member diagnostics, and _init constructor capture. Replace the corresponding switches in the current struct-body parser and parse_struct_file_body() with this helper while preserving their existing statement and unexpected-token behavior.modules/gdscript/gdscript_byte_codegen.h (1)
240-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove leftover empty struct-cast branches (debug scaffolding). Both sites cast a constant
Object*toGDScriptStructClass*and then branch into emptyif (sc) {} else {}bodies with no side effects, so the added control flow does nothing but obscure intent.
modules/gdscript/gdscript_byte_codegen.h#L240-L248: drop theVariant::OBJECT/GDScriptStructClasscast block inget_constant_pos; keep only theconstant_maplookup.modules/gdscript/gdscript_vm.cpp#L693-L701: drop the constant-addressedGDScriptStructClasscast block in theGET_VARIANT_PTRmacro.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/gdscript/gdscript_byte_codegen.h` around lines 240 - 248, Remove the unused Variant::OBJECT/GDScriptStructClass cast and empty branches from get_constant_pos in modules/gdscript/gdscript_byte_codegen.h#L240-L248, leaving only the constant_map lookup. Also remove the equivalent constant-addressed cast block from the GET_VARIANT_PTR macro in modules/gdscript/gdscript_vm.cpp#L693-L701; no other behavior should change.modules/gdscript/gdscript_analyzer.cpp (1)
6006-6070: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant null re-check of
struct_node.
struct_nodeis already validated at Line 6011 (earlyreturn falsewhen null), so theif (struct_node)guard at Line 6023 is dead. Safe to drop for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/gdscript/gdscript_analyzer.cpp` around lines 6006 - 6070, Remove the redundant if (struct_node) guard in the struct-constructor branch of the surrounding type-resolution method, since struct_node has already been null-checked and the null case returns false. Keep the constructor signature extraction and field-based fallback logic unchanged, unindenting their contents as needed.modules/gdscript/gdscript_function.h (1)
191-200: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueStruct type matching is not enforced in
is_type().The STRUCT case accepts any
Variant::STRUCTvalue regardless of its actual struct blueprint (the TODO on Line 198). Combined with the runtime type-adjust/assign paths, a value of one struct type can be stored into a variable typed as a different struct without a type error. This is acceptable only while the feature is experimental; please confirm this gap is tracked so typed-struct assignments are validated before the feature stabilizes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/gdscript/gdscript_function.h` around lines 191 - 200, Track the missing struct blueprint/type validation in the STRUCT branch of is_type() as a known experimental limitation, and ensure the issue is recorded for resolution before typed-struct support stabilizes. Preserve the current NIL and non-STRUCT handling while documenting the required validation for assignments between distinct struct types.modules/gdscript/gdscript_struct.cpp (1)
63-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConstructor argument logic is hard to follow and only covers current-struct members.
Two observations on
create_variant_instance:
expected_args = member_names.size()counts only this struct's members, whileget_member_count()/instance data include inherited members. Positional construction therefore can't initialize base-struct fields. Given struct inheritance is still TODO/experimental this may be intentional, but worth confirming.- The validate/convert/assign flow (Lines 82-118) re-tests
arg_value.get_type() != info->typetwice and splits assignment across branches. Consolidating into a single "compute final value, then oneset_member_direct" path would reduce the risk of a future edit dropping an assignment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/gdscript/gdscript_struct.cpp` around lines 63 - 133, Update GDScriptStruct::create_variant_instance to base positional argument counting and iteration on the full inherited member set used by get_member_count() and instance data, so base fields can also be initialized. Consolidate validation and conversion into one final-value path, then perform exactly one set_member_direct assignment per supplied member without repeating type checks.core/variant/variant_setget.cpp (1)
1501-1516: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueStruct language lookup runs on every
get_property_listcall.The linear scan comparing
String(l->get_name()) == "GDScript"executes for each STRUCT property enumeration. Since STRUCT is GDScript-only, caching the resolvedScriptLanguage*(or comparing against a cachedStringName) would avoid repeated string allocation/comparison on a potentially hot editor/debugger path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/variant/variant_setget.cpp` around lines 1501 - 1516, Cache the resolved GDScript ScriptLanguage (or its StringName) instead of scanning ScriptServer and constructing a String on every STRUCT branch of get_property_list. Initialize or reuse the cache before the lang->get_struct_property_list call, while preserving the existing behavior when no GDScript language is registered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/io/json.cpp`:
- Around line 862-899: Add the existing MAX_RECURSION_DEPTH guard to the STRUCT
branch in _from_native before recursively encoding struct fields, and to the
STRUCT branch in _to_native before recursively decoding them. In
core/io/json.cpp lines 862-899 and 1379-1410, use the same ERR_FAIL_COND_V_MSG
pattern as the OBJECT and DICTIONARY branches, preserving the existing recursive
behavior within the allowed depth.
In `@core/object/object.cpp`:
- Around line 857-869: Update the GDScriptStructClass check in Object::callp to
reuse a cached StringName initialized once, instead of constructing
StringName("GDScriptStructClass") on every invocation. Keep the existing
Callable forwarding and return behavior unchanged.
In `@core/object/script_language.cpp`:
- Around line 592-620: The existence check in
ScriptServer::global_struct_exists() must not call create_struct_by_name(),
since missing types trigger an error. Add or reuse a direct registry
lookup/has-style API on ScriptLanguage/GDScriptLanguage and use it from
global_struct_exists() to return whether p_fully_qualified_name exists without
instantiating or logging.
In `@core/variant/variant_internal.h`:
- Around line 811-826: Remove the GDScriptStructInstance forward declaration and
VariantGetInternalPtr<GDScriptStructInstance> specialization from
VariantInternal in core/variant/variant_internal.h. Add the specialization to
modules/gdscript/gdscript_struct.h after the type is defined, and ensure
module-specific users such as variant_parser.cpp include that header only under
MODULE_GDSCRIPT_ENABLED.
- Around line 348-353: Add a compile-time static_assert near the
GDScriptStructInstance wrapper definition verifying that
sizeof(GDScriptStructInstance) does not exceed sizeof(Variant::_data._mem). Keep
init_struct unchanged and ensure the guard covers the inline storage used by
Variant::STRUCT.
In `@doc/classes/`@GlobalScope.xml:
- Around line 3400-3402: Update the Variant.Type value-type list in is_same to
include Struct alongside the other value-semantics types. Keep the existing
TYPE_STRUCT documentation unchanged and ensure the list now matches its
classification as a Variant value type.
In `@modules/gdscript/gdscript_byte_codegen.cpp`:
- Around line 211-234: Remove the empty Variant::OBJECT cast scaffolding
surrounding the constant write, retaining only the functional
function->constants.write[K.value] assignment. Delete the side-effect-free
verification loop over function->constants, and clean the matching empty-branch
pattern in get_constant_pos() from gdscript_byte_codegen.h.
In `@modules/gdscript/gdscript_compiler.cpp`:
- Around line 377-396: Update the struct-wrapper-not-found branch in
_parse_expression to set r_error to ERR_COMPILATION_FAILED before returning the
empty GDScriptCodeGenerator::Address, alongside the existing _set_error call.
Preserve the successful wrapper lookup path unchanged.
In `@modules/gdscript/gdscript_editor.cpp`:
- Around line 1228-1230: Complete struct handling across the listed editor and
language-server sites: in modules/gdscript/gdscript_editor.cpp at lines
1228-1230, add struct declarations to member completion; at 2619-2621, return
the concrete struct type for member inference; at 3995-3997, resolve structs
during tool-only symbol lookup; and at 4302-4304, resolve members accessed
through struct types. In
modules/gdscript/language_server/gdscript_extend_parser.cpp at lines 474-476,
emit document symbols for structs, and at 1028-1030, include structs in dumped
class APIs, preserving the existing handling patterns for other declaration
kinds.
In `@modules/gdscript/gdscript_tokenizer.h`:
- Around line 134-135: Update TOKENIZER_VERSION in gdscript_tokenizer_buffer.h
because adding STRUCT and STRUCT_NAME changes subsequent token IDs. Increment
the version value so existing tokenized buffers are rejected or handled as
incompatible rather than decoded with incorrect token mappings.
In `@modules/gdscript/gdscript_vm.cpp`:
- Around line 3860-3872: Update the OPCODE_ADJUST_STRUCT handling to construct a
live GDScriptStructInstance in arg’s storage after clearing it, rather than only
calling VariantInternal::initialize with Variant::STRUCT. Preserve the existing
null-struct assignment behavior while ensuring keyed accessors receive a
properly placement-constructed wrapper.
In `@tests/core/object/test_class_db.h`:
- Line 312: Update the Variant::STRUCT branch in the default-validation logic to
reject GDScript structs explicitly rather than routing them through the
OBJECT/ClassDB validation path. Preserve existing validation for other Variant
types, and only allow STRUCT if all downstream ClassDB consumers support named
struct values.
- Around line 284-285: Update the Variant::STRUCT branch in the type-validation
logic to compare the TypeReference name against the concrete struct blueprint or
identifier carried by p_val, rather than
Variant::get_type_name(p_val.get_type()). Preserve matching for the actual
struct name, such as TestPoint, instead of the generic “Struct” type name.
---
Nitpick comments:
In `@core/variant/variant_setget.cpp`:
- Around line 1501-1516: Cache the resolved GDScript ScriptLanguage (or its
StringName) instead of scanning ScriptServer and constructing a String on every
STRUCT branch of get_property_list. Initialize or reuse the cache before the
lang->get_struct_property_list call, while preserving the existing behavior when
no GDScript language is registered.
In `@core/variant/variant_utility.cpp`:
- Around line 954-956: Track the unimplemented STRUCT branch in type_convert as
a follow-up issue rather than treating the unchanged input as a valid
conversion. Open a tracking issue for struct conversion and retain the current
TODO-gated behavior until proper conversion or an explicit error is implemented.
In `@modules/gdscript/gdscript_analyzer.cpp`:
- Around line 6006-6070: Remove the redundant if (struct_node) guard in the
struct-constructor branch of the surrounding type-resolution method, since
struct_node has already been null-checked and the null case returns false. Keep
the constructor signature extraction and field-based fallback logic unchanged,
unindenting their contents as needed.
In `@modules/gdscript/gdscript_byte_codegen.h`:
- Around line 240-248: Remove the unused Variant::OBJECT/GDScriptStructClass
cast and empty branches from get_constant_pos in
modules/gdscript/gdscript_byte_codegen.h#L240-L248, leaving only the
constant_map lookup. Also remove the equivalent constant-addressed cast block
from the GET_VARIANT_PTR macro in modules/gdscript/gdscript_vm.cpp#L693-L701; no
other behavior should change.
In `@modules/gdscript/gdscript_function.h`:
- Around line 191-200: Track the missing struct blueprint/type validation in the
STRUCT branch of is_type() as a known experimental limitation, and ensure the
issue is recorded for resolution before typed-struct support stabilizes.
Preserve the current NIL and non-STRUCT handling while documenting the required
validation for assignments between distinct struct types.
In `@modules/gdscript/gdscript_parser.cpp`:
- Around line 985-1024: Extract the duplicated VAR/FUNC/PASS handling into a
shared parse_struct_member(StructNode *) helper, including variable and function
parsing, duplicate-member diagnostics, and _init constructor capture. Replace
the corresponding switches in the current struct-body parser and
parse_struct_file_body() with this helper while preserving their existing
statement and unexpected-token behavior.
In `@modules/gdscript/gdscript_struct.cpp`:
- Around line 63-133: Update GDScriptStruct::create_variant_instance to base
positional argument counting and iteration on the full inherited member set used
by get_member_count() and instance data, so base fields can also be initialized.
Consolidate validation and conversion into one final-value path, then perform
exactly one set_member_direct assignment per supplied member without repeating
type checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5771128-9eb8-45f6-997d-1d1a8554defb
⛔ Files ignored due to path filters (2)
modules/gdscript/tests/scripts/analyzer/features/global_builtin_and_native_enums.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/global_struct.outis excluded by!**/*.out
📒 Files selected for processing (52)
.gitignorecore/core_constants.cppcore/extension/extension_api_dump.cppcore/extension/gdextension_interface.cppcore/extension/gdextension_interface.hcore/io/json.cppcore/io/marshalls.cppcore/object/object.cppcore/object/script_language.cppcore/object/script_language.hcore/variant/variant.cppcore/variant/variant.hcore/variant/variant_call.cppcore/variant/variant_construct.cppcore/variant/variant_internal.hcore/variant/variant_parser.cppcore/variant/variant_setget.cppcore/variant/variant_utility.cppdoc/classes/@GlobalScope.xmldoc/classes/EditorSettings.xmldoc/classes/ProjectSettings.xmldoc/classes/Struct.xmleditor/settings/editor_settings.cppmodules/gdscript/config.pymodules/gdscript/editor/gdscript_docgen.cppmodules/gdscript/gdscript.cppmodules/gdscript/gdscript.hmodules/gdscript/gdscript_analyzer.cppmodules/gdscript/gdscript_analyzer.hmodules/gdscript/gdscript_byte_codegen.cppmodules/gdscript/gdscript_byte_codegen.hmodules/gdscript/gdscript_compiler.cppmodules/gdscript/gdscript_compiler.hmodules/gdscript/gdscript_disassembler.cppmodules/gdscript/gdscript_editor.cppmodules/gdscript/gdscript_function.hmodules/gdscript/gdscript_parser.cppmodules/gdscript/gdscript_parser.hmodules/gdscript/gdscript_struct.cppmodules/gdscript/gdscript_struct.hmodules/gdscript/gdscript_tokenizer.cppmodules/gdscript/gdscript_tokenizer.hmodules/gdscript/gdscript_vm.cppmodules/gdscript/gdscript_warning.cppmodules/gdscript/gdscript_warning.hmodules/gdscript/language_server/gdscript_extend_parser.cppmodules/gdscript/tests/scripts/runtime/features/global_struct.gdmodules/gdscript/tests/scripts/runtime/features/global_struct.notest.gdmodules/modules_builders.pymodules/mono/editor/bindings_generator.cpptests/core/object/test_class_db.htests/core/variant/test_variant.h
Comment on lines
+862
to
+899
| #ifdef MODULE_GDSCRIPT_ENABLED | ||
| case Variant::STRUCT: { | ||
| // Serialize struct as tagged dictionary with __type__ metadata | ||
| // This allows round-trip deserialization | ||
| const GDScriptStructInstance *struct_instance = reinterpret_cast<const GDScriptStructInstance *>(VariantInternal::get_struct(&p_variant)); | ||
| ERR_FAIL_NULL_V(struct_instance, Variant()); | ||
|
|
||
| Dictionary ret; | ||
| ret[TYPE] = Variant::get_type_name(p_variant.get_type()); | ||
|
|
||
| // Get the serialized data from the struct instance using helper | ||
| Dictionary struct_data = _serialize_struct(struct_instance); | ||
|
|
||
| // Ensure __type__ field exists (it should from serialize()) | ||
| if (!struct_data.has("__type__")) { | ||
| ERR_FAIL_V_MSG(Variant(), "Struct serialization failed: missing __type__ field."); | ||
| } | ||
|
|
||
| // Encode field values using _from_native to ensure proper JSON encoding | ||
| // This handles non-JSON-native types (nested structs, objects, typed arrays, etc.) | ||
| Dictionary encoded_struct_data; | ||
| encoded_struct_data["__type__"] = struct_data["__type__"]; // Copy type identifier as-is | ||
|
|
||
| for (const KeyValue<Variant, Variant> &kv : struct_data) { | ||
| if (kv.key == "__type__") { | ||
| continue; // Already copied above | ||
| } | ||
| // Encode the value using _from_native, keys are strings (field names) and don't need encoding | ||
| encoded_struct_data[kv.key] = _from_native(kv.value, p_full_objects, p_depth + 1); | ||
| } | ||
|
|
||
| // Wrap the encoded serialized data in the expected format | ||
| Array args; | ||
| args.push_back(encoded_struct_data); | ||
| ret[ARGS] = args; | ||
|
|
||
| return ret; | ||
| } break; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add MAX_RECURSION_DEPTH guard to struct (de)serialization. The STRUCT branches recurse through _from_native/_to_native with p_depth + 1 but omit the ERR_FAIL_COND_V_MSG(p_depth > Variant::MAX_RECURSION_DEPTH, ...) check that the OBJECT (Line 822) and DICTIONARY (Line 921) branches use. Nested structs re-enter the STRUCT case unbounded, so deeply nested/untrusted JSON can drive unbounded recursion and stack overflow during deserialization.
core/io/json.cpp#L862-L899: add the depth guard before recursively encoding struct field values in_from_native.core/io/json.cpp#L1379-L1410: add the depth guard before recursively decoding struct field values in_to_native.
📍 Affects 1 file
core/io/json.cpp#L862-L899(this comment)core/io/json.cpp#L1379-L1410
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/io/json.cpp` around lines 862 - 899, Add the existing
MAX_RECURSION_DEPTH guard to the STRUCT branch in _from_native before
recursively encoding struct fields, and to the STRUCT branch in _to_native
before recursively decoding them. In core/io/json.cpp lines 862-899 and
1379-1410, use the same ERR_FAIL_COND_V_MSG pattern as the OBJECT and DICTIONARY
branches, preserving the existing recursive behavior within the allowed depth.
Comment on lines
+857
to
+869
| #ifdef MODULE_GDSCRIPT_ENABLED | ||
| // Special case for GDScriptStructClass to support struct constructors | ||
| // This is needed because GDScriptStructClass::callp needs to be called directly, | ||
| // but callp is not virtual so Object::callp doesn't dispatch to it. | ||
| if (get_class_name() == StringName("GDScriptStructClass")) { | ||
| // Forward to GDScriptStructClass::callp by using a Callable | ||
| Callable callable = Callable(this, p_method); | ||
| if (callable.is_valid()) { | ||
| callable.callp(p_args, p_argcount, ret, r_error); | ||
| return ret; | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid constructing a StringName per call on the Object::callp hot path.
StringName("GDScriptStructClass") is built on every single Object::callp invocation (a locked string-table lookup), penalizing all object calls engine-wide even though only GDScriptStructClass instances take the branch. Cache the name once.
⚡ Proposed fix
`#ifdef` MODULE_GDSCRIPT_ENABLED
// Special case for GDScriptStructClass to support struct constructors
- if (get_class_name() == StringName("GDScriptStructClass")) {
+ static const StringName gdscript_struct_class_name = StringName("GDScriptStructClass");
+ if (get_class_name() == gdscript_struct_class_name) {
// Forward to GDScriptStructClass::callp by using a Callable
Callable callable = Callable(this, p_method);
if (callable.is_valid()) {
callable.callp(p_args, p_argcount, ret, r_error);
return ret;
}
}
`#endif`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #ifdef MODULE_GDSCRIPT_ENABLED | |
| // Special case for GDScriptStructClass to support struct constructors | |
| // This is needed because GDScriptStructClass::callp needs to be called directly, | |
| // but callp is not virtual so Object::callp doesn't dispatch to it. | |
| if (get_class_name() == StringName("GDScriptStructClass")) { | |
| // Forward to GDScriptStructClass::callp by using a Callable | |
| Callable callable = Callable(this, p_method); | |
| if (callable.is_valid()) { | |
| callable.callp(p_args, p_argcount, ret, r_error); | |
| return ret; | |
| } | |
| } | |
| #endif | |
| `#ifdef` MODULE_GDSCRIPT_ENABLED | |
| // Special case for GDScriptStructClass to support struct constructors | |
| // This is needed because GDScriptStructClass::callp needs to be called directly, | |
| // but callp is not virtual so Object::callp doesn't dispatch to it. | |
| static const StringName gdscript_struct_class_name = StringName("GDScriptStructClass"); | |
| if (get_class_name() == gdscript_struct_class_name) { | |
| // Forward to GDScriptStructClass::callp by using a Callable | |
| Callable callable = Callable(this, p_method); | |
| if (callable.is_valid()) { | |
| callable.callp(p_args, p_argcount, ret, r_error); | |
| return ret; | |
| } | |
| } | |
| `#endif` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/object/object.cpp` around lines 857 - 869, Update the
GDScriptStructClass check in Object::callp to reuse a cached StringName
initialized once, instead of constructing StringName("GDScriptStructClass") on
every invocation. Keep the existing Callable forwarding and return behavior
unchanged.
Comment on lines
+592
to
+620
| bool ScriptServer::global_struct_exists(const String &p_fully_qualified_name) { | ||
| // Snapshot the languages under the lock, then release it before calling the | ||
| // language virtuals (see create_struct_instance() above). | ||
| ScriptLanguage *langs[MAX_LANGUAGES]; | ||
| int lang_count = 0; | ||
| { | ||
| MutexLock lock(languages_mutex); | ||
| if (!languages_ready) { | ||
| return false; | ||
| } | ||
| lang_count = _language_count; | ||
| for (int i = 0; i < _language_count; i++) { | ||
| langs[i] = _languages[i]; | ||
| } | ||
| } | ||
|
|
||
| // Check if any language can create this struct | ||
| for (int i = 0; i < lang_count; i++) { | ||
| ScriptLanguage *lang = langs[i]; | ||
| if (lang && lang->can_create_struct_by_name()) { | ||
| // Try to create with empty data to check if it exists | ||
| Variant result = lang->create_struct_by_name(p_fully_qualified_name, Dictionary()); | ||
| if (result.get_type() != Variant::NIL) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'create_struct_by_name' modules/gdscript -C3Repository: Redot-Engine/redot-engine
Length of output: 1655
🏁 Script executed:
#!/bin/bash
sed -n '3300,3385p' modules/gdscript/gdscript.cppRepository: Redot-Engine/redot-engine
Length of output: 2928
🏁 Script executed:
#!/bin/bash
sed -n '3200,3305p' modules/gdscript/gdscript.cppRepository: Redot-Engine/redot-engine
Length of output: 4268
Avoid using create_struct_by_name() as an existence probe
ScriptServer::global_struct_exists() turns a normal “missing struct” result into an error log, because GDScriptLanguage::create_struct_by_name() forwards to create_struct_instance(), which ERR_FAIL_V_MSGs when the type is absent. A direct registry lookup/has check would keep the false path silent and avoid the extra work.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/object/script_language.cpp` around lines 592 - 620, The existence check
in ScriptServer::global_struct_exists() must not call create_struct_by_name(),
since missing types trigger an error. Add or reuse a direct registry
lookup/has-style API on ScriptLanguage/GDScriptLanguage and use it from
global_struct_exists() to return whether p_fully_qualified_name exists without
instantiating or logging.
Comment on lines
+348
to
+353
| _FORCE_INLINE_ static void init_struct(Variant *v) { | ||
| // Zero-initialize the wrapper memory | ||
| // The default constructor will be called via placement new when the struct is actually constructed | ||
| memset(v->_data._mem, 0, sizeof(v->_data._mem)); | ||
| v->type = Variant::STRUCT; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for static_assert verifying the size of GDScriptStructInstance.
# Test: Search for size assertions related to GDScriptStructInstance. Expect: At least one match.
rg -n "static_assert.*\bsizeof\b.*GDScriptStructInstance"Repository: Redot-Engine/redot-engine
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== variant_internal.h excerpt ==\n'
sed -n '220,260p;340,360p;460,475p;545,560p;800,835p' core/variant/variant_internal.h
printf '\n== search for GDScriptStructInstance definition ==\n'
rg -n "struct GDScriptStructInstance|class GDScriptStructInstance|GDScriptStructInstance" modules/gdscript core -g '!**/*.po' -g '!**/*.png'
printf '\n== search for _mem size / Variant storage ==\n'
rg -n "_data\\._mem|MEM_SIZE|sizeof\\(_data\\._mem\\)|struct Variant" core/variant -g '!**/*.po'Repository: Redot-Engine/redot-engine
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Variant data storage definition ==\n'
sed -n '120,230p' core/variant/variant.h
printf '\n== GDScriptStructInstance definition ==\n'
sed -n '180,230p' modules/gdscript/gdscript_struct.h
printf '\n== Any size-related static_asserts for struct variants ==\n'
rg -n "static_assert.*STRUCT|static_assert.*GDScriptStructInstance|type_size_array\\[Variant::STRUCT\\]" core modules/gdscriptRepository: Redot-Engine/redot-engine
Length of output: 5206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Variant storage union ==\n'
sed -n '230,360p' core/variant/variant.h
printf '\n== Search for mem size / inline storage assertions ==\n'
rg -n "_mem|static_assert.*sizeof\\(.*_mem|STATIC_ASSERT|type_size_array\\[Variant::STRUCT\\]" core/variant core/extension modules/gdscriptRepository: Redot-Engine/redot-engine
Length of output: 50382
Add a compile-time size guard for GDScriptStructInstance
Variant::STRUCT is stored inline in _data._mem; add a static_assert(sizeof(GDScriptStructInstance) <= sizeof(Variant::_data._mem)) near the wrapper definition so the placement new can't silently overflow if the struct grows.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/variant/variant_internal.h` around lines 348 - 353, Add a compile-time
static_assert near the GDScriptStructInstance wrapper definition verifying that
sizeof(GDScriptStructInstance) does not exceed sizeof(Variant::_data._mem). Keep
init_struct unchanged and ensure the guard covers the inline storage used by
Variant::STRUCT.
Comment on lines
+811
to
+826
| // Forward declaration for GDScript struct template specialization | ||
| // The actual type is defined in modules/gdscript/gdscript_struct.h | ||
| class GDScriptStructInstance; | ||
|
|
||
| template <> | ||
| struct VariantGetInternalPtr<GDScriptStructInstance> { | ||
| static GDScriptStructInstance *get_ptr(Variant *v) { | ||
| // STRUCT type stores the wrapper by value in _data._mem | ||
| return reinterpret_cast<GDScriptStructInstance *>(VariantInternal::get_struct(v)); | ||
| } | ||
| static const GDScriptStructInstance *get_ptr(const Variant *v) { | ||
| // STRUCT type stores the wrapper by value in _data._mem | ||
| return reinterpret_cast<const GDScriptStructInstance *>(VariantInternal::get_struct(v)); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move module-specific template specialization out of the core header.
Placing a forward declaration of GDScriptStructInstance and its VariantGetInternalPtr specialization inside core/variant/variant_internal.h couples the core engine to a specific module (modules/gdscript). This breaks the engine's strict dependency inversion principle, as core headers should not depend on or reference module-specific types.
Since VariantGetInternalPtr is a template, its specialization can safely reside in modules/gdscript/gdscript_struct.h instead. Any core file that legitimately needs this specialization (e.g., core/variant/variant_parser.cpp inside an #ifdef MODULE_GDSCRIPT_ENABLED block) can simply include the module header, keeping the core headers clean and modular.
♻️ Proposed refactor
Remove these lines from core/variant/variant_internal.h and move them to modules/gdscript/gdscript_struct.h:
-// Forward declaration for GDScript struct template specialization
-// The actual type is defined in modules/gdscript/gdscript_struct.h
-class GDScriptStructInstance;
-
-template <>
-struct VariantGetInternalPtr<GDScriptStructInstance> {
- static GDScriptStructInstance *get_ptr(Variant *v) {
- // STRUCT type stores the wrapper by value in _data._mem
- return reinterpret_cast<GDScriptStructInstance *>(VariantInternal::get_struct(v));
- }
- static const GDScriptStructInstance *get_ptr(const Variant *v) {
- // STRUCT type stores the wrapper by value in _data._mem
- return reinterpret_cast<const GDScriptStructInstance *>(VariantInternal::get_struct(v));
- }
-};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Forward declaration for GDScript struct template specialization | |
| // The actual type is defined in modules/gdscript/gdscript_struct.h | |
| class GDScriptStructInstance; | |
| template <> | |
| struct VariantGetInternalPtr<GDScriptStructInstance> { | |
| static GDScriptStructInstance *get_ptr(Variant *v) { | |
| // STRUCT type stores the wrapper by value in _data._mem | |
| return reinterpret_cast<GDScriptStructInstance *>(VariantInternal::get_struct(v)); | |
| } | |
| static const GDScriptStructInstance *get_ptr(const Variant *v) { | |
| // STRUCT type stores the wrapper by value in _data._mem | |
| return reinterpret_cast<const GDScriptStructInstance *>(VariantInternal::get_struct(v)); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/variant/variant_internal.h` around lines 811 - 826, Remove the
GDScriptStructInstance forward declaration and
VariantGetInternalPtr<GDScriptStructInstance> specialization from
VariantInternal in core/variant/variant_internal.h. Add the specialization to
modules/gdscript/gdscript_struct.h after the type is defined, and ensure
module-specific users such as variant_parser.cpp include that header only under
MODULE_GDSCRIPT_ENABLED.
Comment on lines
+1228
to
+1230
| case GDScriptParser::ClassNode::Member::STRUCT: | ||
| // TODO: Handle struct completion | ||
| break; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Complete struct metadata integration across editor tooling.
Structs are parsed and usable at runtime, but these tooling paths still discard them, so users lose completion, inferred types, navigation, document symbols, and API descriptions.
modules/gdscript/gdscript_editor.cpp#L1228-L1230: add struct declarations to member completion.modules/gdscript/gdscript_editor.cpp#L2619-L2621: return the concrete struct type during member type inference.modules/gdscript/gdscript_editor.cpp#L3995-L3997: resolve struct declarations in tool-only symbol lookup.modules/gdscript/gdscript_editor.cpp#L4302-L4304: resolve members accessed through a struct type.modules/gdscript/language_server/gdscript_extend_parser.cpp#L474-L476: emit document symbols for struct declarations.modules/gdscript/language_server/gdscript_extend_parser.cpp#L1028-L1030: include structs in dumped class APIs.
📍 Affects 2 files
modules/gdscript/gdscript_editor.cpp#L1228-L1230(this comment)modules/gdscript/gdscript_editor.cpp#L2619-L2621modules/gdscript/gdscript_editor.cpp#L3995-L3997modules/gdscript/gdscript_editor.cpp#L4302-L4304modules/gdscript/language_server/gdscript_extend_parser.cpp#L474-L476modules/gdscript/language_server/gdscript_extend_parser.cpp#L1028-L1030
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/gdscript/gdscript_editor.cpp` around lines 1228 - 1230, Complete
struct handling across the listed editor and language-server sites: in
modules/gdscript/gdscript_editor.cpp at lines 1228-1230, add struct declarations
to member completion; at 2619-2621, return the concrete struct type for member
inference; at 3995-3997, resolve structs during tool-only symbol lookup; and at
4302-4304, resolve members accessed through struct types. In
modules/gdscript/language_server/gdscript_extend_parser.cpp at lines 474-476,
emit document symbols for structs, and at 1028-1030, include structs in dumped
class APIs, preserving the existing handling patterns for other declaration
kinds.
Comment on lines
+134
to
+135
| STRUCT, | ||
| STRUCT_NAME, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'gdscript_tokenizer_buffer.h' modules/gdscript | xargs rg -n 'TOKENIZER_VERSION'Repository: Redot-Engine/redot-engine
Length of output: 218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== diff stat =="
git diff --stat -- modules/gdscript/gdscript_tokenizer.h modules/gdscript/gdscript_tokenizer_buffer.h || true
echo
echo "== tokenizer header excerpt =="
sed -n '120,150p' modules/gdscript/gdscript_tokenizer.h
echo
echo "== tokenizer buffer excerpt =="
sed -n '40,60p' modules/gdscript/gdscript_tokenizer_buffer.h
echo
echo "== file diffs =="
git diff -- modules/gdscript/gdscript_tokenizer.h modules/gdscript/gdscript_tokenizer_buffer.h || trueRepository: Redot-Engine/redot-engine
Length of output: 1298
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== HEAD summary =="
git show --stat --oneline --no-patch HEAD
echo
echo "== targeted parent diff =="
git diff --unified=3 HEAD^ HEAD -- modules/gdscript/gdscript_tokenizer.h modules/gdscript/gdscript_tokenizer_buffer.hRepository: Redot-Engine/redot-engine
Length of output: 665
Bump TOKENIZER_VERSION in modules/gdscript/gdscript_tokenizer_buffer.h STRUCT/STRUCT_NAME renumber later token IDs, so existing tokenized buffers will decode incorrectly unless the version changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/gdscript/gdscript_tokenizer.h` around lines 134 - 135, Update
TOKENIZER_VERSION in gdscript_tokenizer_buffer.h because adding STRUCT and
STRUCT_NAME changes subsequent token IDs. Increment the version value so
existing tokenized buffers are rejected or handled as incompatible rather than
decoded with incorrect token mappings.
Comment on lines
+3860
to
+3872
| OPCODE(OPCODE_TYPE_ADJUST_STRUCT) { | ||
| // Struct type adjustment: clear the struct and set to null | ||
| // Similar to OBJECT type adjustment since both are reference-counted pointers | ||
| CHECK_SPACE(2); | ||
| GET_VARIANT_PTR(arg, 0); | ||
| VariantInternal::clear(arg); | ||
| // Create a null struct Variant and assign it | ||
| // Using VariantInternal::initialize which can access private members | ||
| VariantInternal::initialize(arg, Variant::STRUCT); | ||
| ip += 2; | ||
| } | ||
| DISPATCH_OPCODE; | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect VariantInternal::initialize and struct accessors for STRUCT handling.
fd -t f 'variant_internal.h' core | xargs -I{} sh -c 'echo "== {} =="; rg -n "STRUCT|get_struct|initialize" {} -A2'Repository: Redot-Engine/redot-engine
Length of output: 1800
🏁 Script executed:
#!/bin/bash
sed -n '1,170p' core/variant/variant_internal.h
echo '---'
sed -n '330,370p' core/variant/variant_internal.h
echo '---'
sed -n '800,835p' core/variant/variant_internal.hRepository: Redot-Engine/redot-engine
Length of output: 10871
🏁 Script executed:
#!/bin/bash
# Inspect the struct wrapper definition and how it is constructed/used.
fd -t f 'gdscript_struct.h' modules core | xargs -r -I{} sh -c 'echo "== {} =="; rg -n "class GDScriptStructInstance|struct GDScriptStructInstance|is_valid|get\\(|placement new|memnew_placement|Variant::STRUCT" "{}" -A4'Repository: Redot-Engine/redot-engine
Length of output: 1737
🏁 Script executed:
#!/bin/bash
# Find construction and use sites for GDScriptStructInstance and Variant::STRUCT.
rg -n "GDScriptStructInstance|Variant::STRUCT|memnew_placement|new \\(.*GDScriptStructInstance|placement new|get_struct\\(" core modules -g '!**/*.gen.*'Repository: Redot-Engine/redot-engine
Length of output: 43010
🏁 Script executed:
#!/bin/bash
# Inspect STRUCT lifetime/clear handling in Variant and keyed accessors.
sed -n '740,820p' core/variant/variant.cpp
echo '---'
sed -n '900,940p' core/variant/variant.cpp
echo '---'
sed -n '260,330p' core/variant/variant_setget.cpp
echo '---'
sed -n '1120,1275p' core/variant/variant_setget.cppRepository: Redot-Engine/redot-engine
Length of output: 10689
Construct the struct wrapper for Variant::STRUCT
VariantInternal::initialize(arg, Variant::STRUCT) only zero-fills _data._mem; it doesn’t placement-new a GDScriptStructInstance. The keyed accessors still cast that storage to GDScriptStructInstance * and call methods on it, so this path relies on using raw zeroed storage as a live wrapper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/gdscript/gdscript_vm.cpp` around lines 3860 - 3872, Update the
OPCODE_ADJUST_STRUCT handling to construct a live GDScriptStructInstance in
arg’s storage after clearing it, rather than only calling
VariantInternal::initialize with Variant::STRUCT. Preserve the existing
null-struct assignment behavior while ensuring keyed accessors receive a
properly placement-constructed wrapper.
Comment on lines
+284
to
+285
| case Variant::STRUCT: | ||
| return p_arg_type.name == Variant::get_type_name(p_val.get_type()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Match the actual struct name, not the generic Variant type name.
Variant::get_type_name(Variant::STRUCT) can only produce the generic "Struct" name, so a default value of TestPoint cannot match a TypeReference named TestPoint. Use the struct blueprint/identifier carried by the value, or otherwise preserve the concrete struct type during validation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/core/object/test_class_db.h` around lines 284 - 285, Update the
Variant::STRUCT branch in the type-validation logic to compare the TypeReference
name against the concrete struct blueprint or identifier carried by p_val,
rather than Variant::get_type_name(p_val.get_type()). Preserve matching for the
actual struct name, such as TestPoint, instead of the generic “Struct” type
name.
| case Variant::PACKED_VECTOR4_ARRAY: | ||
| case Variant::CALLABLE: | ||
| case Variant::SIGNAL: | ||
| case Variant::STRUCT: |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not validate GDScript structs as ClassDB objects.
Because structs are restricted to GDScript, routing Variant::STRUCT into the existing OBJECT default-validation path can accept unsupported zero-valued struct defaults while rejecting non-zero ones. Reject STRUCT explicitly here unless every downstream ClassDB consumer supports named struct values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/core/object/test_class_db.h` at line 312, Update the Variant::STRUCT
branch in the default-validation logic to reject GDScript structs explicitly
rather than routing them through the OBJECT/ClassDB validation path. Preserve
existing validation for other Variant types, and only allow STRUCT if all
downstream ClassDB consumers support named struct values.
Member
Author
|
IGNORE. This was for info. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reloading for #1152
Summary by CodeRabbit
New Features
structsupport, including declarations, typed fields, methods, inheritance, constructors, and value semantics.Variant.TYPE_STRUCTandStructdocumentation.Documentation
Structtype and its member-access operator.