Implement support for RID #171

lilizoey · 2023-03-11T17:30:35Z

RID is here implemented as an enum

pub enum Rid {
    Valid(NonZeroU64),
    Invalid,
}

This is to take advantage of null-pointer optimization, while bringing type safety in.
Option would not be usable for this without bigger changes in the code, as we cannot easily implement traits on Option<Rid>, and it'd take a lot of work in the codegen to change the right function parameters and return types to Option<Rid>/Rid where appropriate.

Closes #102

Bromeon

Thank you for the PR!

As suggested in comments, I would start with a smaller, less redundant API surface.

Also, some of the tests you add do a lot of bruteforcing over a large design space. Do they increase overall test duration (in debug mode), and if so, how by much?

Bromeon · 2023-03-12T13:21:58Z

godot-core/src/builtin/rid.rs

+/// # Layout:
+///
+/// `Rid` has the same layout as a [`u64`], where [`Rid::Invalid`] is 0, and [`Rid::Valid(i)`] is `i`.


I don't think we should expose this implementation detail, it only limits us in the future (e.g. if we ever add a debug-only field for extra validation or so). We already have new and to_u64 functions that are reasonably cheap.

Also, as far as I know, without #[repr(C)] and #[repr(transparent)], no layout is guaranteed, even if with the enum optimization you mentioned. It's very unlikely to change, but a compiler is theoretically free to do so.

Also, as far as I know, without #[repr(C)] and #[repr(transparent)], no layout is guaranteed, even if with the enum optimization you mentioned. It's very unlikely to change, but a compiler is theoretically free to do so.

The rustnomicon explicitly says the nullable pointer optimization is guaranteed:

This is called an "optimization", but unlike other optimizations it is guaranteed to apply to eligible types.

And considering the fields of the enum, there is only one possible layout.

I also just ran the itests with -Zrandomize-layout and it works. If the layout wasn't guaranteed here, then rid_equiv should fail under that. Since it checks that Rid::Invalid maps to 0, and Rid::Valid(i) maps to i (for one specific i at least)

-Zrandomize-layout passing is a necessary but not sufficient criterion for correct layout -- it only reorders fields and doesn't change fields etc. But to my understanding, it's conforming for a Rust compiler to e.g. add padding bytes at the end, even if the enum optimization is available. But we're really talking about theory here, I also don't see why this would happen.

Anyway, it doesn't matter much: the point is that a guaranteed layout limits potential implementation changes in the future, so we shouldn't document it. There's the existing API for such conversions.

Bromeon · 2023-03-12T13:25:29Z

godot-core/src/builtin/rid.rs

+    /// Returns `true` if this is an invalid RID.
+    #[inline]
+    pub const fn is_invalid(&self) -> bool {
+        matches!(self, Rid::Invalid)
+    }


Providing negated versions of accessors is not something we typically do. I don't think we should start a precedent here, as this will lead to discussions in which cases it's OK or not. Given that RIDs validity checks should ideally not be very common, it's fine if a user has to explicitly type a !.

oh fair, i think the c++ code had it so i just added it. But it does follow the precedent of Option which has both is_some and is_none.

But it does follow the precedent of Option which has both is_some and is_none.

Yes, and it does not follow the precedent of Vec::is_empty() which has no such opposite. Or any other type in godot-rust, which is much more relevant here 🙂

Vec isn't an enum though, i chose to compare it with Option because they're both similar enums, and having an is_x function for each enum variant is not uncommon.

Bromeon · 2023-03-12T13:28:48Z

godot-core/src/builtin/rid.rs

+impl From<u64> for Rid {
+    #[inline]
+    fn from(id: u64) -> Self {
+        Self::new(id)
+    }
+}
+
+impl From<NonZeroU64> for Rid {
+    #[inline]
+    fn from(id: NonZeroU64) -> Self {
+        Self::Valid(id)
+    }
+}
+
+impl From<Rid> for u64 {
+    #[inline]
+    fn from(rid: Rid) -> Self {
+        rid.to_u64()
+    }
+}
+
+impl TryFrom<Rid> for NonZeroU64 {
+    type Error = ();
+
+    #[inline]
+    fn try_from(rid: Rid) -> Result<Self, Self::Error> {
+        rid.to_non_zero_u64().ok_or(())
+    }
+}
+
+impl From<Rid> for Option<NonZeroU64> {
+    #[inline]
+    fn from(rid: Rid) -> Self {
+        match rid {
+            Rid::Invalid => None,
+            Rid::Valid(id) => Some(id),
+        }
+    }
+}
+
+impl From<Option<NonZeroU64>> for Rid {
+    #[inline]
+    fn from(id: Option<NonZeroU64>) -> Self {
+        match id {
+            Some(id) => Rid::Valid(id),
+            None => Rid::Invalid,
+        }
+    }
+}


Again, those are redundant with the inherent methods, and I'd prefer to have explicit versions rather than 323.into(). It's also not very common that a user is actually interested in the integer itself. Rid is mostly intended to be an opaque ID, and there are already named conversions for the other cases.

Bromeon · 2023-03-12T13:31:17Z

itest/rust/src/rid_test.rs

+/// Disable printing errors from Godot. Ideally we should catch and handle errors, ensuring they happen.
+/// But that isn't possible, so for now we just disable printing the error to avoid spamming the terminal.
+fn should_error(mut f: impl FnMut()) {
+    Engine::singleton().set_print_error_messages(false);
+    f();
+    Engine::singleton().set_print_error_messages(true);
+}


Given the semantics, this could be named suppress_godot_print rather than should_error.
Would it make sense to move it next to expect_panic?

Bromeon · 2023-03-12T13:34:34Z

itest/rust/src/rid_test.rs

+                let mut v = vec![];
+                for _ in 0..1000 {
+                    v.push(server.canvas_item_create());
+                }
+                v


This could probably be simplfied by mapping a range 🙂

Bromeon · 2023-03-12T13:36:20Z

itest/rust/src/rid_test.rs

+#[itest]
+fn strange_rids() {


Could you maybe add a quick comment what exactly is being tested here?

lilizoey · 2023-03-12T14:07:07Z

Also, some of the tests you add do a lot of bruteforcing over a large design space. Do they increase overall test duration (in debug mode), and if so, how by much?

I haven't noticed much, checking with Instant, multi_thread_test took 0.05262027 seconds and strange_rids took 0.013599553 seconds.

Bromeon · 2023-03-12T14:53:46Z

In gdnative, we use Rid::is_occupied instead of is_valid as a name. I find this more expressive, as it doesn't suggest that the RID points to a valid (existing, currently loaded) resource.

We also explicitly documented that difference in meaning, see Rid::is_occupied docs.

Here the situation is a bit different as we also have the enum variants 🤔
Do we gain much by having it as an enum, or should we just provide an easy conversion to Option<u64>? The conversion to Option<NonZeroU64> is technically more correct, but it's inconvenient since that type isn't used anywhere else and forces more verbose conversions on the user side...

lilizoey · 2023-03-12T15:11:54Z

In gdnative, we use Rid::is_occupied instead of is_valid as a name. I find this more expressive, as it doesn't suggest that the RID points to a valid (existing, currently loaded) resource.

Honestly to me is_occupied even more suggests that the RID points to a resource than is_valid. I dont mind the name but to me it reads as "this RID is occupied by a resource".

Here the situation is a bit different as we also have the enum variants thinking Do we gain much by having it as an enum[?] (...)

As i see it
Advantages:

easy construction of valid/invalid RID
ability to pattern match
more clear to the user what Valid/Invalid means

Disadvantages:

must make fields public
relies on technically not guaranteed (but highly unlikely to break in our use-cases) type-layout
must use NonZeroU64

(...) or should we just provide an easy conversion to Option<u64>? The conversion to Option<NonZeroU64> is technically more correct, but it's inconvenient since that type isn't used anywhere else and forces more verbose conversions on the user side...

The functions to convert the RID to/from integers are likely not gonna be used much except in the cases where it is useful to maintain the distinction between u64 and NonZeroU64, such as implementing your own custom servers. So of those two i'd advocate for Option<NonZeroU64> at least.

Bromeon · 2023-03-12T15:20:45Z

I'm not generally opposed to enums, but it feels a bit strange that InstanceId and Rid now have completely different APIs, although they are conceptually very similar. This is also why I was advocating the Option first, but I see how that's hard in this case, as Rid is used in all the generated APIs.

But maybe this can be unified later; it's good to already have a first implementation.

Bromeon · 2023-03-12T15:26:53Z

That said, with the currently proposed API it's quite verbose to get a Option<u64> out of an Rid (which gives you both the underlying type and the knowledge whether it's occupied).

rid.to_non_zero_u64().map(NonZeroU64::get);

if rid.is_valid {
    Some(rid.to_u64())
} else {
    None
}

match rid {
    Valid(nz) => Some(nz.get())
    Invalid => None
}

NonZeroU64 is nice in theory, just like NonNull, when you want to have very expressive, type-safe APIs.
However, in practice they're often annoying to work with, à la "get the raw type with extra steps".

lilizoey · 2023-03-12T15:54:23Z

So there are three solutions to that i can think of:

add a function to_option_u64 (or some other name)
make to_u64 return Option<u64> (and rename original?)
add a From<Rid> for Option<u64> impl (and the reverse?)

Which do you prefer?

Bromeon · 2023-03-12T16:37:37Z

Maybe 1. as try_to_u64?

I don't think we need to_non_zero_u64 then, if we already have NonZeroU64 in the enumerator. Most people are not likely to use this type.

Maybe to_u64 should in that case be called something like to_zero_u64 (I don't know a better name right now) 🤔

lilizoey · 2023-03-12T16:59:25Z

What about this? to_u64 and to_valid_u64

Add `RenderingServer` to minimal classes, to be able to test the RID impl against an actual server Add `Engine` to minimal classes, to be able to disable error printing in itests

Bromeon · 2023-03-14T12:37:02Z

Thanks!

I'll merge this, however expect significant API changes as I'll try to make InstanceId and Rid more consistent with each other. I will also have another look at the naming at that time, so I think it's good to have something for now. I still believe "valid" should be "occupied", for example.

bors r+

bors · 2023-03-14T12:54:15Z

Build succeeded:

full-ci

lilizoey force-pushed the feature/rid branch from 8a4f246 to 6aa8e55 Compare March 11, 2023 17:41

Bromeon reviewed Mar 12, 2023

View reviewed changes

Bromeon added feature Adds functionality to the library c: core Core components labels Mar 12, 2023

lilizoey force-pushed the feature/rid branch from 6aa8e55 to 6c22ce9 Compare March 12, 2023 14:16

lilizoey force-pushed the feature/rid branch from 6c22ce9 to 6a47e9b Compare March 12, 2023 16:59

Implement support for RID

1820a11

Add `RenderingServer` to minimal classes, to be able to test the RID impl against an actual server Add `Engine` to minimal classes, to be able to disable error printing in itests

lilizoey force-pushed the feature/rid branch from 6a47e9b to 1820a11 Compare March 12, 2023 17:03

Bromeon mentioned this pull request Mar 13, 2023

Project status overview #24

Open

bors bot merged commit 1dcbc40 into godot-rust:master Mar 14, 2023

lilizoey deleted the feature/rid branch April 16, 2023 16:23

lilizoey mentioned this pull request Jun 14, 2023

Tracking issue for builtin feature-completeness #310

Open

33 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for RID #171

Implement support for RID #171

lilizoey commented Mar 11, 2023 •

edited

Loading

Bromeon left a comment

Bromeon Mar 12, 2023

Bromeon Mar 12, 2023

lilizoey Mar 12, 2023 •

edited

Loading

lilizoey Mar 12, 2023

Bromeon Mar 12, 2023

Bromeon Mar 12, 2023

lilizoey Mar 12, 2023

Bromeon Mar 12, 2023

lilizoey Mar 12, 2023

Bromeon Mar 12, 2023

Bromeon Mar 12, 2023

Bromeon Mar 12, 2023

Bromeon Mar 12, 2023

lilizoey commented Mar 12, 2023

Bromeon commented Mar 12, 2023 •

edited

Loading

lilizoey commented Mar 12, 2023 •

edited

Loading

Bromeon commented Mar 12, 2023

Bromeon commented Mar 12, 2023 •

edited

Loading

lilizoey commented Mar 12, 2023 •

edited

Loading

Bromeon commented Mar 12, 2023 •

edited

Loading

lilizoey commented Mar 12, 2023

Bromeon commented Mar 14, 2023

bors bot commented Mar 14, 2023

		#[itest]
		fn strange_rids() {

Implement support for RID #171

Implement support for RID #171

Conversation

lilizoey commented Mar 11, 2023 • edited Loading

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey Mar 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey commented Mar 12, 2023

Bromeon commented Mar 12, 2023 • edited Loading

lilizoey commented Mar 12, 2023 • edited Loading

Bromeon commented Mar 12, 2023

Bromeon commented Mar 12, 2023 • edited Loading

lilizoey commented Mar 12, 2023 • edited Loading

Bromeon commented Mar 12, 2023 • edited Loading

lilizoey commented Mar 12, 2023

Bromeon commented Mar 14, 2023

bors bot commented Mar 14, 2023

lilizoey commented Mar 11, 2023 •

edited

Loading

lilizoey Mar 12, 2023 •

edited

Loading

Bromeon commented Mar 12, 2023 •

edited

Loading

lilizoey commented Mar 12, 2023 •

edited

Loading

Bromeon commented Mar 12, 2023 •

edited

Loading

lilizoey commented Mar 12, 2023 •

edited

Loading

Bromeon commented Mar 12, 2023 •

edited

Loading