Revisit indexing operators and fix leaks and crashes (#548)

* Fixes #543 (Godot String via Variant subscript operator leaks) and #531 (Variant crash)

* Reduce performance checks iterations x10.

* Fix dictionary leaks and a crash when dictionary[key] = nil

.swiftpm/xcode/xcshareddata/xcbaselines/SwiftGodotTests.xcbaseline/53914494-654F-4AE6-BFE1-6777D0E5FBA3.plist

@@ -11,7 +11,7 @@
  <key>com.apple.XCTPerformanceMetric_WallClockTime</key>
  <dict>
  <key>baselineAverage</key>
- <real>2.190000</real>
+ <real>0.223000</real>
  <key>baselineIntegrationDisplayName</key>
  <string>Local Baseline</string>
  </dict>
@@ -21,7 +21,7 @@
  <key>com.apple.XCTPerformanceMetric_WallClockTime</key>
  <dict>
  <key>baselineAverage</key>
- <real>4.504656</real>
+ <real>0.446000</real>
  <key>baselineIntegrationDisplayName</key>
  <string>Local Baseline</string>
  </dict>
@@ -31,7 +31,7 @@
  <key>com.apple.XCTPerformanceMetric_WallClockTime</key>
  <dict>
  <key>baselineAverage</key>
- <real>3.270000</real>
+ <real>0.332172</real>
  <key>baselineIntegrationDisplayName</key>
  <string>Local Baseline</string>
  </dict>

Generator/Generator/BuiltinGen.swift

@@ -508,28 +508,30 @@ func generateBuiltinMethods (_ p: Printer,
  p.staticVar (visibility: "private ", name: "keyed_checker", type: "GDExtensionPtrKeyedChecker") {
  p ("return gi.variant_get_ptr_keyed_checker (\(variantType))!")
  }
- p ("public subscript (key: Variant) -> Variant?") {
-  p ("get") {
-  p ("let keyCopy = key")
- p ("var result = Variant.zero")
- p ("if Self.keyed_checker (&content, &keyCopy.content) != 0") {
- p ("Self.keyed_getter (&content, &keyCopy.content, &result)")
- p ("return Variant(copying: result)")
- }
- p ("else") {
- p ("return nil")
+ p("""
+ public subscript(key: Variant) -> Variant? {
+ get {
+ var result = Variant.zero
+ if Self.keyed_checker(&content, &key.content) != 0 {
+ Self.keyed_getter (&content, &key.content, &result)
+ // Returns unowned handle
+  return Variant(takingOver: result)
+ } else {
+ return nil
  }
  }
- p ("set") {
- p ("let keyCopy = key")
- p ("if let newCopy = newValue") {
- p ("Self.keyed_setter (&content, &keyCopy.content, &newCopy.content)")
- }
- p ("else") {
- p ("Self.keyed_setter (&content, &keyCopy.content, nil)")
- }
+ 
+ set { 
+ if let newValue {
+ Self.keyed_setter(&content, &key.content, &newValue.content)
+ } else { 
+ var nilContent = Variant.zero
+ // nil will cause a crash, needs a pointer to Nil Variant content instead
+ Self.keyed_setter(&content, &key.content, &nilContent)
+ } 
  }
  }
+ """) 
  }
  if let returnType = bc.indexingReturnType, !bc.isKeyed, !bc.name.hasSuffix ("Array"), bc.name != "String" {
  let godotType = getGodotType (JGodotReturnValue (type: returnType, meta: nil))

Sources/SwiftGodot/Core/GArray.swift

@@ -28,14 +28,25 @@ extension GArray {
  return Variant()
  }
  let ptr = ret.assumingMemoryBound(to: Variant.ContentType.self)
+
+ // We are making a copy of the variant managed by the array. Array is managing its copy, we are managing ours
  return Variant(copying: ptr.pointee)
  }
  set {
  guard let ret = gi.array_operator_index (&content, Int64 (index)) else {
  return
  }
  let ptr = ret.assumingMemoryBound(to: Variant.ContentType.self)
- ptr.pointee = newValue.content
+
+ guard ptr.pointee != newValue.content else {
+ return
+ }
+
+ // We are taking the variant from the array at the `index` and assuming control over it. Since we don't need it, we just destroy it
+ gi.variant_destroy(ptr)
+
+ // We are giving array a copy of `newValue` Variant to manage
+ gi.variant_new_copy(ptr, &newValue.content)
  }
  }
 

Sources/SwiftGodot/Variant.swift

@@ -273,7 +273,8 @@ public class Variant: Hashable, Equatable, CustomDebugStringConvertible {
  if valid == 0 || oob != 0 {
  return nil
  }
- return Variant(copying: _result)
+
+ return Variant(takingOver: _result)
  }
  set {
  guard let newValue else {

Tests/SwiftGodotTests/MarshalTests.swift

@@ -39,7 +39,7 @@ final class MarshalTests: GodotTestCase {
  let child = TestVariant()
 
  measure {
- for _ in 0..<10000000 {
+ for _ in 0..<1_000_000 {
  tv.addChild(node: child)
  tv.removeChild(node: child)
  }
@@ -58,7 +58,7 @@ final class MarshalTests: GodotTestCase {
  let weight = 0.23
 
  measure {
- for _ in 0..<10000000 {
+ for _ in 0..<1_000_000 {
  let _ = a.cubicInterpolate(b: b, preA: preA, postB: preB, weight: weight)
  }
  }
@@ -68,7 +68,7 @@ final class MarshalTests: GodotTestCase {
  let randomValues = (0..<10).map { _ in Variant(Float.random(in: -1.0...1.0)) }
 
  measure {
- for _ in 0..<1000000 {
+ for _ in 0..<100_000 {
  let _ = GD.max(arg1: randomValues[0], arg2: randomValues[1], randomValues[2], randomValues[3], randomValues[4], randomValues[5], randomValues[6], randomValues[7], randomValues[8], randomValues[9])
  }
  }