Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Best-effort checks for Array<Integer> conversions; fix Debug for variants containing typed arrays #853

Merged
merged 7 commits into from
Aug 12, 2024

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Aug 11, 2024

Addresses part of #727: the Variant Debug impl no longer panics.
However, conversions from Array<T> to VariantArray are still not possible, and need more design (OutArray PR #806).

Closes #805.

I decided to not forbid Array with element types u8, i8, u16, i16, u32, i32 and f32, for the following reasons:

  • Array is a general-purpose sequential data structure with focus on versatility/convenience. It's not particularly fast for small elements due to its Variant storage and conversions, but it supports the widest range of element types. The integer types fit somewhat naturally.
  • The number of conversions introduced by enforcing i64 (a not very natural type in everyday gamedev) would make a lot of code more tedious. It's already something I really dislike about f64, and I don't want to proliferate this practice.
  • A typical use case of Array<i16> etc. is exposing data from Rust to GDScript, e.g. returning from #[func] or as a #[var] property. If such an integer type is chosen in place of i64, that's usually because the developer knows the domain of the possible values, and the risk of inputing too big numbers is limited.
  • We do not lose memory safety if an unrepresentable number is stored in the array. It "only" causes a logic error -- which would happen anyway -- but at a later point (on access instead of array construction). There is of course a small risk that this is missed.
  • Because of that risk, Debug mode by default runs a O(n) validity check on the elements for non-i64 integer conversions. I consider justifiable because a) arrays are often small and/or sequential iteration is fast, b) performance is already bad for Array and people who care use packed arrays, c) it's turned off in Release mode. It's also a no-op for all other array element types. This check is best-effort, so it won't catch all scenarios (like appending from GDScript).
  • People who want maximum type safety can simply stick to i64 and f64. This also means no runtime checks.

Furthermore, godot-cpp also allows Array with these element types. They go even further and enable uint64_t, something we don't do.

@Bromeon Bromeon added bug quality-of-life No new functionality, but improves ergonomics/internals c: core Core components c: ffi Low-level components and interaction with GDExtension API labels Aug 11, 2024
@GodotRust
Copy link

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

@Bromeon
Copy link
Member Author

Bromeon commented Aug 12, 2024

Related ongoing conversation on RocketChat. It might be that we're getting distinct integer support inside arrays (by storing the meta at runtime), so this problem would be no longer existent at all 🙂

Until then, this is probably good enough.

@Bromeon Bromeon added this pull request to the merge queue Aug 12, 2024
Merged via the queue into master with commit 3211780 Aug 12, 2024
14 checks passed
@Bromeon Bromeon deleted the qol/array-element-conversions branch August 12, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: core Core components c: ffi Low-level components and interaction with GDExtension API quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array<T> can hold values that are invalid representations of T
2 participants