Cleanup some stuff mainly related to script instance

[lilizoey]

Mainly:

• Uses into/from_owned_sys functions instead of storing entire property lists and such in a hashmap
• Moves functions that only operates on ScriptInstanceData to be methods
• Removes several functions that are a simple wrappers around existing functions, or were only used in one place and which weren't very complicated
• Reword/correct some safety docs
• Adds a sys::conv module for common conversions
• Adds a dedicated bool -> GDExtensionBool conversion function
• Adds a true/false literal for GDExtensionBool
• Adds a function to infallibly convert u32 -> usize (this fails at compile time instead of runtime and should have no overhead, since i do not believe we (will) support any targets where u32 <= usize)
• Abstracts the logic for transferring a ptrlist to and from godot, while keeping track of length, into its own struct
• Adds a #[deny(unsafe_op_in_unsafe_fn)] to all the ffi-functions in script instance so that we're sure all unsafe function calls are explicitly tracked and called out

Also @TitanNano since you made a lot of this originally

[GodotRust]

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

[Bromeon]

Thanks a lot! I'd probably merge this after #736, as there are some possibly conflicting changes.

It's nice that even though there are a lot of changes, the diff is still visible in most places! If you plan further refactorings (like moving modules to files), it could make sense to do that in a separate PR 🙂

[lilizoey]

yeah i was considering moving modules to files and maybe moving around some functions to different places, but i figured it'd be best to avoid doing things that would confuse the diff to make it more easy to read

[Bromeon]

Great idea with the constants and the conversion functions!

[Bromeon]

I find this a bit hard to read... On one hand the multi-line tuples, on the other all the separate unsafe blocks which cover the entire area of this code. It's only 10 lines of actual logic, but the initialization is so sophisticated that the data flow is not very obvious anymore. For example, if arguments_slice were used within a block, it would be immediately clear that it's only used once and not further down the function.

Some unsafe could possibly be collapsed (e.g. slice and box initialization). Imo it's fine if a SAFETY block explains more than one thing, or if multiple explanations are within an unsafe block (next to their respective operations).

I was also considering that instead of alternating a1 b1 a2 b2 statements, we could use a1 a2 b1 b2 -- i.e. do all the arguments stuff first, then all the default_arguments stuff. This keeps data dependencies more local, however it spreads out similar unsafe operations, so it might be worse. Not sure if it's needed at all after above changes.

---

All in all it's not a big deal, just my first impression 🙂

[Bromeon]

I think "StringName" is wrong here.

[Bromeon]

Nice abstraction!

Re name, "list" and "container" are somewhat redundant, or it's not really clear how they relate. Maybe we should somehow express that this stores the lengths? BoundedPtrList or something like that?

You could also mention that you use "list" and not "array" because these refer to "property/method lists" (in Godot terms).

[Bromeon]

sys::to_const_ptr...

or can you call as_ptr() instead of as_mut_ptr() directly? Does the hashmap need mut pointers?

[lilizoey]

we do need to call as_mut_ptr() because otherwise we get a pointer derived from a &[T] reference which we cannot later free again since it doesn't have write permissions.

The issue really is just that c++ const pointers can be freed, whereas in rust a pointer derived from a shared reference cannot. So there really isn't a great way to map c++ const pointers to rust in this case, but the functions in question do take const pointers so that's what we need to use.

[Bromeon]

I see, thanks for elaborating! Then you could use sys::to_const_ptr(ptr) in the return tuple, to avoid as.

[Bromeon]

I'd use bool_to_sys. Even if it takes by value, being Copy means the original isn't consumed. And the shorter these functions are, the better...

Same for other similar ones.

[Bromeon]

Thanks! Nice discovery of cast_mut and cast_const 👍

I think a few safety statements could be slightly more concise, e.g.

// SAFETY: `arguments` was returned from a call to `as_mut_ptr` on a mutable slice, with length equal to `default_argument_count`. This
// length has not changed since it was created so `from_raw_parts_mut` is safe to call on it.

could be the following:

// SAFETY: 
// - `from_raw_parts_mut`: `arguments` comes from `as_mut_ptr()` on a mutable slice of length `default_argument_count`.
// - `Box::from_raw`: ...

(with line breaks if needed, I don't see it well in GitHub).

[Bromeon]

Suggested change

	// The slice was returned from a call to `Box::leak`, and we have ownership of the value behind this pointer, so we `Box::from_raw`
	// is safe to call on it.
	// The slice was returned from a call to `Box::leak`, and we have ownership of the value behind this pointer, so `Box::from_raw`
	// is safe to call on it.

(maybe obsolete with main comment)

[Bromeon]

Suggested change

	// The slice was returned from a call to `Box::leak`, and we have ownership of the value behind this pointer, so we `Box::from_raw`
	// is safe to call on it.
	// The slice was returned from a call to `Box::leak`, and we have ownership of the value behind this pointer, so `Box::from_raw`
	// is safe to call on it.

[lilizoey]

Thanks! Nice discovery of cast_mut and cast_const 👍

I think a few safety statements could be slightly more concise, e.g.

// SAFETY: `arguments` was returned from a call to `as_mut_ptr` on a mutable slice, with length equal to `default_argument_count`. This
// length has not changed since it was created so `from_raw_parts_mut` is safe to call on it.

could be the following:

// SAFETY: 
// - `from_raw_parts_mut`: `arguments` comes from `as_mut_ptr()` on a mutable slice of length `default_argument_count`.
// - `Box::from_raw`: ...

(with line breaks if needed, I don't see it well in GitHub).

Sure though i dont entirely see the benefit of collapsing the two unsafe blocks into one if we're just gonna separately document each call anyway?

[Bromeon]

Sure though i dont entirely see the benefit of collapsing the two unsafe blocks into one if we're just gonna separately document each call anyway?

Yeah, it's a bit unfortunate that unsafe { } is not just a marker but also introduces a scope, and this sometime requires weirder data flow.

In situations where that happens, another option would be:

// SAFETY: see inside.
unsafe {
    // SAFETY: ...
    let a = do_stuff();
    let b = do_other_stuff();

    // SAFETY: ...
    do_more(a, b);
}

but that might also be confusing. Every approach has some downsides... I didn't find a perfect one, so if you find one that works well enough, great 🙂

If the tuples are aggregated on a single line (using variables), that's also more readable than a multiline-tuple expression.

[lilizoey]

One way to at least be consistent is via the multiple_unsafe_ops_per_block lint which would at least force us to only put one unsafe operation in each block if enabled.

[Bromeon]

One way to at least be consistent is via the multiple_unsafe_ops_per_block lint which would at least force us to only put one unsafe operation in each block if enabled.

One problem is that our unsafe surface is huge due to the nature of our project being a binding, and there are different "danger levels". For example, we don't need to repeatedly justify calling a Godot C function, because we make sure it's mapped correctly with bindgen and the header file, and we check version compatibility at startup. With the preconditions checked, they might even be safe functions, but bindgen has no such option.

On the other hand, we do have to justify why reinterpreting pointers, assuming buffer lengths, etc. are sound, so unsafe is appropriate in these situations.

Making unsafe more annoying to use without addressing the above will likely lead to worse readability and higher maintenance effort.

[Bromeon]

I think the PR looks good, could you maybe group some of the commits? E.g. the two "add/use sys::conv" which are already mixed a bit. You can gladly leave those changing one logical things as individual commits.

[lilizoey]

One way to at least be consistent is via the multiple_unsafe_ops_per_block lint which would at least force us to only put one unsafe operation in each block if enabled.

One problem is that our unsafe surface is huge due to the nature of our project being a binding, and there are different "danger levels". For example, we don't need to repeatedly justify calling a Godot C function, because we make sure it's mapped correctly with bindgen and the header file, and we check version compatibility at startup. With the preconditions checked, they might even be safe functions, but bindgen has no such option.

On the other hand, we do have to justify why reinterpreting pointers, assuming buffer lengths, etc. are sound, so unsafe is appropriate in these situations.

Making unsafe more annoying to use without addressing the above will likely lead to worse readability and higher maintenance effort.

Well there are ways to counteract some of these issues. For instance a token system can work, where we check the preconditions once, and return a token of some kind. Then this token allows the user to safely call functions since it can only be created when the preconditions have been checked. But i dont know if such a rewrite is worth it. Worth considering though in some cases maybe. Maybe we can make interface_fn! or similar safer.