migueldeicaza / SwiftGodot

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

`VariantCollection` crashes when working with `ContentTypeStoring` types #392

Closed tishin closed 6 months ago

tishin commented 6 months ago

A simple append is enough to reproduce the crash:

let c = VariantCollection<PackedVector2Array>()
c.append(value: PackedVector2Array())

This probably happens due to incorrect usage of Variant.init:

init<T: VariantRepresentable>(representable value: T) where T: ContentTypeStoring
init<T: VariantRepresentable>(representable value: T)

VariantCollection<PackedVector2Array> invokes the latter while PackedVector2Array should go through the former.

migueldeicaza commented 6 months ago

Ugh - I don't like this.

I can't try this right now, but would a patch along these lines work?

diff --git a/Sources/SwiftGodot/Core/VariantCollection.swift b/Sources/SwiftGodot/Core/VariantCollection.swift
index 090a8f7..53f5ee3 100644
--- a/Sources/SwiftGodot/Core/VariantCollection.swift
+++ b/Sources/SwiftGodot/Core/VariantCollection.swift
@@ -124,6 +124,11 @@ public class VariantCollection<Element: VariantStorable>: Collection, Expressibl
     public final func append (value: Element) {
         array.append (value: Variant(value))
     }
+
+    /// Appends an element at the end of the array (alias of ``pushBack(value:)``).
+    public final func append (value: Element) where Element: ContentTypeStoring {
+       array.append (value: Variant(value))
+    }

     /// Resizes the array to contain a different number of elements. If the array size is smaller, elements are cleared, if bigger, new elements are `null`. Returns ``GodotError/ok`` on success, or one of the other ``GodotError`` values if the operation failed.
     ///
migueldeicaza commented 6 months ago

I have added the above patch, and the tests so far seem to work - but I also added the other patch for the Variant fix.

tishin commented 6 months ago
public final func append (value: Element) where Element: ContentTypeStoring {
   array.append (value: Variant(value))
}

This only fixed append. There's still pushFront, pushBack, insert, etc. causing the same crash. I made a PR that should address the issue more generally.

migueldeicaza commented 6 months ago

Thank you so much - I wanted to see if this was the fix, and then apply the rule across the board. But I was really uneasy about how easily it would be to miss a case.

I am so happy to have seen your other PR that solves this at a more fundamental level. Thank you Mikhail.