Callable::from_local_fn() + Callable::from_sync_fn()

[Bromeon]

Closes #588.

Deprecates Callable::from_fn() which required Send + Sync bounds.
You can now choose:

1. For thread-local callables, use from_local_fn().
   • No bounds.
   • Fails when invoked on other thread (no panic propagated to caller at the moment).
2. For cross-thread callables, use from_sync_fn().
   • Requires Send + Sync.
   • Requires experimental-threads crate feature.
   • Can be called anywhere, passed around in Godot freely, etc.

Also fixes an issue with FFI threading check (if a panic was already in progress, the next check would fail hard, aborting without ever printing the first panic's message).

[GodotRust]

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

[Houtamelo]

1. For thread-local callables, use `from_local_fn()`.
   
   * No bounds.
   * Fails when invoked on other thread (no panic propagated to caller at the moment).

I'd argue for a more relaxed approach:

1. For thread-local callables, use from_local_fn().
   • No bounds.
   • In Debug mode: Fails when invoked on other thread (no panic propagated to caller at the moment).
   • In Release mode: Runs normally, but prints a warning when invoked on other threads.

Rationale:

1. The function can actually be sync:

   • The programmer may have chosen the wrong signature, or refactored code to the point that the function became sync but forgot to update the signature (to use from_sync_fn instead).
   • False positive: The Rust compiler can't prove that the function is sync (E.g: the function carries a pointer but doesn't actually de-reference it).

2. Even if the function is not sync:

   • Invoking it does not necessarily causes undefined behavior.
   • Undefined behavior does not necessarily breaks the game, it may result in no changes or just small/harmless bugs.
   • Crashing/Breaking the game is the worst outcome for the user experience.

3. In any case, invoking a non-sync function in a different thread should be avoided, and my suggestion still reports this to the developer.

[Bromeon]

Hm, that's a more general philosophical question about how we handle errors. Often, godot-rust comes with a "ergonomic but fatal" option (Gd::cast, Object::call, FromGodot::from_godot) and an additional fallible one (Gd::try_cast, Object::try_call, FromGodot::try_from_godot) whose result can explicitly be ignored. But in many of these cases it's not possible to provide an actual result -- even GDScript returns null on failed function calls -- which may not be storable in the result type. (More about this here).

So if I understand your argument right, "runs on wrong thread" is not an error that should be treated the same as other errors, like bad type conversions. Probably you're right -- the other argument to that is that over time, we may want to remove certain runtime checks in Release mode, for performance.

I was actually considering to add a method to ExtensionLibrary which would let the user return a global "error check level":

• Full (check as much as possible, but expensive)
• Minimal (only check certain things like object validity)
• Off (no checks, immediate UB on mistakes)

And maybe Callable::call() could be governed by that as well. The Release default would be either Minimal or Off, and maybe those can disable thread checks 🤔

Threading in godot-rust still has several unknowns (#18), and it's very hard to achieve consistent yet useful guarantees given how loosely Godot itself handles threads. Ideally we can achieve at least internally consistent behavior (between Callable::call and Object::call, among others).

[Houtamelo]

was actually considering to add a method to ExtensionLibrary which would let the user return a global "error check level":

* Full (check as much as possible, but expensive)

* Minimal (only check certain things like object validity)

* Off (no checks, immediate UB on mistakes)

And maybe Callable::call() could be governed by that as well. The Release default would be either Minimal or Off, and maybe those can disable thread checks

That does sound nice.