Add PhantomVar<T> to support properties without a backing field by ttencate · Pull Request #1261 · godot-rust/gdext

ttencate · 2025-08-05T13:27:02Z

Closes #1151.

GodotRust · 2025-08-05T13:41:18Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1261

Bromeon

Fantastic work! Solid implementation, great docs and examples, great tests.
Thanks a lot! 💪

Bromeon · 2025-08-05T19:57:40Z

godot-core/src/obj/phantom_var.rs

+// `PhantomVar` does not _really_ implement `Var`, but it has to, otherwise we cannot implement `Export` either.
+// The `GodotClass` derive macro should ensure that the `Var` implementation is not used.
+impl<T> Var for PhantomVar<T>
+where
+    T: GodotType + Var,
+{
+    fn get_property(&self) -> Self::Via {
+        unreachable!("PhantomVar::get_property should never have been called")
+    }
+
+    fn set_property(&mut self, _value: Self::Via) {
+        unreachable!("PhantomVar::set_property should never have been called");
+    }
+
+    fn var_hint() -> PropertyHintInfo {
+        <T as Var>::var_hint()
+    }
+}


The var_hint part is used though, no? Just get/set_property aren't useful.
So, part of Var is implemented 🤔

Also, "PhantomVar::set_property should never have been called" is somewhat implied by unreachable!... you can maybe just use unreachable!() or add some extra information, like "property getters/setters should be directly called by proc-macro, not through Var".

Updated comment and strings.

Bromeon · 2025-08-05T20:48:08Z

godot-core/src/obj/phantom_var.rs

+impl<T> Default for PhantomVar<T> {
+    fn default() -> Self {
+        Self(Default::default())
+    }
+}
+
+impl<T> Clone for PhantomVar<T> {
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+
+impl<T> Copy for PhantomVar<T> {}
+
+impl<T> fmt::Debug for PhantomVar<T> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        f.debug_tuple("PhantomVar").finish()
+    }
+}
+
+impl<T> PartialEq for PhantomVar<T> {
+    fn eq(&self, _other: &Self) -> bool {
+        true
+    }
+}
+
+impl<T> Eq for PhantomVar<T> {}
+
+impl<T> PartialOrd for PhantomVar<T> {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl<T> Ord for PhantomVar<T> {
+    fn cmp(&self, _other: &Self) -> Ordering {
+        Ordering::Equal
+    }
+}
+
+impl<T> Hash for PhantomVar<T> {
+    fn hash<H: std::hash::Hasher>(&self, _state: &mut H) {}
+}


Can any of these be derived, or does it cause problems due to the generic parameter?

Exactly. The stdlib's derive macros always add a bound T: TraitWeAreDeriving even if it's not strictly needed. This is why crates like derive_where exist.

godot-core/src/obj/phantom_var.rs

godot-macros/src/class/data_models/property.rs

Bromeon · 2025-08-05T21:00:09Z

godot-macros/src/class/derive_godot_class.rs

+                if let Some(field_var) = &mut field.var {
+                    if field_var.getter.is_generated() {
+                        errors.push(error!(
+                            field_var.span,
+                            "PhantomVar<T> stores no data, so it cannot use an autogenerated getter"
+                        ));
+                    }
+                    if field_var.setter.is_generated() {
+                        errors.push(error!(
+                            field_var.span,
+                            "PhantomVar<T> stores no data, so it cannot use an autogenerated setter"
+                        ));
+                    }
+
+                    if field_var.getter.is_omitted() && field_var.setter.is_omitted() {
+                        errors.push(error!(
+                            field_var.span,
+                            "PhantomVar<T> is useless without a custom getter, setter, or both"
+                        ));
+                    }
+                } else {
+                    errors.push(error!(
+                        field.span,
+                        "PhantomVar<T> field is useless without attribute #[var]"
+                    ));
+                }


Maybe extract into a function, e.g. validate_phantomvar(...)

OK but then I'll also extract validate_base_field for the code right above this.

Bromeon · 2025-08-05T21:01:53Z

itest/godot/ManualFfiTests.gd

+# This test is commented out, because Godot seems to allow reading a write-only property, and simply returns null.
+# func test_phantom_var_reading_write_only():
+# 	# This must be untyped, otherwise the parser complains about our invalid read.
+# 	var obj = HasPhantomVar.new()
+# 	expect_fail()
+# 	var x = obj.write_only
+# 	assert_fail("HasPhantomVar.write_only should not be readable")


Do you believe this behavior will change in the future?

If not, we should maybe rather test the current behavior, and document why we can't have write-only. If semantics on Godot's side ever change in that regard, at least we'll notice...

Couldn't we implement a custom getter that panics instead when no_get is specified?

Couldn't we implement a custom getter that panics instead when no_get is specified?

as far as I'm aware the editor must be able to read given property (in case of #[export]), so it can't really panic.

Couldn't we return default values for a type? Is that even more unsound? Right now you get null anyway, so what would be wrong in doing push_error + return null / default value if type is non-nullable?

Wait, how would the editor even read a write only property? Hm, I suppose that would push an error every time the node's inspector is open? We can make it push_warning instead, or disable it when #[var(no_get, set)]#[export]. I would prefer push error if no editor, push warning if editor and add a bit of "Ignore this message if you haven't used the getter from code, the editor is trying to fetch the new value" or something.

I can think of two use cases for write-only properties:

Setting a password, which is immediately hashed so it can never be recovered.

Setting a seed on a random number generator, which becomes meaningless after changing the RNG state. Godot's RandomNumberGenerator actually allows you to read the seed back again but has some warnings in the docs about it.

Both use cases are better served by methods, I think.

As for #[export], it also has the effect of serializing the value, which can never work if the editor can't read it back. So exported write-only properties are also a bit of a contradiction.

Let me just make write-only PhantomVar properties an error for now.

(None of this is specific to PhantomVar by the way, but I don't want to break backwards compatibility yet!)

Do I open an issue for write-only properties and their soundness? And there still has to be the rework on [no_get, get, no_set, set] done, right?

If you have a concrete use case for write-only properties, you could open an issue to investigate. I personally don't think it's worth it, but don't let that stop you 🙂

The compatibility-breaking changes with no_get, no_set etc. should be a separate issue for sure.

Two different issues then?

Bromeon · 2025-08-05T21:05:31Z

godot-core/src/obj/mod.rs

 mod instance_id;
 mod on_editor;
 mod on_ready;
+mod phantom_var;


I think this should be added to the godot::register::property module instead.

Maybe we need to move OnReady/OnEditor as well in the future... 🤔 modules generally need a bit of cleanup at some point.

To me it makes most sense for PhantomVar to be next to OnReady and OnEditor, wherever those are. I'd be happy to move all three of them in a follow-up PR.

It's just that we can't change modules for patch releases (at least not without deprecation), and if we already know now that godot::obj is the "wrong" place, it just means one extra breaking change for users that we could easily avoid...

Fair enough, I thought these modules were all internal. I'd expect that most usage is through the prelude, but breaking is breaking. Moved to godot-core/src/registry/property.rs instead.

For the record, I never use prelude, don't like them, I prefer being explicit with imports and I believe rust-analyzer finds their definition rather than re-exports, so yeah, this would indeed be a breaking change for all my godot-rust code (although an easy one to fix).

I always use the godot prelude, as I can't generally be bothered to micromanage modules. It's also a pain for merge conflicts and the like, without adding real value.

That said, we should have a somewhat structured module hierarchy for people who do want it -- and let's not forget that lots of symbols aren't in the prelude (especially godot::classes::*). However, if someone opts in to use fine-grained modules rather than prelude, they readily accept the risk of more micromanaging. So it's OK to break module paths as long as we respect SemVer -- i.e. remove across minor versions, or deprecate across patch versions.

godot-core/src/obj/phantom_var.rs

Bromeon · 2025-08-06T14:53:32Z

godot-core/src/registry/property.rs

 {
 }

+// ----------------------------------------------------------------------------------------------------------------------------------------------


It's now 200 LoC, I think it deserves its own file phantom_var.rs? I should have mentioned this (sorry!), I just think it's easier to navigate to it in the IDE, especially with the doctests "polluting" a considerable part.

Sure, I would prefer that too.

Would you want the module private with a pub use crate::registry::phantom_var::PhantomVar in property.rs? This is a bit weird because it's a sibling module though. I could also make property.rs into property/mod.rs and put property/phantom_var.rs next to it.

Or just expose the phantom_var module directly?

It can also just be a sister module in https://github.com/godot-rust/gdext/tree/master/godot-core/src/registry. I need to clean up a bit in v0.4 anyway, but if they are already separate files, it's going to be simpler 🙂

Or the mod.rs approach is also fine -- I hope Git picks up the rename.

OK, I hope I got it right. The public name is now godot::register::property::PhantomVar.

godot-core/src/registry/property.rs

Bromeon · 2025-08-06T17:39:26Z

Good from my side, you can squash. Maybe @Yarwin has more comments, otherwise we can merge 🙂

godot-macros/src/class/derive_godot_class.rs

Closes godot-rust#1151.

ttencate force-pushed the feature/synthetic_properties branch from 96fa239 to 59e5b32 Compare August 5, 2025 14:32

Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Aug 5, 2025

Bromeon reviewed Aug 5, 2025

View reviewed changes

Yarwin reviewed Aug 6, 2025

View reviewed changes

godot-core/src/obj/phantom_var.rs Outdated Show resolved Hide resolved

Yarwin reviewed Aug 6, 2025

View reviewed changes

godot-core/src/obj/phantom_var.rs Outdated Show resolved Hide resolved

Bromeon reviewed Aug 6, 2025

View reviewed changes

Bromeon approved these changes Aug 6, 2025

View reviewed changes

ttencate force-pushed the feature/synthetic_properties branch from b9a8288 to e9c26d6 Compare August 6, 2025 17:50

ttencate mentioned this pull request Aug 7, 2025

Support rename for #[var] #1262

Closed

Yarwin reviewed Aug 7, 2025

View reviewed changes

godot-macros/src/class/derive_godot_class.rs Outdated Show resolved Hide resolved

Add PhantomVar<T> to support properties without a backing field

3fa3eb1

Closes godot-rust#1151.

ttencate force-pushed the feature/synthetic_properties branch from e9c26d6 to 3fa3eb1 Compare August 7, 2025 12:23

Bromeon added this pull request to the merge queue Aug 7, 2025

Merged via the queue into godot-rust:master with commit 1606a7f Aug 7, 2025
18 checks passed

sylbeth mentioned this pull request Aug 8, 2025

Clarify semantics of write-only properties #1264

Closed

Uh oh!

Conversation

ttencate commented Aug 5, 2025

Uh oh!

GodotRust commented Aug 5, 2025

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yarwin Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sylbeth Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ttencate Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bromeon Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ttencate Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bromeon Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bromeon commented Aug 6, 2025

Uh oh!

Yarwin Aug 6, 2025 •

edited

Loading

sylbeth Aug 6, 2025 •

edited

Loading

ttencate Aug 6, 2025 •

edited

Loading

Bromeon Aug 6, 2025 •

edited

Loading

ttencate Aug 6, 2025 •

edited

Loading

Bromeon Aug 6, 2025 •

edited

Loading