migueldeicaza / SwiftGodot

New Godot bindings for Swift
https://migueldeicaza.github.io/SwiftGodotDocs/tutorials/swiftgodot-tutorials/
MIT License
1.18k stars 77 forks source link

Revisiting `nil` semantics in `Variant` and `TypedArray` #576

Closed elijah-semyonov closed 1 week ago

elijah-semyonov commented 1 month ago

This PR contains the following changes, all of them are related to nil-correctness.

  1. Godot Variant with gtype == .nil should never surface in the user API. It will be always represented as Swift nil of Variant?. This allows direct integration of Swift nullability checks into Variant processing. We shouldn't have places where we have Variant?.some with Godot nil inside. This is accompanied with a lot of small code changes everywhere in the code to support this logic.
  2. All places that previously used Variant will now be Variant?.
  3. Improved ObjectCollection: TypedArray of objects can now work with nils (just like it does in GDScript).
  4. Godot Nil builtin class is omitted from translation.
  5. Some instrumentation for macro testing to make their updates less painful (generating a new macro test body and pasting it into clipboard, Mac only for now).
  6. New API inside Arguments to streamline gracious failing when Godot calls are done into @Callable or @Exported with mismatching arguments. It shouldn't crash the game/editor anymore and print errors instead.
  7. Update of MacroCallable and MacroExported for newer infrastructure and gracious failing if Godot calls into Swift-exported code with wrong arguments instead of crashing.
  8. Lots and lots of tests to cover all the changes.
migueldeicaza commented 2 weeks ago

The Windows build is breaking due to a change in Arguments:

2024-11-06T11:23:04.8173115Z D:\a\SwiftGodot\SwiftGodot\Sources\SwiftGodot\Core\Arguments.swift:110:38: error: value of type 'any Error' has no member 'localizedDescription'
2024-11-06T11:23:04.8174531Z 
2024-11-06T11:23:04.8174803Z                     fatalError(error.localizedDescription)
2024-11-06T11:23:04.8175316Z 
2024-11-06T11:23:04.8175514Z                                ~~~~~ ^~~~~~~~~~~~~~~~~~~~
elijah-semyonov commented 1 week ago

Oh, that's weird, probably an oversight during a merge-conflict resolution. Should be fixed.

elijah-semyonov commented 1 week ago

Aha, green now

elijah-semyonov commented 1 week ago

Something weird happens to CI. @migueldeicaza is there a way to trigger it manually?

migueldeicaza commented 1 week ago

This patch is catching a few mistakes in Godot for iPad, so that is very cool!

Some problems that we currently face:

Some usability challenges

I wonder if we could improve upon some of these.

Many APIs in Godot of the form:

Variant? getSomething()

Are now a bit obnoxious to us, it used to be:

if let text = String(foo.getSomething()) {

Now it needs to be:

if let variant = foo.getSomething(), let text = String(variant) {

I am scared of adding extension methods like "String(Variant?)" or variations because it would affect any place that passes a nil. But we could add it for some of the Godot-based types like "GDictionary", I have a patch here:

I did cheat and on my large code base, I added those extensions, but I am afraid of unleashing this into the unsuspecting public.

My current patch that I think is necessary:

https://gist.github.com/migueldeicaza/afd7c892bd9faa908fe49c159ee29f81

elijah-semyonov commented 1 week ago

I think something is broken on main branch.

How do you feel about:

extension Optional where Wrapped == Variant {
    public func into<T: VariantStorable>(_ type: T.Type = T.self) -> T? {
        if let self {
            return T.init(self)
        } else {
            return nil
        }
    }

    public func into<T: VariantStorable>(_ type: T.Type = T.self) -> T? where T: Object {
        if let self {
            return self.asObject()
        } else {
            return nil
        }
    }
}

func foo(_ variant: Variant?) {
    if let string = variant.into(String.self) {

    }
}
migueldeicaza commented 1 week ago

Another observation:

I think that we could annotate some APIs that we know would never pass a nil, like Node.getChildren and a handful of others, for usability purposes.