Skip to content

Commit d9a1469

Browse files
committed
Assume never-changing element type after init
Previously, the `ElementType::Untyped` variant was always re-queried, because someone (another GDExtension) could call C functions `array_set_typed` or `dictionary_set_typed` after the fact. This is exceedingly unlikely though, and arguably bad practice. This changes behavior to only assert in Debug mode, and don't re-query via FFI.
1 parent 5bb272e commit d9a1469

File tree

3 files changed

+83
-76
lines changed

3 files changed

+83
-76
lines changed

godot-core/src/builtin/collections/array.rs

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
66
*/
77

8-
use std::cell::Cell;
8+
use std::cell::OnceCell;
99
use std::marker::PhantomData;
1010
use std::{cmp, fmt};
1111

@@ -144,10 +144,7 @@ pub struct Array<T: ArrayElement> {
144144
_phantom: PhantomData<T>,
145145

146146
/// Lazily computed and cached element type information.
147-
///
148-
/// `ElementType::Untyped` means either "not yet queried" or "queried but array was untyped". Since GDScript can call
149-
/// `set_type()` at any time, we must re-query FFI whenever cached value is `Untyped`.
150-
cached_element_type: Cell<ElementType>,
147+
cached_element_type: OnceCell<ElementType>,
151148
}
152149

153150
/// Guard that can only call immutable methods on the array.
@@ -190,7 +187,7 @@ impl<T: ArrayElement> Array<T> {
190187
Self {
191188
opaque,
192189
_phantom: PhantomData,
193-
cached_element_type: Cell::new(ElementType::Untyped),
190+
cached_element_type: OnceCell::new(),
194191
}
195192
}
196193

@@ -997,9 +994,12 @@ impl<T: ArrayElement> Array<T> {
997994

998995
/// Returns the runtime element type information for this array.
999996
///
1000-
/// The result is cached when the array is typed. If the array is untyped, this method
1001-
/// will always re-query Godot's FFI since GDScript may call `set_type()` at any time.
1002-
/// Repeated calls on typed arrays will not result in multiple Godot FFI roundtrips.
997+
/// The result is generally cached, so feel free to call this method repeatedly.
998+
///
999+
/// # Panics (Debug)
1000+
/// In the astronomically rare case where another extension in Godot modifies an array's type (which godot-rust already cached as `Untyped`)
1001+
/// via C function `array_set_typed`, thus leading to incorrect cache values. Such bad practice of not typing arrays immediately on
1002+
/// construction is not supported, and will not be checked in Release mode.
10031003
pub fn element_type(&self) -> ElementType {
10041004
ElementType::get_or_compute_cached(
10051005
&self.cached_element_type,
@@ -1041,13 +1041,18 @@ impl<T: ArrayElement> Array<T> {
10411041
/// Sets the type of the inner array.
10421042
///
10431043
/// # Safety
1044-
///
10451044
/// Must only be called once, directly after creation.
10461045
unsafe fn init_inner_type(&mut self) {
10471046
debug_assert!(self.is_empty());
1048-
debug_assert!(!self.element_type().is_typed());
1047+
debug_assert!(
1048+
self.cached_element_type.get().is_none(),
1049+
"init_inner_type() called twice"
1050+
);
10491051

1052+
// Immediately set cache to static type.
10501053
let elem_ty = ElementType::of::<T>();
1054+
let _ = self.cached_element_type.set(elem_ty);
1055+
10511056
if elem_ty.is_typed() {
10521057
let script = Variant::nil();
10531058

godot-core/src/builtin/collections/dictionary.rs

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
66
*/
77

8-
use std::cell::Cell;
8+
use std::cell::OnceCell;
99
use std::marker::PhantomData;
1010
use std::{fmt, ptr};
1111

@@ -83,24 +83,18 @@ pub struct Dictionary {
8383
opaque: OpaqueDictionary,
8484

8585
/// Lazily computed and cached element type information for the key type.
86-
///
87-
/// `ElementType::Untyped` means either "not yet queried" or "queried but array was untyped". Since GDScript can call
88-
/// `set_type()` at any time, we must re-query FFI whenever cached value is `Untyped`.
89-
cached_key_type: Cell<ElementType>,
86+
cached_key_type: OnceCell<ElementType>,
9087

91-
/// Lazily computed and cached element type information for the key type.
92-
///
93-
/// `ElementType::Untyped` means either "not yet queried" or "queried but array was untyped". Since GDScript can call
94-
/// `set_type()` at any time, we must re-query FFI whenever cached value is `Untyped`.
95-
cached_value_type: Cell<ElementType>,
88+
/// Lazily computed and cached element type information for the value type.
89+
cached_value_type: OnceCell<ElementType>,
9690
}
9791

9892
impl Dictionary {
9993
fn from_opaque(opaque: OpaqueDictionary) -> Self {
10094
Self {
10195
opaque,
102-
cached_key_type: Cell::new(ElementType::Untyped),
103-
cached_value_type: Cell::new(ElementType::Untyped),
96+
cached_key_type: OnceCell::new(),
97+
cached_value_type: OnceCell::new(),
10498
}
10599
}
106100

@@ -423,8 +417,12 @@ impl Dictionary {
423417
///
424418
/// Provides information about Godot typed dictionaries, even though godot-rust currently doesn't implement generics for those.
425419
///
426-
/// The caching strategy is best-effort. In the current implementation, untyped dictionaries are always re-queried over FFI, since
427-
/// it's possible to call `set_key_type()`/`set_value_type()` in Godot. Once typed, the value is cached.
420+
/// The result is generally cached, so feel free to call this method repeatedly.
421+
///
422+
/// # Panics (Debug)
423+
/// In the astronomically rare case where another extension in Godot modifies a dictionary's key type (which godot-rust already cached as `Untyped`)
424+
/// via C function `dictionary_set_typed`, thus leading to incorrect cache values. Such bad practice of not typing dictionaries immediately on
425+
/// construction is not supported, and will not be checked in Release mode.
428426
#[cfg(since_api = "4.4")]
429427
pub fn key_element_type(&self) -> ElementType {
430428
ElementType::get_or_compute_cached(
@@ -439,9 +437,12 @@ impl Dictionary {
439437
///
440438
/// Provides information about Godot typed dictionaries, even though godot-rust currently doesn't implement generics for those.
441439
///
442-
/// The result is cached when the dictionary values are typed. If the values are untyped, this method
443-
/// will always re-query Godot's FFI since GDScript may call `set_type()` at any time.
444-
/// Repeated calls on typed dictionaries will not result in multiple Godot FFI roundtrips.
440+
/// The result is generally cached, so feel free to call this method repeatedly.
441+
///
442+
/// # Panics (Debug)
443+
/// In the astronomically rare case where another extension in Godot modifies a dictionary's value type (which godot-rust already cached as `Untyped`)
444+
/// via C function `dictionary_set_typed`, thus leading to incorrect cache values. Such bad practice of not typing dictionaries immediately on
445+
/// construction is not supported, and will not be checked in Release mode.
445446
#[cfg(since_api = "4.4")]
446447
pub fn value_element_type(&self) -> ElementType {
447448
ElementType::get_or_compute_cached(

godot-core/src/meta/element_type.rs

Lines changed: 49 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -81,33 +81,26 @@ impl ElementType {
8181
}
8282
}
8383

84-
/// Transfer cached element type from source to destination, preserving more specific type info.
84+
/// Transfer cached element type from source to destination, preserving type info.
8585
///
86-
/// This is a helper for cloning operations like duplicate(), slice(), etc. where we want to
87-
/// preserve cached type information to avoid redundant FFI calls. Only transfers if the source
88-
/// has more specific information than the destination (typed vs untyped).
86+
/// Used by clone-like operations like `duplicate()`, `slice()`, etc. where we want to preserve cached type information to avoid
87+
/// redundant FFI calls. Only transfers if the source has computed info and destination doesn't.
8988
pub(crate) fn transfer_cache(
90-
source_cache: &std::cell::Cell<ElementType>,
91-
dest_cache: &std::cell::Cell<ElementType>,
89+
source_cache: &std::cell::OnceCell<ElementType>,
90+
dest_cache: &std::cell::OnceCell<ElementType>,
9291
) {
93-
let source_value = source_cache.get();
94-
let dest_value = dest_cache.get();
95-
96-
// Only transfer if source has more specific info (typed) than destination (untyped)
97-
match (source_value, dest_value) {
98-
// Source is typed, destination is untyped - transfer the typed info
99-
(source, ElementType::Untyped) if !matches!(source, ElementType::Untyped) => {
100-
dest_cache.set(source);
101-
}
102-
// All other cases: don't transfer (dest already has same or better info)
103-
_ => {}
92+
if let Some(source_value) = source_cache.get() {
93+
// Ignore result. If dest is already set, that's fine.
94+
let _ = dest_cache.set(*source_value);
10495
}
10596
}
10697

10798
/// Get element type from cache or compute it via FFI calls.
10899
///
109-
/// Common caching and computation pattern for element type computation. Checks cache first,
110-
/// returns cached value if typed, otherwise computes via FFI and caches the result.
100+
/// Returns cached value if available, otherwise computes via FFI and caches the result.
101+
///
102+
/// In Debug mode, validates cached `Untyped` values to ensure another extension hasn't made an array/dictionary typed via C functions
103+
/// `set_array_type`/`set_dictionary_type` after caching.
111104
///
112105
/// Takes closures for the three FFI operations needed to determine element type:
113106
/// - `get_builtin_type`: Get the variant type (sys variant type as i64)
@@ -116,42 +109,50 @@ impl ElementType {
116109
///
117110
/// Returns the computed `ElementType` and updates the cache.
118111
pub(crate) fn get_or_compute_cached(
119-
cache: &std::cell::Cell<ElementType>,
112+
cache: &std::cell::OnceCell<ElementType>,
120113
get_builtin_type: impl Fn() -> i64,
121114
get_class_name: impl Fn() -> crate::builtin::StringName,
122115
get_script_variant: impl Fn() -> crate::builtin::Variant,
123116
) -> ElementType {
124-
let cached = cache.get();
125-
126-
// Already cached and typed: won't change anymore.
127-
if !matches!(cached, ElementType::Untyped) {
128-
return cached;
129-
}
130-
131-
// Untyped or not queried yet - compute via FFI.
132-
let sys_variant_type = get_builtin_type();
133-
let variant_type =
134-
VariantType::from_sys(sys_variant_type as crate::sys::GDExtensionVariantType);
135-
136-
let element_type = if variant_type == VariantType::NIL {
137-
ElementType::Untyped
138-
} else if variant_type == VariantType::OBJECT {
139-
let class_name_stringname = get_class_name();
140-
let class_name = ClassName::new_dynamic(class_name_stringname.to_string());
141-
142-
// If there's a script associated, the class is interpreted as the native base class of the script.
143-
let script_variant = get_script_variant();
144-
if let Some(script) = Self::script_from_variant(&script_variant) {
145-
ElementType::ScriptClass(ElementScript::new(script))
117+
let cached = *cache.get_or_init(|| {
118+
let sys_variant_type = get_builtin_type();
119+
let variant_type =
120+
VariantType::from_sys(sys_variant_type as crate::sys::GDExtensionVariantType);
121+
122+
if variant_type == VariantType::NIL {
123+
ElementType::Untyped
124+
} else if variant_type == VariantType::OBJECT {
125+
let class_name_stringname = get_class_name();
126+
let class_name = ClassName::new_dynamic(class_name_stringname.to_string());
127+
128+
// If there's a script associated, the class is interpreted as the native base class of the script.
129+
let script_variant = get_script_variant();
130+
if let Some(script) = Self::script_from_variant(&script_variant) {
131+
ElementType::ScriptClass(ElementScript::new(script))
132+
} else {
133+
ElementType::Class(class_name)
134+
}
146135
} else {
147-
ElementType::Class(class_name)
136+
ElementType::Builtin(variant_type)
148137
}
149-
} else {
150-
ElementType::Builtin(variant_type)
151-
};
138+
});
139+
140+
// Debug validation for cached Untyped values.
141+
#[cfg(debug_assertions)]
142+
if matches!(cached, ElementType::Untyped) {
143+
let sys_variant_type = get_builtin_type();
144+
let variant_type =
145+
VariantType::from_sys(sys_variant_type as crate::sys::GDExtensionVariantType);
146+
147+
assert_eq!(
148+
variant_type,
149+
VariantType::NIL,
150+
"Array/Dictionary element type validation failed: cached as Untyped but FFI reports {variant_type:?}. \
151+
This indicates that another extension modified the type after godot-rust cached it.",
152+
);
153+
}
152154

153-
cache.set(element_type);
154-
element_type
155+
cached
155156
}
156157

157158
/// Convert a script variant to a `Gd<Script>`, or `None` if nil.

0 commit comments

Comments
 (0)