Rename builtin `hash()` -> `hash_u32()`; add tests #1366

markogalevski · 2025-10-12T20:13:53Z

Aiming to fix issue #505

Checked Godot's cpp implementation. Only the following function was an i64 hash, which was just casting Variant's u32 hash: https://github.com/godotengine/godot/blob/cb7cd815eeeb11aaa1f68453a515c76bec5ba73d/core/variant/variant_utility.cpp#L1120
All other hashes (e.g. array.h, callable.h) are explicit uint32_t return types (https://github.com/godotengine/godot/blob/cb7cd815eeeb11aaa1f68453a515c76bec5ba73d/core/variant/callable.h)
Therefore renamed all Godot hashes to hash_u32()
Updated tests to ensure they always check for hash_u32() equality (Most tests were there, some needed some extension)

…sh_u32. Renamed and added tests

GodotRust · 2025-10-13T01:17:30Z

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

Yarwin · 2025-10-13T09:28:13Z

godot-core/src/builtin/variant/mod.rs

+    /// @GlobalScope.hash() actually calls the VariantUtilityFunctions::hash(&Variant) function (cpp).
+    /// This function calls the passed reference's `hash` method, which returns a uint32_t.
+    /// Therefore, casting this function to u32 is always safe
+    pub fn hash_u32(&self) -> u32 {
        unsafe { interface_fn!(variant_hash)(self.var_sys()) }
+            .try_into()
+            .expect("Godot hashes are uint32_t")


This method is public thus included in docs and IDE tips. IMO user doesn't need to know about safety guarantees – especially if they are always held. I would just use the same docs as for everything else:

Suggested change

/// @GlobalScope.hash() actually calls the VariantUtilityFunctions::hash(&Variant) function (cpp).

/// This function calls the passed reference's `hash` method, which returns a uint32_t.

/// Therefore, casting this function to u32 is always safe

pub fn hash_u32(&self) -> u32 {

unsafe { interface_fn!(variant_hash)(self.var_sys()) }

.try_into()

.expect("Godot hashes are uint32_t")

/// Returns a 32-bit unsigned integer hash value representing the Variant.

///

/// _Godot equivalent : `@GlobalScope.hash()`_

pub fn hash_u32(&self) -> u32 {

// Note: Calls the passed reference's underlying `hash` method which returns an uint32_t.

// SAFETY: we pass in the valid pointer.

unsafe { interface_fn!(variant_hash)(self.var_sys()) }

.try_into()

.expect("Godot hashes are uint32_t")

idk if we should keep info about VariantUtilityFunctions::hash(&Variant), your call 😅

Agree that this should be in comment rather than RustDoc.

Note that the int conversion isn't safety relevant, so shouldn't be part of // SAFETY:. Making a mistake here will result in a worse hash function, not UB.

Okay, I'll move it into a non-safety dev comment.

true true, I updated my suggestion

to elaborate – the only way to cause an UB here is by passing invalid GDExtensionConstVariantPtr.

static GDExtensionInt gdextension_variant_hash(GDExtensionConstVariantPtr p_self) { const Variant *self = (const Variant *)p_self; return self->hash(); }

Yarwin · 2025-10-13T10:03:52Z

godot-core/src/builtin/callable.rs

+    pub fn hash_u32(&self) -> u32 {
+        self.as_inner()
+            .hash()
+            .try_into()
+            .expect("Godot hashes are uin32_t")


Note: I wondered if it would be worth it to put it into same macro (or even covering it directly in builtin), such as

macro_rules! impl_hash_32 { ( $( #[$maybe_docs:meta] )* $Type:ty ) => { impl $Type { #[doc=concat!("Returns a 32-bit integer hash value representing the ", stringify!($Type), " and its contents.")] $( #[$maybe_docs] )* pub fn hash_u32(&self) -> u32 { self.as_inner() .hash() .try_into() .expect("Godot hashes are uint32_t") } } } }

and called as

impl_hash_32!( /// /// Note: Arrays with equal content will always produce identical hash values. However, the /// reverse is not true. Returning identical hash values does not imply the arrays are equal, /// because different arrays can have identical hash values due to hash collisions. Array<T> ); (...) impl_hash_32!(StringName);

Since having one source for multiple instances of same code would be nice – but IMO it is not worth it getting into consideration all the differences (impl<T:ArrayElement> Array<T> vs. concrete Type). Ctrl+c ctrl+v ftw

I'll give that a crack too.

don't sweat too much though (unless you'll find/figure out something nice); In this context repetition is fine.

Chucking it in as you suggested won't work because we'd end up with multiple impl blocks for the same struct. I could define hash_u32 as the sole member of a GodotHash trait, which we could then implement in built-ins, but then you would need to have the trait in scope whenever calling hash_u32. This would be really annoying. I think I'll just leave this as is.

Can the macro not just define the fn instead of the impl? And then place it inside an existing impl.

I don't see why we need traits here...

Yarwin · 2025-10-13T10:05:56Z

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

        // The GDExtension interface only deals in `i64`, but the engine's own `hash()` function
        // actually returns `uint32_t`.


I would cut out this comment and move it to builtin Hash macro.

Yarwin

LGTM, outside of this one SAFETY comment exposed in public docs

and just to confirm, in my view hash_u32 shortcut (avoiding conflict with Hash::hash) is fine too

Bromeon

Renames of public functions are breaking changes, as such it would have to wait for v0.5. We can merge it sooner if you provide the old methods with a deprecation (no docs, just "renamed to xy" in deprecation notice).

Bromeon · 2025-10-13T10:25:17Z

godot-core/src/builtin/variant/mod.rs

+    /// @GlobalScope.hash() actually calls the VariantUtilityFunctions::hash(&Variant) function (cpp).
+    /// This function calls the passed reference's `hash` method, which returns a uint32_t.
+    /// Therefore, casting this function to u32 is always safe
+    pub fn hash_u32(&self) -> u32 {
        unsafe { interface_fn!(variant_hash)(self.var_sys()) }
+            .try_into()
+            .expect("Godot hashes are uint32_t")


Agree that this should be in comment rather than RustDoc.

Note that the int conversion isn't safety relevant, so shouldn't be part of // SAFETY:. Making a mistake here will result in a worse hash function, not UB.

…ated to prevent breaks in v0.4

…ki/gdext into rust_vs_godot_hashes

markogalevski · 2025-10-13T19:25:18Z

@Bromeon Added deprecated methods too.
I think this PR is ready for another round of viewing

Bromeon · 2025-10-13T23:28:54Z

godot-core/src/builtin/variant/mod.rs

+            .expect("Godot hashes are uint32_t")
+    }
+
+    #[deprecated = "renamed to hash_32 and type changed to u32"]


Suggested change

#[deprecated = "renamed to hash_32 and type changed to u32"]

#[deprecated = "renamed to `hash_u32` and type changed to u32"]

Bromeon · 2025-10-13T23:31:25Z

godot-core/src/builtin/callable.rs

+    pub fn hash_u32(&self) -> u32 {
+        self.as_inner()
+            .hash()
+            .try_into()
+            .expect("Godot hashes are uin32_t")


Can the macro not just define the fn instead of the impl? And then place it inside an existing impl.

I don't see why we need traits here...

Bromeon · 2025-10-13T23:32:35Z

godot-core/src/builtin/macros.rs

+                // The GDExtension interface only deals in `i64`, but the engine's own `hash()` function
+                // actually returns `uint32_t`.


Suggested change

// The GDExtension interface only deals in `i64`, but the engine's own `hash()` function

// actually returns `uint32_t`.

// The GDExtension interface only deals in `int64_t`, but the engine's own `hash()` function

// actually returns `uint32_t`.

markogalevski · 2025-10-14T06:36:57Z

@Bromeon Yes Ican do that. I was thinking of these other bulitin macros you guys have.
I can definitely just make a impl_builtin_hash_u32!() macro and throw it in to all the places.

markogalevski · 2025-10-14T06:59:57Z

Just writing docstring comments above a macro! call doesn't work

/* Warning: Unused docstring */

/// My cool docstring
impl_builtin_hash_u32!{}

So in terms of making the appropriate docstrings available, I'm stuck between two solutions - neither of which I particularly like.

The first is only implementing an inner macro with the contents of the functions:

macro_rules! impl_builtin_inner_hash_u32 {
    () => {
    self.as_inner().try_into().expect("Godot hashes are always uint32_t")
    };
}

Which then lets you write

/// My cool docstring
pub fn hash_u32(&self) -> u32 {
    impl_builtin_inner_hash_u32!()
}

The other is accepting a docstring in the macro itself

macro_rules! impl_builtin_hash_u32 {
(docstr: literal) => {
    #[doc = $docstr]
    pub fn hash_u32(&self) -> u32 {
        self.as_inner().hash().try_into().expect("Godot hashes are always uint32_t")
    }
}
}

What do you think, should I use any of these methods, is there another method I don't know about, or should we just live with the code duplication?

…ki/gdext into rust_vs_godot_hashes

Yarwin · 2025-10-14T07:36:46Z

(edited since it wasn't important, oops)
One thing – doc /// comments are syntax sugar for #[doc = "..."] https://doc.rust-lang.org/reference/comments.html?#doc-comments an thus can be matched with $($maybe_docs:meta)*.
When in doubt look into rust reference.

As for details – I would like such macros to be self-contained, i.e. I would expect make_some_impl! to generate given impl.

Macros which don't generate seperate impl (i.e. are not self contained) && are not more-or-less intuitive shortcuts (like println!, interface_fn!, godot_print! etc) are anti-pattern – they require more brain power and focus because of all the moving parts (impl_n generates impl for n, but n depends on context, and impl_n works only in body of given impl... – it gets too easy to induce cognitive overload).

Sometimes, non-intuitively, repetition is fine – strictly admitting to DRY is silly. It depends on what is simpler to grasp and/or ignore (meaning that, as long as it satisfies these two aforementioned principles, it is up to individual/collective taste. I'm not making it too simple, yeah).

Bromeon · 2025-10-14T08:11:04Z

As for details – I would like such macros to be self-contained, i.e. I would expect make_some_impl! to generate given impl.

Not sure I agree, imo it's not mentally very challenging to call it make_some_fn instead and let it define a fn. I think we do this already in some places?

Also note that separate impl blocks appear separated in RustDoc, so this implementation detail leaks into user docs. We sometimes accept this, but I'm still not sure if it's helpful to see an impl just with one hash_u32.

Yarwin · 2025-10-14T08:41:50Z

Not sure I agree, imo it's not mentally very challenging to call it make_some_fn instead and let it define a fn. I think we do this already in some places?

We do it a lot in macros obviously, but haven't seen it in the rest of the code.

Also note that separate impl blocks appear separated in RustDoc, so this implementation detail leaks into user docs.

oooh, that's very valid point. Silly!

In such a case I would go with repetition

For example this one looks silly:

    make_hash_32_fn!(
        /// Returns a 32-bit integer hash value representing the array and its contents.
        ///
        /// Note: Arrays with equal content will always produce identical hash values. However, the
        /// reverse is not true. Returning identical hash values does not imply the arrays are equal,
        /// because different arrays can have identical hash values due to hash collisions.
    );

This one would be fine, but gains are minuscule 🤔 :

    /// Returns a 32-bit integer hash value representing ....
    pub fn hash(&self) -> u32 {
        builtin_hash_u32!()
    }

markogalevski · 2025-10-14T09:11:40Z

Although I think that a macro like this should be fine? Then we just call this within the pre-existing impl

impl SomeBuiltin {
   /* -- snip -- */
   ///doc stuff
   make_builtin_hash_u32!()
  /* -- snip -- */
}

macro_rules! make_builtin_hash_u32 {
   <doc matching> => {
        //insert docs here
       pub fn hash_u32(&self) -> u32 {
        self.as_inner().hash().try_into().expect("Godot hashes are uint32_t")
       }
    }
}

Bromeon · 2025-10-15T14:32:39Z

Let's not bikeshed this too much. As mentioned, we already do this, just use the same approach:

gdext/godot-core/src/builtin/string/gstring.rs

Lines 244 to 260 in 245ef3a

    
               meta::declare_arg_method! { 
        
                   /// Use as argument for an [`impl AsArg<StringName|NodePath>`][crate::meta::AsArg] parameter. 
        
                   /// 
        
                   /// This is a convenient way to convert arguments of similar string types. 
        
                   /// 
        
                   /// # Example 
        
                   /// [`Node::has_node()`][crate::classes::Node::has_node] takes `NodePath`, let's pass a `GString`: 
        
                   /// ```no_run 
        
                   /// # use godot::prelude::*; 
        
                   /// let name = GString::from("subnode"); 
        
                   /// 
        
                   /// let node = Node::new_alloc(); 
        
                   /// if node.has_node(name.arg()) { 
        
                   ///     // ... 
        
                   /// } 
        
                   /// ``` 
        
               }

markogalevski · 2025-10-15T14:35:25Z

Yes, ok. I’ll implement it this way shortly.

…

On Wed 15. Oct 2025 at 16:33, Jan Haller ***@***.***> wrote: *Bromeon* left a comment (godot-rust/gdext#1366) <#1366 (comment)> Let's not bikeshed this too much. As mentioned, we already do this, just use the same approach: https://github.com/godot-rust/gdext/blob/245ef3aa8248d6ed0a6902dc2de5e43979dd5fe9/godot-core/src/builtin/string/gstring.rs#L244-L260 — Reply to this email directly, view it on GitHub <#1366 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANBBS3RYPDMFD3GFXFYCW333XZLJ5AVCNFSM6AAAAACI7MHPICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTIMBWG43DINRVHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

…ki/gdext into rust_vs_godot_hashes

markogalevski · 2025-10-16T06:50:42Z

I think this is finally it, what do you think? @Bromeon

markogalevski · 2025-10-19T16:38:34Z

@Yarwin @Bromeon Just don't want things to drift too much.I think we can merge this, no?

Checked cpp (and left comments) detailing why all godot hashes are ha…

229dd2c

…sh_u32. Renamed and added tests

markogalevski mentioned this pull request Oct 12, 2025

Godot vs. Rust hashes #505

Open

Merge branch 'master' into rust_vs_godot_hashes

64fc5b3

Yarwin added quality-of-life No new functionality, but improves ergonomics/internals c: core Core components c: engine Godot classes (nodes, resources, ...) and removed c: core Core components labels Oct 13, 2025

Yarwin reviewed Oct 13, 2025

View reviewed changes

Bromeon reviewed Oct 13, 2025

View reviewed changes

markogalevski added 2 commits October 13, 2025 21:23

Moved comments out of public API. Re-added old hash methods as deprec…

9e536c3

…ated to prevent breaks in v0.4

Merge branch 'rust_vs_godot_hashes' of https://github.com/markogalevs…

74d15b6

…ki/gdext into rust_vs_godot_hashes

Merge branch 'master' into rust_vs_godot_hashes

9bde1ee

Bromeon reviewed Oct 13, 2025

View reviewed changes

Bromeon changed the title ~~Checked cpp and commented detailing why all godot hashes are hash_u32. Renamed and added tests~~ Rename builtin hash() -> hash_u32(); add tests Oct 13, 2025

markogalevski added 2 commits October 14, 2025 09:00

fixed small spelling mistakes in deprec strings and doc comments

67b1a74

Merge branch 'rust_vs_godot_hashes' of https://github.com/markogalevs…

641a271

…ki/gdext into rust_vs_godot_hashes

implemented hash_u32 macro implementer

7a83936

markogalevski and others added 2 commits October 16, 2025 08:50

Merge branch 'master' into rust_vs_godot_hashes

3f8af8d

Merge branch 'rust_vs_godot_hashes' of https://github.com/markogalevs…

6622ede

…ki/gdext into rust_vs_godot_hashes

Merge branch 'master' into rust_vs_godot_hashes

2dd05c0

		// The GDExtension interface only deals in `i64`, but the engine's own `hash()` function
		// actually returns `uint32_t`.

	#[deprecated = "renamed to hash_32 and type changed to u32"]
	#[deprecated = "renamed to `hash_u32` and type changed to u32"]

Uh oh!

Rename builtin hash() -> hash_u32(); add tests #1366

Are you sure you want to change the base?

Rename builtin hash() -> hash_u32(); add tests #1366

Uh oh!

Conversation

markogalevski commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GodotRust commented Oct 13, 2025

Uh oh!

Yarwin Oct 13, 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!

Yarwin Oct 13, 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!

Yarwin Oct 13, 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!

Yarwin Oct 13, 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!

markogalevski Oct 13, 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!

Yarwin left a comment • 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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markogalevski commented Oct 13, 2025

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!

markogalevski commented Oct 14, 2025

Uh oh!

markogalevski commented Oct 14, 2025

Uh oh!

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

Uh oh!

Uh oh!

Bromeon commented Oct 14, 2025

Uh oh!

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

Uh oh!

Uh oh!

markogalevski commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bromeon commented Oct 15, 2025

Uh oh!

markogalevski commented Oct 15, 2025 via email

Uh oh!

markogalevski commented Oct 16, 2025

Uh oh!

markogalevski commented Oct 19, 2025

Rename builtin `hash()` -> `hash_u32()`; add tests #1366

Rename builtin `hash()` -> `hash_u32()`; add tests #1366

markogalevski commented Oct 12, 2025 •

edited

Loading

Yarwin Oct 13, 2025 •

edited

Loading

Yarwin Oct 13, 2025 •

edited

Loading

Yarwin Oct 13, 2025 •

edited

Loading

Yarwin Oct 13, 2025 •

edited

Loading

markogalevski Oct 13, 2025 •

edited

Loading

Yarwin left a comment •

edited

Loading

Yarwin commented Oct 14, 2025 •

edited

Loading

Yarwin commented Oct 14, 2025 •

edited

Loading

markogalevski commented Oct 14, 2025 •

edited

Loading