Skip to content

fix: safer property setter/getter parameters#336

Open
archee565 wants to merge 2 commits into
vhspace:masterfrom
archee565:fix!-safer_property_naming
Open

fix: safer property setter/getter parameters#336
archee565 wants to merge 2 commits into
vhspace:masterfrom
archee565:fix!-safer_property_naming

Conversation

@archee565

Copy link
Copy Markdown
Contributor

The syntax of Properties::set is confusing.
The first parameter is called "name", but it expects PropertyName::value, while the second paramter is called "value". SDL_Set...Property functions don't return error, when a wrong string is supplied.

How about changing the setter's and getter's parameter from &str to &PropertyName and throw error on type mismatch?

@archee565 archee565 changed the title Fix! safer property naming Fix: safer property setter/getter parameters Feb 23, 2026
@archee565 archee565 changed the title Fix: safer property setter/getter parameters fix: safer property setter/getter parameters Feb 23, 2026
@revmischa

Copy link
Copy Markdown
Member

Thanks for looking into this! I agree the current set/get API with bare strings is a bit confusing, especially with PropertyName existing right there.

A few concerns with this approach though:

  • PropertyName was really designed for the auto-generated SDL property name constants (the ones in names). It has fields like module, doc, available_since that don't make sense for user-defined properties. The example kind of shows this -- you need a macro that fills all those fields with dummy values just to call set. That's a lot of ceremony for what used to be properties.set("foo", 42).

  • The type checking here is runtime only. Rust's trait dispatch already handles the Rust-side type safety -- set(name, true) goes to Setter<bool>, set(name, 42i64) goes to Setter<i64>, etc. The TypeMismatch error can only fire if the user constructs a PropertyName with the wrong ty field, which is a problem introduced by requiring PropertyName in the first place.

  • Some methods (has, get_type, get_string, clear, set_with_cleanup) still take &str, so the API ends up inconsistent.

  • The raw pointer construction with concat!($name, "\0").as_ptr() only works for string literals. No way to use runtime strings.

I think the right direction here might be something like a trait (e.g. Into<PropertyKey>) that accepts both &str and &PropertyName, so you get the convenience of strings and the structure of PropertyName when you want it. Or if we really want compile-time type safety, a generic TypedPropertyName<T> that carries the expected type as a type parameter rather than a runtime enum.

Would love to hear your thoughts on either of those approaches.

@revmischa revmischa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the effort here, type-safe property names is a reasonable idea. A few things I think need addressing before this can land:

  1. This is a breaking change to the public Setter/Getter traits, but there's no way for users to construct PropertyName values from the library side. The helper macro is only in the example. Users doing properties.set("my_prop", value) today would have no migration path without building raw pointers by hand.

  2. Some methods (contains, clear, get_string, set_with_cleanup, get_type) still take &str while the trait methods now take &PropertyName. It'd be good to pick one approach and apply it consistently.

  3. The type check is runtime (if name.ty != PropertyType::BOOLEAN), but Rust's trait dispatch already prevents passing the wrong value type. If you want compile-time safety for names too, something like PropertyName<T> with a type parameter might work better than a runtime check.

Would be nice to either provide a safe constructor/macro for custom property names from the library, or maybe accept both &str and &PropertyName through a trait bound so it's not a hard break for existing users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants