Refactor GodotString, NodePath, and StringName

[lilizoey]

Creates a new module in builtin that contains all the string-related modules.

Make all the string-files look more similar where it made sense. And add things that only existed in one of the three but not the others, such as: Hash impl (now using godot's hashing), new-constructor, conversions.

Added pass-by-value From impls in addition to pass-by-reference From impls, so we can do just

let node_path: NodePath = string.into();

instead of needing to do

let node_path: NodePath = NodePath::from(&string);

Moves String-specific stuff into builtin/string/mod.rs and renamed string.rs to godot_string.rs. And adds a VariantMetadata impl for String.

Adds some more tests to test all the types a bit more extensively.

Since this is gonna conflict with #231 (as i added some stuff to StringName there) i wanna wait with merging this until that is merged. but otherwise the PR is ready to be merged.

[Bromeon]

Nice work, thanks! 😎

---

Added pass-by-value From impls in addition to pass-by-reference From impls, so we can do just

let node_path: NodePath = string.into();

instead of needing to do

let node_path: NodePath = NodePath::from(&string);

Makes sense. However the by-value From needlessly consumes the source, and suggests that the destination can reuse the string, which is not the case. Could you document that there is no performance benefit over From<&SourceType>?

---

I would remove new() for StringName and NodePath. They are rarely used if ever, redundant with Self::default(), and definitely not the primary way to construct those types.

It's probably OK for GodotString, for consistency with String and since it's more meaningful to have empty strings than empty paths/interned strings.

[Bromeon]

Could we maybe reuse chars_checked() here?
Would save us an extra allocation.

As a bonus, it would give the obscure chars API an opportunity to be more widely tested in the real world.

[Bromeon]

This name may shadow the Hash::hash(&mut H) method from the standard library. Should probably be fine though, as the trait can be invoked with fully-qualified syntax.

[Bromeon]

Should be at the top

[lilizoey]

Nice work, thanks! sunglasses

Added pass-by-value From impls in addition to pass-by-reference From impls, so we can do just

let node_path: NodePath = string.into();

instead of needing to do

let node_path: NodePath = NodePath::from(&string);

Makes sense. However the by-value From needlessly consumes the source, and suggests that the destination can reuse the string, which is not the case. Could you document that there is no performance benefit over From<&SourceType>?

yep

I would remove new() for StringName and NodePath. They are rarely used if ever, redundant with Self::default(), and definitely not the primary way to construct those types.

It's probably OK for GodotString, for consistency with String and since it's more meaningful to have empty strings than empty paths/interned strings.

Rust's closest analogy to NodePath is probably PathBuf, which does have an empty new impl. Im not sure what the best analogy for StringName is, maybe Ident, which has a non-empty new impl.

But also the rust api guidelines say:

Note that it is common and expected for types to implement both Default and an empty new constructor. new is the constructor convention in Rust, and users expect it to exist, so if it is reasonable for the basic constructor to take no arguments, then it should, even if it is functionally identical to default.

Nowhere there does it suggest that frequency of use, redundancy with default (in fact it explicitly says you should do it even if it is), or not being the primary way of constructing something are the relevant criteria. The only criteria is that "it is reasonable for the basic constructor to take no arguments", which in this case it is.

It is reasonable to create both an empty NodePath and empty StringName with new(). So they should have a new() constructor.

[Bromeon]

But also the rust api guidelines say:

Note that it is common and expected for types to implement both Default and an empty new constructor. new is the constructor convention in Rust, and users expect it to exist, so if it is reasonable for the basic constructor to take no arguments, then it should, even if it is functionally identical to default.

Nowhere there does it suggest that frequency of use, redundancy with default (in fact it explicitly says you should do it even if it is), or not being the primary way of constructing something are the relevant criteria. The only criteria is that "it is reasonable for the basic constructor to take no arguments", which in this case it is.

Thanks for the link to the guidelines! 👍

What we need to consider in our case is that we're not desigining the API from scratch -- we're merely mapping an existing Godot API to Rust. As such, we have less freedom than in the in the idealistic Rust-only world. For example, a lot of our builtin types would never have a Default impl if designed in Rust, because all it does is create an invalid/useless state: Rid, Callable, Signal, arguably even some geometric primitives like Plane (which are degenerate in their default state, aka useless).

Yet we do provide Default for builtins, because it's a common and required operation in Godot, with well-known limitations. But that doesn't mean we have to advertise it additionally by making popular new() constructors for cases that have little to no practical use. Especially not because a Rust guideline that was crafted in sterile lab conditions says so. Don't get me wrong, the guidelines have a lot of valid points; but they're guidelines, not rules, and context is important. This is a general issue with official Rust docs -- even the Rustonomicon doesn't account for the scenario we're operating in, which you see when it comes to safety.

---

It is reasonable to create both an empty NodePath and empty StringName with new(). So they should have a new() constructor.

Isn't an empty NodePath always wrong, because the resulting path is invalid? At least from Godot docs, no semantics for it are explained. The relative path for the current path is explicitly ^"." and not ^"".

Your assessment with StringName being close to Ident is correct, and also here empty identifiers are rarely valid. There are some cases (mainly because GDScript doesn't have optionals), but having StringName::default() is more than enough for those.

[lilizoey]

But also the rust api guidelines say:

Note that it is common and expected for types to implement both Default and an empty new constructor. new is the constructor convention in Rust, and users expect it to exist, so if it is reasonable for the basic constructor to take no arguments, then it should, even if it is functionally identical to default.

Nowhere there does it suggest that frequency of use, redundancy with default (in fact it explicitly says you should do it even if it is), or not being the primary way of constructing something are the relevant criteria. The only criteria is that "it is reasonable for the basic constructor to take no arguments", which in this case it is.

Thanks for the link to the guidelines! +1

What we need to consider in our case is that we're not desigining the API from scratch -- we're merely mapping an existing Godot API to Rust. As such, we have less freedom than in the in the idealistic Rust-only world. For example, a lot of our builtin types would never have a Default impl if designed in Rust, because all it does is create an invalid/useless state: Rid, Callable, Signal, arguably even some geometric primitives like Plane (which are degenerate in their default state, aka useless).

Yet we do provide Default for builtins, because it's a common and required operation in Godot, with well-known limitations. But that doesn't mean we have to advertise it additionally by making popular new() constructors for cases that have little to no practical use. Especially not because a Rust guideline that was crafted in sterile lab conditions says so. Don't get me wrong, the guidelines have a lot of valid points; but they're guidelines, not rules, and context is important. This is a general issue with official Rust docs -- even the Rustonomicon doesn't account for the scenario we're operating in, which you see when it comes to safety.

It is reasonable to create both an empty NodePath and empty StringName with new(). So they should have a new() constructor.

Isn't an empty NodePath always wrong, because the resulting path is invalid? At least from Godot docs, no semantics for it are explained. The relative path for the current path is explicitly ^"." and not ^"".

Godot's source code explicitly just always returns a null pointer for empty paths:

if (p_path.is_empty()) {
	return nullptr;
}

Your assessment with StringName being close to Ident is correct, and also here empty identifiers are rarely valid. There are some cases (mainly because GDScript doesn't have optionals), but having StringName::default() is more than enough for those.

Looking at the methods of both i think it makes more sense now to not have an empty new, initially i believed StringName had fake-mutable operators but they all return String and not StringName. And NodePath is just fully immutable as well. Meaning that StringName is probaly most similar to &'static str (as rust generally treats string literals like godot does StringName, though there's no guarantee) and NodePath to Path. But they're not exactly identical as they both semantically act like owned immutable strings.

I do however think it'd make sense to have new(string: GodotString) or maybe new(string: impl Into<GodotString>) for both of them. As i feel like when you want to explicitly construct them, you almost always want to use a string. Not to mention that they both basically are strings, just with different restrictions/optimizations/use-cases. We do of course have NodePath/StringName::from(GodotString) but i'd definitely think an explicit constructor for both would be good here.

[Bromeon]

I'm not sure it helps clarity, it could as well have the opposite effect because there would then already be 3 syntaxes to convert strings:

StringName::new(string);
StringName::from(string);
string.into();

I'm generally not a fan of having too many ways to achieve the same thing. In my eyes, APIs with narrower surface and less redundancy are easier to discover.

[lilizoey]

I'm not sure it helps clarity, it could as well have the opposite effect because there would then already be 3 syntaxes to convert strings:

StringName::new(string);
StringName::from(string);
string.into();

I'm generally not a fan of having too many ways to achieve the same thing. In my eyes, APIs with narrower surface and less redundancy are easier to discover.

True, but in this case i think not having new will hurt discoverability rather than help. Mainly because from and into are from a trait and not inherent to the type.

When i try to find new methods and constructors for a type, the first thing i always check for is something named new or new_x of some kind. Not having this means people need to look further for it. I would then admittedly find it in the next step, which is usually looking for a method named from_x or a From impl.

In addition, i usually find that constructors are easier to see if they are their own explicit methods and not just a trait. For one then the constructors actually show up on top of the docs, unlike trait impls that always show up far down. Having a new constructor in general helps discoverability.

Having an explicit constructor also signals to the user that this type can be constructed, and isn't merely a reference or something like that. Many types have From impls and no constructors, such as str. But they generally lack any constructors because they are references of some kind. If we make NodePath and StringName not have any explicit constructors then that would give the wrong message that these types are borrowed types or something similar.

And in general i dont really think constructors are very hurt by general redundancy. Once you get how a type is structured, extra constructors that dont do something very magical are generally pretty easy to understand even when they're technically redundant, as long as they're not actually identical except for the name. In this case from and new would do the same thing, however one is a trait impl and one is an inherent impl. So they are not completely identical.

I think as a general rule, types that can be constructed should have a new constructor, unless there are strong reasons not to include one, even when it overlaps in behavior with a trait-impl (such as Default or From). And if we dont make a new constructor, there should at least be some basic explicit constructor, preferrably named something like new_x or from_x.

I can understand how you feel about redundancy as a general sentiment, but for new constructors i think we should be less wary of redundancy than other places.

[Bromeon]

When i try to find new methods and constructors for a type, the first thing i always check for is something named new or new_x of some kind. Not having this means people need to look further for it. I would then admittedly find it in the next step, which is usually looking for a method named from_x or a From impl.

We already had this discussion for Gd::with_base. Rust developers must know that not every constructor starts with a new; especially from and with prefixes are very common. And these methods are literally on the top of both docs and IDE suggestions.

A lot of our work goes into documentation. At some point I expect that users show minimum effort toward API discovery.

I think as a general rule, types that can be constructed should have a new constructor, unless there are strong reasons not to include one, even when it overlaps in behavior with a trait-impl (such as Default or From). And if we dont make a new constructor, there should at least be some basic explicit constructor, preferrably named something like new_x or from_x.

I disagree that almost every type needs new. As mentioned, lower redundancy is a more important factor in my opinion. Not only does it keep the number of methods smaller, but it doesn't even bring up "are these two things the same" style questions in the first place.

[lilizoey]

i still dont think i agree on this but I'll concede, i dont think bikeshedding this more is gonna help and i trust you. (i might not make final commit + squash today tho)

[Bromeon]

Just wanted to add (was too slow with my edit):

I can understand how you feel about redundancy as a general sentiment, but for new constructors i think we should be less wary of redundancy than other places.

The points still apply. Redundancy means:

• Larger API surface, complexity and documentation.
• I need to make a concious and arbitrary decision whether I use new() or from().
• When reading other code, I need to be aware of both ways. If I've always seen from() being used for conversions, I wonder if new() now has a subtle difference; and if not, why it exists.

It can be considered case-by-case, as mentioned I agree that GodotString::new() is fine despite redundant. But for the others I don't see the benefits. Difference in signature to GodotString::new() is just one more point of confusion.

[lilizoey]

bors try

[bors]

try

Build succeeded:

• full-ci

[Bromeon]

One last thing, you can merge once that's ready 🙂

[Bromeon]

In the case of $Ty == GodotString, this would first call

GodotString::from(&str)

and then

GodotString::from(&GodotString)

Which trait impl powers the 2nd conversion? There is no From<&GodotString> for GodotString. While From is reflexive, it shouldn't be for references...?

The background behind my question is that it might perform an unnecessary conversion/copy.
Could you implement from() directly like this?

            fn from(string: S) -> Self {
                // note: no &
                Self::from(GodotString::from(string.as_ref()));
            }

[lilizoey]

I don't use this macro for GodotString, precisely because this macro relies on there already existing a From conversion for GodotString and String. it's only used for StringName and NodePath, because their conversions are identical. But GodotString is a special case.

[Bromeon]

Ah, I see. In that case you can go ahead! 🙂

[lilizoey]

bors r+

[bors]

Build succeeded:

• full-ci