Implement Structs in GDScript#1152
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds core serialization and ScriptServer integration for GDScript structs, including tagged JSON conversion, binary marshaling helpers, struct creation APIs, extension metadata, and guarded method/constructor lookup behavior. ChangesStruct language API
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ScriptServer
participant ScriptLanguage
participant JSON
participant Marshalls
participant GDScriptHelpers
Caller->>ScriptServer: create_struct_instance(name, data)
ScriptServer->>ScriptLanguage: can_create_struct_by_name()
ScriptServer->>ScriptLanguage: create_struct_by_name(name, data)
ScriptLanguage-->>ScriptServer: STRUCT Variant
ScriptServer-->>Caller: STRUCT Variant
JSON->>ScriptServer: reconstruct tagged struct
Marshalls->>GDScriptHelpers: encode/decode STRUCT
GDScriptHelpers-->>Marshalls: serialized data or STRUCT Variant
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
| // Forward declaration for struct type | ||
| class GDScriptStructInstance; | ||
|
|
There was a problem hiding this comment.
Not sure about this methodology
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
modules/gdscript/gdscript_function.h (1)
251-257: Missingstruct_typeinoperator==andoperator=.The equality and assignment operators don't handle the new
struct_typemember, which will cause incorrect comparisons and incomplete copies when the kind isSTRUCT.Proposed fix
bool operator==(const GDScriptDataType &p_other) const { return kind == p_other.kind && has_type == p_other.has_type && builtin_type == p_other.builtin_type && native_type == p_other.native_type && (script_type == p_other.script_type || script_type_ref == p_other.script_type_ref) && + struct_type == p_other.struct_type && container_element_types == p_other.container_element_types; }void operator=(const GDScriptDataType &p_other) { kind = p_other.kind; has_type = p_other.has_type; builtin_type = p_other.builtin_type; native_type = p_other.native_type; script_type = p_other.script_type; script_type_ref = p_other.script_type_ref; + struct_type = p_other.struct_type; container_element_types = p_other.container_element_types; }Also applies to: 264-272
modules/gdscript/gdscript_tokenizer.cpp (1)
105-128: Back-compat: consider allowing$structby addingSTRUCTtoToken::is_node_name()Since many keywords are allowed in
Token::is_node_name()specifically to support$keywordnode access, leaving outSTRUCTlikely introduces an avoidable breaking change for node names like"struct".Proposed fix
bool GDScriptTokenizer::Token::is_node_name() const { // This is meant to allow keywords with the $ notation, but not as general identifiers. switch (type) { @@ case SIGNAL: case STATIC: + case STRUCT: case SUPER: case TRAIT: case UNDERSCORE: case VAR: @@ return true; default: return false; } }core/variant/variant_internal.h (1)
49-131: Critical: STRUCT pointer accessor returns the storage address, not the stored pointer value (likely UB/crashes).
- Line 221-224:
get_struct()currently returnsv->_data._mem(address of the inline buffer).- Line 799-806:
VariantGetInternalPtr<GDScriptStructInstance>treatsget_struct()as the stored pointer value and casts it toGDScriptStructInstance*. That will return a pointer into the Variant’s inline buffer, not the heap instance.- Line 49-131:
initialize(STRUCT)is a no-op; withneeds_deinit[STRUCT]=true(invariant.h), leaving the stored pointer uninitialized is dangerous (stale pointer could be unref’d/freed later).Fix by explicitly storing/loading a
void*inside_memand null-initializing it onSTRUCTinit.Proposed fix
diff --git a/core/variant/variant_internal.h b/core/variant/variant_internal.h @@ _FORCE_INLINE_ static void initialize(Variant *v, Variant::Type p_type) { v->clear(); v->type = p_type; switch (p_type) { @@ case Variant::OBJECT: init_object(v); break; case Variant::STRUCT: - // TODO: Implement struct initialization + // Store a pointer value in _data._mem; ensure it is initialized. + *reinterpret_cast<void **>(v->_data._mem) = nullptr; break; default: break; } } @@ // Struct access - stores pointer directly in _mem - _FORCE_INLINE_ static void *get_struct(Variant *v) { return v->_data._mem; } - _FORCE_INLINE_ static const void *get_struct(const Variant *v) { return v->_data._mem; } + _FORCE_INLINE_ static void *get_struct(Variant *v) { return *reinterpret_cast<void **>(v->_data._mem); } + _FORCE_INLINE_ static const void *get_struct(const Variant *v) { return *reinterpret_cast<void *const *>(v->_data._mem); } @@ case Variant::OBJECT: return get_object(v); case Variant::STRUCT: - // TODO: Implement struct pointer access - return nullptr; + // If GDExtension needs an opaque pointer for STRUCT, define whether it should be: + // - pointer value (GDScriptStructInstance*), or + // - pointer-to-pointer slot (like OBJECT), or + // - intentionally unsupported. + return nullptr; @@ case Variant::OBJECT: return get_object(v); case Variant::STRUCT: - // TODO: Implement struct pointer access - return nullptr; + // See non-const overload comment. + return nullptr;Also applies to: 221-224, 368-455, 457-544, 794-808
core/variant/variant_utility.cpp (1)
1065-1075:str_to_var()ignores parse errors and uses an uninitializedline(regression risk).Right now parse failures are silently swallowed and callers can’t distinguish “parsed NIL” from “parse failed”. Also
lineshould be initialized defensively. Please capture the returnedErrorand either returnVariant()on failure and/or logerrs+line.Proposed fix
Variant VariantUtilityFunctions::str_to_var(const String &p_var) { VariantParser::StreamString ss; ss.s = p_var; String errs; - int line; + int line = 0; Variant ret; - (void)VariantParser::parse(&ss, ret, errs, line); + const Error err = VariantParser::parse(&ss, ret, errs, line); + if (err != OK) { + // Keep behavior consistent and observable. + ERR_PRINT(vformat("str_to_var(): parse error at line %d: %s", line, errs)); + return Variant(); + } return ret; }core/variant/variant.cpp (3)
1146-1330: CRITICAL:STRUCTreference path likely treats the inline buffer address as the struct pointer (crash/UAF).
reinterpret_cast<const GDScriptStructInstance *>(p_variant._data._mem)does not read “the pointer stored in_mem”; it reinterprets the address of the buffer as aGDScriptStructInstance*. Same issue exists in_clear_internal. This will callreference()on garbage.Also,
Variant::operator=currently has noSTRUCTcase; assigning one STRUCT Variant to another of the same type will no-op (leaving stale pointer/refcount state).Proposed fix (store/load the pointer explicitly)
case STRUCT: { - // Reference the struct instance - GDScriptStructInstance *struct_instance = const_cast<GDScriptStructInstance *>(reinterpret_cast<const GDScriptStructInstance *>(p_variant._data._mem)); - if (struct_instance) { - struct_instance->reference(); - } - // Copy the pointer - only copy pointer size, not entire _mem - memcpy(_data._mem, p_variant._data._mem, sizeof(void *)); + // Read pointer stored in the inline storage. + GDScriptStructInstance *struct_instance = nullptr; + memcpy(&struct_instance, p_variant._data._mem, sizeof(struct_instance)); + if (struct_instance) { + struct_instance->reference(); + } + // Store pointer back into our inline storage. + memcpy(_data._mem, &struct_instance, sizeof(struct_instance)); } break;Additional required fix (outside this hunk): handle STRUCT in `Variant::operator=`
diff --git a/core/variant/variant.cpp b/core/variant/variant.cpp @@ switch (p_variant.type) { @@ + case STRUCT: { + // Treat like other refcounted pointer-backed payloads: ref new, unref old. + GDScriptStructInstance *old_instance = nullptr; + memcpy(&old_instance, _data._mem, sizeof(old_instance)); + + GDScriptStructInstance *new_instance = nullptr; + memcpy(&new_instance, p_variant._data._mem, sizeof(new_instance)); + + if (new_instance) { + new_instance->reference(); + } + if (old_instance) { + // See note in _clear_internal comment about unreference semantics. + old_instance->unreference(); + } + memcpy(_data._mem, &new_instance, sizeof(new_instance)); + } break;
33-41: Confirmed layering/build issue:core/variant/includesmodules/gdscript/gdscript_struct.hwith direct method calls.The forward declaration in
variant.h:70is insufficient. The implementation invariant.cpp(lines 1320, 1498),variant_setget.cpp, andvariant_parser.cppcallsreference(),unreference(),get_reference_count(), andserialize()onGDScriptStructInstance, requiring the full header. This creates a hard dependency from core to modules and will break builds with GDScript disabled or modules optional.The STRUCT type is integral to Variant's enum, but its reference-counting and method dispatch are tightly coupled to GDScript. Consider:
- Moving
GDScriptStructInstanceinterface intocore/variant/with GDScript-specific logic remaining in modules, or- Implementing an opaque pointer + abstract refcount interface in core that GDScript implements.
1393-1513: Fix STRUCT cleanup to properly load the stored pointer and use safe refcount pattern.The current implementation has two critical bugs:
Pointer loading error:
reinterpret_cast<GDScriptStructInstance *>(_data._mem)treats the buffer address as a pointer, but the pointer is actually stored in the buffer's first bytes (viamemcpy(_data._mem, ..., sizeof(void *))). Must load it:memcpy(&struct_instance, _data._mem, sizeof(void *)).Use-after-free risk:
unreference()returns aboolindicating whether refcount reached zero (true = delete). The current code ignores this return value and callsget_reference_count()afterward, risking access after deletion. Use the return value instead:if (struct_instance->unreference()) memdelete(struct_instance);Buffer clear size:
memset(_data._mem, 0, sizeof(_data._mem))clears the entire buffer; should besizeof(void *)to match storage size.Proposed fix
case STRUCT: { - // Unreference the struct instance - GDScriptStructInstance *struct_instance = reinterpret_cast<GDScriptStructInstance *>(_data._mem); + GDScriptStructInstance *struct_instance = nullptr; + memcpy(&struct_instance, _data._mem, sizeof(struct_instance)); if (struct_instance) { - struct_instance->unreference(); - if (struct_instance->get_reference_count() == 0) { - memdelete(struct_instance); - } + if (struct_instance->unreference()) { + memdelete(struct_instance); + } } - // Clear the pointer - memset(_data._mem, 0, sizeof(_data._mem)); + memset(_data._mem, 0, sizeof(void *)); } break;modules/gdscript/gdscript_compiler.cpp (1)
3232-3282: Moveold_structscapture outside thep_keep_statecondition to always clean up old struct definitions.The cleanup code at lines 3250-3263 is ineffective when
p_keep_state == falsebecauseold_structsremains empty. While old struct wrappers are indirectly cleaned whenp_script->constantsis cleared in_prepare_compilation(), relying on this implicit mechanism is fragile and makes the code's intent unclear.Currently, when
p_keep_state == false:
old_structsis not populated- The explicit cleanup loops (erase constants, unreference structs) do nothing
- Old structs must wait for indirect cleanup via wrapper destruction in
_prepare_compilation()Recommend capturing
old_structs = p_script->structsunconditionally (outside theif (p_keep_state)block), then always execute the cleanup loops. This makes struct lifecycle management explicit and correct across all compilation paths.Separately, confirm that
GDScriptStructInstancevalues holding rawGDScriptStruct*pointers remain safe after struct definition cleanup—the wrapper references prevent premature deletion during hot-reload.modules/gdscript/gdscript_analyzer.cpp (1)
6322-6367: Struct type compatibility is broken—struct-to-struct assignments always fail.The STRUCT cases in both switches simply
break, causing the function to returnfalseat the end, regardless of whether the source and target are the same struct type. This means even assigning a value of the same struct type to a struct variable will be rejected.The early type checks for VARIANT, BUILTIN, and ENUM short-circuit before reaching the switches, so STRUCT targets never reach polymorphism checking. You need a check before the "From here on the target type is an object" comment to handle struct compatibility:
if (p_target.kind == GDScriptParser::DataType::STRUCT) { if (p_source.kind == GDScriptParser::DataType::STRUCT) { // Compare struct definitions (struct_type pointers match for same struct) return p_source.struct_type == p_target.struct_type; } return false; // Structs cannot be assigned from other types (null not allowed) }Alternatively, implement full struct inheritance compatibility if that's planned later.
modules/gdscript/gdscript_parser.h (2)
108-142:DataTypecopy/assignment dropsstruct_type(struct types will “disappear”)
You addedStructNode *struct_typebutDataType::operator=doesn’t copy it, so any assignment/copy will lose the struct reference.Proposed fix
void operator=(const DataType &p_other) { kind = p_other.kind; type_source = p_other.type_source; is_read_only = p_other.is_read_only; is_constant = p_other.is_constant; is_meta_type = p_other.is_meta_type; is_pseudo_type = p_other.is_pseudo_type; is_coroutine = p_other.is_coroutine; builtin_type = p_other.builtin_type; native_type = p_other.native_type; enum_type = p_other.enum_type; script_type = p_other.script_type; script_path = p_other.script_path; class_type = p_other.class_type; + struct_type = p_other.struct_type; method_info = p_other.method_info; enum_values = p_other.enum_values; container_element_types = p_other.container_element_types; }Also applies to: 240-257
674-704: Setbuiltin_typeandtype_sourcewhen creating struct DataType inMember::get_datatype()The STRUCT case creates a DataType but only sets
kindandstruct_type, leavingbuiltin_typeto default toVariant::NIL. This breaks type checking sincecan_reference()comparesbuiltin_typevalues. Set:
dt.builtin_type = Variant::STRUCT;dt.type_source = DataType::ANNOTATED_EXPLICIT;This matches how struct DataTypes are created elsewhere in the codebase (e.g., gdscript_compiler.cpp).
🤖 Fix all issues with AI agents
In @core/variant/variant_construct.cpp:
- Around line 266-276: The STRUCT early-return in Variant::construct sets
r_error.error but leaves r_error.argument and r_error.expected uninitialized;
update the STRUCT branch to fully initialize the Callable::CallError struct by
setting r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD and also
resetting r_error.argument and r_error.expected to safe defaults (e.g. 0 or
appropriate empty/invalid values for their types) and ensure base is set to
Variant() before returning so no garbage is exposed in error reporting.
In @core/variant/variant_parser.cpp:
- Line 41: The file variant_parser.cpp unconditionally includes
modules/gdscript/gdscript_struct.h and directly references
GDScriptStructInstance in the STRUCT serialization path, which breaks builds
when GDScript is disabled; wrap the include and any code that uses
GDScriptStructInstance (the STRUCT handling branch in
variant_parser.cpp/serialize/deserialize routines) in #ifdef
MODULE_GDSCRIPT_ENABLED ... #endif, and provide an #else fallback (e.g., skip
STRUCT handling or return/report an unsupported-variant error) so the file
compiles cleanly with the GDScript module turned off.
In @core/variant/variant_utility.cpp:
- Around line 1059-1063: The header declares
VariantUtilityFunctions::var_to_str_with_objects and ::str_to_var_with_objects
but there are no implementations, causing linker errors; implement both
functions in variant_utility.cpp with the exact signatures from the header: for
var_to_str_with_objects, mirror var_to_str but call the object-aware serializer
(e.g., use the VariantWriter API that serializes objects — write the variant
into a String output variable using the object-capable writer) and return that
string; for str_to_var_with_objects, mirror str_to_var but use the
object-capable parser/reader to convert the string back into a Variant and
return it (include the same error handling/path as the existing
var_to_str/str_to_var functions). If you prefer not to support object-aware
serialization, remove the declarations, XML docs, and any test calls to
var_to_str_with_objects/str_to_var_with_objects instead so the build and tests
no longer reference them.
In @modules/gdscript/gdscript_analyzer.cpp:
- Around line 1259-1261: The STRUCT branch currently bypasses name conflict
checks; update the case handling for GDScriptParser::ClassNode::Member::STRUCT
to invoke check_class_member_name_conflict(...) (same validation used for
VAR/CONST/ENUM/CLASS) before deferring to resolve_struct_body (or breaking), so
struct declarations are validated for duplicate/shadowed names even though their
bodies are resolved separately.
- Around line 1591-1623: In resolve_struct_body: when assigning field types,
avoid setting the metatype directly; replace the current
set_datatype(field.variable->datatype_specifier->get_datatype()) flow by
resolving the datatype through resolve_datatype(...) and converting the metatype
to the instance type with type_from_metatype(...) before calling
field.variable->set_datatype(...). Also thread p_source into resolution calls so
diagnostics point to the struct site: pass p_source into resolve_datatype (if
applicable) and into resolve_function_signature(method, p_source) and
resolve_function_body(method, p_source) instead of calling those functions
without the source node.
In @modules/gdscript/gdscript_byte_codegen.cpp:
- Around line 551-553: The switch case handling Variant::STRUCT currently
early-returns and must be changed to follow other reference-counted types:
remove the early return in gdscript_byte_codegen.cpp, add an
OPCODE_TYPE_ADJUST_STRUCT opcode (matching the pattern used by
OPCODE_TYPE_ADJUST_OBJECT/ARRAY/CALLABLE/etc.), and emit that opcode from the
same code path that emits type-adjust opcodes for other reference types (mirror
the emission logic used for OBJECT/CALLABLE/SIGNAL/DICTIONARY/ARRAY and
PACKED_*_ARRAY). Also add the corresponding enum entry and handler stub where
type-adjust opcodes are decoded/handled, and add a unit test that exercises
STRUCT value type adjustment to ensure it behaves consistently with other
ref-counted Variants.
In @modules/gdscript/gdscript_compiler.cpp:
- Around line 198-211: The assignment of result.kind = GDScriptDataType::STRUCT
causes null-deref because callers like _is_exact_type and method dispatch assume
script_type/struct_type is populated when kind != BUILTIN && kind != NATIVE;
since struct_type is TODO/uninitialized, change the value assigned in the STRUCT
case to keep result.kind = GDScriptDataType::BUILTIN (and result.builtin_type =
Variant::STRUCT as already set) until you implement and populate
result.struct_type and update callers to handle GDScriptDataType::STRUCT
explicitly; update the comment to note this temporary mitigation and ensure no
other code paths assume struct_type exists when kind == BUILTIN.
- Around line 3153-3210: The struct refcount lifecycle and method registration
are broken: create a strong ownership for gdstruct immediately after
construction by calling gdstruct->reference() right after
memnew(GDScriptStruct(...)) (before any parsing) so unreference() on error
cannot underflow; then in the methods loop capture the compiled function
produced by _parse_function (either by changing _parse_function to return a
GDScriptFunction* or by looking it up from p_script after _parse_function
succeeds) and call gdstruct->add_method(method->identifier->name, compiled_func)
for each method; keep the existing error path that calls gdstruct->unreference()
and memdelete(gdstruct) on failure so cleanup remains correct.
In @modules/gdscript/gdscript_parser.cpp:
- Around line 900-995: parse_struct currently double-calls end_statement for
fields/methods and never terminates for single-line (multiline == false) bodies;
remove the extra end_statement() calls inside the VAR, FUNC and PASS cases (rely
on parse_variable(), parse_function()/parse_suite() to terminate) and after
handling an entry in the loop, if multiline == false break the loop so inline
structs terminate; also extend the extends parsing: after parsing the first
IdentifierNode into base_identifier, loop while match(PERIOD) consume PERIOD and
consume IDENTIFIER to build the dotted chain (either by appending subsequent
identifier parts to base_identifier->name or by creating/attaching additional
IdentifierNode parts) and push the final base_identifier into n_struct->extends;
finally, confirm parse_self() semantics if structs allow instance methods so it
binds to struct context rather than current_class.
- Around line 1139-1141: The parse path for GDScriptTokenizer::Token::STRUCT
uses parse_class_member with AnnotationInfo::CLASS which lets class-level
annotations slip through only to error later; change the annotation target to
either AnnotationInfo::NONE to reject annotations early (consistent with enum
handling) or add a new AnnotationInfo::STRUCT enum value and use that here
(matching SIGNAL/FUNCTION/VARIABLE) and update any annotation filtering logic in
parse_class_member and related handlers to treat STRUCT accordingly so
annotations are validated at the same stage as other member kinds.
In @modules/gdscript/gdscript_parser.h:
- Around line 862-879: add_field() and add_method() currently allow duplicate
names by appending to fields/methods while overwriting field_indices/method_map;
change each to detect existing entries (check
field_indices.has(p_variable->identifier->name) and
method_map.has(p_function->identifier->name)) and emit/propagate a parser error
instead of silently overwriting — either call the parser's error reporting
(push_error) from inside add_field/add_method or change both signatures to
return bool so parse_struct() can push_error on false; ensure you stop adding
the duplicate to fields/methods when a duplicate is detected.
- Around line 834-884: StructNode declares its own start_line/end_line which
shadow Node::start_line/end_line and causes parsed extents to be lost; remove
the start_line and end_line members from StructNode so it uses the inherited
Node::start_line and Node::end_line, update any constructor or initialization
code in StructNode (and any places that set these fields) to assign the base
class fields (e.g., use this->start_line / this->end_line or plain start_line)
and verify any external uses (e.g., m_struct->start_line) continue to reference
the inherited fields.
In @modules/gdscript/gdscript_struct.cpp:
- Around line 46-55: The destructor ~GDScriptStruct currently manually calls
child->unreference(), checks child->get_reference_count(), and memdelete(child)
which can orphan children or double-free when multiple parents share a child;
change children from raw pointers to a proper reference type (e.g., Godot Ref<>
or a smart pointer) so ownership and lifetime are managed automatically, update
the children container and all uses to store Ref<GDScriptStruct> (or an
appropriate Ref wrapper) instead of GDScriptStruct*, remove manual
unreference/get_reference_count/memdelete calls from ~GDScriptStruct(), and
ensure any places that appended/removed children use the Ref<> API
(reference()/unreference() equivalents) so shared children are not leaked or
freed prematurely.
In @modules/gdscript/gdscript.cpp:
- Around line 1710-1716: The loop uses non-atomic unreference() followed by
get_reference_count() == 0, creating the same race as before; implement a
thread-safe helper on GDScriptStruct (e.g. unreference_and_delete_if_zero() or
unreference_and_get()) that atomically decrements the refcount and returns
whether it reached zero (and performs memdelete internally or returns the
count), then replace the loop over clear_data->structs to call that single
atomic helper instead of calling unreference() and get_reference_count()
separately (remove the separate memdelete(E) call).
- Around line 137-145: Destructor GDScriptStructClass::~GDScriptStructClass
currently calls struct_type->unreference() and then checks
struct_type->get_reference_count() == 0, which can race; change the logic to use
the boolean return value of struct_type->unreference() to decide deletion (i.e.,
call unreference() and if it returns true then memdelete(struct_type)), then set
struct_type = nullptr; this modifies the block inside
GDScriptStructClass::~GDScriptStructClass to perform the atomic check-and-delete
via unreference() instead of querying get_reference_count().
🟡 Minor comments (3)
modules/gdscript/gdscript_struct.cpp-264-274 (1)
264-274:deserialize()doesn't validate__type__matches the struct type.The serialized data contains a
__type__field (line 251), butdeserialize()ignores it. This could allow deserializing data from a different struct type into an incompatible instance.modules/gdscript/gdscript_tokenizer.h-128-128 (1)
128-128: STRUCT token addition is correct, but TOKENIZER_VERSION was not incremented.The STRUCT token is properly positioned in the Token::Type enum. However, the code comment at line 50 requires that
TOKENIZER_VERSIONingdscript_tokenizer_buffer.hbe incremented whenever the enum changes. The version remains at 101 and needs to be incremented.modules/gdscript/gdscript.h-66-81 (1)
66-81: Initializestruct_typetonullptrin-class declaration.The field at line 69 lacks in-class initialization, creating inconsistency with similar patterns elsewhere in the codebase (e.g.,
GDScriptStructInstance::struct_typeat gdscript_struct.h:146 and gdscript_function.h:68 both initialize with= nullptr). While the constructor correctly callsreference()and the destructor properly callsunreference()with cleanup, the uninitialized pointer poses a risk ifget_struct_type()is accessed before the constructor runs.Fix
class GDScriptStructClass : public RefCounted { GDCLASS(GDScriptStructClass, RefCounted); - GDScriptStruct *struct_type; + GDScriptStruct *struct_type = nullptr;
🧹 Nitpick comments (21)
modules/gdscript/gdscript_struct.cpp (3)
39-44: Reference count initialization at 0 requires careful lifecycle management.Starting
ref_countat 0 means the struct is in an "unreferenced" state upon creation. The comment indicates owners will add references, but this is error-prone—if any code path forgets to callreference(), the struct could be prematurely deleted or never cleaned up. Consider initializing to 1 (the standard pattern for ref-counted objects) and adjusting the ownership model accordingly.
168-183: Instance constructor doesn't initialize ref_count.Unlike
GDScriptStruct, the instance'sref_countis never explicitly initialized.SafeRefCountmay have a default, but explicit initialization ensures predictable behavior.Proposed fix
GDScriptStructInstance::GDScriptStructInstance(GDScriptStruct *p_struct_type) : - struct_type(p_struct_type) { + struct_type(p_struct_type) { ERR_FAIL_NULL(struct_type); + ref_count.init(1); // Initialize members
197-205:set()doesn't validate the value type against the member's declared type.The member's
MemberInfocontains type information, butset()accepts anyVariantwithout type checking. This could lead to runtime type errors when the struct is used.modules/gdscript/gdscript_struct.h (1)
64-69:MethodInfostruct name shadows the global::MethodInfo.The nested
MethodInfostruct has the same name as::MethodInfofrom Godot's core API, which could cause confusion or ambiguous references. Consider renaming toStructMethodInfoor similar.modules/gdscript/language_server/gdscript_extend_parser.cpp (1)
468-470: TODO placeholder for struct symbol handling.The TODO indicates incomplete language server support for structs. This will affect code navigation and symbol lookup for struct types.
Would you like me to help implement struct symbol parsing, or should this be tracked as a follow-up issue?
core/io/json.cpp (2)
827-831: Drop redundantvformat()in STRUCT serialization placeholder (and consider a targeted test)
vformat("Struct serialization not yet implemented.")has no format args—can be a plain string literal.Proposed fix
case Variant::STRUCT: { // TODO: Implement struct serialization - ERR_FAIL_V_MSG(Variant(), vformat("Struct serialization not yet implemented.")); + ERR_FAIL_V_MSG(Variant(), "Struct serialization not yet implemented."); } break;
1305-1309: Drop redundantvformat()in STRUCT deserialization placeholder (and consider a targeted test)
Same as serialization: no format args. Also consider a small test thatJSON::from_native()/JSON::to_native()fails deterministically for STRUCT, so callers don’t misinterpret a silentVariant()return as valid data.Proposed fix
case Variant::STRUCT: { // TODO: Implement struct deserialization - ERR_FAIL_V_MSG(Variant(), vformat("Struct deserialization not yet implemented.")); + ERR_FAIL_V_MSG(Variant(), "Struct deserialization not yet implemented."); } break;core/extension/gdextension_interface.cpp (2)
819-833: Good guard; prefer typed enum compare forp_type
Early return for STRUCT is the right behavior here; consider casting once and comparingVariant::Typeto avoid relying onintequality.Proposed fix
static GDExtensionPtrBuiltInMethod gdextension_variant_get_ptr_builtin_method(GDExtensionVariantType p_type, GDExtensionConstStringNamePtr p_method, GDExtensionInt p_hash) { // STRUCT type doesn't support built-in methods - if (p_type == (int)Variant::STRUCT) { + if (Variant::Type(p_type) == Variant::STRUCT) { return nullptr; }
834-841: Good guard; prefer typed enum compare forp_type
Same rationale as above.Proposed fix
static GDExtensionPtrConstructor gdextension_variant_get_ptr_constructor(GDExtensionVariantType p_type, int32_t p_constructor) { // STRUCT type doesn't support constructors (yet) - if (p_type == (int)Variant::STRUCT) { + if (Variant::Type(p_type) == Variant::STRUCT) { return nullptr; }core/extension/extension_api_dump.cpp (1)
43-44: Forward declaration should be insideTOOLS_ENABLEDguard.The
GDScriptStructInstanceforward declaration is placed outside the#ifdef TOOLS_ENABLEDblock (which starts at line 46), but it's only used within that block (in thestatic_assertat line 259). This may cause an unused declaration warning when building withoutTOOLS_ENABLED.♻️ Suggested fix
Move the forward declaration inside the
TOOLS_ENABLEDblock:-// Forward declaration for struct type -class GDScriptStructInstance; - #ifdef TOOLS_ENABLED +// Forward declaration for struct type +class GDScriptStructInstance; + #include "editor/doc/editor_help.h"core/variant/variant_setget.cpp (1)
261-267: Consider adding explicit return for clarity and consistency.When
struct_instanceis null, the code setsr_valid = falsebut doesn't explicitly return. While this works correctly due to the if-else chain structure, adding an explicitreturnwould be more consistent with the OBJECT case pattern and clearer for maintainability.♻️ Suggested improvement
} else if (type == Variant::STRUCT) { GDScriptStructInstance *struct_instance = VariantGetInternalPtr<GDScriptStructInstance>::get_ptr(this); if (struct_instance) { r_valid = struct_instance->set(p_member, p_value); return; } r_valid = false; + return; } else if (type == Variant::DICTIONARY) {modules/gdscript/gdscript_editor.cpp (2)
1222-1224: Verify that incomplete struct completion doesn't impact usability.The placeholder implementation immediately breaks without adding any completion options for struct members. While appropriate for a baseline framework PR, please verify:
- Structs can still be declared and used without causing editor crashes
- There's a follow-up plan to implement struct completion (consider linking an issue in the TODO)
- Users will receive appropriate feedback when attempting to use structs
Consider enhancing the TODO comment with a reference to a tracking issue:
case GDScriptParser::ClassNode::Member::STRUCT: - // TODO: Handle struct completion + // TODO: Handle struct completion (tracked in issue #XXXX) break;
2613-2615: Document the implications of incomplete struct type inference.Returning
falsehere means that expressions involving struct members will have unknown types, which will cascade to:
- No type-based code completion for chained member access (e.g.,
my_struct.member.)- Reduced effectiveness of type checking and validation
- Limited editor intelligence when working with struct data
This is acceptable for a baseline implementation, but ensure this limitation is documented for users or tracked for future implementation.
core/variant/variant_call.cpp (1)
1473-1476: Good: STRUCT is consistently hidden from builtin-method reflection; consider deduping the guard.
The repeated early-returns forSTRUCTacross reflection APIs (e.g.,has_method,get_builtin_method_*,get_method_list) are consistent and avoid populating method lists with non-existent builtins. If you want to reduce repetition, consider a tiny helper (or rely on the empty builtin tables) to keep this from drifting over time.Also applies to: 1483-1488, 1495-1499, 1508-1512, 1521-1525, 1534-1538, 1547-1551, 1561-1564, 1578-1582, 1591-1595, 1604-1607, 1617-1621, 1628-1632, 1641-1645, 1654-1658, 1674-1677, 1703-1706
core/variant/variant_utility.cpp (1)
868-955:type_convert(..., TYPE_STRUCT)currently “converts” by returning the original Variant (may hide bugs).If callers request
TYPE_STRUCTand the input isn’t alreadySTRUCT, returningp_variantsilently defeats the function’s contract and can lead to hard-to-spot logic errors. Consider either (a) failing with an error print and returningVariant(), or (b) implementing an actual conversion path (e.g., from Dictionary) once defined.modules/gdscript/gdscript_analyzer.cpp (1)
1643-1646:resolve_node(STRUCT)passestrueas “recursive”, but loses the source node for errors.
Prefer using the(StructNode*, const Node*)overload and passp_node(or the original parse node) so any errors are attributed correctly.Proposed tweak
case GDScriptParser::Node::STRUCT: // Resolve struct body - resolve_struct_body(static_cast<GDScriptParser::StructNode *>(p_node), true); + resolve_struct_body(static_cast<GDScriptParser::StructNode *>(p_node), p_node); break;modules/gdscript/gdscript_parser.cpp (4)
5359-5364: Prefer a fully-qualified name for structs inDataType::to_string()
Returning onlyidentifier->namecan be ambiguous for nested/duplicate struct names across scripts; consider returningfqsnwhen available (similar intent toClassNode::fqcn).
5416-5422: Array-of-struct hint string likely needs disambiguation
ForPROPERTY_HINT_ARRAY_TYPE,hint_stringis currently set to the bare struct name; this will collide across namespaces. Preferfqsnif it exists.
5532-5537:to_property_info()for STRUCT: confirm expectedclass_namecontract
Settingresult.type = Variant::STRUCTis fine, but storing only the short name inclass_namemay be insufficient if the property system expects a globally-unique identifier (esp. for editor serialization). Consider usingfqsnif that’s the canonical identity.
6079-6113: Avoidconst_castin TreePrinter struct printing
print_function()taking non-const forces aconst_castwhen iteratingconst FunctionNode *. Consider either storing non-const pointers inStructNode::methods(if mutation is expected) or overloadingprint_function(const FunctionNode *).modules/gdscript/gdscript_parser.h (1)
565-771:ClassNode::MemberSTRUCT accessors assume non-null identifiers; add guards or enforce invariants
get_name()/get_line()dereferencem_struct->identifierdirectly. If parser recovery ever creates aStructNodewithout an identifier (or it’s intentionally allowed later), this becomes a crash vector. Either assert the invariant or guard like the class/enum printing paths do.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
core/extension/extension_api_dump.cppcore/extension/gdextension_interface.cppcore/io/json.cppcore/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.cppmodules/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_compiler.cppmodules/gdscript/gdscript_compiler.hmodules/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/language_server/gdscript_extend_parser.cpp
🧰 Additional context used
🧬 Code graph analysis (10)
modules/gdscript/gdscript_analyzer.h (1)
modules/gdscript/gdscript_analyzer.cpp (4)
resolve_struct_body(1591-1623)resolve_struct_body(1591-1591)resolve_struct_body(1625-1628)resolve_struct_body(1625-1625)
core/variant/variant_internal.h (2)
modules/gdscript/gdscript_struct.h (1)
GDScriptStructInstance(142-177)core/variant/variant_setget.cpp (2)
get_ptr(594-594)get_ptr(603-603)
modules/gdscript/gdscript.cpp (2)
modules/gdscript/gdscript.h (3)
GDScriptStructClass(66-75)Variant(377-614)Variant(612-612)core/variant/variant_call.cpp (2)
callp(1359-1394)callp(1359-1359)
modules/gdscript/gdscript_compiler.h (1)
modules/gdscript/gdscript_compiler.cpp (2)
_compile_struct(3153-3210)_compile_struct(3153-3153)
core/variant/variant.h (4)
modules/gdscript/gdscript_struct.cpp (2)
GDScriptStructInstance(168-183)GDScriptStructInstance(185-187)modules/gdscript/gdscript_struct.h (1)
GDScriptStructInstance(142-177)modules/gdscript/gdscript.cpp (2)
GDScriptStructClass(129-135)GDScriptStructClass(137-145)modules/gdscript/gdscript.h (1)
GDScriptStructClass(66-75)
modules/gdscript/gdscript_parser.h (1)
modules/gdscript/gdscript_parser.cpp (4)
parse_struct(900-995)parse_struct(900-900)print_struct(6079-6113)print_struct(6079-6079)
modules/gdscript/gdscript_struct.cpp (1)
modules/gdscript/gdscript_struct.h (2)
GDScriptStruct(47-110)GDScriptStructInstance(142-177)
modules/gdscript/gdscript_parser.cpp (1)
modules/gdscript/gdscript_parser.h (1)
is_at_end(1583-1684)
core/variant/variant_utility.cpp (1)
core/variant/variant_parser.cpp (4)
write_to_string(2626-2630)write_to_string(2626-2626)parse(1981-1993)parse(1981-1981)
modules/gdscript/gdscript_struct.h (1)
modules/gdscript/gdscript_struct.cpp (40)
GDScriptStructInstance(168-183)GDScriptStructInstance(185-187)GDScriptStruct(39-44)GDScriptStruct(46-55)reference(57-59)reference(57-57)reference(189-191)reference(189-189)unreference(61-63)unreference(61-61)unreference(193-195)unreference(193-193)add_member(89-100)add_member(89-89)has_member(102-113)has_member(102-102)get_member_index(115-127)get_member_index(115-115)add_method(129-139)add_method(129-129)has_method(141-152)has_method(141-141)is_child_of(154-164)is_child_of(154-154)set(197-205)set(197-197)get(207-215)get(207-207)get_member_ptr(217-224)get_member_ptr(217-217)call(226-233)call(226-226)get_struct_name(235-237)get_struct_name(235-235)get_property_list(239-245)get_property_list(239-239)serialize(247-262)serialize(247-247)deserialize(264-275)deserialize(264-264)
🔇 Additional comments (38)
modules/gdscript/gdscript_byte_codegen.cpp (1)
113-114: Approved. The reference-counted claim is correct—GDScriptStruct and GDScriptStructInstance both implement reference counting via SafeRefCount, making the pooling prevention logic appropriate and consistent with other reference-counted types (OBJECT, DICTIONARY, ARRAY).modules/gdscript/gdscript_function.h (2)
58-58: LGTM for the new STRUCT kind and struct_type member.The enum extension and new member field are correctly placed to enable struct type handling in the GDScript type system.
Also applies to: 68-68
184-193: The STRUCT type checking logic is reasonable for the baseline.The NIL check allowing null values and the TODO for struct type matching are appropriate for an initial implementation. The struct type matching should be implemented before full release to ensure type safety.
modules/gdscript/gdscript_struct.h (2)
45-138: Well-structured struct type definition with clear API.The class provides a clean separation between type definition (
GDScriptStruct) and instance data (GDScriptStructInstance), with appropriate friend declarations and member/method management APIs.
140-177: Instance class provides necessary member access and serialization.The interface is complete for basic struct instance operations including reference counting, member access, and serialization.
modules/gdscript/language_server/gdscript_extend_parser.cpp (1)
1022-1024: TODO placeholder for struct API dumping.Similar to the symbol parsing, struct API information won't be available for documentation generation or API exports until implemented.
modules/gdscript/editor/gdscript_docgen.cpp (1)
152-159: STRUCT doctype handling is correctly implemented.The null checks for
struct_typeandidentifierare appropriate, and the fallback to "Struct" provides a reasonable default when the struct name isn't available.core/variant/variant_construct.cpp (1)
308-315: Constructor API short-circuiting forSTRUCTis consistent and safe
The null/empty returns for constructor lookup APIs avoid invalid indexing and match the “no built-in constructors” model for dynamic structs.Also applies to: 320-327, 332-339, 344-351, 356-362, 367-373
modules/gdscript/gdscript_tokenizer.cpp (1)
530-536: Keyword registration forstructlooks correct
Token name mapping + keyword dispatch update are consistent (and guarded by thetoken_names/TK_MAXstatic_assert).modules/gdscript/gdscript_compiler.h (1)
164-166: No action needed._compile_struct()is properly wired into the compiler pipeline—it's called at line 3046 during class/struct member compilation and correctly registers structs in bothp_script->structsandp_script->constants.Likely an incorrect or invalid review comment.
core/extension/extension_api_dump.cpp (2)
216-216: LGTM!The STRUCT type size entry correctly specifies pointer sizes for all build configurations, consistent with how other pointer-based types (OBJECT, STRING_NAME, etc.) are defined.
259-259: LGTM!The compile-time size assertion ensures ABI compatibility for the STRUCT type in the extension API, following the established pattern for other variant types.
core/variant/variant_setget.cpp (2)
299-310: LGTM!The STRUCT case in
get_namedcorrectly handles member retrieval with proper null checking and validity propagation. The const-correctness is maintained appropriately.
37-37: Coupling between core and GDScript module is intentional, not an architectural violation.The include of
modules/gdscript/gdscript_struct.hexists across multiple core files (variant_setget.cpp, variant_parser.cpp, variant.cpp, logger.cpp, project_settings.cpp), indicating this is an accepted design pattern in Redot.Variant::STRUCTis integrated as a first-class variant type with template specializations in core headers, andGDScriptStructInstanceis directly called in set_named/get_named operations.There is no evidence that the GDScript module can be disabled, and forward declarations alone cannot work because the code instantiates and calls methods on GDScriptStructInstance objects. If architectural decoupling is a goal, consider whether GDScriptStruct should be promoted to core rather than attempting to decouple a tightly integrated system through callback patterns.
modules/gdscript/gdscript_analyzer.h (1)
83-84: LGTM!The
resolve_struct_bodymethod declarations follow the established pattern used byresolve_class_body, maintaining API consistency. Based on the implementation in the relevant code snippets, the recursive variant currently delegates to the non-recursive version, which is appropriate given the comment that nested structs aren't supported yet.modules/gdscript/gdscript_editor.cpp (2)
3989-3991: Confirm consistent error handling for unresolved struct symbols.Returning
ERR_CANT_RESOLVEis consistent with the error handling pattern used elsewhere in this function (e.g., line 3991 for GROUP members, though that's a no-op case). This will cause IDE features like:
- Go-to-definition on struct members
- Symbol hover information
- Find references
to fail gracefully. Verify that the calling code handles
ERR_CANT_RESOLVEappropriately without surfacing confusing errors to users.
4296-4298: LGTM: Consistent error handling for unresolved struct types.The placement and error code (
ERR_CANT_RESOLVE) align well with the adjacentRESOLVINGandUNRESOLVEDcases (lines 4299-4302), which also cannot be looked up. This maintains a consistent error handling pattern for types that cannot be resolved at lookup time.core/variant/variant_call.cpp (1)
1379-1383: STRUCT call paths correctly short-circuit to “invalid method”.
At Line 1379-1383 / Line 1417-1422 / Line 1442-1446, returningCALL_ERROR_INVALID_METHODforSTRUCTprevents accidental builtin-method resolution and matches the stated “no built-in methods” contract.Also applies to: 1417-1422, 1442-1446
core/variant/variant.h (1)
70-71: Verify all Type-indexed tables are updated for Variant::STRUCT.Adding
Variant::STRUCTto the enum at line 127 requires updating all VARIANT_MAX-sized arrays and type-switch statements throughout the codebase. Theneeds_deinit[STRUCT]=truemarking at line 316 is correct since_clear_internal()properly handles STRUCT cleanup viastruct_instance->unreference(). However, ensure these tables have been updated:
construct_data[VARIANT_MAX](variant_construct.cpp:44)variant_setters_getters[VARIANT_MAX],variant_indexed_setters_getters[VARIANT_MAX],variant_keyed_setters_getters[VARIANT_MAX](variant_setget.cpp)destruct_pointers[VARIANT_MAX](variant_destruct.cpp:35)- Operator evaluation tables
[OP_MAX][VARIANT_MAX][VARIANT_MAX](variant_op.cpp:37-40)- Type registration allocations in
_register_variant_call()(variant_call.cpp:1942-1945)core/variant/variant.cpp (2)
46-181:Variant::get_type_name(STRUCT) -> "Struct"looks consistent.No concerns with the naming addition.
3516-3521:get_construct_string()updated call looks fine.Assuming
VariantWriter::write_to_string(const Variant&, String&)exists and uses the intended defaults, this is a nice simplification.modules/gdscript/gdscript.h (4)
37-40: Forward declarations for struct types are a good include-time improvement.Keeps compile dependencies down and enables pointers/references safely in the header.
118-120: Friend access additions are reasonable given struct integration needs.No concerns, assuming this is required for efficient construction/inspection during compilation/runtime.
98-107:ClearData::structsis properly used inGDScript::clear().The implementation correctly handles struct ownership release. When clearing, it removes the constant wrapper references (line 1668), collects structs into
clear_data->structs(line 1670), then properly unreferences and deletes them when ref count reaches zero (lines 1711-1714). This pattern is consistent with how functions and scripts are handled.
134-140: No action needed—GDScript::structsuses reference counting with deterministic cleanup.The
structsmap storesGDScriptStruct*pointers, but they are managed via reference counting, not raw pointers. Onclear(), the script erases the constant wrapper (which holds aRef<GDScriptStructClass>), collects all structs, then unreferences each and deletes them when the reference count reaches zero. This ensures no leaks or use-after-free.modules/gdscript/gdscript_compiler.cpp (2)
38-38: Include looks fine; ensure it’s actually needed (and doesn’t create a cycle).
gdscript_struct.his now required by this TU, so the include is reasonable.
3016-3050: Struct compilation wired in OK, but verify ordering + name collision behavior.
This compiles structs alongside functions/variables, but_compile_struct()also injects a constant with the struct’s name. Please confirm the analyzer/parser already rejects conflicts (struct name vs existing constant/class/member), otherwise this can silently overwrite constants.modules/gdscript/gdscript_analyzer.cpp (3)
1584-1586: Good: recursive class-body resolution now visits structs.
Callingresolve_struct_body(member.m_struct, true)from the recursive wrapper matches how nested classes are handled.
2726-2727: Good:STRUCTincluded in the “unreachable” expression-node cases.
Helps ensure new node kinds don’t accidentally get treated as expressions.
1625-1628: Compilation will succeed; the compilation concern is unfounded.
resolve_struct_body(p_struct)compiles without issue because the first overload at line 83 of the header declaresconst GDScriptParser::Node *p_source = nullptr, providing a default argument. The call matches this overload correctly.However, the parameter
p_recursivein this overload is unused, which aligns with the comment noting recursive resolution isn't needed. Consider removing the unused parameter from the signature.Likely an incorrect or invalid review comment.
modules/gdscript/gdscript.cpp (5)
40-40: LGTM!The include for
gdscript_struct.his correctly placed with other GDScript-related includes.
125-127: LGTM!The design to handle "new" via
callprather than static binding is reasonable and the comment documents the intent.
184-191: LGTM!The method dispatch follows the same pattern as
GDScriptNativeClass::callpand correctly routes "new" calls while returning an error for unsupported methods.
1664-1672: LGTM!The struct cleanup logic correctly removes the constant wrapper references first, then defers struct deletion to the root cleanup phase. This prevents dangling pointers during the cleanup process.
165-182: This approach is intentional interim design, not a flaw. The Variant class explicitly grants friendship to GDScriptStructClass with the comment "Needed for proper struct construction," proving the designers were aware of and approved this pattern. Additionally,gdextension_interface.cppdocuments that "STRUCT type doesn't support constructors (yet)," meaning no public constructor API currently exists. Thememcpyapproach mirrors Variant's own copy constructor implementation for STRUCT types and aligns with the recommendedVariantGetInternalPtrpattern used elsewhere in the codebase. This is a necessary interim solution that will be replaced when proper STRUCT constructors are implemented.Likely an incorrect or invalid review comment.
modules/gdscript/gdscript_parser.cpp (2)
4303-4304: Good: STRUCT token registered in parse rules table
No concerns—keeping itPREC_NONEprevents it from being treated as an expression starter.
5949-5951: Good: TreePrinter handles struct members in class body
This keeps debug output aligned with the AST changes.modules/gdscript/gdscript_parser.h (1)
1591-1605: Good: public hooks for parsing and debug printing are in place
parse_struct(bool)andTreePrinter::print_struct()are wired consistently with the new node type.Also applies to: 1756-1757
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
modules/gdscript/gdscript_function.h (2)
52-69:GDScriptDataTypecopy/equality ignorestruct_type(typed struct info will be lost).
Right now,struct_typecan be set but will not participate inoperator==/operator=, so typed-struct comparisons and copies will silently degrade.Proposed fix
bool operator==(const GDScriptDataType &p_other) const { return kind == p_other.kind && has_type == p_other.has_type && builtin_type == p_other.builtin_type && native_type == p_other.native_type && (script_type == p_other.script_type || script_type_ref == p_other.script_type_ref) && + struct_type == p_other.struct_type && container_element_types == p_other.container_element_types; } void operator=(const GDScriptDataType &p_other) { kind = p_other.kind; has_type = p_other.has_type; builtin_type = p_other.builtin_type; native_type = p_other.native_type; script_type = p_other.script_type; script_type_ref = p_other.script_type_ref; + struct_type = p_other.struct_type; container_element_types = p_other.container_element_types; }
70-196: Struct type checking is incomplete inis_type(). TheSTRUCTcase (line 192) accepts any struct variant without comparing thestruct_typefield, making struct type enforcement ineffective. The TODO comment at line 192 confirms this is a known gap.All other type cases perform strict identity checks (SCRIPT/GDSCRIPT compares
script_typepointers), but the STRUCT case skips this. Sincestruct_typeis stored inGDScriptDataTypeandVariantInternalprovides helpers to extractGDScriptStructInstancefrom struct variants, add a comparison following the pattern used for scripts:case STRUCT: { if (p_variant.get_type() == Variant::NIL) { return true; } if (p_variant.get_type() != Variant::STRUCT) { return false; } GDScriptStructInstance *inst = VariantInternal::gdscript_struct_instance_from_variant(&p_variant); if (!inst) { return false; } return inst->get_struct_type() == struct_type; }modules/gdscript/gdscript_parser.h (2)
104-266:DataType::operator=dropsstruct_type(copied types lose struct identity).
This will break any flow that copiesDataType(very common in the parser/analyzer).Proposed fix
void operator=(const DataType &p_other) { kind = p_other.kind; type_source = p_other.type_source; is_read_only = p_other.is_read_only; is_constant = p_other.is_constant; is_meta_type = p_other.is_meta_type; is_pseudo_type = p_other.is_pseudo_type; is_coroutine = p_other.is_coroutine; builtin_type = p_other.builtin_type; native_type = p_other.native_type; enum_type = p_other.enum_type; script_type = p_other.script_type; script_path = p_other.script_path; class_type = p_other.class_type; + struct_type = p_other.struct_type; method_info = p_other.method_info; enum_values = p_other.enum_values; container_element_types = p_other.container_element_types; }
565-770: Settype_sourceon STRUCT datatype to match CLASS member handling.The STRUCT case in
Member::get_datatype()constructs aDataTypewithout settingtype_source, leaving it at the defaultUNDETECTEDvalue. SinceDataType::operator==short-circuits when either operand hastype_source == UNDETECTED, this makes struct member type comparisons unreliable.The CLASS case delegates to
m_class->get_datatype(), which returns a properly initialized datatype. Either delegate STRUCT tom_struct->get_datatype()(if such a method exists) or explicitly settype_sourceto match how the analyzer initializes struct datatypes.
🤖 Fix all issues with AI agents
In @core/variant/variant_parser.cpp:
- Around line 2177-2196: The Variant::STRUCT branch (GDScriptStructInstance +
serialize()) currently calls write(...) on the resulting Dictionary without
accounting for recursion depth; update the STRUCT case to mirror
ARRAY/DICTIONARY handling by incrementing p_recursion_count before recursing
(e.g., pass p_recursion_count+1 or increment then restore) and only recurse if
within the same recursion limits used elsewhere, otherwise call
p_store_string_func(..., "null"); also consider embedding the struct type
identity (e.g., a "__struct" field) into the Dictionary returned by
GDScriptStructInstance::serialize() to preserve type information for future
deserialization.
In @modules/gdscript/gdscript_analyzer.cpp:
- Around line 6325-6326: The struct type path is falling through in
check_type_compatibility because STRUCT wasn't treated as an "already solved"
type; update the switch handling inside check_type_compatibility (the branches
that currently list GDScriptParser::DataType::RESOLVING) to include
GDScriptParser::DataType::STRUCT as well, or alternatively add an explicit check
that if both left and right types are DataType::STRUCT (or one side is Variant)
then return true; modify the same mirrored switch/logic later in the function
(the second occurrence) so struct-to-struct compatibility is allowed
consistently.
- Around line 5829-5846: get_function_signature() currently treats any struct
method except a no-arg new() as an error, even though resolve_struct_body() can
supply method signatures; change get_function_signature() to query the struct's
resolved body for method names and signatures instead of unconditionally pushing
an error. Specifically, inside the block that checks p_base_type.kind ==
GDScriptParser::DataType::STRUCT, keep the existing new() handling (setting
r_return_type, r_method_flags.set_flag(METHOD_FLAG_STATIC) etc.) but for other
p_function values look up the method in the struct definition (via the same
resolution used by resolve_struct_body() or the struct's method table), populate
r_return_type and any parameter info from that resolved signature, and only call
push_error when the method truly does not exist; if struct methods are not yet
supported, instead ensure get_function_signature() rejects method declarations
earlier by checking the struct declaration path rather than emitting this
runtime-style error here.
- Around line 1646-1649: The call in resolve_node for
GDScriptParser::Node::STRUCT uses
resolve_struct_body(static_cast<GDScriptParser::StructNode *>(p_node), true)
which calls the overload that omits p_source and thus loses diagnostic context;
update the call in resolve_node to invoke the overload that accepts and forwards
the p_source argument (e.g.,
resolve_struct_body(static_cast<GDScriptParser::StructNode *>(p_node), true,
p_source) or otherwise pass the source through) so the source location is
preserved when emitting diagnostics from resolve_struct_body.
- Around line 1593-1631: The resolver currently ignores StructNode::extends and
silently skips untyped fields; update
GDScriptAnalyzer::resolve_struct_body(GDScriptParser::StructNode *p_struct,
const GDScriptParser::Node *p_source) to fail fast on unsupported struct
inheritance by emitting a diagnostic via the analyzer's reporting facility
(e.g., call the project’s report/diagnostic API referencing p_struct->extends)
and return; for field handling, if field.variable->datatype_specifier is null
then either call the variable inference routine
(resolve_variable(field.variable, p_source) or the analyzer's existing
resolution helper) to populate the datatype or emit a clear diagnostic that
struct fields must be explicitly typed, and keep the existing path of
resolve_datatype(...) + type_from_metatype(...) for typed fields; ensure you
reference resolve_struct_body, GDScriptParser::StructNode, resolve_datatype,
type_from_metatype, and resolve_variable (or the analyzer’s diagnostic API) so
the changes are applied in the correct place.
In @modules/gdscript/gdscript_parser.cpp:
- Around line 5435-5441: The Dictionary hint builder is missing a case for
STRUCT, so typed dictionaries like Dictionary[StructK, StructV] fall back to
"Variant"; add a case STRUCT in the switch blocks that build key/value hints
(the same place that sets result.hint and result.hint_string for Arrays and
direct struct properties) and mirror the logic used for ARRAY/STRUCT handling:
when elem_type.struct_type and elem_type.struct_type->identifier are non-null,
set result.hint_string to elem_type.struct_type->identifier->name and set the
same hint enum used for struct-typed properties (match the assignment used in
the ARRAY branch), ensuring the key and value switch sections both include this
STRUCT handling as you added for lines ~5435-5441 and ~5551-5557.
- Around line 920-935: The extends-chain parsing currently mutates
base_identifier->name (in parse_struct) and also pushes each identifier part
into n_struct->extends, which causes duplicated/garbled output when
TreePrinter::print_struct joins entries; fix by stopping the mutation: do not
concatenate into base_identifier->name inside the while loop—simply parse and
push each next_part (via parse_identifier) into n_struct->extends, leaving
base_identifier and next_part names intact; if a fully-qualified single
IdentifierNode is required elsewhere, create a new IdentifierNode for the joined
name instead of mutating existing nodes (refer to parse_identifier,
base_identifier, next_part, n_struct->extends, and TreePrinter::print_struct).
- Around line 900-1014: parse_struct currently ignores Token::STATIC and
unconditionally reassigns n_struct->constructor for any parsed _init even if
add_method failed; update parse_struct to handle
GDScriptTokenizer::Token::STATIC by either parsing the following VAR/FUNC as
static or emitting a clear "static not supported in struct" error (use current
token checks around the switch and produce a specific push_error when
encountering Token::STATIC), and change the _init handling so the assignment to
n_struct->constructor only occurs after parse_function returned non-null AND
n_struct->add_method(function) succeeded (i.e., move the if
(function->identifier->name == SNAME("_init")) { n_struct->constructor =
function; } inside the block where add_method returned true).
In @modules/gdscript/gdscript_struct.cpp:
- Around line 185-191: The unreference() method in GDScriptStructInstance
currently returns ref_count.unref() without deleting the object, causing a leak;
change unreference() to call ref_count.unref(), and if it returns true perform
self-deletion (delete this) and then return true, otherwise return false,
ensuring callers never access the instance after unreference() returns true;
references: GDScriptStructInstance::unreference(),
GDScriptStructInstance::reference(), and ref_count.unref().
- Around line 243-271: serialize() and deserialize() only handle the current
struct's members; update both GDScriptStructInstance::serialize and
GDScriptStructInstance::deserialize to walk the inheritance chain (using
struct_type and struct_type->get_base_struct()) and process get_members() for
each base struct as well, writing/reading into the instance's members array (use
the existing members and members.write access patterns) so inherited members are
included in the Dictionary and restored on load; ensure keys use each member's
StringName (E.key) and map to the correct E.value.index when assigning from
p_data.
- Around line 164-179: The GDScriptStructInstance constructor only sizes and
initializes members from struct_type->get_members(), ignoring inherited members;
update the ctor to walk the inheritance chain (follow struct_type and its
base_struct pointers) to compute the total member count, then call
members.resize(total_count) so space includes base members, and when applying
defaults iterate over all members from the entire chain (e.g., aggregate each
struct's get_members() entries) and assign members.write[info.index] =
info.default_value for every GDScriptStruct::MemberInfo encountered so inherited
default_value and indices are applied and no out-of-bounds access occurs.
- Around line 61-83: The create_instance implementation must validate allocation
and arguments: check the memnew result and return nullptr with an error/log if
allocation fails; verify p_argcount exactly matches the expected member count
(or explicitly handle extra args by returning an error instead of silently
ignoring) and report the mismatch; for each positional argument in
GDScriptStruct::create_instance, look up MemberInfo from members and validate
the Variant type against the member's declared type (use the MemberInfo
type/variant type metadata) before assigning into
GDScriptStructInstance::members, and if the type check fails, log/return an
error (nullptr) with a clear message identifying the struct name, member_name
and expected vs actual type; ensure all error returns are consistent so callers
can detect and surface failures.
- Around line 85-96: The member index in GDScriptStruct::add_member currently
uses members.size(), which ignores members inherited from base_struct and causes
index collisions; add a helper GDScriptStruct::get_member_count() that returns
members.size() plus (base_struct ? base_struct->get_member_count() : 0) and
change add_member to set info.index = get_member_count() so new members are
indexed after inherited ones; update any code that relies on member indexing to
use get_member_count where appropriate.
In @modules/gdscript/gdscript.cpp:
- Around line 129-135: The constructor comment and null handling for
GDScriptStructClass are misleading and unsafe; either add an explicit default
constructor GDScriptStructClass() that sets struct_type to nullptr and keep the
parameterized GDScriptStructClass(GDScriptStruct *p_struct) as-is, or enforce
non-null by adding ERR_FAIL_NULL(p_struct) in the parameterized constructor and
remove the misleading "default constructor" comment; also update the comment
above the constructors to accurately state whether null struct_type is allowed
and, if null is allowed, audit and add null checks in methods like _new() to
avoid dereferencing a null struct_type.
- Around line 147-163: In GDScriptStructClass::set_struct_type, remove the
manual memdelete call on struct_type after checking get_reference_count() == 0
to avoid the race condition; instead only call struct_type->unreference() and
rely on the reference-counting system to delete the object when appropriate
(leave the assignment struct_type = p_struct and the subsequent
struct_type->reference() logic intact).
- Around line 165-182: In GDScriptStructClass::_new, the STRUCT Variant pointer
is copied using memcpy(result._data._mem, &instance, sizeof(instance)); update
this to use sizeof(void *) instead of sizeof(instance) to make the intent
explicit and match core Variant patterns (i.e., memcpy(result._data._mem,
&instance, sizeof(void *))); keep the rest of the logic (reference(),
Variant::STRUCT setup) unchanged and ensure references to instance and result
remain the same.
🧹 Nitpick comments (8)
core/variant/variant_utility.cpp (1)
920-922: Track the incomplete STRUCT conversion implementation.The STRUCT case currently returns the input variant unchanged. While this is a reasonable placeholder for initial struct support, ensure that proper struct type conversion is implemented before the feature is considered complete.
Do you want me to open an issue to track implementing proper struct conversion logic, or is this already planned in a follow-up PR?
modules/gdscript/gdscript_parser.h (1)
214-234:DataType::operator==for structs probably needs anfqsnfallback (like classes do withfqcn).
Right now,STRUCTequality isstruct_type == p_other.struct_typeonly; if the same struct is represented by differentStructNode*pointers (e.g., dependent parser instances), comparisons may fail unexpectedly.Possible adjustment (match class behavior)
case STRUCT: - return struct_type == p_other.struct_type; + return struct_type == p_other.struct_type || + (struct_type && p_other.struct_type && struct_type->fqsn == p_other.struct_type->fqsn);core/variant/variant_construct.cpp (1)
266-303: STRUCT constructor short-circuit is reasonable; consider tightening the error payload for better diagnostics.
If any error-string formatting usesargument/expected, setting them to 0 may produce odd messages.Possible tweak (optional)
base = Variant(); r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD; - r_error.argument = 0; - r_error.expected = 0; + r_error.argument = p_argcount; // or -1 if convention exists + r_error.expected = 0; return;modules/gdscript/gdscript_byte_codegen.cpp (1)
68-120: Intentional fallthrough forVariant::STRUCTshould be made explicit (avoid-Wimplicit-fallthrough).
Right now it relies on a comment only.Proposed fix
case Variant::PACKED_VECTOR4_ARRAY: case Variant::STRUCT: // Structs are reference counted, so we don't use the pool for them. + [[fallthrough]]; case Variant::VARIANT_MAX: // Arrays, dictionaries, and objects are reference counted, so we don't use the pool for them. temp_type = Variant::NIL; break;modules/gdscript/gdscript_vm.cpp (2)
33-38: Include looks fine; consider whether a forward-declare would suffice.
gdscript_struct.his only needed here because the opcode handler assigns a typedGDScriptStructInstance*nullptr. If you switch the handler to re-initialize viaVariantInternal::initialize(arg, Variant::STRUCT), you can likely drop this include (compile-time win).
3840-3849: Double-check STRUCT “type-adjust” semantics (STRUCT-null vs NIL) and consider a safer init pattern.Today the handler does:
VariantInternal::clear(arg);*arg = (GDScriptStructInstance *)nullptr;Two things to verify:
- After this, does
arg->get_type()becomeVariant::STRUCT(null instance) rather thanVariant::NIL? Typed-slot/type-check logic often assumes the slot is in the target Variant type after an adjust opcode.- Is
VariantInternal::clear()the correct way to decref/release the previous STRUCT payload (no double-decref / no leak)?If
VariantInternal::initialize(arg, Variant::STRUCT)is supported, it’s typically a clearer/safer way to guarantee the slot ends in the STRUCT type without relying on pointer-assignment overload resolution or the struct header include:Proposed refactor (if VariantInternal supports STRUCT init)
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); - *arg = (GDScriptStructInstance *)nullptr; + VariantInternal::initialize(arg, Variant::STRUCT); ip += 2; } DISPATCH_OPCODE;modules/gdscript/gdscript_parser.cpp (2)
5378-5383: Consider returning a stable struct display name (not just"<struct>").
DataType::to_string()returns"<struct>"when unnamed; but for named structs it currently returns only the short identifier (Line 5379-5382). If you havefqsnavailable, it’s usually more useful for diagnostics/exports to returnfqsn(oridentifier+fqsn).
5968-5970: TreePrinter struct support is good, but avoidconst_cast(const-correctness).
print_struct()iteratesconst FunctionNode *thenconst_casts to callprint_function()(Line 6127-6129). Prefer makingprint_function(and possibly other printer helpers) acceptconst FunctionNode *to keep printers read-only.Also applies to: 6098-6133
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/variant/variant_construct.cppcore/variant/variant_parser.cppcore/variant/variant_utility.cppmodules/gdscript/gdscript.cppmodules/gdscript/gdscript_analyzer.cppmodules/gdscript/gdscript_byte_codegen.cppmodules/gdscript/gdscript_compiler.cppmodules/gdscript/gdscript_disassembler.cppmodules/gdscript/gdscript_function.hmodules/gdscript/gdscript_parser.cppmodules/gdscript/gdscript_parser.hmodules/gdscript/gdscript_struct.cppmodules/gdscript/gdscript_vm.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/gdscript/gdscript_compiler.cpp
🧰 Additional context used
🧬 Code graph analysis (3)
modules/gdscript/gdscript_function.h (2)
modules/gdscript/gdscript_struct.cpp (2)
GDScriptStruct(39-44)GDScriptStruct(46-51)modules/gdscript/gdscript_struct.h (1)
GDScriptStruct(47-110)
core/variant/variant_utility.cpp (1)
core/variant/variant_parser.cpp (4)
write_to_string(2635-2639)write_to_string(2635-2635)parse(1983-1995)parse(1983-1983)
modules/gdscript/gdscript.cpp (2)
modules/gdscript/gdscript.h (3)
GDScriptStructClass(66-75)Variant(377-614)Variant(612-612)core/variant/variant_call.cpp (2)
callp(1359-1394)callp(1359-1359)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🐧 Linux / Minimal template (target=template_release, everything disabled)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🏁 Windows / Editor (target=editor)
- GitHub Check: 🤖 Android / Template arm64 (target=template_release, arch=arm64)
- GitHub Check: 🍎 macOS / Template (target=template_release)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🤖 Android / Template arm32 (target=template_release, arch=arm32)
- GitHub Check: 🍏 iOS / Template (target=template_release)
🔇 Additional comments (20)
core/variant/variant_utility.cpp (2)
1061-1061: The simplifiedVariantWriter::write_to_stringcall is correct. The method has a single 6-parameter signature with all four omitted parameters defaulting to:p_encode_res_func = nullptr,p_encode_res_ud = nullptr,p_compat = true,p_full_objects = false.The 2-parameter call properly leverages these defaults, ensuring
var_to_strusesp_full_objects=false, whilevar_to_str_with_objectson line 1067 explicitly sets it totrueas intended.
1076-1078: No action needed. The parse overload exists with proper defaults (last two parameters have default values), the uninitializedlinevariable is safe since it's passed as a reference parameter thatparsewill initialize, and the error handling is deliberately ignored via the(void)cast—a design choice for these utility functions that lack error reporting mechanisms. Bothstr_to_varandstr_to_var_with_objectsfollow the intended pattern: attempt to parse and return the resultingVariantregardless of success or failure.modules/gdscript/gdscript_parser.h (2)
834-889: StructNode shape looks solid; duplicate guards are a good start.
Thefield_indices/method_mapchecks are simple and effective.
1596-1609: All struct parsing and printing hooks are properly integrated.Verification confirms
parse_structis called fromparse_class_body()viaparse_class_member()when the STRUCT token is encountered (line 1159), and the TreePrinter properly dispatches toprint_struct()for STRUCT nodes (line 5969). No missing integration points detected.modules/gdscript/gdscript_disassembler.cpp (1)
1259-1307: Disassembler coverage forOPCODE_TYPE_ADJUST_STRUCTis consistent with other type-adjust opcodes.
No concerns.core/variant/variant_construct.cpp (1)
310-392: STRUCT “no constructors” behavior is consistent across the constructor introspection APIs.
Looks good.core/variant/variant_parser.cpp (1)
33-44: Include gating and build layering are correct. The conditional compilation via#ifdef MODULE_GDSCRIPT_ENABLEDmatches the established pattern used elsewhere in core for module-dependent code (e.g., regex incore/io/logger.cpp, mono incore/config/project_settings.cpp). Core→modules includes are acceptable in this project when properly gated.modules/gdscript/gdscript_byte_codegen.cpp (1)
435-560: STRUCT type-adjust implementation is sound. The opcode emission correctly delegates to dedicatedOPCODE_TYPE_ADJUST_STRUCT, and the VM semantics properly handle the reference-counted value by callingVariantInternal::clear()followed by null initialization, matching the approach used forOBJECTtypes.modules/gdscript/gdscript_function.h (1)
283-443: Opcode implementation verified across all components.The
OPCODE_TYPE_ADJUST_STRUCTaddition is properly implemented:
- VM handler (gdscript_vm.cpp:3840): Clears the variant and sets to nullptr, consistent with reference-counted types like OBJECT
- Bytecode generator (gdscript_byte_codegen.cpp:553): Emits the opcode for
Variant::STRUCTcase- Disassembler (gdscript_disassembler.cpp): Includes DISASSEMBLE_TYPE_ADJUST(STRUCT) case
- Dispatch validation: Static assert ensures jump table and enum opcodes remain synchronized
modules/gdscript/gdscript_vm.cpp (1)
255-418: The jump table ordering is correct and matches the enum ordering exactly. Verification confirms all 159 opcodes are in the same sequence in bothswitch_table_ops[](gdscript_vm.cpp) and the enum definition (gdscript_function.h), includingOPCODE_TYPE_ADJUST_STRUCTat the correct position.modules/gdscript/gdscript_parser.cpp (2)
1158-1161: Struct member parsing is wired in cleanly.
DelegatingSTRUCTtoparse_class_member(&GDScriptParser::parse_struct, ...)matches the existing class/enum flow.
4322-4323: Parse rule table update forSTRUCTlooks consistent.
No prefix/infix rules means it won’t be (incorrectly) parsed as an expression token.modules/gdscript/gdscript_analyzer.cpp (3)
1586-1588: Recursive struct body resolution wiring looks good.
2729-2730: Good:STRUCTincluded in the “non-expression” exhaustiveness guard.
5027-5028: Indexing checks mentionVariant::STRUCT, but verify struct instances can’t slip through the non-builtin path.If a struct instance is represented as
DataType::STRUCT(notDataType::BUILTINwithbuiltin_type == Variant::STRUCT), the current indexing validation path may not hit theseVariant::STRUCTcases and could end up treating it like a generic “object-ish” base. Worth confirming with a couple of analyzer tests (s.x,s["x"],s[0]) to ensure expected behavior.Also applies to: 5096-5097
modules/gdscript/gdscript.cpp (2)
1664-1672: LGTM - Proper struct cleanup ordering.The approach of clearing constants first (which may hold
Ref<GDScriptStructClass>references) before adding structs toclear_dataensures proper reference management during teardown.
1710-1715: LGTM - Consistent reference counting approach.Unreferencing without manual deletion is consistent with the
GDScriptStructClassdestructor's documented approach and prevents race conditions during cleanup.modules/gdscript/gdscript_struct.cpp (3)
39-51: LGTM - Standard reference counting initialization.The constructor properly initializes the reference count to 0, and the destructor correctly delegates cleanup to the reference counting system.
53-59: LGTM - Standard reference management for GDScriptStruct.These methods correctly wrap the SafeRefCount operations for managing the struct's lifetime.
98-123: LGTM - Proper inheritance chain traversal.Both
has_memberandget_member_indexcorrectly check the current struct first, then fall back to the base struct, properly supporting struct inheritance.
| case GDScriptParser::Node::STRUCT: | ||
| // Resolve struct body | ||
| resolve_struct_body(static_cast<GDScriptParser::StructNode *>(p_node), true); | ||
| break; |
There was a problem hiding this comment.
Preserve source location when resolving structs from resolve_node().
resolve_struct_body(..., true) ends up calling the overload without passing p_source, so diagnostics lose context.
Proposed fix
case GDScriptParser::Node::STRUCT:
// Resolve struct body
- resolve_struct_body(static_cast<GDScriptParser::StructNode *>(p_node), true);
+ resolve_struct_body(static_cast<GDScriptParser::StructNode *>(p_node), p_node);
break;🤖 Prompt for AI Agents
In @modules/gdscript/gdscript_analyzer.cpp around lines 1646 - 1649, The call in
resolve_node for GDScriptParser::Node::STRUCT uses
resolve_struct_body(static_cast<GDScriptParser::StructNode *>(p_node), true)
which calls the overload that omits p_source and thus loses diagnostic context;
update the call in resolve_node to invoke the overload that accepts and forwards
the p_source argument (e.g.,
resolve_struct_body(static_cast<GDScriptParser::StructNode *>(p_node), true,
p_source) or otherwise pass the source through) so the source location is
preserved when emitting diagnostics from resolve_struct_body.
Still chasing a massive memroy leak
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/gdscript/gdscript_analyzer.cpp (1)
6387-6492: Type compatibility for STRUCT looks directionally right, but meta-types are hard-rejected and inheritance relies onbase_struct_typebeing populated.
Two things to confirm:
- If the language allows
MyStructas a type (meta-type) in annotations, rejectingp_source.is_meta_type/p_target.is_meta_typemight break type-checking flows (or produce confusing errors) unless structs truly cannot be used as types yet.- The inheritance walk assumes
src_struct->base_struct_type.kind == STRUCTis correctly set;resolve_struct_body()currently has TODO for extends resolution, so this may never succeed and make all struct inheritance compatibility fail.Given the PR’s “work ongoing” state, consider pushing a clearer diagnostic when
extendsis present but not supported yet.
🤖 Fix all issues with AI agents
In @modules/gdscript/gdscript_analyzer.cpp:
- Around line 1593-1631: resolve_struct_body currently lacks idempotence guards
and runs class-centric _init logic via
resolve_function_signature/resolve_function_body, causing wrong constructor
handling and unsafe body resolution; add a guard on GDScriptParser::StructNode
(e.g., checked/set via p_struct->is_resolving()/p_struct->resolved_body or add a
bool like p_struct->resolving) to return early if already resolving and
set/clear it around the work, and make struct resolution skip or use
struct-aware signature logic by passing an explicit context flag to
resolve_function_signature/resolve_function_body (or temporarily save/clear
parser->current_class and/or introduce parser->current_struct) so that _init
special-casing does not run for struct methods; also avoid resolving method
bodies for structs until method-calling is implemented (i.e., resolve signatures
only, not call resolve_function_body for struct methods).
In @modules/gdscript/gdscript_parser.cpp:
- Around line 900-1012: parse_struct() parses struct methods with the parser's
current_class still pointing at the enclosing class, so
parse_self()/reduce_self() capture the wrong context and struct methods bind
self to the outer class; to fix, add a parser state current_struct (analogous to
current_class), set current_struct = n_struct while parsing the struct body in
parse_struct() (restore after), update parse_function() invocation in
parse_struct() to use that context, modify parse_self() to prefer current_struct
over current_class when setting self->current_class, and ensure the analyzer's
resolve_struct_body() (and any places that temporarily set
parser->current_class) uses parser->current_struct (or temporarily sets
parser->current_class = current_struct->owner_class/type) when analyzing struct
methods so self resolves to the struct type rather than the enclosing class;
also ensure StructNode::owner_class remains assigned and is used only for
fallback where needed.
- Around line 5433-5439: In the ARRAY case where elem_type.struct_type is
handled, set result.hint_string to elem_type.struct_type->fqsn (not
identifier->name) and add a fallback to "Variant" if struct_type is null or fqsn
is empty; similarly replace short-name usages elsewhere that build hint_string
from identifier->name with fqsn; finally, when constructing a direct STRUCT
PropertyInfo (the code that currently assigns the struct name into class_name),
move that value into hint_string (using fqsn with the same "Variant" fallback)
so editors that read hint_string for typed collections receive the
fully-qualified name.
In @modules/gdscript/gdscript_struct.cpp:
- Around line 94-98: The current check uses arg_value.is_ref_counted() which
only indicates ref-counting, not that the Variant is an Object of the expected
class; remove the is_ref_counted() check and instead require
arg_value.get_type() == Variant::OBJECT for OBJECT parameters, and if
MemberInfo::type_name is set, validate the actual object's class against
info->type_name (e.g., compare the object's class name or use the engine's
class/parent-check utilities) so you accept any Object when type_name is empty
and only allow matching subclasses when type_name is provided.
In @modules/gdscript/gdscript_struct.h:
- Around line 143-180: The header advertises working method calls via
GDScriptStructInstance::call but the implementation currently returns
CALL_ERROR_INVALID_METHOD; either (A) implement GDScriptStructInstance::call to
locate the struct's method (via GDScriptStruct or existing
GDScriptFunction/GDMethod invocation path), prepare arguments and forward the
call with proper Callable::CallError handling, or (B) update the
analyzer/compiler to reject/strip methods on structs so callers cannot reach
call() until implementation exists; additionally, make GDScriptStructInstance
non-copyable/non-movable by deleting copy/move constructors and assignment
operators to match SafeRefCount semantics.
- Around line 54-63: MemberInfo currently leaves
property_info/default_value/has_default_value uninitialized while
add_member(...) only sets index/type/type_name; ensure add_member (in
gdscript_struct.cpp) fully initializes MemberInfo.property_info,
MemberInfo.default_value and MemberInfo.has_default_value (populate PropertyInfo
hint/name/type and set has_default_value accordingly) whenever a member is
registered, or alternatively add clear documentation on MemberInfo which fields
are valid and under what conditions (refer to MemberInfo and add_member symbols
and get_property_list usage to guide callers).
In @modules/gdscript/gdscript.cpp:
- Around line 164-181: The _new method in GDScriptStructClass is unsafely
constructing a STRUCT Variant by directly setting result.type and memcpying the
instance pointer; replace this brittle internal manipulation by using a proper
Variant factory API (e.g., add/ call Variant::from_struct or
Variant::create_struct that accepts a GDScriptStructInstance*) to construct the
STRUCT Variant safely, or if such an API cannot be added immediately, wrap the
memcpy in a well-documented helper function and add safety
assertions/validations (verify sizeof(void*) fits Variant storage, assert
Variant::STRUCT constant correctness, document why this is temporary) and add a
TODO to remove the hack once Variant exposes a safe constructor; target symbols:
GDScriptStructClass::_new, GDScriptStructInstance, Variant (create/from_struct
helper).
🧹 Nitpick comments (9)
core/variant/variant_parser.cpp (1)
41-43: Guarded module include in core is a layering risk.
Even though it’s behindMODULE_GDSCRIPT_ENABLED,core/variant/variant_parser.cppnow directly depends on a modules header. Consider hiding STRUCT serialization behind a core-level interface (e.g., Variant hook/bridge) so core doesn’t include module headers.modules/gdscript/gdscript_parser.cpp (2)
5376-5380: Prefer fully-qualified struct name inDataType::to_string()to avoid ambiguity.
Using onlyidentifier->name(or"<struct>") can collide across scopes/files. IfStructNodehasfqsn, consider returning that when available.
6110-6144: Avoidconst_castin TreePrinter; prefer const-correct printer APIs.
print_struct()iteratesconst FunctionNode*and thenconst_casts to callprint_function. Consider overloadingprint_function(const FunctionNode*)(and other printers) to keep the debug code const-correct.modules/gdscript/gdscript_struct.cpp (3)
100-124: Confusing type conversion logic with redundant assignments.The type conversion logic has confusing control flow:
- Lines 105-107: If conversion succeeds, assigns converted value to
instance->members- Lines 119-124: Another conditional that might assign again, but the else branch (lines 122-123) does nothing
This creates unnecessary complexity and potential for bugs if the logic is modified later.
♻️ Proposed simplification
// Validate argument type against member's declared type bool type_valid = true; +Variant value_to_assign = arg_value; + if (info->type != Variant::NIL && arg_value.get_type() != Variant::NIL) { if (info->type == Variant::OBJECT) { type_valid = (arg_value.get_type() == Variant::OBJECT || arg_value.get_type() == Variant::NIL); } else if (arg_value.get_type() != info->type) { - // Type mismatch - try to convert - Variant converted; if (Variant::can_convert(arg_value.get_type(), info->type)) { - converted = VariantUtilityFunctions::type_convert(arg_value, info->type); - type_valid = (converted.get_type() == info->type); - if (type_valid) { - instance->members.write[info->index] = converted; - } + value_to_assign = VariantUtilityFunctions::type_convert(arg_value, info->type); + type_valid = (value_to_assign.get_type() == info->type); } else { type_valid = false; } } } if (!type_valid) { memdelete(instance); ERR_FAIL_V_MSG(nullptr, vformat("Type mismatch for struct '%s' member '%s': expected %s, got %s.", name, member_name, Variant::get_type_name(info->type), Variant::get_type_name(arg_value.get_type()))); } -// Type is valid (or passed validation), assign the value -if (info->type != Variant::NIL && arg_value.get_type() != Variant::NIL && arg_value.get_type() != info->type) { - // Already converted above, or will use the original value -} else { - instance->members.write[info->index] = arg_value; -} +// Assign the value (either original or converted) +instance->members.write[info->index] = value_to_assign;
132-133: Incomplete constructor support.The TODO comment indicates that actual
_initconstructor functions are not yet called during instance creation. This means custom initialization logic cannot be executed, limiting struct functionality.Do you want me to help outline an implementation approach for calling the
_initconstructor, or would you prefer to track this as a separate issue?
295-302: Struct method calling not implemented.The
callmethod is a stub that always returnsCALL_ERROR_INVALID_METHOD. This limits struct functionality as methods cannot be invoked on instances.Is this planned for a future PR, or should this be tracked as a follow-up task? I can help outline the implementation if needed.
modules/gdscript/gdscript_analyzer.cpp (2)
1578-1591: Recursiveresolve_class_body(..., true)now resolves STRUCT bodies — butresolve_struct_body()must be idempotent.
This change is sensible, but it increases the importance ofresolve_struct_body()having a “resolved body” guard and/or cycle protection, otherwise repeated passes can re-resolve methods and inflate work (and potentially exacerbate leak-like behavior).
1646-1649: STRUCT dispatch looks fine, but it can double-resolve if class-body recursion also covers it.
If STRUCT nodes are reachable both fromresolve_node()and viaresolve_class_body(..., true), ensureresolve_struct_body()is idempotent (otherwise this is a hidden “resolve twice” path).modules/gdscript/gdscript_struct.h (1)
45-141: Delete copy/move ops forGDScriptStructto avoid accidental lifetime/refcount bugs.
As a “lightweight” type with raw pointers (base_struct,owner) and a refcount, implicit copying is almost certainly unintended.Proposed change
class GDScriptStruct { @@ public: + GDScriptStruct(const GDScriptStruct &) = delete; + GDScriptStruct &operator=(const GDScriptStruct &) = delete; + GDScriptStruct(GDScriptStruct &&) = delete; + GDScriptStruct &operator=(GDScriptStruct &&) = delete;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/variant/variant_parser.cppmodules/gdscript/gdscript.cppmodules/gdscript/gdscript_analyzer.cppmodules/gdscript/gdscript_parser.cppmodules/gdscript/gdscript_struct.cppmodules/gdscript/gdscript_struct.h
🧰 Additional context used
🧬 Code graph analysis (1)
modules/gdscript/gdscript_struct.h (2)
modules/gdscript/gdscript.h (4)
GDScript(83-287)Vector(578-593)Variant(377-614)Variant(612-612)modules/gdscript/gdscript_struct.cpp (44)
GDScriptStructInstance(217-247)GDScriptStructInstance(249-251)GDScriptStruct(40-45)GDScriptStruct(47-50)reference(52-54)reference(52-52)reference(253-255)reference(253-253)unreference(56-58)unreference(56-56)unreference(257-264)unreference(257-257)create_instance(69-136)create_instance(69-69)get_member_count(60-67)get_member_count(60-60)add_member(138-149)add_member(138-138)has_member(151-162)has_member(151-151)get_member_index(164-176)get_member_index(164-164)add_method(178-188)add_method(178-178)has_method(190-201)has_method(190-190)is_child_of(203-213)is_child_of(203-203)set(266-274)set(266-266)get(276-284)get(276-276)get_member_ptr(286-293)get_member_ptr(286-286)call(295-302)call(295-295)get_struct_name(304-306)get_struct_name(304-304)get_property_list(308-314)get_property_list(308-308)serialize(316-343)serialize(316-316)deserialize(345-368)deserialize(345-345)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🐧 Linux / Minimal template (target=template_release, everything disabled)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
- GitHub Check: 🤖 Android / Template arm64 (target=template_release, arch=arm64)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
- GitHub Check: 🍏 iOS / Template (target=template_release)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
- GitHub Check: 🍎 macOS / Template (target=template_release)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🤖 Android / Template arm32 (target=template_release, arch=arm32)
- GitHub Check: 🏁 Windows / Editor (target=editor)
🔇 Additional comments (9)
modules/gdscript/gdscript_parser.cpp (3)
4320-4320: Good: STRUCT token is not treated as an expression atom.
KeepingSTRUCTout of expression parse rules prevents confusing/ambiguous parses.
5980-5982: TreePrinter now covers struct members.
Nice to see debug tooling updated alongside the parser changes.
1156-1158: This concern is incorrect. Theparse_class_member(...)function declaration ingdscript_parser.h:1604shows that the 4th parameterp_is_statichas a default value offalse:void parse_class_member(T *(GDScriptParser::*p_parse_function)(bool), AnnotationInfo::TargetKind p_target, const String &p_member_kind, bool p_is_static = false);The 3-argument call at line 1157 is valid and will compile correctly, using the default value. This pattern is also used consistently at lines 1145, 1148, 1154, and 1160 for other class members (constant, signal, class, enum).
Likely an incorrect or invalid review comment.
modules/gdscript/gdscript.cpp (1)
1663-1714: Struct cleanup logic looks correct.The cleanup sequence properly handles reference counting:
- First erases constants holding struct wrappers (line 1667) to drop references
- Then collects structs for deletion (line 1669)
- Uses atomic unreference() check before deletion (lines 1711-1713)
This mirrors the pattern used for functions and scripts cleanup above, ensuring no double-free or leaked memory.
modules/gdscript/gdscript_struct.cpp (3)
138-213: Member and method management looks correct.The implementation properly handles inheritance:
add_memberusesget_member_count()to index members after inherited ones (line 142)has_member,get_member_index, andhas_methodrecursively check the base structis_child_ofcorrectly implements inheritance checkingThe flattened member array approach with indices accounting for inherited members is a sound design choice.
217-247: Inheritance-aware initialization looks correct.The constructor properly handles default values across the inheritance chain:
- Builds chain from derived to base (lines 228-233)
- Iterates in reverse (base to derived) to apply defaults (lines 236-246)
- Ensures derived struct defaults override base defaults
This is the correct approach for flattened member arrays with inheritance.
316-368: Serialization implementation looks correct.Both
serializeanddeserialize:
- Walk the inheritance chain consistently (base to derived)
- Include type information via
__type__key- Handle all members from the entire inheritance hierarchy
deserializegracefully handles missing keys (line 361), allowing for schema evolutionThe implementation properly supports struct persistence.
modules/gdscript/gdscript_analyzer.cpp (2)
2729-2734: Good: STRUCT is explicitly classified as non-expression in the reducer switch.
This helps prevent silent omissions when adding new AST node kinds.
5027-5099: Good: Variant::STRUCT is treated as non-indexable for subscripts.
The additions in the “cannot index at all” lists make sense and prevent nonsensicalstruct_instance[...]patterns from being accepted as typed operations.
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
modules/gdscript/gdscript_parser.h (2)
240-257: Fix missing copy ofDataType::struct_typeinDataType::operator=(will corrupt struct types on copy/assign).
Right nowDataTypecopies losestruct_type, so any assignment/copy of a STRUCT DataType can silently become unusable.Proposed fix
void operator=(const DataType &p_other) { kind = p_other.kind; type_source = p_other.type_source; is_read_only = p_other.is_read_only; is_constant = p_other.is_constant; is_meta_type = p_other.is_meta_type; is_pseudo_type = p_other.is_pseudo_type; is_coroutine = p_other.is_coroutine; builtin_type = p_other.builtin_type; native_type = p_other.native_type; enum_type = p_other.enum_type; script_type = p_other.script_type; script_path = p_other.script_path; class_type = p_other.class_type; + struct_type = p_other.struct_type; method_info = p_other.method_info; enum_values = p_other.enum_values; container_element_types = p_other.container_element_types; }
674-704: STRUCTDataTypeproduced byClassNode::Member::get_datatype()is likely “typeless” (type_source UNDETECTED) and compares equal to everything.
BecauseDataType::operator==treatstype_source == UNDETECTEDas equal-for-parsing, returning a STRUCT DataType with defaulttype_sourcecan leak into analyzer/codegen as “matches anything”.Proposed fix (align with class/typed symbols)
case STRUCT: - if (m_struct) { - DataType dt; - dt.kind = DataType::STRUCT; - dt.struct_type = m_struct; - return dt; - } - return DataType(); + ERR_FAIL_NULL_V(m_struct, DataType()); + DataType dt; + dt.kind = DataType::STRUCT; + dt.type_source = DataType::ANNOTATED_EXPLICIT; + // If structs behave like types when referenced by name (like classes), consider mirroring class behavior: + // dt.is_meta_type = true; + dt.struct_type = m_struct; + return dt;modules/gdscript/gdscript_compiler.cpp (1)
3227-3276: CallGDScriptLanguage::unregister_struct()during old struct cleanup to prevent dangling pointers in the global registry.The cleanup loop erases constants and calls
unreference()/memdelete()on old structs, but omits the correspondingunregister_struct(E.value->get_fully_qualified_name())call. Since structs are registered inglobal_structsby their fully qualified name during compilation, skipping unregistration leaves raw pointers to freed memory in the global registry. Any subsequentget_struct_by_name()query could access freed memory.The correct cleanup pattern is already demonstrated in
gdscript.cpp's GDScript destructor, which unregisters each struct before cleanup. The constant key erasure usingE.keyis correct.modules/gdscript/gdscript_analyzer.cpp (1)
6430-6535: Type-compatibility: ensure STRUCT↔non-STRUCT comparisons fail deterministically (don’t rely on emptysrc_native).In
check_type_compatibility(), whenp_source.kind == STRUCT,src_nativeremains empty (Line 6472-6479). If the target is NATIVE/SCRIPT/CLASS, the laterClassDB::is_parent_class(src_native, ...)checks are at best meaningless and could produce surprising results depending on how empty names are handled.Proposed fix (early return when source is STRUCT but target isn’t)
@@ bool GDScriptAnalyzer::check_type_compatibility(const GDScriptParser::DataType &p_target, const GDScriptParser::DataType &p_source, bool p_allow_implicit_conversion, const GDScriptParser::Node *p_source_node) { @@ switch (p_source.kind) { @@ case GDScriptParser::DataType::STRUCT: if (p_source.is_meta_type) { // Struct meta types are not currently supported. return false; } else { src_struct = p_source.struct_type; } break; @@ } + + // Structs are only compatible with structs (or Variant handled earlier). + if (p_source.kind == GDScriptParser::DataType::STRUCT && p_target.kind != GDScriptParser::DataType::STRUCT) { + return false; + } @@ switch (p_target.kind) {
🤖 Fix all issues with AI agents
In @core/io/json.cpp:
- Around line 850-873: The STRUCT serializer is leaving struct field Variants
un-encoded, causing lossy JSON round-trips; update _serialize_struct (used in
the Variant::STRUCT case) to walk the returned Dictionary and call _from_native
on each field value before packaging into ret[ARGS], and likewise update the
matching STRUCT deserialization code path (the block around lines ~1348-1366) to
call _to_native on each field when unpacking struct_data so non-JSON-native
field types are properly encoded/decoded; keep the existing __type__ handling
and only transform field values, not keys.
- Around line 1348-1366: The Variant::STRUCT deserialization branch currently
calls ScriptServer::create_struct_instance unconditionally; update it to respect
the allow_objects flag by checking allow_objects (same policy used elsewhere)
before calling create_struct_instance and fail fast if allow_objects is false
(use ERR_FAIL_COND_V_MSG or equivalent to return Variant() with a clear message
like "Struct instantiation disabled by allow_objects policy"); keep the rest of
the validation (args size, __type__) intact and only call
ScriptServer::create_struct_instance when allow_objects is true.
In @core/object/script_language.cpp:
- Around line 553-593: The code currently calls virtuals like
ScriptLanguage::create_struct_by_name while holding languages_mutex in
create_struct_instance and global_struct_exists; instead, under languages_mutex
snapshot languages_ready, _language_count and copy non-null _languages pointers
into a local small vector, then release the lock and iterate that snapshot
calling can_create_struct_by_name/create_struct_by_name; keep the initial
languages_ready check inside the lock and use the copied ScriptLanguage*
pointers (from the snapshot) for all subsequent checks/calls so the mutex is not
held during callbacks or long operations.
In @core/variant/variant_parser.cpp:
- Around line 1722-1737: In _parse_dictionary(), remove the code path that
attempts to instantiate structs (the block that checks object.has("__type__")
and calls ScriptServer::create_struct_instance), since it causes side effects,
spams errors, ignores allow_objects and discards the result; simply return the
parsed Dictionary as-is (respecting the allow_objects flag where appropriate)
and do not call ScriptServer::create_struct_instance or perform any runtime type
instantiation when token == TK_CURLY_BRACKET_CLOSE.
In @modules/gdscript/gdscript_analyzer.cpp:
- Around line 1259-1263: The issue is that resolve_class_member() skips
resolving struct bodies, causing get_function_signature() (which requires
FunctionNode::resolved_signature) to miss struct methods; fix by making
get_function_signature() detect when the receiver type is a struct whose
body/signature isn't resolved and invoke the resolver there (call
resolve_struct_body or the existing struct-resolving path used by
resolve_class_member for STRUCT members) before attempting signature lookup,
ensuring FunctionNode::resolved_signature is set for Struct methods and avoiding
order-dependent false negatives; keep the existing name-conflict check
(check_class_member_name_conflict) intact.
- Around line 1593-1645: resolve_struct_body clears parser->current_class which
lets later reduction of a 'self' expression crash; update reduce_self() to guard
against a null parser->current_class and emit a clear compile error (fail-fast)
when 'self' is used outside a class (e.g., in struct context) instead of
dereferencing it, and ensure resolve_struct_body behavior remains (keep setting
parser->current_class = nullptr) so struct contexts continue to be treated as
non-class; refer to the reduce_self() function and resolve_struct_body() /
GDScriptAnalyzer::resolve_struct_body for where to add the null check and error
emission.
In @modules/gdscript/gdscript_compiler.cpp:
- Around line 198-205: The STRUCT branch currently maps
GDScriptParser::DataType::STRUCT to GDScriptDataType::BUILTIN/Variant::STRUCT
and drops the actual struct identity; update GDScriptDataType to carry a struct
reference (e.g. a GDScriptStruct* or fully-qualified name field), populate that
field in gdscript_compiler.cpp’s case for GDScriptParser::DataType::STRUCT using
the existing p_datatype.struct_type (or convert it to the new representation)
instead of only setting builtin_type, and remove the TODO so downstream
validation/member typing can use result.struct_type for overload/member
resolution.
- Around line 3146-3204: The _compile_struct function has refcount and ownership
bugs: explicitly reference() the newly created GDScriptStruct after memnew and
whenever you store it into p_script->structs, GDScriptLanguage::register_struct,
or the struct_wrapper Ref so unreference() calls are balanced (the constructor
actually sets ref_count=0); alter the error path in
GDScriptCompiler::_compile_struct so you do not unreference a zero-ref object
(or ensure you reference() first), and use RAII-style cleanup to always
unregister_struct() from GDScriptLanguage before unreference()/memdelete on
failures. Also change the method compile call to pass struct context (use the
struct node or a struct-specific ClassNode) instead of p_class when calling
_parse_function so "self" semantics are correct, and expand field handling in
_compile_struct to handle non-builtin datatype_specifier cases
(script/native/class/struct) and default values. Finally, in make_scripts ensure
you call GDScriptLanguage::unregister_struct() for old structs before
unreference()/memdelete (mirroring GDScript::clear).
In @modules/gdscript/gdscript_parser.cpp:
- Around line 2928-2932: parse_self() clears SelfNode::current_class for struct
methods which causes TreePrinter::print_self() to dereference
p_self->current_class without a null-check; update TreePrinter::print_self() to
be null-safe and struct-aware: check if p_self->current_class is non-null before
accessing ->identifier, otherwise fall back to p_self->current_struct (or print
a suitable placeholder) and handle formatting accordingly so printing a SelfNode
in a struct method no longer dereferences nullptr.
In @modules/gdscript/gdscript_struct.cpp:
- Around line 70-149: GDScriptStruct::create_instance currently leaves member
slots NIL when fewer args are passed; update it to enforce required members and
initialize defaults: after applying positional args (in the loop over
member_names / arg_idx), iterate remaining members (use
members.getptr(member_name) / MemberInfo) and if MemberInfo->has_default_value
is false and MemberInfo->type != Variant::NIL then memdelete(instance) and
ERR_FAIL_V_MSG(...) indicating missing required argument for that member; if
has_default_value is true, assign the member's stored default (e.g.,
MemberInfo->default_value or an appropriate typed default based on
MemberInfo->type/type_name) into instance->members.write[info->index]; apply
same fix for the other occurrence mentioned (lines ~230-260) so no typed member
remains silently NIL.
- Around line 70-149: The constructor currently only considers this struct's
member_names and uses member_names.size() for expected args, which breaks
positional initialization for inherited members; change create_instance in
GDScriptStruct to use the total slot count and iterate members in index order:
set expected_args = get_member_count(), validate p_argcount against that, build
an ordered array/vector of MemberInfo* sized expected_args by iterating the
struct's members container and placing each info at position info->index, then
loop i from 0 to min(p_argcount, expected_args)-1 using that ordered array to
map each p_args[i] to the correct member (use info->index and members write
access as before) and keep the existing type-validation/conversion logic; this
ensures base→derived positional mapping works for inherited members (also adjust
any early returns/memdelete to reference the new expected_args and ordered
lookup).
In @modules/gdscript/gdscript_struct.h:
- Around line 35-41: Remove the unused include "core/object/object_id.h" from
gdscript_struct.h and instead add an explicit include for
"core/templates/list.h" so the List<PropertyInfo> type is defined; update both
include blocks where object_id.h appears (including the later occurrence around
the PropertyInfo/List usage) to drop object_id.h and add list.h, leaving other
includes and gdscript_function.h unchanged.
In @modules/gdscript/gdscript.cpp:
- Around line 3150-3167: get_struct_by_name() currently returns a raw
GDScriptStruct* while releasing the mutex, which can race with
unregister_struct() and teardown; modify get_struct_by_name() to take a
reference under the lock (call reference()/refcounting method on the found
GDScriptStruct while still holding MutexLock) and return that refcounted handle
(or a raw pointer with the ref held) so the object cannot be destroyed after the
lock is released, and ensure callers call unreference()/release when finished;
apply the same pattern to the other lookup functions in the 3169-3203 region so
all registry lookups increment refcount under the mutex and decrement when the
caller no longer needs the struct.
- Around line 125-162: The set_struct_type method currently unreferences the old
struct without checking if unreference() returned true, which can leak a
GDScriptStruct that reached zero refs; update
GDScriptStructClass::set_struct_type to call struct_type->unreference() and if
it returns true call memdelete(struct_type) before assigning the new p_struct,
and ensure you still set struct_type = p_struct and reference the new struct if
non-null (mirroring the destructor's logic).
- Around line 3169-3191: The new GDScriptStructInstance is created with memnew
but never refcounted; call instance->reference() immediately after
memnew(GDScriptStructInstance(...)) so the instance meets the
"_variant_from_struct_instance() caller holds a reference" contract, and replace
the failure path memdelete(instance) with instance->unreference() (so the
refcounted object is released properly) while leaving the reference intact when
returning the Variant; apply these changes around
GDScriptLanguage::create_struct_instance, touching the memnew site, the
deserialize failure cleanup, and the return path.
In @modules/gdscript/gdscript.h:
- Around line 66-85: The raw pointer member GDScriptStructClass::struct_type is
not explicitly initialized which risks UB; initialize it to nullptr in its
declaration (change "GDScriptStruct *struct_type;" to "GDScriptStruct
*struct_type = nullptr;") and/or ensure all constructors
(GDScriptStructClass::GDScriptStructClass) explicitly set struct_type
consistently and match destructor/unref logic in ~GDScriptStructClass; prefer
the declaration-level nullptr initialization for safety and clarity.
- Around line 628-633: register_struct currently overwrites entries in
global_structs without releasing the previous GDScriptStruct, causing leaks;
modify register_struct to check if an entry already exists for
p_fully_qualified_name, retrieve the existing value and release it appropriately
(call the class's unref/deref or reset the Ref<> wrapper depending on how
GDScriptStruct is stored) before inserting the new p_struct, and likewise ensure
unregister_struct releases and erases the stored struct; use the existing
symbols register_struct, unregister_struct and global_structs to locate and
update the logic so the previous struct pointer is properly unreferenced before
being replaced or removed.
🧹 Nitpick comments (6)
core/variant/variant_parser.cpp (1)
2193-2217: VariantWriter STRUCT-as-dict is fine as a stopgap; verify it won’t be mistaken for user data.
Given the parser can’t round-trip STRUCT yet, emitting a dict with"__type__"is okay, but it increases the chance of collisions with user dictionaries. Consider a dedicated tagged wrapper (e.g.,Struct("FQSN", { ... })) when you implement “tagged variant support”.core/object/script_language.h (1)
107-110: Public API additions look consistent; consider side-effect-free existence check long-term.
global_struct_exists()as “try create with empty Dictionary” can allocate/execute language logic; eventually a dedicatedvirtual bool struct_exists_by_name(...)would avoid surprises.Also applies to: 453-457
modules/gdscript/gdscript_struct.cpp (3)
279-307: Memberset()should validate types (currently bypasses the struct’s declared member types).
GDScriptStruct::create_instance()attempts type validation/conversion, butGDScriptStructInstance::set()(Line 279-287) accepts anyVariant, which can break invariants for typed structs.
308-357: Avoid ERR_FAIL on expected runtime errors incall()(prefer CallError-only).For non-static methods,
call()setsCALL_ERROR_INVALID_METHODand also triggersERR_FAIL_V_MSG(Line 339-344). That’s noisy for a normal runtime error path.Proposed fix (return cleanly with CallError)
@@ if (!method_info->is_static) { r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD; r_error.argument = 0; r_error.expected = 0; - ERR_FAIL_V_MSG(Variant(), vformat("Non-static struct methods are not yet supported. Cannot call instance method '%s' on struct '%s'.", p_method, struct_type->get_name())); + return Variant(); }
363-369: Include inherited members in property list + ensure serialization key safety / stable ordering.
get_property_list()only listsstruct_type->get_members()(Line 363-369), so base members are omitted.serialize()writes__type__plus members (Line 371-398); a member named__type__would collide.- Iterating
HashMapfor members means ordering isn’t deterministic.Also applies to: 371-398, 400-423
modules/gdscript/gdscript_parser.cpp (1)
1169-1171:static structhandling is inconsistent / likely unreachable.
parse_class_body()doesn’t passnext_is_staticintoparse_struct()(Line 1169-1171), soparse_struct(bool p_is_static)will almost always seefalse. Either wirenext_is_staticthrough (and decide desired syntax), or treatp_is_staticas a non-semantic parser hook and avoid relying on it for validation.
-->
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
core/io/json.cppcore/object/script_language.cppcore/object/script_language.hcore/variant/variant_parser.cppmodules/gdscript/gdscript.cppmodules/gdscript/gdscript.hmodules/gdscript/gdscript_analyzer.cppmodules/gdscript/gdscript_compiler.cppmodules/gdscript/gdscript_parser.cppmodules/gdscript/gdscript_parser.hmodules/gdscript/gdscript_struct.cppmodules/gdscript/gdscript_struct.h
🧰 Additional context used
🧬 Code graph analysis (11)
core/variant/variant_parser.cpp (2)
core/object/script_language.cpp (2)
create_struct_instance(555-573)create_struct_instance(555-555)modules/gdscript/gdscript.cpp (2)
create_struct_instance(3169-3191)create_struct_instance(3169-3169)
core/io/json.cpp (2)
modules/gdscript/gdscript.cpp (4)
gdscript_struct_instance_serialize(3359-3363)gdscript_struct_instance_serialize(3359-3359)create_struct_instance(3169-3191)create_struct_instance(3169-3169)core/object/script_language.cpp (2)
create_struct_instance(555-573)create_struct_instance(555-555)
modules/gdscript/gdscript_parser.h (2)
modules/gdscript/gdscript_struct.cpp (4)
add_method(191-201)add_method(191-191)has_method(203-214)has_method(203-203)modules/gdscript/gdscript_parser.cpp (4)
parse_struct(900-1025)parse_struct(900-900)print_struct(6130-6164)print_struct(6130-6130)
core/object/script_language.h (2)
core/object/script_language.cpp (4)
create_struct_instance(555-573)create_struct_instance(555-555)global_struct_exists(575-593)global_struct_exists(575-575)modules/gdscript/gdscript.cpp (7)
create_struct_instance(3169-3191)create_struct_instance(3169-3169)bool(384-384)can_create_struct_by_name(3195-3197)can_create_struct_by_name(3195-3195)create_struct_by_name(3199-3203)create_struct_by_name(3199-3199)
core/object/script_language.cpp (1)
modules/gdscript/gdscript.cpp (2)
create_struct_instance(3169-3191)create_struct_instance(3169-3169)
modules/gdscript/gdscript_compiler.cpp (3)
modules/gdscript/gdscript.h (1)
GDScriptStruct(79-79)modules/gdscript/gdscript_struct.cpp (2)
GDScriptStruct(41-46)GDScriptStruct(48-51)modules/gdscript/gdscript_struct.h (1)
GDScriptStruct(47-113)
modules/gdscript/gdscript_parser.cpp (2)
modules/gdscript/gdscript.cpp (2)
canonicalize_path(1593-1598)canonicalize_path(1593-1593)modules/gdscript/gdscript_parser.h (1)
is_at_end(1593-1694)
modules/gdscript/gdscript.cpp (5)
core/io/json.cpp (2)
_bind_methods(633-648)_bind_methods(633-633)modules/gdscript/gdscript.h (3)
GDScriptStructClass(66-85)Variant(381-621)Variant(619-619)core/variant/variant_call.cpp (18)
p_instance(685-697)p_instance(685-685)p_instance(699-714)p_instance(699-699)p_instance(716-731)p_instance(716-716)p_instance(733-748)p_instance(733-733)p_instance(750-757)p_instance(750-750)p_instance(759-766)p_instance(759-759)p_instance(768-775)p_instance(768-768)p_instance(777-788)p_instance(777-777)callp(1359-1394)callp(1359-1359)modules/gdscript/gdscript_struct.h (1)
GDScriptStructInstance(145-186)core/variant/variant_internal.h (3)
Dictionary(189-189)Dictionary(190-190)Dictionary(1162-1164)
modules/gdscript/gdscript_struct.cpp (3)
modules/gdscript/gdscript_struct.h (2)
GDScriptStruct(47-113)GDScriptStructInstance(145-186)core/variant/variant.cpp (20)
reference(1146-1330)reference(1146-1146)can_convert(195-536)can_convert(195-195)get_type_name(46-181)get_type_name(46-46)Variant(2335-2338)Variant(2340-2343)Variant(2345-2348)Variant(2350-2353)Variant(2355-2358)Variant(2360-2363)Variant(2365-2368)Variant(2370-2373)Variant(2375-2378)Variant(2380-2383)Variant(2385-2388)Variant(2390-2393)Variant(2395-2399)Variant(2401-2405)core/variant/variant_utility.cpp (2)
type_convert(868-955)type_convert(868-868)
modules/gdscript/gdscript.h (6)
modules/gdscript/gdscript_struct.cpp (4)
GDScriptStruct(41-46)GDScriptStruct(48-51)GDScriptStructInstance(230-260)GDScriptStructInstance(262-264)modules/gdscript/gdscript_struct.h (2)
GDScriptStruct(47-113)GDScriptStructInstance(145-186)modules/gdscript/gdscript.cpp (32)
GDScriptStructClass(131-136)GDScriptStructClass(138-147)_variant_from_struct_instance(178-193)_variant_from_struct_instance(178-178)set_struct_type(149-162)set_struct_type(149-149)_new(94-104)_new(94-94)_new(195-209)_new(195-195)_new(306-349)_new(306-306)callp(110-123)callp(110-110)callp(211-218)callp(211-211)callp(1038-1055)callp(1038-1038)callp(2171-2189)callp(2171-2171)register_struct(3150-3153)register_struct(3150-3150)unregister_struct(3155-3158)unregister_struct(3155-3155)get_struct_by_name(3160-3167)get_struct_by_name(3160-3160)create_struct_instance(3169-3191)create_struct_instance(3169-3169)can_create_struct_by_name(3195-3197)can_create_struct_by_name(3195-3195)create_struct_by_name(3199-3203)create_struct_by_name(3199-3199)core/object/script_language.cpp (4)
callp(823-834)callp(823-823)create_struct_instance(555-573)create_struct_instance(555-555)core/variant/variant_internal.h (9)
Callable(185-185)Callable(186-186)Callable(1152-1154)StringName(179-179)StringName(180-180)StringName(1142-1144)String(141-141)String(142-142)String(1112-1114)core/extension/gdextension_interface.cpp (1)
StringName(141-143)
modules/gdscript/gdscript_struct.h (2)
modules/gdscript/gdscript.h (3)
GDScript(87-291)GDScriptStruct(79-79)GDScriptStructClass(66-85)modules/gdscript/gdscript_struct.cpp (42)
GDScriptStructInstance(230-260)GDScriptStructInstance(262-264)GDScriptStruct(41-46)GDScriptStruct(48-51)reference(53-55)reference(53-53)reference(266-268)reference(266-266)unreference(57-59)unreference(57-57)unreference(270-277)unreference(270-270)create_instance(70-149)create_instance(70-70)get_member_count(61-68)get_member_count(61-61)add_member(151-162)add_member(151-151)has_member(164-175)has_member(164-164)get_member_index(177-189)get_member_index(177-177)add_method(191-201)add_method(191-191)has_method(203-214)has_method(203-203)set(279-287)set(279-279)get(289-297)get(289-289)get_member_ptr(299-306)get_member_ptr(299-299)call(308-357)call(308-308)get_struct_name(359-361)get_struct_name(359-359)get_property_list(363-369)get_property_list(363-363)serialize(371-398)serialize(371-371)deserialize(400-423)deserialize(400-400)
🪛 Clang (14.0.6)
modules/gdscript/gdscript_struct.h
[error] 35-35: 'core/object/object_id.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🐧 Linux / Minimal template (target=template_release, everything disabled)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🤖 Android / Template arm64 (target=template_release, arch=arm64)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🍎 macOS / Template (target=template_release)
- GitHub Check: 🤖 Android / Template arm32 (target=template_release, arch=arm32)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
- GitHub Check: 🏁 Windows / Editor (target=editor)
- GitHub Check: 🍏 iOS / Template (target=template_release)
🔇 Additional comments (18)
modules/gdscript/gdscript_parser.h (2)
834-892: StructNode container/map duplication: verify invariants for inheritance and lookups.
StructNode::has_method()/has_field()only check local maps; if analyzer expects parser nodes to answer inherited members too, this will under-report (while runtimeGDScriptStruct::has_method()recurses to base). If inheritance is resolved later (and these helpers are only for “declared here”), this is fine.
1141-1144: Current struct context plumbing looks consistent; verifySelfNoderesolution prioritization.
Having bothSelfNode::current_classandSelfNode::current_structis good, but make sure downstream code (analyzer/compiler) doesn’t assumecurrent_class != nullptrimplies “self is class instance”.Also applies to: 1462-1464, 1603-1605, 1764-1767
modules/gdscript/gdscript_compiler.cpp (2)
38-39: Good: required include for struct compilation path.
3009-3043: Struct compilation is properly guarded against name collisions.The parser validates struct name uniqueness through
members_indiceschecking inparse_class_member()before compilation occurs. A struct name cannot collide with existing functions, variables, constants, classes, enums, or signals in the same class—attempting to do so triggers a parse error. The compiler's safe assignment toconstants[]relies on this prior validation.modules/gdscript/gdscript.h (6)
37-40: Forward decls are appropriate to keep compile dependencies down.
122-124: Friend additions look reasonable for tight integration.
509-514: The thread-safety concern is already properly addressed. All accesses toglobal_structsare guarded withMutexLock lock(mutex):register_struct(),unregister_struct(), andget_struct_by_name()all protect their operations. No unguarded direct accesses exist, and serialization code does not bypass these guarded methods.Likely an incorrect or invalid review comment.
711-714: No action required—implementation correctly handles failure modes.The
create_struct_by_name()implementation properly ensures that:
- When the struct is not found, it returns NIL (via
ERR_FAIL_V_MSG(Variant(), ...)) without creating any instance.- If deserialization fails, it cleans up the partially allocated instance with
memdelete()and returns NIL.- Error propagation matches the
ScriptLanguagecontract (returningVariant()for error cases).
102-110: The struct tracking implementation correctly prevents leaks and dangling pointers.The
ClearData::structsset is actively used inGDScript::clear(): constants holding struct references are erased first (decrementing refcount), structs are unregistered from the global registry, and the root invocation performs atomic reference-counted deletion (if (E->unreference()) { memdelete(E); }). This ensures no dangling pointers or memory leaks during both normal clearing and hot-reload cycles.
142-143: No action needed. The ownership model, reference counting lifecycle, and cleanup semantics are already properly implemented.GDScript::clear()explicitly unreferences all structs via deferred cleanup inClearData, unregisters them fromGDScriptLanguage::global_structs, and the destructor invokesclear(). The pattern follows: compiler creates struct (initial reference), script owns viaHashMap<StringName, GDScriptStruct *>, and cleanup unreferences atomically usingSafeRefCount.modules/gdscript/gdscript_analyzer.cpp (1)
1900-2052: Good guard: constructor / polymorphism logic now avoids struct contexts.Treating struct
_initas a normal method (Line 1902-1913) and gating override-signature checks behindparser->current_class != nullptr(Line 1952-2052) prevents accidental constructor/override rules from firing on structs.modules/gdscript/gdscript.cpp (3)
1691-1702: Struct teardown ordering looks reasonable; verify no double-unreference via constant wrapper.
You eraseconstantsfirst (droppingRef<GDScriptStructClass>), then later unreference/delete theGDScriptStruct*in the root clear. This assumes the script itself owns a separate reference to each struct (so two decrements are expected). Please confirm the struct creation path incrementsGDScriptStruct::ref_countfor script ownership.Also applies to: 1739-1745
3357-3373:extern "C"JSON helpers: OK, but ensure they’re declared somewhere shared.
Implementation looks fine; just verify the declarations used bycore/io/json.cppmatch signatures/constness to avoid ODR/link issues across compilers.
178-193: The Variant class properly handlesVariant::STRUCTreference counting—this concern is not applicable.Verification confirms that
Varianthas explicitreference()/unreference()handling forSTRUCTtype: the copy constructor (variant.cpp:1318–1321) callsstruct_instance->reference(), and the destructor (variant.cpp:1496–1505) callsstruct_instance->unreference()with proper cleanup. The code in_variant_from_struct_instance()works safely becauseVariant's copy/dtor paths are explicitly designed to manage the reference count of stored struct instances.Likely an incorrect or invalid review comment.
modules/gdscript/gdscript_struct.h (1)
143-186:GDScriptStructInstancelifetime: ensure refcount initial value and delete path are consistent with Variant integration.
Given the engine-wide plan to store instances insideVariant::STRUCT, the instance refcount must have a clear “first owner” rule (constructor starts at 0 vs 1) and a single canonical delete path (likelyunreference()). Please confirm this is consistent with theVariantSTRUCT destructor and the two construction sites (_newand JSONcreate_struct_instance).modules/gdscript/gdscript_parser.cpp (3)
4336-4336: STRUCT parse rule entry looks consistent with “not an expression token.”
No prefix/infix parse functions forSTRUCTis correct given it’s a declaration-level construct.
-->
5392-5397: Property/type string wiring for STRUCT looks reasonable; confirmhint_stringconventions forVariant::STRUCT.
DataType::to_string()returningfqsnis helpful for diagnostics (Line 5392-5397).to_property_info()setsresult.type = Variant::STRUCTand useshint_stringto carry thefqsn(Line 5581-5589), and mirrors that in typed Array/Dictionary hint strings (Line 5449-5457, 5491-5498, 5526-5533).Make sure the editor/inspector and serialization paths interpret
PropertyInfo.hint_stringforVariant::STRUCT(and container hints referencing structs) the same way as your other STRUCT plumbing expects.
-->Also applies to: 5449-5457, 5491-5498, 5526-5533, 5581-5589
6000-6002: TreePrinter struct integration is good; keep output consistent with other node kinds.
print_struct()nicely mirrors class printing (identifier + extends + members) and wiring it intoprint_class()makes debug dumps much more usable (Line 6000-6002, 6130-6164). Only follow-up is makingprint_self()struct-aware (see earlier comment).
-->Also applies to: 6130-6164
| // Prefer current_struct over current_class for struct methods | ||
| // This ensures self binds to the struct type, not the enclosing class | ||
| self->current_struct = current_struct; | ||
| self->current_class = current_struct ? nullptr : current_class; | ||
| return self; |
There was a problem hiding this comment.
Null-deref risk: parse_self() sets SelfNode::current_class = nullptr for struct methods.
TreePrinter::print_self() later does p_self->current_class->identifier without a null-check, so printing a tree containing self inside a struct method can crash in debug builds. Also, other analysis/codegen paths may have similar assumptions.
Proposed fix (make TreePrinter null-safe + struct-aware)
diff --git a/modules/gdscript/gdscript_parser.cpp b/modules/gdscript/gdscript_parser.cpp
@@ void GDScriptParser::TreePrinter::print_self(SelfNode *p_self) {
push_text("Self(");
- if (p_self->current_class->identifier != nullptr) {
- print_identifier(p_self->current_class->identifier);
- } else {
- push_text("<main class>");
- }
+ if (p_self->current_struct != nullptr) {
+ if (p_self->current_struct->identifier != nullptr) {
+ print_identifier(p_self->current_struct->identifier);
+ } else {
+ push_text("<unnamed struct>");
+ }
+ } else if (p_self->current_class != nullptr) {
+ if (p_self->current_class->identifier != nullptr) {
+ print_identifier(p_self->current_class->identifier);
+ } else {
+ push_text("<main class>");
+ }
+ } else {
+ push_text("<unknown>");
+ }
push_text(")");
}-->
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @modules/gdscript/gdscript_parser.cpp around lines 2928 - 2932, parse_self()
clears SelfNode::current_class for struct methods which causes
TreePrinter::print_self() to dereference p_self->current_class without a
null-check; update TreePrinter::print_self() to be null-safe and struct-aware:
check if p_self->current_class is non-null before accessing ->identifier,
otherwise fall back to p_self->current_struct (or print a suitable placeholder)
and handle formatting accordingly so printing a SelfNode in a struct method no
longer dereferences nullptr.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/gdscript/gdscript_compiler.cpp (1)
3299-3363: Fix double-unreference bug in struct cleanup and defer irreversible operations until after successful compilation.The cleanup sequence in
make_scripts()has two critical issues:
Double-unreference bug: Step 2 calls
unregister_struct(), which already callsunreference()and conditionallymemdelete(). Step 3 then callsunreference()again on the same pointer, causing a use-after-free if the struct was deleted in Step 2.Compilation failure leaves broken state:
make_scripts()is called at line 3505, but_prepare_compilation()(3507) and_compile_class()(3512) can fail. If either fails, old structs have already been unregistered/deleted, leaving the system unable to roll back to a working state. Deferred structs that reference the old types will then fail to resolve.Remove the explicit
unreference()and delete logic in Step 3, sinceunregister_struct()already handles both. Better yet, defer all struct cleanup until afterfinish_compiling()succeeds (line 3529), so failures don't corrupt the registry.
🤖 Fix all issues with AI agents
In @core/io/json.cpp:
- Around line 1361-1392: The code assumes args[0] is a Dictionary when handling
Variant::STRUCT; add an explicit type check before assigning args[0] to
Dictionary encoded_data (e.g. ERR_FAIL_COND_V_MSG(args[0].get_type() !=
Variant::DICTIONARY, Variant(), "Invalid struct data: expected dictionary
argument.");) then only create Dictionary encoded_data = args[0] and continue
with the existing checks/decoding that use _to_native and
ScriptServer::create_struct_instance; reference symbols: Variant::STRUCT,
LOAD_ARGS(), args, encoded_data, _to_native,
ScriptServer::create_struct_instance.
- Around line 38-60: Guard the GDScript-specific extern "C" declarations and
helper with MODULE_GDSCRIPT_ENABLED: wrap the declarations for
gdscript_struct_instance_serialize and gdscript_struct_instance_get_type_name
and the _serialize_struct implementation in #ifdef MODULE_GDSCRIPT_ENABLED /
#else blocks and provide a fallback _serialize_struct stub that returns an empty
Dictionary and calls ERR_FAIL_V_MSG with a clear "STRUCT serialization requires
MODULE_GDSCRIPT_ENABLED" message. Likewise, wrap the STRUCT variant handling in
the serialization/deserialization paths (_from_native and _to_native) so
references to _serialize_struct and ScriptServer::create_struct_instance are
only compiled when MODULE_GDSCRIPT_ENABLED is defined, and provide alternative
branches that fail gracefully or return errors when the module is disabled.
In @core/variant/variant_parser.cpp:
- Around line 2177-2202: The current Variant::STRUCT text serialization (in the
VariantParser write branch using struct_instance->serialize() and
VariantWriter::write_to_string()) loses type fidelity by emitting a Dictionary
that cannot be parsed back as a STRUCT; update the textual format and parser:
add an explicit textual constructor form such as Struct("fqsn", {...}) and
modify VariantParser::parse_value() to recognize that constructor and
reconstruct a GDScriptStructInstance (or, alternatively, make write() emit a
clear failing/tagged placeholder and have parse_value() return an error for
STRUCTs); ensure ResourceFormatSaverTextInstance and ResourceFormatLoaderText
paths that use write_to_string()/parse_value() either round-trip via the new
Struct(...) syntax or fail loudly instead of silently returning a Dictionary.
In @modules/gdscript/gdscript_compiler.cpp:
- Around line 3146-3277: In GDScriptCompiler::_compile_struct, the initial
gdstruct->reference() (after memnew) is only unreferenced on the error path,
leaking one reference on success; add gdstruct->unreference() just before the
final return OK to match the error cleanup; also avoid mutating gdstruct via
const_cast on gdstruct->get_members(): instead add a GDScriptStruct method such
as set_member_default_value(name, value) or extend add_member(...) to accept an
optional default so you can set member_info->default_value without casting away
const.
In @modules/gdscript/gdscript.cpp:
- Around line 149-162: The set_struct_type method currently calls
struct_type->unreference() without checking its return value; update
GDScriptStructClass::set_struct_type to mirror the destructor's pattern by
checking the boolean result of unreference() and calling memdelete(struct_type)
when it returns true, and ensure this logic is applied before assigning the new
p_struct to struct_type and before calling reference() on the new struct.
- Around line 3244-3251: get_struct_by_name returns a raw GDScriptStruct* while
only holding the mutex, risking a use-after-free if unregister_struct deletes it
after the lock is released; modify GDScriptLanguage::get_struct_by_name to
increment the struct's refcount (call reference()) while still under the
MutexLock and then return the pointer, and update callers (e.g.,
create_struct_instance) to call unreference() when finished using the returned
GDScriptStruct* so ownership is correctly balanced.
🧹 Nitpick comments (4)
core/io/json.cpp (1)
850-886: STRUCT serialization hardcodes GDScript in core/io, creating an architectural layering violation.While
Variant::STRUCTis currently GDScript-only (only GDScript'scan_create_struct_by_name()returns true), the code at lines 850–886 makes an unsafe assumption by directly castingVariantInternal::get_struct()toGDScriptStructInstance*. If another language ever producesVariant::STRUCT, this cast will break silently or crash.Additionally, the code hard-fails when
__type__is missing, despite already havinggdscript_struct_instance_get_type_name()available as a fallback.Recommended fixes:
- Delegate struct serialization to
ScriptServeror through aScriptLanguageinterface rather than pinning to GDScript in core/io- If keeping this approach: use
gdscript_struct_instance_get_type_name()as fallback instead of hard-failingmodules/gdscript/gdscript_compiler.cpp (1)
198-204: UseGDScriptDataType::STRUCTkind instead of mapping structs toBUILTINtype.The code correctly identifies that
struct_typefield is available (already inGDScriptDataType), yet intentionally discards struct identity by mapping toVariant::STRUCTwithBUILTINkind. This loses actual struct type information needed for validated calls and editor hints.The
GDScriptDataType::STRUCTkind andstruct_typefield infrastructure are already implemented and in use elsewhere in the codebase (e.g., line 3190–3192 in the same file). Simply uncommenting and populatingresult.struct_type = p_datatype.struct_typewhile settingresult.kind = GDScriptDataType::STRUCTwould preserve struct identity and enable proper type checking via the existingis_type()method.modules/gdscript/gdscript.cpp (1)
164-241: Track TODO for Variant API improvement.The
_variant_from_struct_instancefunction directly manipulates Variant internals usingmemcpy, which is fragile and could break if Variant's internal layout changes. The extensive TODO comment (lines 165-197) proposes proper API additions.Consider creating a tracking issue for the recommended API improvements:
static Variant Variant::from_struct(GDScriptStructInstance *p_instance)void Variant::set_struct(GDScriptStructInstance *p_instance)- Friend class declaration
The current implementation has comprehensive safety checks, but a proper Variant API would be more maintainable and less error-prone.
Do you want me to create a tracking issue for this API improvement?
modules/gdscript/gdscript_struct.h (1)
96-98: Remove unusedchildrenvector.The comment states this vector is "currently unused and not populated." Dead code adds memory overhead to every
GDScriptStructinstance and can confuse future maintainers.If child tracking is needed in the future, it can be added then. For now, consider removing it.
♻️ Remove unused field
- // Inheritance - // NOTE: The children vector is currently unused and not populated. - // Child struct tracking is handled through the owning GDScript's structs HashMap. - Vector<GDScriptStruct *> children; - // Fully qualified name for unique identification String fully_qualified_name;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/io/json.cppcore/variant/variant_parser.cppmodules/gdscript/gdscript.cppmodules/gdscript/gdscript_compiler.cppmodules/gdscript/gdscript_struct.h
🧰 Additional context used
🧬 Code graph analysis (4)
core/io/json.cpp (2)
modules/gdscript/gdscript.cpp (4)
gdscript_struct_instance_serialize(3457-3461)gdscript_struct_instance_serialize(3457-3457)create_struct_instance(3253-3289)create_struct_instance(3253-3253)core/object/script_language.cpp (2)
create_struct_instance(555-573)create_struct_instance(555-555)
modules/gdscript/gdscript.cpp (3)
modules/gdscript/gdscript.h (3)
GDScriptStructClass(66-85)Variant(381-621)Variant(619-619)modules/gdscript/gdscript_struct.h (1)
GDScriptStructInstance(145-186)modules/gdscript/gdscript_struct.cpp (2)
GDScriptStructInstance(230-260)GDScriptStructInstance(262-264)
modules/gdscript/gdscript_compiler.cpp (3)
modules/gdscript/gdscript_struct.h (1)
GDScriptStruct(47-113)modules/gdscript/gdscript.h (1)
GDScriptStruct(79-79)modules/gdscript/gdscript_struct.cpp (2)
GDScriptStruct(41-46)GDScriptStruct(48-51)
core/variant/variant_parser.cpp (1)
core/variant/variant.cpp (8)
p_variant(870-872)p_variant(870-870)p_variant(874-883)p_variant(874-874)p_variant(885-893)p_variant(885-885)p_variant(2680-2821)p_variant(2680-2680)
🪛 Clang (14.0.6)
modules/gdscript/gdscript_struct.h
[error] 35-35: 'core/templates/hash_map.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🤖 Android / Template arm32 (target=template_release, arch=arm32)
- GitHub Check: 🐧 Linux / Template w/ Mono, release (target=template_release)
- GitHub Check: 🏁 Windows / Template w/ GCC (target=template_release, use_mingw=yes)
- GitHub Check: 🐧 Linux / Editor with ThreadSanitizer (target=editor, dev_build=yes, use_tsan=yes, use_llvm=yes, linker=lld)
- GitHub Check: 🐧 Linux / Minimal template (target=template_release, everything disabled)
- GitHub Check: 🐧 Linux / Editor w/ Mono (target=editor)
- GitHub Check: 🏁 Windows / Template (target=template_release)
- GitHub Check: 🏁 Windows / Editor (target=editor)
- GitHub Check: 🐧 Linux / Template w/ Mono, debug (target=template_debug)
- GitHub Check: 🌐 Web / Template w/o threads (target=template_release, threads=no)
- GitHub Check: 🍎 macOS / Editor (target=editor)
- GitHub Check: 🍎 macOS / Template (target=template_release)
- GitHub Check: 🌐 Web / Template w/ threads (target=template_release, threads=yes)
- GitHub Check: 🤖 Android / Template arm64 (target=template_release, arch=arm64)
- GitHub Check: 🍏 iOS / Template (target=template_release)
🔇 Additional comments (7)
core/variant/variant_parser.cpp (1)
41-43: Conditional include for structs is fine.modules/gdscript/gdscript_compiler.cpp (2)
38-39: Include addition is OK.
3009-3043: Struct compilation hook in _compile_class looks reasonable.modules/gdscript/gdscript.cpp (3)
125-136: LGTM! Proper reference counting in constructor.The constructor correctly increments the reference count when taking ownership of a struct, and the comment clearly documents that
nullptris allowed.
3253-3289: LGTM! Proper reference counting increate_struct_instance.The reference counting flow is correct:
- Instance created with
ref_count = 0(line 3261)reference()increments to 1 before creating Variant (line 3279)- Variant holds the reference for its lifetime
- Deserialization failure cleanup uses
unreference()return value correctly (lines 3268-3271)The extensive comments (lines 3275-3284) clearly document the ownership transfer contract.
1754-1764: The code is correct. Struct wrapper constants are always added to the constants HashMap with the struct name as the key during compilation (gdscript_compiler.cpp, line 3263:p_script->constants[p_struct->identifier->name] = struct_wrapper;). This matches the key used in the structs HashMap (line 3249), so the erase operation in the cleanup code (line 1758) will always succeed. There is no risk of missing constants or name conflicts.Likely an incorrect or invalid review comment.
modules/gdscript/gdscript_struct.h (1)
145-186: LGTM! Well-designed struct instance class.The deleted copy/move constructors and operators (lines 154-157) are correct for
SafeRefCountsemantics, preventing accidental copies that would break reference counting. The API surface is clean and appropriate for a lightweight, reference-counted struct instance.
| case Variant::STRUCT: { | ||
| // Deserialize struct from tagged dictionary | ||
| LOAD_ARGS(); | ||
|
|
||
| ERR_FAIL_COND_V_MSG(args.size() != 1, Variant(), "Invalid struct data: expected single dictionary argument."); | ||
|
|
||
| Dictionary encoded_data = args[0]; | ||
| ERR_FAIL_COND_V_MSG(!encoded_data.has("__type__"), Variant(), "Invalid struct data: missing __type__ field."); | ||
|
|
||
| String type_name = encoded_data["__type__"]; | ||
| ERR_FAIL_COND_V_MSG(type_name.is_empty(), Variant(), "Invalid struct data: empty __type__ field."); | ||
|
|
||
| // Decode field values using _to_native to properly reconstruct non-JSON-native types | ||
| // This handles nested structs, objects, typed arrays, etc. | ||
| Dictionary decoded_struct_data; | ||
| decoded_struct_data["__type__"] = encoded_data["__type__"]; // Copy type identifier as-is | ||
|
|
||
| for (const KeyValue<Variant, Variant> &kv : encoded_data) { | ||
| if (kv.key == "__type__") { | ||
| continue; // Already copied above | ||
| } | ||
| // Decode the value using _to_native, keys are strings (field names) and don't need decoding | ||
| decoded_struct_data[kv.key] = _to_native(kv.value, p_allow_objects, p_depth + 1); | ||
| } | ||
|
|
||
| // Use ScriptServer to create the struct instance with decoded data | ||
| Variant struct_instance = ScriptServer::create_struct_instance(type_name, decoded_struct_data); | ||
| ERR_FAIL_COND_V_MSG(struct_instance.get_type() != Variant::STRUCT, Variant(), vformat("Failed to create struct instance for type '%s'.", type_name)); | ||
|
|
||
| return struct_instance; | ||
| } break; | ||
|
|
There was a problem hiding this comment.
Harden decoding: validate args[0] is a Dictionary before using it.
Right now Dictionary encoded_data = args[0]; will rely on Variant conversion rules and can quietly yield an empty dictionary for malformed inputs. This makes error reporting less precise.
Targeted hardening
LOAD_ARGS();
ERR_FAIL_COND_V_MSG(args.size() != 1, Variant(), "Invalid struct data: expected single dictionary argument.");
- Dictionary encoded_data = args[0];
+ ERR_FAIL_COND_V_MSG(((Variant)args[0]).get_type() != Variant::DICTIONARY, Variant(), "Invalid struct data: expected Dictionary argument.");
+ Dictionary encoded_data = args[0];
ERR_FAIL_COND_V_MSG(!encoded_data.has("__type__"), Variant(), "Invalid struct data: missing __type__ field.");🤖 Prompt for AI Agents
In @core/io/json.cpp around lines 1361 - 1392, The code assumes args[0] is a
Dictionary when handling Variant::STRUCT; add an explicit type check before
assigning args[0] to Dictionary encoded_data (e.g.
ERR_FAIL_COND_V_MSG(args[0].get_type() != Variant::DICTIONARY, Variant(),
"Invalid struct data: expected dictionary argument.");) then only create
Dictionary encoded_data = args[0] and continue with the existing checks/decoding
that use _to_native and ScriptServer::create_struct_instance; reference symbols:
Variant::STRUCT, LOAD_ARGS(), args, encoded_data, _to_native,
ScriptServer::create_struct_instance.
| // Misc types. | ||
| #ifdef MODULE_GDSCRIPT_ENABLED | ||
| case Variant::STRUCT: { | ||
| // Serialize struct as a Dictionary with __type__ metadata | ||
| // JSON format supports round-trip, binary format does not yet | ||
| const GDScriptStructInstance *struct_instance = VariantGetInternalPtr<GDScriptStructInstance>::get_ptr(&p_variant); | ||
| if (struct_instance) { | ||
| if (unlikely(p_recursion_count > MAX_RECURSION)) { | ||
| ERR_PRINT("Max recursion reached"); | ||
| p_store_string_func(p_store_string_ud, "null"); | ||
| } else { | ||
| p_recursion_count++; | ||
| Dictionary dict = struct_instance->serialize(); | ||
| // Recursively write the dictionary with incremented recursion count | ||
| write(dict, p_store_string_func, p_store_string_ud, p_encode_res_func, p_encode_res_ud, p_recursion_count, p_compat, p_full_objects); | ||
| } | ||
| } else { | ||
| p_store_string_func(p_store_string_ud, "null"); | ||
| } | ||
| } break; | ||
| #else | ||
| case Variant::STRUCT: { | ||
| // Structs not available without GDScript module | ||
| p_store_string_func(p_store_string_ud, "null"); | ||
| } break; | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether VariantWriter/VariantParser are used for text scene/resource serialization paths.
rg -n --hidden --no-ignore-vcs -S "VariantWriter::write" . | head -50Repository: Redot-Engine/redot-engine
Length of output: 4052
🏁 Script executed:
rg -n --hidden --no-ignore-vcs -S "VariantParser::parse" . | head -50Repository: Redot-Engine/redot-engine
Length of output: 6431
🏁 Script executed:
rg -n --hidden --no-ignore-vcs -S "ResourceFormat.*Text" core scene modules || trueRepository: Redot-Engine/redot-engine
Length of output: 8133
🏁 Script executed:
rg -n "parse_value" core/variant/variant_parser.cpp | head -20Repository: Redot-Engine/redot-engine
Length of output: 1211
🏁 Script executed:
# Check if there's STRUCT parsing support in VariantParser
rg -n -A 10 "case Variant::STRUCT" core/variant/variant_parser.cppRepository: Redot-Engine/redot-engine
Length of output: 1170
🏁 Script executed:
# Search for any Struct constructor syntax or parsing
rg -n "Struct\(" core/variant/variant_parser.cppRepository: Redot-Engine/redot-engine
Length of output: 51
🏁 Script executed:
# Check if there's any STRUCT reconstruction or special handling
rg -n "create_struct_instance\|GDScriptStructInstance" core/variant/variant_parser.cppRepository: Redot-Engine/redot-engine
Length of output: 51
🏁 Script executed:
# Look at parse_value function to see what types it handles
sed -n '682,1000p' core/variant/variant_parser.cppRepository: Redot-Engine/redot-engine
Length of output: 9190
🏁 Script executed:
# Search for constructor-like parsing patterns (Color, Vector, etc.) to understand the pattern
rg -n "\"Color\(\"" core/variant/variant_parser.cppRepository: Redot-Engine/redot-engine
Length of output: 257
🏁 Script executed:
# Check the full scope of types handled in parse_value
rg -n "token.type == Token::" core/variant/variant_parser.cpp | head -30Repository: Redot-Engine/redot-engine
Length of output: 51
🏁 Script executed:
# Check if there's a Struct constructor or special case in parse_value
rg -n "else if \(id == \"Struct" core/variant/variant_parser.cppRepository: Redot-Engine/redot-engine
Length of output: 51
🏁 Script executed:
# Check the rest of parse_value to see all constructors it supports
sed -n '900,1100p' core/variant/variant_parser.cpp | head -100Repository: Redot-Engine/redot-engine
Length of output: 2972
🏁 Script executed:
# Look for any mention of STRUCT or struct reconstruction in parsing
rg -n -i "struct" core/variant/variant_parser.cpp | grep -i "parse\|read\|reconstruct"Repository: Redot-Engine/redot-engine
Length of output: 2190
🏁 Script executed:
# Get the complete list of all constructors/identifiers handled in parse_value
rg -n "} else if \(id ==" core/variant/variant_parser.cpp | grep -E "Vector|Color|Plane|Quat|Transform|Projection|Node|RID|Signal|Callable|Object|Struct"Repository: Redot-Engine/redot-engine
Length of output: 1296
🏁 Script executed:
# Check the end of the parse_value identifier chain to see what happens with unknown identifiers
sed -n '1040,1080p' core/variant/variant_parser.cppRepository: Redot-Engine/redot-engine
Length of output: 1004
🏁 Script executed:
# Check what happens when an unknown identifier is encountered in parse_value
sed -n '1000,1050p' core/variant/variant_parser.cppRepository: Redot-Engine/redot-engine
Length of output: 1362
🏁 Script executed:
# Check the fallback for unknown identifiers - what comes after all the known cases?
sed -n '1630,1700p' core/variant/variant_parser.cppRepository: Redot-Engine/redot-engine
Length of output: 1736
STRUCT text serialization is not round-trippable (silent data loss).
Writing Variant::STRUCT serializes it as a Dictionary (via struct_instance->serialize()), and when reading back from text resources, it returns as Dictionary—not as STRUCT. This affects text resource, scene, and config file serialization through ResourceFormatSaverTextInstance and ResourceFormatLoaderText, which both use VariantWriter::write_to_string() and VariantParser.
The parser has no Struct() constructor form to reconstruct the original type—all unknown identifiers fail with ERR_PARSE_ERROR, and the Dictionary is silently accepted instead. This is a silent data loss issue.
Recommended direction:
- Add an explicit textual constructor form (e.g.
Struct("fqsn", { ... })) and teachVariantParser::parse_value()to recognize and reconstruct it, or - Fail loudly (error / tagged placeholder) instead of silently writing a different type.
🤖 Prompt for AI Agents
In @core/variant/variant_parser.cpp around lines 2177 - 2202, The current
Variant::STRUCT text serialization (in the VariantParser write branch using
struct_instance->serialize() and VariantWriter::write_to_string()) loses type
fidelity by emitting a Dictionary that cannot be parsed back as a STRUCT; update
the textual format and parser: add an explicit textual constructor form such as
Struct("fqsn", {...}) and modify VariantParser::parse_value() to recognize that
constructor and reconstruct a GDScriptStructInstance (or, alternatively, make
write() emit a clear failing/tagged placeholder and have parse_value() return an
error for STRUCTs); ensure ResourceFormatSaverTextInstance and
ResourceFormatLoaderText paths that use write_to_string()/parse_value() either
round-trip via the new Struct(...) syntax or fail loudly instead of silently
returning a Dictionary.
|
@coderabbitai There is still a substantial memory leak here somewhere can you pinpoint it? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@coderabbitai Yes please |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/gdscript/gdscript_analyzer.cpp (1)
6440-6544: Add defensive null check for struct_typeThe code accesses
p_target.struct_type->fqsn(line 6534) without verifyingp_target.struct_typeis non-null. Whilestruct_typeis populated whenkind == STRUCTis set, this code patterns mirrors the defensive check already present in the same function:current->base_struct_type.kind == GDScriptParser::DataType::STRUCT && current->base_struct_type.struct_type. Similar null guards appear throughout the codebase (gdscript_docgen.cpp, gdscript_parser.cpp, gdscript_compiler.cpp), suggesting this is a standard precaution against partially-resolved or invalid datatypes.Struct meta types are currently hard-rejected in both branches (lines 6483–6485, 6528–6531). If struct types are intended to support
MyStruct.new()-style construction, this area will need coherent meta-type handling rather than blanket rejection.Suggested null check
case GDScriptParser::DataType::STRUCT: if (p_target.is_meta_type) { // Struct meta types are not currently supported. return false; } + if (p_target.struct_type == nullptr) { + return false; + } // Check if source struct is the same as target struct or inherits from it.
🤖 Fix all issues with AI agents
In @modules/gdscript/gdscript_analyzer.cpp:
- Around line 4854-4862: The check should detect a struct context by testing
parser->current_struct (not parser->current_class) and after emitting the
push_error for p_self keep p_self->is_constant = false but assign a safe
fallback datatype instead of an empty GDScriptParser::DataType to avoid later
“unset type” asserts; update the condition to use parser->current_struct, call
push_error(...) as before, and call p_self->set_datatype(...) with a safe dummy
DataType representing a valid Variant fallback (e.g., a nil/variant fallback)
before returning.
- Around line 1259-1263: The conflict detection is missing STRUCT: update
has_member_name_conflict_in_script_class to treat
GDScriptParser::ClassNode::Member::STRUCT as a conflicting member type (add
STRUCT to the set/conditions that cause a name conflict), so
check_class_member_name_conflict calls for Member::STRUCT properly detect
inheritance-name collisions with structs.
- Around line 5875-6005: Struct method lookup should use safe map access and
propagate vararg/static flags: replace direct operator[] access to
struct_node->method_map[p_function] with a safe lookup (e.g.,
method_map.getptr(p_function)) when obtaining FunctionNode pointers; after
obtaining func, if func->resolved_signature is false, push an error (or return
false) to ensure unresolved signatures don't silently skip registration; when
extracting a method signature set r_method_flags.set_flag(METHOD_FLAG_VARARG) if
the FunctionNode indicates varargs (e.g., func->is_vararg or func flag) in
addition to the existing static check (func->is_static); ensure these checks are
applied both for methods found on struct_node and on base structs before
returning true.
In @modules/gdscript/gdscript_compiler.cpp:
- Around line 198-204: The branch handling GDScriptParser::DataType::STRUCT
currently downgrades structs to GDScriptDataType::BUILTIN and sets
result.builtin_type = Variant::STRUCT but does not record the specific struct
definition; update this case to assign the struct reference into
result.struct_type (e.g., result.struct_type = p_datatype.struct_type) instead
of — or in addition to — treating it as BUILTIN so downstream type checking can
validate compatibility, and if struct_type isn't available yet, add a TODO
comment and create a follow-up issue tracking implementation of
result.struct_type so the limitation is tracked.
In @modules/gdscript/gdscript.cpp:
- Around line 149-162: The set_struct_type method currently calls
struct_type->unreference() without checking its return value; change it to
capture the boolean result and, if unreference() returns true (meaning the
refcount hit zero), free/delete the struct (e.g. memdelete or appropriate
destructor call) before overwriting struct_type, ensuring the same ownership
pattern as the destructor; keep the existing reference() call for the new
struct_type.
- Around line 3213-3228: register_struct currently erases/replaces old entries
but inserts p_struct without taking a registry reference, causing asymmetric
ownership with unregister_struct which calls unreference(); fix by calling
p_struct->reference() (or equivalent ownership increment) before inserting into
global_structs in GDScriptLanguage::register_struct so the registry owns a
reference, keeping the existing unreference/delete logic correct; update
comments to state registry holds a reference and ensure symmetry with
unregister_struct.
🧹 Nitpick comments (7)
core/io/json.cpp (1)
859-901: Verify STRUCT variant type before casting.Line 863 performs a
reinterpret_casttoGDScriptStructInstance*without first verifying thatp_variant.get_type() == Variant::STRUCT. While the switch-case ensures we're in the STRUCT branch, adding an explicit assertion would improve safety and match the pattern used in other type-specific handlers.🔍 Suggested defensive check
#ifdef MODULE_GDSCRIPT_ENABLED case Variant::STRUCT: { // Serialize struct as tagged dictionary with __type__ metadata // This allows round-trip deserialization + ERR_FAIL_COND_V(p_variant.get_type() != Variant::STRUCT, Variant()); const GDScriptStructInstance *struct_instance = reinterpret_cast<const GDScriptStructInstance *>(VariantInternal::get_struct(&p_variant)); ERR_FAIL_NULL_V(struct_instance, Variant());modules/gdscript/gdscript_compiler.cpp (1)
3217-3219: TODO: Struct methods may have incorrect 'self' context.The TODO comment notes that struct methods currently receive
p_classcontext instead of struct-specific context. This could cause issues with 'self' semantics inside struct methods. Consider tracking this as a follow-up issue or adding a runtime check to prevent accessing invalid members.modules/gdscript/gdscript_struct.cpp (2)
112-137: Clarify type validation and assignment logic.The type validation correctly handles:
- Direct type matches (assigned at line 136)
- Type conversions (assigned at line 119)
- Type mismatches (error at lines 128-130)
However, the comment on line 134 is misleading. It states "or will use the original value", but original values are actually assigned in the else block (line 136), not in the if block. Consider updating the comment to: "Already converted and assigned above."
193-203: Note: Method override not supported.Line 194 prevents adding a method that already exists in the inheritance chain. This means derived structs cannot override base struct methods. If method polymorphism is intended for structs, this should be changed to allow overrides. If structs are meant to be non-polymorphic value types, consider adding a comment explaining this design decision.
modules/gdscript/gdscript.cpp (2)
198-241: Extensive safety documentation is appreciated, but consider simplifying.The safety checks and documentation are thorough. However, consider extracting this logic into Variant itself as suggested in the TODO comments (lines 165-179) to avoid direct manipulation of Variant internals from an external class. This pattern is fragile and will break if Variant's internal layout changes.
3297-3301: const_cast in virtual method implementation.The
create_struct_by_namemethod usesconst_castto call a non-const method. However, this method is declared as non-const (based on usage). If the base class declares it const, consider makingcreate_struct_instanceconst-qualified or fixing the base class signature.modules/gdscript/gdscript_struct.h (1)
95-98: Dead code:childrenvector is unused per comment.The comment on line 96-97 explicitly states this vector is "currently unused and not populated." Consider removing it to reduce confusion, or add a TODO with a tracking issue if it's planned for future use.
♻️ Suggestion: Remove unused member or add tracking issue
- // Inheritance - // NOTE: The children vector is currently unused and not populated. - // Child struct tracking is handled through the owning GDScript's structs HashMap. - Vector<GDScriptStruct *> children; + // NOTE: Child struct tracking is handled through the owning GDScript's structs HashMap.Or add a TODO with issue reference:
// Inheritance - // NOTE: The children vector is currently unused and not populated. - // Child struct tracking is handled through the owning GDScript's structs HashMap. + // TODO(#XXX): Implement direct child tracking for faster inheritance queries Vector<GDScriptStruct *> children;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/io/json.cppmodules/gdscript/gdscript.cppmodules/gdscript/gdscript_analyzer.cppmodules/gdscript/gdscript_compiler.cppmodules/gdscript/gdscript_struct.cppmodules/gdscript/gdscript_struct.h
🧰 Additional context used
🧬 Code graph analysis (4)
core/io/json.cpp (2)
modules/gdscript/gdscript.cpp (4)
gdscript_struct_instance_serialize(3457-3461)gdscript_struct_instance_serialize(3457-3457)create_struct_instance(3253-3289)create_struct_instance(3253-3253)core/object/script_language.cpp (2)
create_struct_instance(555-573)create_struct_instance(555-555)
modules/gdscript/gdscript_struct.cpp (2)
modules/gdscript/gdscript_struct.h (3)
GDScriptStruct(47-113)get_member_count(108-112)GDScriptStructInstance(145-186)core/variant/variant_utility.cpp (2)
type_convert(868-955)type_convert(868-868)
modules/gdscript/gdscript_compiler.cpp (3)
modules/gdscript/gdscript_struct.cpp (2)
GDScriptStruct(41-46)GDScriptStruct(48-51)modules/gdscript/gdscript_struct.h (1)
GDScriptStruct(47-113)modules/gdscript/gdscript.h (1)
GDScriptStruct(79-79)
modules/gdscript/gdscript.cpp (1)
core/variant/variant_internal.h (3)
Dictionary(189-189)Dictionary(190-190)Dictionary(1162-1164)
🪛 Clang (14.0.6)
modules/gdscript/gdscript_struct.h
[error] 35-35: 'core/templates/hash_map.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (29)
core/io/json.cpp (2)
38-69: LGTM! Forward declarations and extern "C" wrappers follow proper layering.The use of forward declarations and extern "C" linkage correctly avoids circular dependencies between core/io and modules/gdscript. The stub implementation for when
MODULE_GDSCRIPT_ENABLEDis disabled properly returns an error.
1376-1412: LGTM! STRUCT deserialization properly validates inputs and uses ScriptServer abstraction.The deserialization logic correctly:
- Validates single dictionary argument (line 1381)
- Ensures
__type__field exists and is non-empty (lines 1384-1387)- Recursively decodes field values with depth tracking (line 1399)
- Uses
ScriptServer::create_struct_instanceabstraction (line 1403)- Validates the result type (line 1404)
modules/gdscript/gdscript_compiler.cpp (4)
3036-3043: LGTM! Struct compilation is properly integrated.Struct compilation is correctly positioned after function compilation and includes proper error propagation.
3223-3236: Error path properly unreferences compiler ownership.The error cleanup correctly:
- Unreferences the compiler's reference (only owner at error time)
- Verifies
ref_count == 0before callingmemdelete- Warns if unexpected references exist
This follows RAII-style cleanup and prevents memory corruption.
3333-3361: LGTM! Struct cleanup follows correct reference counting order.The three-step cleanup properly mirrors the reference counting lifecycle:
- Removes wrapper constants (reference #4)
- Unregisters from global registry (reference #3)
- Unreferences from script's HashMap (reference #2) and deletes when
ref_count == 0This matches the pattern in
GDScript::clear()and ensures no leaks or use-after-free issues.
3254-3258: The reference counting implementation is correct. Theset_struct_type()method (modules/gdscript/gdscript.cpp:149-162) does callreference()on the passed struct (line 160), matching the documented behavior in the comment on line 3256. The four references are properly balanced:
- Compiler (line 3155, released at line 3271)
- p_script->structs HashMap (line 3246)
- Global registry (line 3250)
- struct_wrapper via set_struct_type() (line 3257, confirmed at gdscript.cpp:160)
No issues found.
modules/gdscript/gdscript_struct.cpp (5)
41-59: LGTM! Reference counting follows explicit ownership pattern.The constructor correctly initializes
ref_count = 0, requiring explicitreference()calls for each owner. This pattern prevents accidental leaks and makes ownership transfers explicit.
232-262: LGTM! Instance initialization properly handles inheritance.The constructor correctly:
- Allocates members for the entire inheritance chain (line 237)
- Applies defaults from base to derived (lines 250-261)
- Allows derived defaults to override base defaults
This ensures correct inheritance semantics for struct instances.
268-279: LGTM! Reference counting implements correct self-deletion pattern.The
unreference()method correctly deletes the instance when the reference count reaches zero (lines 273-276). This matches the standard Godot RefCounted pattern.
310-359: Note: Only static struct methods are currently supported.Lines 341-346 explicitly prevent calling non-static struct methods, as they would require VM integration to provide proper 'self' context. This limitation is clearly documented in the error message and aligns with the TODO in
gdscript_compiler.cppabout struct method context.
373-425: LGTM! Serialization properly handles inheritance chain.Both
serialize()anddeserialize()correctly:
- Store/retrieve the fully qualified type name
- Walk the inheritance chain from base to derived (lines 381-397, 405-422)
- Handle all members across the inheritance hierarchy
This ensures consistent serialization of struct hierarchies.
modules/gdscript/gdscript_analyzer.cpp (6)
1586-1588: Good: struct bodies get resolved during recursive class-body resolutionHooking
Member::STRUCTintoresolve_class_body(..., true)keeps the struct resolution phase consistent with class recursion.
1665-1669: STRUCT nodes routed into analyzer: OKThe
resolve_node()switch wiring forNode::STRUCTlooks fine (assumingresolve_struct_body()is made safe as noted above).
1902-1924:_initsemantics for structs need a crisp rule (and tests)Treating struct
_initas a normal method avoids constructor-only rules, but it also means a struct can define a method named_initwith any return type (Line 1905-1912). Please confirm this is intended language behavior (vs. reserving_initfor construction-like patterns in structs too).
1952-2052: Good: skip parent override checks when not in a classGating the “signature matches parent” logic on
parser->current_class != nullptrprevents struct methods from going through class polymorphism checks.
2765-2766: Good: STRUCT added to unreachable expression casesKeeps the “new nodes aren’t forgotten” invariant.
5073-5074: STRUCT added to “cannot index” paths: makes senseDisallowing
[]onVariant::STRUCTis consistent with “no subscript operator” unless you add explicit support later.Also applies to: 5142-5144
modules/gdscript/gdscript.cpp (6)
125-127: LGTM - Empty bind_methods is intentional.The comment correctly explains that "new" is handled via
callprather than binding.
131-146: Constructor and destructor correctly manage reference counting.The constructor increments the ref count when taking ownership, and the destructor properly uses the return value of
unreference()to determine whether to delete the struct. This follows the proper ownership pattern.
243-272: LGTM - Instance creation with proper reference counting.The
_newmethod correctly:
- Creates the instance
- Increments the reference count before creating the Variant
- Documents the ownership transfer clearly
1754-1767: Good improvement to struct cleanup logic.The struct cleanup now properly:
- Erases constants wrapping structs
- Unregisters from the global registry
- Releases the script's ownership reference
This addresses the memory leak concerns raised in the PR comments.
1793-1809: Updated comments accurately reflect the new cleanup behavior.The comments correctly explain that structs are cleaned up immediately and not deferred to the
is_rootblock.
3455-3471: extern "C" functions are well-implemented for cross-module serialization.The
extern "C"linkage allows these functions to be called fromcore/io/json.cppwithout header dependencies. Both functions include proper null checks.modules/gdscript/gdscript_struct.h (6)
33-40: LGTM - Standard header structure with appropriate includes.The
#pragma onceguard and includes are correct. The static analysis warning aboutcore/templates/hash_map.hnot being found is a false positive from running analysis outside the full engine build environment.
47-52: Friend declarations are appropriate for internal access.The friend classes (GDScriptStructInstance, GDScriptCompiler, GDScriptAnalyzer, GDScriptStructClass) legitimately need access to private members for their functionality.
54-69: LGTM - MemberInfo and MethodInfo structs are well-designed.The nested structs properly encapsulate member and method metadata with sensible defaults.
71-76: LGTM - Reference counting interface is correctly defined.The
reference()andunreference()pattern with proper return value semantics for deletion control.
143-166: LGTM - GDScriptStructInstance is well-designed.The class correctly:
- Uses friend access for GDScriptStruct
- Deletes copy/move operations to match SafeRefCount semantics
- Provides a clean public interface for member access and serialization
168-185: LGTM - Clean public API for struct instance operations.The interface provides appropriate accessors for member access, method calling, type information, and serialization without exposing implementation details.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
modules/gdscript/gdscript_parser.cpp (1)
2924-2934: Potential null-deref introduced byparse_self()settingSelfNode::current_classto nullptr in struct context.With
self->current_class = current_struct ? nullptr : current_class;, any code that assumesSelfNode::current_classis always non-null (e.g. debug TreePrinter) can crash when printing/parsing struct methods.Proposed fix (make TreePrinter handle struct self safely)
@@ void GDScriptParser::TreePrinter::print_self(SelfNode *p_self) { push_text("Self("); - if (p_self->current_class->identifier != nullptr) { - print_identifier(p_self->current_class->identifier); - } else { - push_text("<main class>"); - } + if (p_self->current_struct != nullptr) { + if (p_self->current_struct->identifier != nullptr) { + print_identifier(p_self->current_struct->identifier); + } else { + push_text("<unnamed struct>"); + } + } else if (p_self->current_class != nullptr) { + if (p_self->current_class->identifier != nullptr) { + print_identifier(p_self->current_class->identifier); + } else { + push_text("<main class>"); + } + } else { + push_text("<unknown>"); + } push_text(")"); }modules/gdscript/gdscript_parser.h (1)
1142-1149: Fixprint_self()to handle nullablecurrent_classin struct context.The
SelfNode::current_classfield is set tonullptrwhen used in struct context (line 2933:self->current_class = current_struct ? nullptr : current_class). However,print_self()(line 6388) directly dereferencesp_self->current_class->identifierwithout a null check, causing a crash when printing aSelfNodefrom a struct method.The
reduce_self()function handles this correctly by checkingif (parser->current_class == nullptr)before use. Apply the same guard inprint_self():Current unsafe code in print_self()
void GDScriptParser::TreePrinter::print_self(SelfNode *p_self) { push_text("Self("); if (p_self->current_class->identifier != nullptr) { print_identifier(p_self->current_class->identifier); } else { push_text("<main class>"); } push_text(")"); }modules/gdscript/gdscript_compiler.h (1)
108-139: Remove/guardprint_line("DEBUG ...")inCodeGen::add_constant().
Compiler hot paths shouldn’t emit unconditional logs.Proposed diff
@@ - print_line("DEBUG add_constant: obj=" + String(obj->get_class()) + ", type.kind set to NATIVE"); - - // Check if this is a GDScriptStructClass (used for struct constructors) - GDScriptStructClass *struct_class = Object::cast_to<GDScriptStructClass>(obj); - print_line("DEBUG add_constant: struct_class=" + itos(uint64_t(struct_class))); + // Check if this is a GDScriptStructClass (used for struct constructors) + GDScriptStructClass *struct_class = Object::cast_to<GDScriptStructClass>(obj); if (struct_class != nullptr) { - print_line("DEBUG add_constant: Detected GDScriptStructClass, setting kind to STRUCT"); type.kind = GDScriptDataType::STRUCT; type.struct_type = struct_class->get_struct_type(); } else {modules/gdscript/gdscript_analyzer.cpp (1)
4886-4899: The analyzer incorrectly rejectsselfin struct methods despite parser support.
The parser intentionally setscurrent_class = nullptrfor struct methods (gdscript_parser.cpp:2933) with explicit comments: "Prefer current_struct over current_class for struct methods." However, the analyzer'sreduce_self()checksif (parser->current_class == nullptr)and hard-errors, blocking the intended struct method behavior. The condition should check bothcurrent_class == nullptr && current_struct == nullptr, since struct methods have a validcurrent_structcontext. A compiler TODO (gdscript_compiler.cpp:3253) already flags this as a known limitation.modules/gdscript/gdscript_compiler.cpp (1)
3031-3066: Struct methods registered inp_script->member_functionswill collide with class methods by bare name.
_compile_struct()calls_parse_function(err, p_script, p_class, method)for struct methods (line 3259). With default parametersp_for_ready=falseandp_for_lambda=false, the registration condition at line 2570 evaluates to true:if (!is_implicit_initializer && !is_implicit_ready && !p_for_lambda) { p_script->member_functions[func_name] = gd_function; // Line 2570 }This registers struct methods into the same
GDScript::member_functionsHashMap as class methods, using bare names only. A struct method and a class method with identical names will collide and overwrite each other. Struct methods should be isolated (e.g., by passing ap_register_in_scriptflag to_parse_functionor using a dedicated struct method registry).
🤖 Fix all issues with AI agents
In @core/variant/variant_call.cpp:
- Around line 1667-1678: Remove the two debug print_line calls and the redundant
null-check before ERR_FAIL_NULL_V in the is_builtin_method_vararg
implementation: when p_type == STRUCT simply return false without printing; then
retrieve const VariantBuiltInMethodInfo *method =
builtin_method_info[p_type].getptr(p_method); call ERR_FAIL_NULL_V(method,
false); and return method->is_vararg; delete the print_line for the null case
and the extra if (!method) block so only ERR_FAIL_NULL_V handles the null
pointer.
In @modules/gdscript/gdscript_analyzer.cpp:
- Around line 4344-4359: The current switch case in reduce_identifier_from_base
constructs a DataType for STRUCT manually to work around bugs in
GDScriptParser::ClassNode::Member::get_datatype(); instead fix get_datatype() in
ClassNode::Member to return a complete STRUCT DataType (setting struct_type,
script_path, is_meta_type, is_constant, type_source, etc.) and then replace this
manual construction in gdscript_analyzer.cpp with
p_identifier->set_datatype(member.get_datatype()); preserve setting
p_identifier->source = IdentifierNode::MEMBER_CLASS and
p_identifier->is_constant as needed after the call so behavior stays identical.
- Around line 1602-1612: In resolve_struct_body, add null-safety checks around
p_struct->identifier and any method->identifier usages: guard before accessing
p_struct->identifier->name and method->identifier->name (and similar fields) so
nullptrs are handled; update the debug print to use a conditional fallback
(e.g., "(null)" or empty string) when identifier is null, and keep the existing
idempotence check on p_struct->resolved_body; apply the same defensive null
checks to every place inside resolve_struct_body that dereferences
p_struct->identifier or method->identifier to avoid crashes.
In @modules/gdscript/gdscript_compiler.cpp:
- Around line 3168-3320: When GDScriptLanguage::get_struct_by_name(...) returns
an existing GDScriptStruct in _compile_struct, the code currently only inserts
it into p_script->structs and returns, leaving p_script->constants missing the
GDScriptStructClass wrapper; fix by creating a Ref<GDScriptStructClass> wrapper,
instantiate it, call set_struct_type(existing) and store it in
p_script->constants[p_struct->identifier->name] before returning so
StructName.new() and hot-reload work correctly; ensure you do this in the
early-return branch that handles the existing struct (the same symbol names are
get_struct_by_name, p_script->structs, p_script->constants, and
GDScriptStructClass).
In @modules/gdscript/gdscript_struct.cpp:
- Around line 150-163: GDScriptStruct::add_member currently leaves
MemberInfo.property_info uninitialized, so update add_member to populate
info.property_info.name = p_name, info.property_info.type = p_type (and if
p_type is PROPERTY_HINT_RESOURCE or object/struct use
info.property_info.class_name = p_type_name or appropriate class_name), and set
any other fields required by get_property_list to ensure inspector/debugger
property entries are correct; locate MemberInfo and get_property_list to mirror
fields used there and ensure info.property_info matches expectations before
assigning members[p_name] and pushing member_names/member_types.
- Around line 372-424: deserialize() currently ignores the "__type__" written by
serialize(), which is a type-mismatch footgun; update
GDScriptStructInstance::deserialize to first check p_data.has("__type__") and
compare p_data["__type__"] (as String or StringName) against
struct_type->get_fully_qualified_name(), and if they differ return false (or
fail early) instead of applying fields; keep the rest of the inheritance-member
iteration intact and only apply members when the type check succeeds.
In @modules/gdscript/gdscript_struct.h:
- Around line 71-76: GDScriptStruct::unreference() and
GDScriptStructInstance::unreference() must share the same contract: they should
decrement ref_count, return true when the count reaches zero, and must not
perform self-deletion; update the implementation of
GDScriptStructInstance::unreference() in the .cpp to remove any "delete this" or
equivalent and instead just decrement ref_count and return whether it hit zero,
ensure GDScriptStruct::unreference() does the same, and keep
get_reference_count()/ref_count usage consistent so callers are responsible for
deleting when unreference() returns true.
🧹 Nitpick comments (9)
modules/gdscript/gdscript_parser.cpp (4)
900-1027: Struct body parsing skips theparse_class_memberpath (annotations/doc comments/completion parity).
parse_struct()manually parsesvar/func, so you don’t get the same behavior as class members (annotation consumption, doc comment attachment, and any other shared member handling). If structs are meant to be “class-like”, consider reusingparse_class_member(with a new annotation target kind for structs) or duplicating the minimal missing pieces intentionally.Also:
parse_variable(p_is_static, true)currently allows property syntax in struct “fields”; if structs are meant to be plain data, this likely should befalseto prevent accidentalget/setblocks in structs.
1171-1173:STRUCTmembers currently reject all annotations (target kind isAnnotationInfo::NONE).
parse_class_member(..., AnnotationInfo::NONE, "struct")means any preceding annotations will be treated as invalid and cleared. If you intend to allow things like@deprecated/ doc comments / tooling annotations on structs, you likely want a dedicated target kind (e.g.AnnotationInfo::STRUCT) and to pass that here.
5451-5459:DataType::to_property_info()struct fallback uses"Variant"as hint_string; consider a clearer contract.For STRUCT (and container element STRUCT) you set
hint_stringto"Variant"whenstruct_typeis missing. That’s potentially misleading (it’s not actually Variant) and might confuse editor/serialization code expecting a struct id/name. Consider using an empty hint_string (or"<unknown struct>") and let downstream treat it as untyped/unknown.Also applies to: 5493-5500, 5528-5535, 5583-5591
6132-6167: TreePrinterprint_struct()usesconst_castfor methods; prefer const-correctness.
for (const FunctionNode *function : p_struct->methods) { print_function(const_cast<FunctionNode *>(function)); }suggestsprint_functionshould probably takeconst FunctionNode *(like other print_* methods) to avoid mutability assumptions in debug tooling.modules/gdscript/gdscript_parser.h (1)
108-118:DataType::operator==for STRUCT likely needs an fqsn-based fallback (like CLASS).Right now,
STRUCTequality isstruct_type == p_other.struct_type. If the same struct can be represented by differentStructNode*instances across parse/cache boundaries, type equality will fail unexpectedly. Consider mirroring CLASS’s pattern: pointer equality OR fqsn equality (with null checks).Also applies to: 141-142, 201-234
modules/gdscript/gdscript_compiler.cpp (2)
682-756: Remove committed debug logging + avoid identifier TDZ/unsafe casts in static-call handling.
Theprint_line("DEBUG compiler: ...")spam will land in runtime logs (and potentially editor output) and should not ship. Also,idis created by castingsubscript->basebefore verifying its type (Line 684-686); while only used under anif, this is fragile and invites future misuse.Proposed change (guard logs + safe cast ordering)
- const GDScriptParser::IdentifierNode *id = static_cast<const GDScriptParser::IdentifierNode *>(subscript->base); - if (subscript->base->type == GDScriptParser::Node::IDENTIFIER) { - print_line("DEBUG compiler: Checking static call: " + String(id->name) + "." + String(call->function_name) + ", kind=" + itos(id->datatype.kind) + ", STRUCT=" + itos(GDScriptParser::DataType::STRUCT) + ", is_meta_type=" + itos(id->datatype.is_meta_type)); - } + const GDScriptParser::IdentifierNode *id = nullptr; + if (subscript->base->type == GDScriptParser::Node::IDENTIFIER) { + id = static_cast<const GDScriptParser::IdentifierNode *>(subscript->base); +#ifdef DEBUG_ENABLED + // Optional: keep behind DEBUG_ENABLED if still needed temporarily. + // print_line(vformat("compiler: checking static call %s.%s", id->name, call->function_name)); +#endif + } // Check if this is a struct TYPE (not instance) for static method calls ... - if (!call->is_super && subscript->base->type == GDScriptParser::Node::IDENTIFIER && id->datatype.kind == GDScriptParser::DataType::STRUCT && id->datatype.is_meta_type) { + if (!call->is_super && id && id->datatype.kind == GDScriptParser::DataType::STRUCT && id->datatype.is_meta_type) { // Struct static method call (e.g., TestStruct.new()) - print_line("DEBUG compiler: Detected struct static call: " + String(id->name) + "." + String(call->function_name)); GDScriptCodeGenerator::Address base = _parse_expression(codegen, r_error, subscript->base); - print_line("DEBUG compiler: base address mode=" + itos(base.mode) + ", type.kind=" + itos(base.type.kind)); if (r_error) { return GDScriptCodeGenerator::Address(); } gen->write_call(result, base, call->function_name, arguments);
3379-3407: Cleanup is directionally good, but deletion should follow the engine’s “unref returns true => delete” convention consistently.
You callE.value->unreference();and then checkget_reference_count()==0(Line 3401-3405). Prefer using the boolean return to reduce races/assumptions and match typical patterns used elsewhere in the codebase.Proposed change
- for (KeyValue<StringName, GDScriptStruct *> &E : old_structs) { - E.value->unreference(); - if (E.value->get_reference_count() == 0) { - memdelete(E.value); - } - } + for (KeyValue<StringName, GDScriptStruct *> &E : old_structs) { + if (E.value->unreference()) { + memdelete(E.value); + } + }modules/gdscript/gdscript_struct.cpp (1)
309-358: Struct method calling model is incomplete (static-only) but currently returns INVALID_METHOD for non-static.
Given compiler currently compiles struct methods via_parse_function(... p_class ...)(class context), this is likely to miscompile any method that expects struct “self” semantics. If onlyStructName.new()is intended for now, consider rejecting non-static methods earlier (during compilation) with a clear error until there’s VM/self support.modules/gdscript/gdscript_struct.h (1)
99-136: Missing invariants: inheritance/shadowing and ownership rules aren’t enforced.
Givenhas_member()consults base structs (Line 105-106 / cpp impl),add_member()should likely prevent shadowing a base member name (or define/handle it explicitly), otherwise indices/serialization can become ambiguous.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignorecore/variant/variant_call.cppmodules/gdscript/gdscript.cppmodules/gdscript/gdscript_analyzer.cppmodules/gdscript/gdscript_compiler.cppmodules/gdscript/gdscript_compiler.hmodules/gdscript/gdscript_parser.cppmodules/gdscript/gdscript_parser.hmodules/gdscript/gdscript_struct.cppmodules/gdscript/gdscript_struct.h
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/gdscript/gdscript.cpp
🧰 Additional context used
🧬 Code graph analysis (4)
modules/gdscript/gdscript_parser.h (1)
modules/gdscript/gdscript_parser.cpp (4)
parse_struct(900-1027)parse_struct(900-900)print_struct(6132-6166)print_struct(6132-6132)
modules/gdscript/gdscript_parser.cpp (1)
modules/gdscript/gdscript_parser.h (1)
is_at_end(1594-1695)
modules/gdscript/gdscript_compiler.h (4)
modules/gdscript/gdscript_struct.cpp (2)
GDScriptStruct(41-47)GDScriptStruct(49-50)modules/gdscript/gdscript_struct.h (1)
GDScriptStruct(47-108)modules/gdscript/gdscript.h (5)
GDScriptStruct(79-79)GDScriptStructClass(66-85)String(634-654)Ref(439-620)GDScript(87-291)modules/gdscript/gdscript_function.h (2)
GDScriptDataType(48-115)GDScript(610-610)
modules/gdscript/gdscript_struct.cpp (1)
modules/gdscript/gdscript_struct.h (3)
GDScriptStruct(47-108)get_member_count(103-107)GDScriptStructInstance(140-181)
🪛 Clang (14.0.6)
modules/gdscript/gdscript_struct.h
[error] 35-35: 'core/templates/hash_map.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (8)
modules/gdscript/gdscript_parser.cpp (1)
4338-4339: LGTM: STRUCT token wiring + type string/printing hooks are consistent and minimally invasive.Adding
STRUCTtoparse_class_body, parse rules,DataType::to_string(), and TreePrinter class-member printing is coherent and keeps struct out of expression parsing (no prefix/infix rules).Also applies to: 5394-5399, 6002-6004
modules/gdscript/gdscript_parser.h (3)
566-771:ClassNode::MemberSTRUCT integration looks consistent; watch theadd_membertemplate preconditions.The STRUCT cases in
get_name/get_type_name/get_line/get_datatype/get_source_nodeare coherent. Just ensure any callsites that useClassNode::add_member(T*)withStructNode*only do so whenStructNode::identifier != nullptr(the template assumes it).Also applies to: 805-818
835-894: LGTM:StructNodelayout + duplicate detection helpers are straightforward.
field_indices/method_mapchecks are clear, andtype = Node::STRUCTis set in the ctor.
1603-1606: Public API surface:parse_struct()+ TreePrinter hook are appropriately placed.The parser interface and debug printer hook additions are clean and match existing patterns.
Also applies to: 1766-1768
modules/gdscript/gdscript_compiler.h (1)
179-181: No critical reference-count or cleanup issues found in struct compilation and teardown paths.The implementation properly manages ownership with explicit reference/unreference calls: compiler (initial),
p_script->structs, global registry, and wrapper each hold their own reference. The cleanup sequence inGDScript::clear()correctly releases all three owner references before deletion. Error paths safely handle cleanup before any ownership transfers occur.modules/gdscript/gdscript_struct.h (1)
35-41: Headers exist and include paths are correct—no code defect found.
The headerscore/templates/hash_map.handcore/templates/safe_refcount.hare present in the repository. The include statement at line 35 is valid and consistent with 60+ other files across the codebase using identical paths. The static analysis report about a missing file appears to be a false positive from tooling configuration rather than an actual compilation issue.Likely an incorrect or invalid review comment.
modules/gdscript/gdscript_compiler.cpp (1)
3188-3318: Reference counting assumptions verified as correct.
The implementations confirm both assumptions:
GDScriptLanguage::register_struct()does take a reference (callsp_struct->reference())GDScriptStructClass::set_struct_type()does take a reference (callsstruct_type->reference())The 4-reference model is accurate and properly implemented. The contract is already documented in code via explicit comments at lines 3288-3291 and 3297-3302. No memory leaks or reference counting issues found.
modules/gdscript/gdscript_struct.cpp (1)
41-58: GDScriptStructInstance has critical refcount lifecycle bugs: missing init(1) and internal delete in unreference() cause UAF and double-free.
GDScriptStructInstancenever initializesref_countto 1 (constructor lines 231–261), so refcount starts at 0, violating the expected ownership model.GDScriptStructInstance::unreference()callsmemdelete(this)internally (lines 271–278), then Variant's cleanup code (variant.cpp:1500–1502) attempts to readget_reference_count()on the deleted object and memdeletes again, causing UAF and double-free.- The correct pattern (used by RefCounted) is:
unreference()returns a bool, does not delete itself, and the caller handles cleanup.Proposed fix
GDScriptStructInstance::GDScriptStructInstance(GDScriptStruct *p_struct_type) : struct_type(p_struct_type) { ERR_FAIL_NULL(struct_type); + // Initial creator owns 1 ref, matching GDScriptStruct's convention. + ref_count.init(1); // Calculate total member count including inherited members int total_count = struct_type->get_member_count(); bool GDScriptStructInstance::unreference() { - if (ref_count.unref()) { - // Reference count reached zero, delete this instance - memdelete(this); - return true; - } - return false; + return ref_count.unref(); }Likely an incorrect or invalid review comment.
| // Reference counting for proper lifecycle management | ||
| // This ensures structs are only deleted when all references are gone | ||
| void reference(); | ||
| bool unreference(); // Returns true if reference count reached zero | ||
| int get_reference_count() const { return ref_count.get(); } | ||
|
|
There was a problem hiding this comment.
Public refcount APIs should follow one consistent contract for both struct type and instance.
Right now GDScriptStruct::unreference() is documented “Returns true if reference count reached zero” (Line 74-75) and does not delete; GDScriptStructInstance is declared similarly (Line 159-162) but the .cpp currently deletes itself (see other comment). Align both to “caller deletes when unreference() returns true” to match typical engine patterns and avoid container/Variant misuse.
Also applies to: 158-162
🤖 Prompt for AI Agents
In @modules/gdscript/gdscript_struct.h around lines 71 - 76,
GDScriptStruct::unreference() and GDScriptStructInstance::unreference() must
share the same contract: they should decrement ref_count, return true when the
count reaches zero, and must not perform self-deletion; update the
implementation of GDScriptStructInstance::unreference() in the .cpp to remove
any "delete this" or equivalent and instead just decrement ref_count and return
whether it hit zero, ensure GDScriptStruct::unreference() does the same, and
keep get_reference_count()/ref_count usage consistent so callers are responsible
for deleting when unreference() returns true.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @modules/gdscript/gdscript.cpp:
- Around line 1755-1768: The loop clearing the structs HashMap calls
E.value->unreference() but ignores its boolean return, so when unreference()
returns true (last reference) the struct must be freed; modify the loop in
gdscript.cpp to capture the result of E.value->unreference() and if it returns
true call memdelete(E.value) instead of leaking it, ensuring you don't access
E.value after deletion (e.g., store E.value in a local GDScriptStruct* s before
unregistering/unreferencing, check if s->unreference() returns true and call
memdelete(s)).
- Around line 3213-3234: In GDScriptLanguage::register_struct remove all
temporary debug print_line calls used for tracing (the print_line(...) calls
that log "DEBUG register_struct: ...") so the function only handles
reference/unreference, mutex locking, and the global_structs erase/insert logic;
delete those print_line lines and ensure no other debug-only prints remain in
register_struct and that reference/unreference and insert calls are unchanged.
- Around line 138-145: The destructor GDScriptStructClass::~GDScriptStructClass
currently calls struct_type->unreference() but ignores its boolean return;
change it to mirror set_struct_type() by checking the return value and deleting
the struct when unreference() returns true: if struct_type and
struct_type->unreference() then delete struct_type, then set struct_type =
nullptr; ensure you only call unreference() once and handle the nullptr case.
- Around line 147-148: There are two identical declarations of
GDScriptStructClass::set_struct_type(GDScriptStruct *p_struct) causing a
duplicate-symbol/syntax error; remove the redundant duplicate so only a single
definition of GDScriptStructClass::set_struct_type remains (or merge any
differing bodies into that single implementation) ensuring the function
signature appears once in the compilation unit.
- Around line 3236-3249: unregister_struct currently calls
GDScriptStruct::unreference() and ignores its return value, causing a leak when
that call indicates the struct reached zero refs; update
GDScriptLanguage::unregister_struct to capture the boolean return from
old_struct->unreference(), and if it returns true delete the old_struct (or call
the appropriate destructor/cleanup) before erasing from global_structs; ensure
you still remove the key from global_structs and protect with MutexLock as
before (use symbols: GDScriptLanguage::unregister_struct,
GDScriptStruct::unreference, global_structs).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/gdscript/gdscript.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
modules/gdscript/gdscript.cpp (4)
core/variant/variant.cpp (14)
Variant(2335-2338)Variant(2340-2343)Variant(2345-2348)Variant(2350-2353)Variant(2355-2358)Variant(2360-2363)Variant(2365-2368)Variant(2370-2373)Variant(2375-2378)Variant(2380-2383)Variant(2385-2388)Variant(2390-2393)Variant(2395-2399)Variant(2401-2405)modules/gdscript/gdscript_struct.cpp (2)
GDScriptStructInstance(231-261)GDScriptStructInstance(263-265)modules/gdscript/gdscript_struct.h (1)
GDScriptStructInstance(140-181)core/variant/variant_internal.h (3)
Dictionary(189-189)Dictionary(190-190)Dictionary(1162-1164)
🪛 Cppcheck (2.19.0)
modules/gdscript/gdscript.cpp
[error] 147-147: Unmatched '{'. Configuration
(syntaxError)
[error] 147-147: Unmatched '{'. Configuration
(syntaxError)
[error] 147-147: Unmatched '{'. Configuration
(syntaxError)
[error] 147-147: Unmatched '{'. Configuration
(syntaxError)
[error] 147-147: Unmatched '{'. Configuration
(syntaxError)
🔇 Additional comments (5)
modules/gdscript/gdscript.cpp (5)
125-136: LGTM: Constructor correctly increments reference count.The constructor properly calls
reference()when taking ownership of the struct, establishing correct reference counting from the start.
165-242: LGTM: Excellent safety checks and documentation.The implementation is thorough with comprehensive safety validations and clear documentation of the reference counting contract. The debug assertion at lines 221-225 ensures callers meet the reference counting precondition.
244-273: LGTM: Reference counting correctly managed in instance creation.The method properly increments the reference count (line 266) before creating the Variant, ensuring the instance lifecycle is correctly managed. The extensive documentation clearly explains the reference counting semantics.
3260-3296: LGTM: Reference counting correctly handled, but note inconsistency.The cleanup path at lines 3273-3277 correctly uses the
if (unreference()) { memdelete(...); }pattern. This demonstrates awareness of the correct pattern, which makes the missingmemdeletecalls at lines 142, 1763, and 3245 more concerning—they appear to be oversight rather than intentional design.Please ensure the same
unreference() + memdeletepattern is applied consistently throughout the codebase, particularly in the locations flagged in previous comments.
3457-3478: LGTM: Serialization helpers are safe.The extern C functions provide simple delegation with appropriate null checks.
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/gdscript/gdscript_compiler.cpp (1)
3348-3419: Old-struct cleanup: preferif (unreference()) memdelete(...)and avoid multi-step refcount reads.
Step 3 should use theunreference()return value, notget_reference_count()==0, to avoid TOCTOU-style mistakes and to match patterns elsewhere.Proposed fix
for (KeyValue<StringName, GDScriptStruct *> &E : old_structs) { - E.value->unreference(); - if (E.value->get_reference_count() == 0) { - memdelete(E.value); - } + if (E.value->unreference()) { + memdelete(E.value); + } }modules/gdscript/gdscript_analyzer.cpp (1)
6509-6613: Type compatibility for STRUCT: add a defensive null check onp_target.struct_type.
If a malformed/partialDataType::STRUCTever leaks through withstruct_type == nullptr, Line 6603 (p_target.struct_type->fqsn) can crash. A cheap guard makes this more robust.Proposed diff
@@ case GDScriptParser::DataType::STRUCT: if (p_target.is_meta_type) { // Struct meta types are not currently supported. return false; } + if (p_target.struct_type == nullptr) { + return false; + } // Check if source struct is the same as target struct or inherits from it. while (src_struct != nullptr) {
🤖 Fix all issues with AI agents
In @modules/gdscript/gdscript_analyzer.cpp:
- Around line 1259-1268: Interface-only pass (resolve_class_interface with
p_recursive=false) currently skips resolving struct bodies, leaving base-class
structs with unresolved fields/method signatures; update resolve_class_interface
(and the branch handling GDScriptParser::ClassNode::Member::STRUCT in the
non-recursive path) to call resolve_struct_body for member.m_struct (after the
existing check_class_member_name_conflict and the identifier-null check) so
struct bodies are resolved during interface analysis as well; ensure this
applies to all similar branches referenced (around the code handling
Member::STRUCT in resolve_class_interface and its recursive wrapper).
- Around line 1910-1932: The override-signature validation is being run for
struct methods because parser->current_class is set even when
parser->current_struct != nullptr; modify the code that matches/validates a
function against the parent signature so it first checks parser->current_struct
== nullptr (i.e., only run the parent override check when not inside a struct),
updating the block that references parser->current_class and p_function
accordingly; apply the same gating to the analogous validation block later in
the file (the section around the 1960-2059 range) so struct methods never
trigger class override errors.
- Around line 1602-1658: Guard against null identifier and re-entrancy in
GDScriptAnalyzer::resolve_struct_body by first checking p_struct->identifier
before dereferencing and returning early if missing; add a minimal "resolving"
sentinel on StructNode (e.g., bool resolving_body) and at the top return if
p_struct->resolving_body is true, then set p_struct->resolving_body = true
before any recursive work and clear it on all exit paths; ensure
p_struct->resolved_body is set only after successful resolution; additionally,
explicitly resolve the constructor signature by calling
resolve_function_signature(p_struct->constructor, p_source) if
p_struct->constructor exists and its resolved_signature is false (in case
constructor isn't present in p_struct->methods); restore previous contexts and
clear the resolving flag in all return paths.
- Around line 5939-6074: get_function_signature currently uses
method_map[p_function] which can insert/mutate the map and also calls push_error
when a method isn't found; change every direct operator[] access (e.g., the
initial const GDScriptParser::FunctionNode *func =
p_base_type.struct_type->method_map[p_function]; and later uses) to a
non-mutating lookup (use has_method() and
method_map.find()/const_iterator/at-style access) so you only read existing
entries without creating null ones, and remove the final push_error(...) for
"Method not found" so the function simply returns false and lets the caller
report the error.
- Around line 4918-4926: The error handler for using 'self' outside a class
should set a safe fallback datatype and avoid unnecessary vformat: after
push_error(...) set p_self->set_datatype(...) to a VARIANT DataType (use the
same helper/pattern used elsewhere in the analyzer, e.g. the DataType variant
factory or constant such as GDScriptParser::DataType::variant()/a VARIANT
DataType) so downstream is_set() checks succeed, and pass the literal error
string directly to push_error instead of wrapping it in vformat.
In @modules/gdscript/gdscript_compiler.cpp:
- Around line 694-716: Remove the unguarded print_line debug statements and stop
pre-casting subscript->base to IdentifierNode before verifying it's an
IDENTIFIER: only cast subscript->base to const GDScriptParser::IdentifierNode*
inside branches where subscript->base->type == GDScriptParser::Node::IDENTIFIER
(e.g., before using id->name or id->datatype), delete the top-level debug
print_line calls (and any other print_line debug spam in this block) or wrap
them with an explicit debug flag if logging is needed, and keep the logic that
detects struct static calls (check id->datatype.kind ==
GDScriptParser::DataType::STRUCT && id->datatype.is_meta_type) and the
builtin-type static call branch unchanged aside from moving casts into their
respective IDENTIFIER branches so no unsafe cast occurs.
- Around line 3180-3332: _struct methods are being stored as raw pointers into
p_script->member_functions causing use-after-free when the script is freed while
the struct (registered globally via GDScriptLanguage::register_struct) still
holds those pointers; also several print_line(...) calls use invalid comma
syntax and must be removed. Fix by changing _parse_function to accept a flag
(e.g., bool skip_register_to_script) or add a new helper for struct method
parsing so methods compiled for structs are not inserted into
p_script->member_functions, or alternatively make GDScriptStruct take ownership
(increase ref_count or store Ref<GDScriptFunction> / cloned functions) when you
call _parse_function for struct methods; update calls in _compile_struct to call
_parse_function(..., skip_register_to_script=true) (or the new helper) and
ensure GDScriptStruct::add_method stores owning references, and remove all debug
print_line(...) lines in _compile_struct that use comma-separated args.
In @modules/gdscript/gdscript.cpp:
- Around line 129-162: The destructor GDScriptStructClass::~GDScriptStructClass
currently calls struct_type->unreference() but does not memdelete when that call
returns true, causing a leak; update the dtor to mirror set_struct_type()’s
behavior by calling memdelete(struct_type) if unreference() returns true, then
set struct_type = nullptr, ensuring constructor, set_struct_type(), and the
destructor have consistent ownership semantics.
- Around line 3217-3256: Remove the debug print_line calls in register_struct
(they cause compile issues and are unnecessary) and fix the replace-path leak by
deleting the old struct when its reference count reaches zero: in
GDScriptLanguage::register_struct, when an existing->value (old_struct) is
found, call old_struct->unreference() and if that returns true then
memdelete(old_struct) before erasing the entry; then take a reference on
p_struct (p_struct->reference()) and insert it as before. Also ensure
unregister_struct's current logic (calling unreference() and memdelete on true)
is preserved.
- Around line 3267-3303: The deserialize-failure path in
GDScriptLanguage::create_struct_instance calls instance->unreference() even
though instance is created with ref_count==0, which can underflow/assert; fix by
ensuring the instance has at least one reference before any possible unreference
call (e.g., call instance->reference() immediately after
memnew(GDScriptStructInstance(...))) or alternatively, remove the unreference()
in the failure branch and use memdelete(instance) directly; update the code
around the memnew, the deserialize check, and the failure cleanup to use the
chosen approach so unreference() is never called on a never-referenced instance.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modules/gdscript/gdscript.cppmodules/gdscript/gdscript_analyzer.cppmodules/gdscript/gdscript_compiler.cpp
🧰 Additional context used
🧬 Code graph analysis (3)
modules/gdscript/gdscript_analyzer.cpp (1)
modules/gdscript/gdscript_parser.h (3)
DataType(104-264)DataType(175-178)DataType(180-185)
modules/gdscript/gdscript.cpp (3)
modules/gdscript/gdscript.h (3)
GDScriptStructClass(66-85)Variant(381-621)Variant(619-619)modules/gdscript/gdscript_struct.h (1)
GDScriptStructInstance(140-181)core/variant/variant_internal.h (3)
Dictionary(189-189)Dictionary(190-190)Dictionary(1162-1164)
modules/gdscript/gdscript_compiler.cpp (2)
modules/gdscript/gdscript_struct.cpp (4)
call(309-358)call(309-309)GDScriptStruct(41-47)GDScriptStruct(49-50)modules/gdscript/gdscript_struct.h (1)
GDScriptStruct(47-108)
🔇 Additional comments (4)
modules/gdscript/gdscript.cpp (2)
3469-3485: C-linkage JSON helpers look fine.
Null checks and type name extraction are straightforward.
198-241: No changes needed: Variant STRUCT lifecycle is correctly implemented.The copy/assign and destructor operations are properly handled. Variant::reference() (line 1318-1326 in core/variant/variant.cpp) correctly calls struct_instance->reference(), and Variant::_clear_internal() (line 1496-1506) correctly calls struct_instance->unreference() with automatic deletion when refcount reaches zero. STRUCT is marked as true in the needs_deinit array, ensuring _clear_internal() is always invoked on destruction. The assumption that the caller has incremented the instance refcount before calling _variant_from_struct_instance() is the standard pattern matching how OBJECT and other pointer types are handled.
Likely an incorrect or invalid review comment.
modules/gdscript/gdscript_compiler.cpp (1)
3043-3077: Struct member compilation hook looks reasonable.
Calling_compile_structfrom_compile_classis the right place structurally; error propagation is consistent with other member compilers.modules/gdscript/gdscript_analyzer.cpp (1)
5137-5140: Structs treated as non-subscriptable: looks consistent.
IncludingVariant::STRUCTin the “no indexing / no subscript operator” cases matches how most value types behave unless you explicitly add[]semantics for structs later.Also applies to: 5206-5209
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/variant/variant.cpp (1)
3494-3504: STRUCT should likely be considered “shared” like OBJECT/ARRAY/DICTIONARY.
If structs are reference-counted and you introducedOPCODE_JUMP_IF_SHAREDpaths, not adding STRUCT tois_type_shared()can lead to wrong runtime decisions.Proposed fix
bool Variant::is_type_shared(Variant::Type p_type) { switch (p_type) { case OBJECT: + case STRUCT: case ARRAY: case DICTIONARY: return true; default: { } }modules/gdscript/gdscript_analyzer.cpp (1)
6547-6651:check_type_compatibility()STRUCT support needs null-guards forstruct_typeIn the STRUCT target branch you dereference
p_target.struct_type->fqsnwithout checkingp_target.struct_type. If a “kind==STRUCT but struct_type==nullptr” slips through (partially-initialized DataType), this will crash deep in type checking. AddERR_FAIL_NULL_V(p_target.struct_type, false)(and same forp_source.struct_typeif needed).
🤖 Fix all issues with AI agents
In @core/object/object.cpp:
- Around line 851-862: The Callable::callp invocation is incorrect and must pass
the actual argument pointer, arg count, an output Variant and the CallError;
replace the invalid Variant constructor call with a local Variant ret and call
Callable::callp as callable.callp(p_args, p_argcount, ret, r_error), then return
ret; ensure you do not attempt to construct a Variant from p_args (the deleted
constructor) and use the existing r_error (CallError) for error reporting.
In @core/variant/variant_setget.cpp:
- Around line 37-38: The file core/variant/variant_setget.cpp is directly
including modules/gdscript/gdscript_struct.h and using STRUCT-related branches
(and get_named()) without compile guards; wrap the include and any code paths
that reference GDScript-specific types or macros (the include line for
modules/gdscript/gdscript_struct.h, the STRUCT handling branches, and the
get_named() implementation that touches GDScript structs) with #ifdef
MODULE_GDSCRIPT_ENABLED / #endif so non-GDScript builds won't reference module
headers or symbols, keeping the core/module layering intact.
In @core/variant/variant.cpp:
- Around line 40-41: Wrap the unguarded include of
modules/gdscript/gdscript_struct.h with #ifdef MODULE_GDSCRIPT_ENABLED / #endif
and likewise wrap each switch-case that handles Variant::STRUCT with the same
preprocessor guard so those case blocks are compiled only when
MODULE_GDSCRIPT_ENABLED is defined; ensure the include sits inside the guard and
that each guarded case encloses the entire case body (from the case label
through its break/return) to avoid dangling switches.
In @modules/gdscript/gdscript_analyzer.cpp:
- Around line 5957-6112: The struct branch of get_function_signature is unsafe
and noisy: first null-check p_base_type.struct_type before any access; remove
all print_line debug calls; avoid using
p_base_type.struct_type->method_map[p_function] (which can insert) — use
has_method(p_function) and method_map.find(p_function) (or equivalent) to get
the FunctionNode only when present; before extracting ctor or method
parameter/return types ensure the struct body/signatures are resolved (call or
rely on resolve_struct_body() to populate constructor and set
func->resolved_signature, or attempt to resolve the function node here if
unresolved) so you don’t require resolved_signature to be true without
attempting resolution; finally, only read func when func!=nullptr and ensure you
don’t mutate AST/type maps during lookup, and keep push_error behavior when
method/ctor truly missing.
In @modules/gdscript/gdscript_struct.cpp:
- Around line 41-58: GDScriptStructInstance currently holds a raw pointer to
GDScriptStruct (struct_type) without taking ownership, creating a UAF when
GDScript::clear() deletes structs; fix by having GDScriptStructInstance
increment the struct's refcount (call GDScriptStruct::reference()) when an
instance is created or assigned the struct_type and decrement it (call
unreference()) in the instance destructor/reset path, and ensure all instance
methods that use struct_type (get(), set(), callp()) assume the pointer is valid
because of this refcount; alternatively, if you prefer invalidation, add
instance tracking in GDScript to invalidate all GDScriptStructInstance objects
when GDScript::clear() removes struct entries (but do not leave raw unreferenced
pointers).
In @modules/gdscript/gdscript_vm.cpp:
- Around line 1927-1931: Remove the unconditional debug print_line calls in
gdscript_vm.cpp that run in OPCODE_CALL (the lines printing "DEBUG VM:
OPCODE_CALL, method=" with String(*methodname) and the base object class print
using obj->get_class()), and the equivalent prints around lines ~1950-1952;
either delete them or wrap them in an #ifdef DEBUG_ENABLED / #endif (or use the
existing debug logging macro) so they only run in debug builds, ensuring you
reference the same symbols (methodname, base, obj, get_type/get_class) when
adding the conditional.
In @modules/gdscript/gdscript.cpp:
- Around line 140-147: The destructor GDScriptStructClass::~GDScriptStructClass
currently calls struct_type->unreference() but never deletes the struct if the
ref count drops to zero; update the destructor to mirror set_struct_type and
unregister_struct by capturing the bool result of struct_type->unreference(),
calling memdelete(struct_type) when that returns true, and then setting
struct_type to nullptr so ownership is correctly released.
🟠 Major comments (18)
core/object/object.cpp-887-887 (1)
887-887: Remove debug logging from production code.These
print_linedebug statements will output messages for every method call, causing console spam and performance degradation.🧹 Proposed fix
MethodBind *method = ClassDB::get_method(get_class_name(), p_method); if (method) { - print_line("DEBUG Object::callp: Found method via ClassDB: " + String(p_method) + " on class " + get_class_name()); ret = method->call(this, p_args, p_argcount, r_error); } else { - print_line("DEBUG Object::callp: Method NOT found via ClassDB: " + String(p_method) + " on class " + get_class_name()); r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD; }If this logging is needed for debugging struct dispatch, wrap it in
#ifdef DEBUG_ENABLEDand use a more targeted condition (e.g., only log for GDScriptStructClass).Also applies to: 890-890
modules/gdscript/gdscript_byte_codegen.h-230-253 (1)
230-253: Gate debug logging inget_constant_pos()behindDEBUG_ENABLED.This function is called 25+ times during script compilation and should not include unconditional
print_line()calls with string concatenation in hot paths. Additionally, logging raw pointer addresses (obj,struct_type) is a non-deterministic info leak. Wrap the debug statements behind#ifdef DEBUG_ENABLEDand prefer stable identifiers (e.g.,get_instance_id()) instead of pointer values.Proposed diff
int get_constant_pos(const Variant &p_constant) { - print_line("DEBUG get_constant_pos: ENTRY, Variant type=" + itos(p_constant.get_type()) + ", current constant_map.size()=" + itos(constant_map.size())); - if (p_constant.get_type() == Variant::OBJECT) { - Object *obj = p_constant; - if (obj) { - print_line("DEBUG get_constant_pos: Variant is OBJECT, class=" + String(obj->get_class()) + ", obj=" + itos(uint64_t(obj))); - GDScriptStructClass *sc = Object::cast_to<GDScriptStructClass>(obj); - if (sc) { - print_line("DEBUG get_constant_pos: IS GDScriptStructClass, struct_type=" + itos(uint64_t(sc->get_struct_type()))); - } else { - print_line("DEBUG get_constant_pos: NOT GDScriptStructClass"); - } - } - } +#ifdef DEBUG_ENABLED + print_line("DEBUG get_constant_pos: ENTRY, Variant type=" + itos(p_constant.get_type()) + ", current constant_map.size()=" + itos(constant_map.size())); + if (p_constant.get_type() == Variant::OBJECT) { + Object *obj = p_constant; + if (obj) { + print_line("DEBUG get_constant_pos: Variant is OBJECT, class=" + String(obj->get_class()) + ", instance_id=" + itos(uint64_t(obj->get_instance_id()))); + GDScriptStructClass *sc = Object::cast_to<GDScriptStructClass>(obj); + print_line(sc ? "DEBUG get_constant_pos: IS GDScriptStructClass" : "DEBUG get_constant_pos: NOT GDScriptStructClass"); + } + } +#endif if (constant_map.has(p_constant)) { int existing_pos = constant_map[p_constant]; - print_line("DEBUG get_constant_pos: FOUND existing constant at pos=" + itos(existing_pos)); +#ifdef DEBUG_ENABLED + print_line("DEBUG get_constant_pos: FOUND existing constant at pos=" + itos(existing_pos)); +#endif return existing_pos; } int pos = constant_map.size(); - print_line("DEBUG get_constant_pos: ADDING new constant at pos=" + itos(pos)); +#ifdef DEBUG_ENABLED + print_line("DEBUG get_constant_pos: ADDING new constant at pos=" + itos(pos)); +#endif constant_map[p_constant] = pos; - print_line("DEBUG get_constant_pos: After add, constant_map.size()=" + itos(constant_map.size())); +#ifdef DEBUG_ENABLED + print_line("DEBUG get_constant_pos: After add, constant_map.size()=" + itos(constant_map.size())); +#endif return pos; }modules/gdscript/gdscript_vm.cpp-678-718 (1)
678-718: Remove temporary debug code before merging.The GET_VARIANT_PTR macro contains extensive unconditional debug prints that will execute on every variant pointer fetch in debug builds, severely impacting performance. These appear to be temporary debugging aids for struct development and should be removed before merging.
🧹 Proposed cleanup
-#define GET_VARIANT_PTR(m_v, m_code_ofs) \ - Variant *m_v; \ - { \ - int address = _code_ptr[ip + 1 + (m_code_ofs)]; \ - int address_type = (address & ADDR_TYPE_MASK) >> ADDR_BITS; \ - int address_index = address & ADDR_MASK; \ - if (address_type == ADDR_TYPE_CONSTANT) { \ - print_line("DEBUG GET_VARIANT_PTR: Loading CONSTANT, address_index=" + itos(address_index)); \ - if (address_index >= 0 && address_index < _constant_count) { \ - Variant *c = &_constants_ptr[address_index]; \ - print_line("DEBUG GET_VARIANT_PTR: Constant type=" + itos(c->get_type())); \ - if (c->get_type() == Variant::OBJECT) { \ - Object *obj = *c; \ - if (obj) { \ - print_line("DEBUG GET_VARIANT_PTR: Constant is OBJECT, class=" + String(obj->get_class()) + ", obj=" + itos(uint64_t(obj))); \ - GDScriptStructClass *sc = Object::cast_to<GDScriptStructClass>(obj); \ - if (sc) { \ - print_line("DEBUG GET_VARIANT_PTR: CONSTANT IS GDScriptStructClass!"); \ - } else { \ - print_line("DEBUG GET_VARIANT_PTR: CONSTANT is NOT GDScriptStructClass"); \ - } \ - } \ - } \ - } \ - } \ - if (unlikely(address_type < 0 || address_type >= ADDR_TYPE_MAX)) { \ - err_text = "Bad address type."; \ - OPCODE_BREAK; \ - } \ - if (unlikely(address_index < 0 || address_index >= variant_address_limits[address_type])) { \ - if (address_type == ADDR_TYPE_MEMBER && !p_instance) { \ - err_text = "Cannot access member without instance."; \ - } else { \ - err_text = "Bad address index."; \ - } \ - OPCODE_BREAK; \ - } \ - m_v = &variant_addresses[address_type][address_index]; \ - if (unlikely(!m_v)) \ - OPCODE_BREAK; \ +#define GET_VARIANT_PTR(m_v, m_code_ofs) \ + Variant *m_v; \ + { \ + int address = _code_ptr[ip + 1 + (m_code_ofs)]; \ + int address_type = (address & ADDR_TYPE_MASK) >> ADDR_BITS; \ + int address_index = address & ADDR_MASK; \ + if (unlikely(address_type < 0 || address_type >= ADDR_TYPE_MAX)) { \ + err_text = "Bad address type."; \ + OPCODE_BREAK; \ + } \ + if (unlikely(address_index < 0 || address_index >= variant_address_limits[address_type])) { \ + if (address_type == ADDR_TYPE_MEMBER && !p_instance) { \ + err_text = "Cannot access member without instance."; \ + } else { \ + err_text = "Bad address index."; \ + } \ + OPCODE_BREAK; \ + } \ + m_v = &variant_addresses[address_type][address_index]; \ + if (unlikely(!m_v)) \ + OPCODE_BREAK; \ }modules/gdscript/gdscript_struct.cpp-150-163 (1)
150-163: MemberInfo.property_info is never populated → property lists will be wrong/empty.
get_property_list()pushesE.value.property_info, butadd_member()never sets it (name/type/hints). This will break editor/introspection and likely JSON tooling expecting valid PropertyInfo.Proposed fix (initialize PropertyInfo in add_member)
void GDScriptStruct::add_member(const StringName &p_name, const Variant::Type p_type, const StringName &p_type_name, const Variant &p_default_value, bool p_has_default_value) { ERR_FAIL_COND(members.has(p_name)); MemberInfo info; info.index = get_member_count(); // Index after all inherited members info.type = p_type; info.type_name = p_type_name; info.default_value = p_default_value; info.has_default_value = p_has_default_value; + info.property_info = PropertyInfo(p_type, p_name); + if (p_type == Variant::OBJECT && !p_type_name.is_empty()) { + info.property_info.class_name = p_type_name; + } members[p_name] = info; member_names.push_back(p_name); member_types.push_back(p_type); }Also applies to: 385-391
modules/gdscript/gdscript_struct.cpp-235-236 (1)
235-236: Unconditional debug print_line() will spam logs in normal usage.
The constructor andget()are hot-path-ish; these should be behind#ifdef DEBUG_ENABLEDand/or a runtime debug flag.Also applies to: 306-317
modules/gdscript/gdscript_byte_codegen.cpp-200-243 (1)
200-243: Unconditional print_line() in codegen finalization will spam logs / hurt perf in non-debug builds.
These traces run whenever a function has constants and are not behind#ifdef DEBUG_ENABLED(or a runtime flag), which is risky for shipping builds.Proposed fix (guard debug tracing)
if (constant_map.size()) { - print_line("DEBUG FINALIZE: constant_map.size()=" + itos(constant_map.size())); + #ifdef DEBUG_ENABLED + print_line("DEBUG FINALIZE: constant_map.size()=" + itos(constant_map.size())); + #endif function->_constant_count = constant_map.size(); function->constants.resize(constant_map.size()); function->_constants_ptr = function->constants.ptrw(); for (const KeyValue<Variant, int> &K : constant_map) { - print_line("DEBUG FINALIZE: Writing constant at index " + itos(K.value) + ", Variant type=" + itos(K.key.get_type())); + #ifdef DEBUG_ENABLED + print_line("DEBUG FINALIZE: Writing constant at index " + itos(K.value) + ", Variant type=" + itos(K.key.get_type())); if (K.key.get_type() == Variant::OBJECT) { Object *obj = K.key; if (obj) { print_line("DEBUG FINALIZE: Constant is OBJECT, class=" + String(obj->get_class()) + ", obj=" + itos(uint64_t(obj))); GDScriptStructClass *sc = Object::cast_to<GDScriptStructClass>(obj); if (sc) { print_line("DEBUG FINALIZE: CONSTANT at index " + itos(K.value) + " IS GDScriptStructClass!"); } else { print_line("DEBUG FINALIZE: CONSTANT at index " + itos(K.value) + " is NOT GDScriptStructClass"); } } } + #endif function->constants.write[K.value] = K.key; } - print_line("DEBUG FINALIZE: After writing all constants"); + #ifdef DEBUG_ENABLED + print_line("DEBUG FINALIZE: After writing all constants"); // Verify all constants were written correctly for (int i = 0; i < function->constants.size(); i++) { Variant &v = function->constants.write[i]; print_line("DEBUG FINALIZE VERIFY: Constant[" + itos(i) + "] type=" + itos(v.get_type())); if (v.get_type() == Variant::OBJECT) { Object *obj = v; if (obj) { print_line("DEBUG FINALIZE VERIFY: Constant[" + itos(i) + "] is OBJECT, class=" + String(obj->get_class())); GDScriptStructClass *sc = Object::cast_to<GDScriptStructClass>(obj); if (sc) { print_line("DEBUG FINALIZE VERIFY: Constant[" + itos(i) + "] IS GDScriptStructClass!"); } else { print_line("DEBUG FINALIZE VERIFY: Constant[" + itos(i) + "] is NOT GDScriptStructClass"); } } } } + #endif } else { - print_line("DEBUG FINALIZE: No constants in function"); + #ifdef DEBUG_ENABLED + print_line("DEBUG FINALIZE: No constants in function"); + #endif function->_constants_ptr = nullptr; function->_constant_count = 0; }modules/gdscript/gdscript_struct.cpp-69-148 (1)
69-148: create_instance() ignores inherited members for positional args (likely wrong for derived structs).
expected_args = member_names.size()and iteration uses onlymember_namesfrom the current struct, so base fields can’t be initialized positionally on derived structs. Given you already sizememberstoget_member_count()and apply defaults via the inheritance chain, it’s more consistent to accept/apply args in base→derived declaration order.Proposed fix (apply args across inheritance chain, base→derived)
GDScriptStructInstance *GDScriptStruct::create_instance(const Variant **p_args, int p_argcount) { // Allocate instance GDScriptStructInstance *instance = memnew(GDScriptStructInstance(this)); if (instance == nullptr) { ERR_FAIL_V_MSG(nullptr, vformat("Failed to allocate memory for struct '%s'.", name)); } - // Validate argument count matches expected member count - int expected_args = member_names.size(); + // Validate argument count matches expected member count (including inherited) + int expected_args = get_member_count(); if (p_argcount > expected_args) { memdelete(instance); ERR_FAIL_V_MSG(nullptr, vformat("Too many arguments for struct '%s': expected %d, got %d.", name, expected_args, p_argcount)); } - int arg_idx = 0; - for (const StringName &member_name : member_names) { - if (arg_idx < p_argcount) { - const MemberInfo *info = members.getptr(member_name); - if (info) { + // Build inheritance chain (base -> derived) + Vector<const GDScriptStruct *> chain; + for (const GDScriptStruct *cur = this; cur; cur = cur->get_base_struct()) { + chain.push_back(cur); + } + + int arg_idx = 0; + for (int i = chain.size() - 1; i >= 0 && arg_idx < p_argcount; --i) { + const GDScriptStruct *st = chain[i]; + for (const StringName &member_name : st->get_member_names()) { + if (arg_idx >= p_argcount) { + break; + } + const MemberInfo *info = st->get_members().getptr(member_name); + if (info) { const Variant &arg_value = *p_args[arg_idx]; ... } arg_idx++; - } else { - break; // No more arguments } } return instance; }modules/gdscript/gdscript_compiler.cpp-3414-3442 (1)
3414-3442: Struct cleanup in make_scripts follows the expected ownership model.The three-step cleanup (unregister wrapper → unregister struct → unreference/delete) correctly mirrors the ownership model. However, remove the debug print statement.
♻️ Remove debug print statement
// Step 1: Unregister struct wrappers from global wrapper registry (removes reference #3) for (const KeyValue<StringName, GDScriptStruct *> &E : old_structs) { - print_line("DEBUG make_scripts: Unregistering struct wrapper for '" + String(E.value->get_fully_qualified_name()) + "' from global registry"); GDScriptLanguage::get_singleton()->unregister_struct_wrapper(E.value->get_fully_qualified_name()); }modules/gdscript/gdscript_compiler.cpp-370-396 (1)
370-396: Remove debug print statements before merging.These
print_linedebug statements should be removed or converted to conditional debug macros. They will clutter the console output during normal usage.♻️ Remove debug print statements
- print_line("DEBUG IDENTIFIER: " + String(identifier) + ", source=" + itos(in->source) + ", kind=" + itos(in->get_datatype().kind) + ", is_meta_type=" + itos(in->get_datatype().is_meta_type)); if (in->get_datatype().kind == GDScriptParser::DataType::STRUCT && in->get_datatype().is_meta_type) { - // Struct type identifier - look up the wrapper from the global registry - // The wrapper is registered during _compile_struct and persists across compilations - print_line("DEBUG STRUCT IDENTIFIER: Processing struct type '" + String(identifier) + "'"); - print_line("DEBUG STRUCT IDENTIFIER: Looking up in global wrapper registry with fqsn='" + String(in->get_datatype().struct_type->fqsn) + "'"); Ref<GDScriptStructClass> struct_wrapper = GDScriptLanguage::get_singleton()->get_struct_wrapper(in->get_datatype().struct_type->fqsn); if (struct_wrapper.is_valid()) { - print_line("DEBUG STRUCT IDENTIFIER: Found wrapper in global registry, wrapper=" + itos(uint64_t(struct_wrapper.ptr()))); - // Use the wrapper from global registry - // CRITICAL: Must return the CONSTANT address directly, not create a new address with CLASS mode - // CLASS mode points to stack[1] which is the script, NOT the constant table Variant wrapper_variant = struct_wrapper; GDScriptCodeGenerator::Address result = codegen.add_constant(wrapper_variant); - // Override the type to ensure it's recognized as STRUCT result.type = _gdtype_from_datatype(in->get_datatype(), codegen.script); - print_line("DEBUG STRUCT IDENTIFIER: Returning address: mode=" + itos(result.mode) + ", address=" + itos(result.address) + ", type.kind=" + itos(result.type.kind)); return result; - } else { - print_line("DEBUG STRUCT IDENTIFIER: ERROR - Wrapper not found in global registry for fqsn='" + String(in->get_datatype().struct_type->fqsn) + "'"); }modules/gdscript/gdscript_compiler.cpp-709-731 (1)
709-731: Remove debug print statements from struct static call handling.Similar to above, these debug statements should be removed before merging.
♻️ Remove debug print statements
- const GDScriptParser::IdentifierNode *id = static_cast<const GDScriptParser::IdentifierNode *>(subscript->base); - if (subscript->base->type == GDScriptParser::Node::IDENTIFIER) { - print_line("DEBUG compiler: Checking static call: " + String(id->name) + "." + String(call->function_name) + ", kind=" + itos(id->datatype.kind) + ", STRUCT=" + itos(GDScriptParser::DataType::STRUCT) + ", is_meta_type=" + itos(id->datatype.is_meta_type)); - } - // Check if this is a struct TYPE (not instance) for static method calls - // We need to check: - // 1. kind == STRUCT - // 2. is_meta_type == true (means it's the type itself, not an instance) - // This distinguishes TestStruct.new() (meta_type=true) from s.get_value() (meta_type=false) - if (!call->is_super && subscript->base->type == GDScriptParser::Node::IDENTIFIER && id->datatype.kind == GDScriptParser::DataType::STRUCT && id->datatype.is_meta_type) { - // Struct static method call (e.g., TestStruct.new()) - print_line("DEBUG STATIC CALL: Detected struct static call: " + String(id->name) + "." + String(call->function_name)); - print_line("DEBUG STATIC CALL: About to call _parse_expression on subscript->base"); + if (!call->is_super && subscript->base->type == GDScriptParser::Node::IDENTIFIER) { + const GDScriptParser::IdentifierNode *id = static_cast<const GDScriptParser::IdentifierNode *>(subscript->base); + if (id->datatype.kind == GDScriptParser::DataType::STRUCT && id->datatype.is_meta_type) { + // Struct static method call (e.g., TestStruct.new()) GDScriptCodeGenerator::Address base = _parse_expression(codegen, r_error, subscript->base); - print_line("DEBUG STATIC CALL: After _parse_expression, base address mode=" + itos(base.mode) + ", address=" + itos(base.address) + ", type.kind=" + itos(base.type.kind)); if (r_error) { - print_line("DEBUG STATIC CALL: ERROR after _parse_expression"); return GDScriptCodeGenerator::Address(); } - print_line("DEBUG STATIC CALL: About to write_call with function=" + String(call->function_name)); gen->write_call(result, base, call->function_name, arguments); - print_line("DEBUG STATIC CALL: After write_call"); - } else if (!call->is_super && subscript->base->type == GDScriptParser::Node::IDENTIFIER && GDScriptParser::get_builtin_type(static_cast<GDScriptParser::IdentifierNode *>(subscript->base)->name) < Variant::VARIANT_MAX) { - print_line("DEBUG compiler: Detected builtin static call: " + String(static_cast<GDScriptParser::IdentifierNode *>(subscript->base)->name) + "." + String(subscript->attribute->name)); + } else if (GDScriptParser::get_builtin_type(id->name) < Variant::VARIANT_MAX) { gen->write_call_builtin_type_static(result, GDScriptParser::get_builtin_type(static_cast<GDScriptParser::IdentifierNode *>(subscript->base)->name), subscript->attribute->name, arguments); + } + }modules/gdscript/gdscript_compiler.cpp-3198-3225 (1)
3198-3225: Remove debug print statements from _compile_struct.The function contains numerous debug print statements that should be removed before merging.
♻️ Remove debug print statements
Error GDScriptCompiler::_compile_struct(GDScript *p_script, const GDScriptParser::ClassNode *p_class, const GDScriptParser::StructNode *p_struct) { if (p_struct == nullptr) { return OK; } - print_line("DEBUG _compile_struct: Called for struct '" + p_struct->identifier->name + "', fqsn='" + p_struct->fqsn + "'"); // Check if struct is already compiled if (p_script->structs.has(p_struct->identifier->name)) { return OK; } // Also check if it exists globally (from a previous reload) GDScriptStruct *existing = GDScriptLanguage::get_singleton()->get_struct_by_name(p_struct->fqsn); if (existing) { // Already compiled globally, just add it to this script's struct list existing->reference(); // Take a reference for p_script->structs p_script->structs[p_struct->identifier->name] = existing; return OK; } - // Create the GDScriptStruct object - // CRITICAL: The constructor sets ref_count = 1, which is the compiler's initial ownership - // We do NOT call reference() here - the constructor provides the first reference GDScriptStruct *gdstruct = memnew(GDScriptStruct(p_struct->identifier->name)); - print_line("DEBUG _compile_struct: After memnew, ref_count=", itos(gdstruct->get_reference_count())); - // No reference() call needed - constructor already set ref_count = 1 for the compiler gdstruct->set_owner(p_script); gdstruct->set_fully_qualified_name(p_struct->fqsn);Continue removing all other
print_linedebug statements in this function (lines 3222, 3314, 3319, 3321, 3325, 3328, 3331, 3335, 3337, 3350, 3352).modules/gdscript/gdscript_analyzer.cpp-4262-4289 (1)
4262-4289: Struct member access/type creation: missing “ensure resolved” + meta-type attribute typing is wrong forPoint.newuse-cases
For instance field access (Line 4275+), you rely on
field.variable->datatype.is_set(); if struct body hasn’t been resolved yet, this can fail even for valid code. Consider callingresolve_struct_body(base.struct_type, p_identifier)(or similar) before reading field datatypes.For meta-type access (Line 4268+), setting the attribute’s datatype to
baseis not a good approximation:Point.new(without calling) should type asCallable, not “struct meta type”. Otherwise storing/passingPoint.newas a callable will be mistyped.In the
Member::STRUCTcase you build a freshDataTypemanually (Line 4394+). That’s OK, but it should be consistent with the engine’s struct typing (e.g., ensure any required fields likebuiltin_type/pseudo-typing are set the same way everywhere), and all dereferences ofmember.m_struct->identifiershould be null-guarded.Also applies to: 4394-4409
modules/gdscript/gdscript_analyzer.cpp-4303-4316 (1)
4303-4316: Avoid O(n) scans + null-deref in “struct takes precedence over class”This walks
parser->current_class->membersand dereferencesmember.m_struct->identifier->namewithout checkingidentifierfirst. Also, doing an O(n) scan for every identifier resolution is likely to show up in analyzer hot paths. Prefer a precomputed name→member map for structs (mirroringmembers_indices) if available, or add one.modules/gdscript/gdscript_analyzer.cpp-1914-1936 (1)
1914-1936: Struct context handling inresolve_function_signature()is inconsistent (override checks can still run for struct methods)
- The
_initspecial-case is correctly disabled for structs viaparser->current_struct != nullptr.- But the TOOLs “match parent signature” block is guarded by
if (parser->current_class != nullptr), which is still true while resolving struct signatures (you never clearcurrent_classin struct context). This contradicts the comment (“Skip this check for struct methods…”) and can produce bogus override errors/warnings for struct methods.Proposed fix (skip override checks when inside a struct)
- // Skip this check for struct methods since structs don't have polymorphism in the same way - if (parser->current_class != nullptr) { + // Skip this check for struct methods since structs don't have polymorphism in the same way. + if (parser->current_class != nullptr && parser->current_struct == nullptr) { GDScriptParser::DataType base_type = parser->current_class->base_type;Also applies to: 1964-2064
modules/gdscript/gdscript_analyzer.cpp-4538-4540 (1)
4538-4540: Remove hard-coded debug hook (TestStruct)This is effectively a stray breakpoint/log trigger in a hot path and will surprise users. Please remove it or gate behind
#ifdef DEBUG_ENABLED+ a runtime toggle.modules/gdscript/gdscript_analyzer.cpp-1611-1611 (1)
1611-1611: Remove/guardprint_line()debug spam across analyzer hot pathsThese debug traces are currently unconditional and will impact performance + user logs. If you need them, gate behind
#ifdef DEBUG_ENABLEDplus a dedicated toggle; otherwise remove.Also applies to: 1643-1649, 3694-3696, 3713-3715, 4404-4404, 4538-4540, 5960-5964
modules/gdscript/gdscript_analyzer.cpp-4936-4944 (1)
4936-4944:reduce_self()should key off struct-context, notcurrent_class == nullptrAs written,
selfwill likely still be considered valid inside structs becauseparser->current_classremains set during struct analysis; meanwhile, setting datatype to a default-constructedDataType()is ambiguous (“void” vs “unresolved”). Prefer checkingparser->current_struct != nullptrand setting a deterministic error type (e.g.,VARIANTwithUNDETECTED) after emitting the error.Proposed fix
- // Check if 'self' is being used outside of a class context (e.g., in a struct) - if (parser->current_class == nullptr) { + // Disallow `self` inside struct context. + if (parser->current_struct != nullptr) { p_self->is_constant = false; - // Emit a clear compile error: 'self' is not valid outside of class context - push_error(vformat(R"(Using "self" outside of a class context is not allowed.)"), p_self); - // Set type to void to indicate invalid usage - p_self->set_datatype(GDScriptParser::DataType()); + push_error(R"(Using "self" inside a struct is not allowed.)", p_self); + GDScriptParser::DataType dummy; + dummy.kind = GDScriptParser::DataType::VARIANT; + dummy.type_source = GDScriptParser::DataType::UNDETECTED; + p_self->set_datatype(dummy); return; }modules/gdscript/gdscript.cpp-3230-3242 (1)
3230-3242: Inconsistent reference counting: register_struct doesn't delete when ref_count reaches zero.When replacing an existing struct,
register_structcallsunreference()but doesn't check the return value ormemdeletethe struct if the reference count reaches zero. This is inconsistent withunregister_structwhich correctly deletes whenunreference()returns true (lines 3258-3262).The comment "don't delete even if ref_count reaches zero" contradicts the established reference counting pattern used elsewhere in the codebase.
🐛 Proposed fix
HashMap<String, GDScriptStruct *>::Iterator existing = global_structs.find(p_fully_qualified_name); if (existing) { - // Just release our reference to the old struct - don't delete even if ref_count reaches zero - // The struct may still be owned by other entities (script HashMap, wrappers, etc.) GDScriptStruct *old_struct = existing->value; if (old_struct) { - print_line("DEBUG register_struct: Found existing struct '" + p_fully_qualified_name + "' with ref_count=", itos(old_struct->get_reference_count())); - old_struct->unreference(); - print_line("DEBUG register_struct: After unreference old struct, ref_count=", itos(old_struct->get_reference_count())); + if (old_struct->unreference()) { + // Reference count reached zero - delete the struct + memdelete(old_struct); + } } global_structs.erase(p_fully_qualified_name); }
🧹 Nitpick comments (10)
modules/gdscript/gdscript_vm.cpp (1)
3866-3875: The struct reference counting is correctly handled, but the implementation should use the VariantTypeAdjust pattern for consistency.
VariantInternal::clear()properly decrements the reference count by invokingVariant::_clear_internal(), which callsGDScriptStructInstance::unreference()for STRUCT types (with reference count validation in debug builds). This correctly mirrors the OBJECT type adjustment.However, the manual implementation deviates from the established pattern. Rather than duplicating OBJECT's logic, create a
VariantTypeAdjust<GDScriptStructInstance*>specialization and use theOPCODE_TYPE_ADJUST(STRUCT, GDScriptStructInstance*)macro. This maintains consistency across all type adjustments and avoids the redundant assignment afterclear().Current implementation:
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); *arg = (GDScriptStructInstance *)nullptr; ip += 2; }Should mirror:
VariantTypeAdjust<Object *> { VariantInternal::clear(r_ret); *r_ret = (Object *)nullptr; }modules/gdscript/gdscript_byte_codegen.cpp (1)
113-118: STRUCT temporaries: comment/behavior mismatch risk—verify intended “no pool” semantics.
Variant::STRUCTfalls through toVARIANT_MAXand setstemp_type = Variant::NIL, so it still uses the NIL pool. That may be fine (it avoids typed pooling), but the comment reads like “don’t pool at all”. Consider clarifying the comment (or introduce a dedicated non-pooled path if that was the intent).modules/gdscript/gdscript_struct.cpp (1)
330-379: Method lookup usesget()then takes address; prefergetptr()to avoid any chance of referencing a temporary + reduce double lookups.
This is small but removes ambiguity and extra hashing.Proposed refactor (use getptr)
const GDScriptStruct::MethodInfo *method_info = nullptr; -// Check current struct -if (struct_type->get_methods().has(p_method)) { - method_info = &struct_type->get_methods().get(p_method); -} +method_info = struct_type->get_methods().getptr(p_method); // Check base struct if not found if (method_info == nullptr && struct_type->get_base_struct() != nullptr) { GDScriptStruct *base = struct_type->get_base_struct(); while (base != nullptr) { - if (base->get_methods().has(p_method)) { - method_info = &base->get_methods().get(p_method); + if ((method_info = base->get_methods().getptr(p_method))) { break; } base = base->get_base_struct(); } }modules/gdscript/gdscript_compiler.cpp (2)
3286-3308: Error path cleanup logic is correct but verify ref_count check.The error handling correctly unreferences and deletes the struct when compilation fails. However, the
get_reference_count() == 0check afterunreference()is redundant ifunreference()returnstruewhen ref_count reaches zero (as the method signature suggests).♻️ Simplify error path cleanup
if (err) { - // ERROR PATH: Clean up properly with RAII-style unwinding - // At this point, only the compiler's reference exists (ref_count = 1) - // The struct hasn't been added to p_script->structs or global registry yet - gdstruct->unreference(); // Release compiler's reference (ref_count goes 1 -> 0) - - // Verify ref_count is 0 (should be, since we had the only reference) - if (gdstruct->get_reference_count() == 0) { - memdelete(gdstruct); // Safe to delete now - } else { - ERR_PRINT(vformat("Struct '%s' has unexpected references after compilation error: ref_count = %d", - p_struct->identifier->name, gdstruct->get_reference_count())); - } + // ERROR PATH: Only the compiler's reference exists (ref_count = 1) + if (gdstruct->unreference()) { + memdelete(gdstruct); + } return err; }
3339-3355: Reference counting summary is helpful but should be a code comment, not debug output.The detailed reference counting summary is valuable documentation. However, the debug print statements should be removed while keeping the explanatory comments.
♻️ Keep comments, remove debug prints
// REFERENCE COUNTING SUMMARY: // After successful compilation, gdstruct has 4 references: // 1. Constructor/Compiler (released at function end) // 2. p_script->structs HashMap (released when script is cleared/struct is removed) // 3. Global struct registry (released when GDScriptLanguage::unregister_struct is called) // 4. struct_wrapper in global wrapper registry (released when wrapper is unregistered) // // Total: 4 references (constructor + script + registry + wrapper) // // We must explicitly release the compiler's reference (ref #1) before returning. // The struct stays alive because the other 3 references remain. - print_line("DEBUG _compile_struct: Before final unreference, ref_count=", itos(gdstruct->get_reference_count())); - gdstruct->unreference(); // Release compiler's reference (ref_count: 4 -> 3) - print_line("DEBUG _compile_struct: After final unreference, ref_count=", itos(gdstruct->get_reference_count())); + gdstruct->unreference(); // Release compiler's reference return OK;modules/gdscript/gdscript_analyzer.cpp (2)
1263-1272:resolve_class_member()does not actually resolve structs (only name-conflicts); ensure callers rely onresolve_struct_body()The added
Member::STRUCTcase only runs conflict checks and exits. That’s OK if intentional, but then anything that expects struct fields/method signatures resolved must go throughresolve_struct_body()(and notresolve_class_member()), otherwise you’ll get “field type not set” style failures later.
1367-1371: Struct resolution entry points look consistent (interface/body/node), but pass a usefulp_sourcefor better diagnosticsRight now the interface path calls
resolve_struct_body(member.m_struct)without a source node. Preferresolve_struct_body(member.m_struct, /*p_source=*/p_class)or the member’s source node if available, so errors point somewhere actionable.Also applies to: 1599-1601, 1677-1681
modules/gdscript/gdscript.cpp (3)
240-245: Remove debug print statements before production release.These debug print statements should be removed or wrapped in
#ifdef DEBUG_ENABLEDguards to avoid performance overhead and console spam in production builds.♻️ Proposed fix
Variant result; result.type = Variant::STRUCT; // Copy the pointer into Variant's internal storage // memcpy is used instead of assignment for type safety and to avoid strict aliasing issues - print_line("DEBUG _variant_from_struct_instance: p_instance (value) = " + itos((uint64_t)p_instance)); - print_line("DEBUG _variant_from_struct_instance: &p_instance (address) = " + itos((uint64_t)&p_instance)); memcpy(result._data._mem, &p_instance, sizeof(void *)); - GDScriptStructInstance *copied_ptr = *(GDScriptStructInstance **)result._data._mem; - print_line("DEBUG _variant_from_struct_instance: after memcpy, copied_ptr = " + itos((uint64_t)copied_ptr)); - print_line("DEBUG _variant_from_struct_instance: about to return, result._data._mem ptr = " + itos(*(uint64_t *)result._data._mem)); return result;
261-262: Remove debug print statements before production release.These debug print statements should be removed or wrapped in
#ifdef DEBUG_ENABLEDguards.♻️ Proposed fix
GDScriptStructInstance *instance = struct_type->create_instance(p_args, p_argcount); if (!instance) { r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD; return Variant(); } - print_line("DEBUG _new: Created struct instance at " + itos((uint64_t)instance)); - print_line("DEBUG _new: ref_count after creation = " + itos(instance->get_reference_count())); - // REFERENCE COUNTING MANAGEMENT:
3237-3239: Remove debug print statements before production release.The
register_structmethod contains debug print statements that should be removed or wrapped in#ifdef DEBUG_ENABLEDguards to avoid console spam in production.♻️ Proposed fix
HashMap<String, GDScriptStruct *>::Iterator existing = global_structs.find(p_fully_qualified_name); if (existing) { - // Just release our reference to the old struct - don't delete even if ref_count reaches zero - // The struct may still be owned by other entities (script HashMap, wrappers, etc.) GDScriptStruct *old_struct = existing->value; if (old_struct) { - print_line("DEBUG register_struct: Found existing struct '" + p_fully_qualified_name + "' with ref_count=", itos(old_struct->get_reference_count())); old_struct->unreference(); - print_line("DEBUG register_struct: After unreference old struct, ref_count=", itos(old_struct->get_reference_count())); } global_structs.erase(p_fully_qualified_name); } // Take a reference for the registry's ownership p_struct->reference(); - print_line("DEBUG register_struct: Added new struct '" + p_fully_qualified_name + "' with ref_count=", itos(p_struct->get_reference_count())); global_structs.insert(p_fully_qualified_name, p_struct);Also applies to: 3246-3246
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
core/object/object.cppcore/variant/variant.cppcore/variant/variant_setget.cppmodules/gdscript/gdscript.cppmodules/gdscript/gdscript.hmodules/gdscript/gdscript_analyzer.cppmodules/gdscript/gdscript_byte_codegen.cppmodules/gdscript/gdscript_byte_codegen.hmodules/gdscript/gdscript_compiler.cppmodules/gdscript/gdscript_compiler.hmodules/gdscript/gdscript_struct.cppmodules/gdscript/gdscript_vm.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/gdscript/gdscript_compiler.h
🧰 Additional context used
🧬 Code graph analysis (8)
modules/gdscript/gdscript_byte_codegen.h (3)
core/string/print_string.h (1)
print_line(74-76)modules/gdscript/gdscript.h (2)
String(642-662)GDScriptStructClass(66-86)modules/gdscript/gdscript.cpp (2)
GDScriptStructClass(133-138)GDScriptStructClass(140-147)
core/object/object.cpp (2)
core/object/object.h (3)
get_class_name(851-880)StringName(758-760)StringName(983-991)core/string/print_string.h (1)
print_line(74-76)
modules/gdscript/gdscript_vm.cpp (2)
core/string/print_string.h (1)
print_line(74-76)modules/gdscript/gdscript.cpp (2)
clear(1713-1824)clear(1713-1713)
core/variant/variant_setget.cpp (2)
core/variant/variant.cpp (34)
type(1554-1562)type(1576-1584)type(1792-1808)type(1810-1826)type(1828-1836)type(1838-1846)type(1848-1864)type(1866-1882)type(1884-1900)type(1902-1919)type(1921-1927)type(1929-1935)type(1937-1947)type(1949-1959)type(1961-1983)type(1985-2007)get_type_name(46-181)get_type_name(46-46)Variant(2352-2355)Variant(2357-2360)Variant(2362-2365)Variant(2367-2370)Variant(2372-2375)Variant(2377-2380)Variant(2382-2385)Variant(2387-2390)Variant(2392-2395)Variant(2397-2400)Variant(2402-2405)Variant(2407-2410)Variant(2412-2416)Variant(2418-2422)Variant(2424-2428)Variant(2430-2434)core/string/print_string.h (1)
print_line(74-76)
modules/gdscript/gdscript_analyzer.cpp (1)
modules/gdscript/gdscript_parser.h (3)
DataType(104-264)DataType(175-178)DataType(180-185)
modules/gdscript/gdscript_struct.cpp (2)
core/variant/variant.cpp (18)
reference(1146-1331)reference(1146-1146)can_convert(195-536)can_convert(195-195)Variant(2352-2355)Variant(2357-2360)Variant(2362-2365)Variant(2367-2370)Variant(2372-2375)Variant(2377-2380)Variant(2382-2385)Variant(2387-2390)Variant(2392-2395)Variant(2397-2400)Variant(2402-2405)Variant(2407-2410)Variant(2412-2416)Variant(2418-2422)core/variant/variant_utility.cpp (2)
type_convert(868-955)type_convert(868-868)
modules/gdscript/gdscript_compiler.cpp (2)
modules/gdscript/gdscript_struct.cpp (4)
call(330-379)call(330-330)GDScriptStruct(41-47)GDScriptStruct(49-50)modules/gdscript/gdscript_struct.h (1)
GDScriptStruct(47-108)
modules/gdscript/gdscript.cpp (3)
modules/gdscript/gdscript.h (4)
GDScriptStructClass(66-86)Variant(382-624)Variant(622-622)Ref(440-623)modules/gdscript/gdscript_struct.cpp (2)
GDScriptStructInstance(231-272)GDScriptStructInstance(274-277)modules/gdscript/gdscript_struct.h (1)
GDScriptStructInstance(140-181)
🔇 Additional comments (13)
modules/gdscript/gdscript_vm.cpp (2)
36-36: LGTM!The include is necessary to support struct operations in the VM runtime.
412-412: LGTM!The jump table entry is correctly placed after the other TYPE_ADJUST opcodes, and the static_assert on line 418 will validate that the table size matches the opcode enum.
core/variant/variant.cpp (1)
1318-1327: [Rewritten review comment]
[Classification tag]modules/gdscript/gdscript_byte_codegen.cpp (1)
584-588: VM and disassembler support confirmed for OPCODE_TYPE_ADJUST_STRUCT.Both the VM bytecode dispatch handler (with struct adjustment logic) and disassembler case have been implemented. The opcode is properly integrated in the enum, dispatch table, VM execution handler at line 3866, and disassembler at line 1306. No risk of "opcode exists but not decoded" regression.
modules/gdscript/gdscript.h (3)
66-86: GDScriptStructClass wrapper class declaration looks correct.The wrapper class provides a clean interface for struct construction via
StructName.new(). The class properly inherits fromRefCountedfor automatic memory management of the wrapper itself.
510-513: Global struct registries for serialization support.The dual registry approach (raw pointers for structs,
Ref<>for wrappers) requires careful synchronization during registration and unregistration to avoid dangling references.
103-111: ClearData struct properly extended for struct cleanup.The addition of
RBSet<GDScriptStruct *> structsallows tracking structs for deferred cleanup, andGDScript::clear()properly unreferences these structs before deletion (lines 1774-1777 of gdscript.cpp callunreference()on each struct, withmemdelete()invoked if the reference count reaches zero).modules/gdscript/gdscript_compiler.cpp (4)
3061-3095: Struct compilation integration in _compile_class looks correct.The struct compilation is properly integrated into the class compilation flow, handling STRUCT members alongside FUNCTION and VARIABLE members.
198-202: STRUCT case in _gdtype_from_datatype is correct.The new STRUCT case properly sets the kind and builtin_type for struct data types.
3326-3337:set_struct_typedoes not take a reference—verify reference counting logic.The implementation in
modules/gdscript/gdscript.cpp:149-159shows thatset_struct_type()does NOT callreference()on the incomingp_structparameter. It only unreferences the old struct and assigns the new pointer directly. This contradicts the inline comment claiming it "takes another reference." The reference counting for the struct may be unbalanced if the caller expectsset_struct_type()to adopt ownership via reference counting.
3311-3321: The inline comment is correct. At line 3245 inmodules/gdscript/gdscript.cpp,register_struct()explicitly callsp_struct->reference()before storing the struct in the global registry, confirming the comment's claim. The reference counting is properly balanced: the script HashMap takes one reference (line 3315), and the global registry takes another viaregister_struct()(line 3320), which is correctly managed throughunregister_struct()callingunreference().modules/gdscript/gdscript_analyzer.cpp (1)
869-872: The struct resolution ordering is correct—resolve_struct_body()is called during the interface resolution phase (line 1370 inresolve_class_interface()), which executes beforeresolve_body()in the mainanalyze()flow. This ensures struct types are resolved before they're used as datatypes inresolve_datatype(). The implementation at lines 1268–1270 documents this pattern explicitly. The 1-argument call site at line 1370 works because the header declares the default parameterp_source = nullptr.modules/gdscript/gdscript.cpp (1)
1764-1782: LGTM! Struct cleanup properly implements reference counting.This cleanup logic correctly addresses the memory leak issue described in the PR objectives. The three-step cleanup process properly:
- Removes the constant wrapper (releasing wrapper's reference)
- Unregisters from global registry (releasing registry's reference)
- Releases the script HashMap's ownership reference and deletes when ref_count reaches zero
The pattern matches
unregister_structimplementation (lines 3254-3265) and ensures structs are cleaned up regardless ofis_rootstatus, fixing the leak where structs were moved toclear_data->structsbut never unreferenced.
… defaulting to dict.
| case Variant::Type::OBJECT: | ||
| return p_variant.operator Object *(); | ||
| case Variant::Type::STRUCT: | ||
| // TODO: Implement struct conversion |
There was a problem hiding this comment.
When will this be implemented?
| r_symbol.children.push_back(symbol); | ||
| } break; | ||
| case ClassNode::Member::STRUCT: | ||
| // TODO: Handle struct symbols |
There was a problem hiding this comment.
When will this be implemented?
| } | ||
| } break; | ||
| case ClassNode::Member::STRUCT: | ||
| // TODO: Handle struct API dumping |
There was a problem hiding this comment.
When will this be implemented?
| r_type.type.is_meta_type = true; | ||
| return true; | ||
| case GDScriptParser::ClassNode::Member::STRUCT: | ||
| // TODO: Handle struct type inference |
There was a problem hiding this comment.
When will this be implemented?
| r_result.type = ScriptLanguage::LOOKUP_RESULT_CLASS_SIGNAL; | ||
| break; | ||
| case GDScriptParser::ClassNode::Member::STRUCT: | ||
| // TODO: Handle struct symbol lookup |
There was a problem hiding this comment.
When will this be implemented?
| return ERR_CANT_RESOLVE; | ||
| } break; | ||
| case GDScriptParser::DataType::STRUCT: | ||
| // TODO: Handle struct symbol lookup |
There was a problem hiding this comment.
When will this be implemented?
| if (p_variant.get_type() != Variant::STRUCT) { | ||
| return false; | ||
| } | ||
| // TODO: Check struct type matching |
There was a problem hiding this comment.
When will this be implemented?
| const GDScriptStructInstance *wrapper = reinterpret_cast<const GDScriptStructInstance *>(_data._mem); | ||
| if (wrapper && wrapper->is_valid()) { | ||
| // Simple string representation for now to avoid crashes | ||
| // TODO: Add proper field serialization |
There was a problem hiding this comment.
When will this be implemented?
There was a problem hiding this comment.
Simply put, all of the TODOSs will need someone willing to fix them we're just trying to get a mergeable functional POC because the longer we wait the harder it is going to be to merge
There was a problem hiding this comment.
Simply put, all of the TODOSs will need someone willing to fix them we're just trying to get a mergeable functional POC because the longer we wait the harder it is going to be to merge
Makes sense
Shakai-Dev
left a comment
Shakai-Dev
left a comment
There was a problem hiding this comment.
LGTM but at some point, we do need to fix those TODO comments
4 participants
This adds Structs to GDScript
Status
Functional POC
Tasks:
Refactoring this to fit Godot's COW model. It shouldn't be anymore heavy than the Variant based Refcounted instance, and solve the fundamental issue.This will need extensive debug, testing and cleanup. but is now functional
Summary by CodeRabbit
Summary
New Features
Breaking Changes
Bug Fixes / Behavior Changes
Tests