Skip to content

Commit

Permalink
Reference types are now mostly optional.
Browse files Browse the repository at this point in the history
Godot sadly does not annotate their functions and properties to
include information whether they are optional or not.  This is an
issue that has been raised a long time ago and has gained little to no
traction:

godotengine/godot-proposals#2241

This poses a problem, because certain Godot APIs would return nil, and
I would try to create an object out of it, and crash, while a nil
return is ok.

It also posed a problem for APIs that could take a nil parameter, but
instead, in SwiftGodot, you had to create an instance of the object,
even if it is unnecesary or not the desired outcome.

So I have now settled on an evolutionary plan:

* For now, all reference types that could have been nil either as a
  parameter or a return value, are declared as swift optionals.

* I have introduced a hardcoded list of method return types and method
  arguments that can be used to flag exceptions to this: scenarios
  where we can verify that the return value would never be nil, or the
  parameter demands a non-nil argument.

The list currently is hardcoded in Swift, but will eventually move to
a Json file to make it easier to work with.

Sadly, this means that there will be some churn, until all this is
properly validated.

This is a fix for this issue:

#63

Some additional data:

* Some 114 property returns were affected
* Some 432 method returns were affected
  • Loading branch information
migueldeicaza committed May 25, 2023
1 parent 35480c8 commit 8b76a62
Show file tree
Hide file tree
Showing 6 changed files with 117 additions and 17 deletions.
4 changes: 2 additions & 2 deletions Generator/Generator/Arguments.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func isSmallInt (_ arg: JGodotArgument) -> Bool {
}
}

func getArgumentDeclaration (_ argument: JGodotArgument, eliminate: String, kind: ArgumentKind = .classes) -> String {
func getArgumentDeclaration (_ argument: JGodotArgument, eliminate: String, kind: ArgumentKind = .classes, isOptional: Bool) -> String {
//let optNeedInOut = isCoreType(name: argument.type) ? "inout " : ""
let optNeedInOut = ""

Expand Down Expand Up @@ -52,7 +52,7 @@ func getArgumentDeclaration (_ argument: JGodotArgument, eliminate: String, kind
}
}
}
return "\(eliminate)\(godotArgumentToSwift (argument.name)): \(optNeedInOut)\(getGodotType(argument, kind: kind))\(def)"
return "\(eliminate)\(godotArgumentToSwift (argument.name)): \(optNeedInOut)\(getGodotType(argument, kind: kind))\(isOptional ? "?" : "")\(def)"
}

func getArgRef (arg: JGodotArgument) -> String {
Expand Down
4 changes: 2 additions & 2 deletions Generator/Generator/BuiltinGen.swift
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func generateBuiltinCtors (_ p: Printer,

for arg in m.arguments ?? [] {
if args != "" { args += ", " }
args += getArgumentDeclaration(arg, eliminate: "", kind: .builtInField)
args += getArgumentDeclaration(arg, eliminate: "", kind: .builtInField, isOptional: false)
}

// Find the document for this constructor
Expand Down Expand Up @@ -348,7 +348,7 @@ func generateBuiltinMethods (_ p: Printer,

for arg in m.arguments ?? [] {
if args != "" { args += ", " }
args += getArgumentDeclaration(arg, eliminate: "")
args += getArgumentDeclaration(arg, eliminate: "", isOptional: false)
}

if let docClass, let methods = docClass.methods {
Expand Down
27 changes: 24 additions & 3 deletions Generator/Generator/ClassGen.swift
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,12 @@ func generateVirtualProxy (_ p: Printer,
return
}
let virtRet: String?
var returnOptional = false
if let ret = method.returnValue {
let godotReturnType = ret.type
let godotReturnTypeIsReferenceType = classMap [godotReturnType] != nil
returnOptional = godotReturnTypeIsReferenceType && isReturnOptional(className: cdef.name, method: methodName)

virtRet = getGodotType(ret)
} else {
virtRet = nil
Expand Down Expand Up @@ -145,8 +150,18 @@ func generateVirtualProxy (_ p: Printer,
} else if ret.type.starts(with: "enum::") {
p ("retPtr!.storeBytes (of: Int32 (ret.rawValue), as: Int32.self)")
} else {
let derefField: String
let derefType: String
if classMap [ret.type] != nil {
derefField = "handle"
derefType = "UnsafeRawPointer?.self"
} else {
derefField = "content"
derefType = "type (of: ret.content)"
}

let target = classMap [ret.type] != nil ? "handle" : "content"
p ("retPtr!.storeBytes (of: ret.\(target), as: type (of: ret.\(target)))")
p ("retPtr!.storeBytes (of: ret\(returnOptional ? "?" : "").\(derefField), as: \(derefType))")

// Poor man's transfer the ownership: we clear the content
// so the destructor has nothing to act on, because we are
Expand Down Expand Up @@ -322,9 +337,15 @@ func generateProperties (_ p: Printer,
print ("WARNING \(loc) Could not get a return type for method")
continue
}
let godotReturnType = method.returnValue?.type
let godotReturnTypeIsReferenceType = classMap [godotReturnType ?? ""] != nil

let propertyOptional = godotReturnTypeIsReferenceType && isReturnOptional(className: cdef.name, method: property.getter)

// Lookup the type from the method, not the property,
// sometimes the method is a GString, but the property is a StringName
type = getGodotType (method.returnValue)
type = getGodotType (method.returnValue) + (propertyOptional ? "?" : "")


// Ok, we have an indexer, this means we call the property with an int
// but we need the type from the method
Expand Down Expand Up @@ -445,7 +466,7 @@ func generateSignalType (_ p: Printer, _ cdef: JGodotExtensionAPIClass, _ signal
lambdaIgnore += ", "
lambdaFull += ", "
}
args += getArgumentDeclaration(arg, eliminate: "_ ")
args += getArgumentDeclaration(arg, eliminate: "_ ", isOptional: false)
let construct: String

if let cmap = classMap [arg.type] {
Expand Down
89 changes: 82 additions & 7 deletions Generator/Generator/MethodGen.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,57 @@ enum MethodGenType {
case `class`
case `utility`
}

// To test the design, will use an external file later
// determines whether the className/method returns an optional reference type
func isReturnOptional (className: String, method: String) -> Bool {
switch className {
case "RenderingServer":
switch method {
case "get_rendering_device":
return false
default:
return true
}
default:
return true
}
}

// To test the design, will use an external file later
// determines whether the className/method/argument is an optional reference type
func isRefParameterOptional (className: String, method: String, arg: String) -> Bool {
switch className {
case "Node":
switch method {
case "_input":
switch arg {
case "event":
return false
default:
return true
}
default:
return true
}
return true
case "Image":
switch method {
case "blit_rect":
switch arg {
case "src":
return false
default:
return true
}
default:
return true
}
default:
return true
}
}

/// Generates a method definition
/// - Parameters:
/// - p: Our printer to generate the method
Expand Down Expand Up @@ -99,7 +150,11 @@ func methodGen (_ p: Printer, method: MethodDefinition, className: String, cdef:
if let margs = method.arguments {
for arg in margs {
if args != "" { args += ", " }
args += getArgumentDeclaration(arg, eliminate: eliminate)
var isRefOptional = false
if classMap [arg.type ?? ""] != nil {
isRefOptional = isRefParameterOptional (className: className, method: method.name, arg: arg.name)
}
args += getArgumentDeclaration(arg, eliminate: eliminate, isOptional: isRefOptional)
var reference = escapeSwift (snakeToCamel (arg.name))

if method.isVararg {
Expand Down Expand Up @@ -132,6 +187,8 @@ func methodGen (_ p: Printer, method: MethodDefinition, className: String, cdef:
var argref: String
var optstorage: String
var needAddress = "&"
var isRefParameter = false
var refParameterIsOptional = false
if method.isVararg {
argref = "copy_\(arg.name)"
optstorage = ".content"
Expand All @@ -149,14 +206,22 @@ func methodGen (_ p: Printer, method: MethodDefinition, className: String, cdef:
if builtinSizes [arg.type] != nil && arg.type != "Object" || arg.type.starts(with: "typedarray::"){
optstorage = ".content"
} else {
// The next two are unused, because we set isRefParameter,
// but for documentation/clarity purposes
optstorage = ".handle"
// No need to take the address for handles
needAddress = ""
isRefParameter = true

refParameterIsOptional = isRefParameterOptional (className: className, method: method.name, arg: arg.name)
}
}
}
argSetup += " UnsafeRawPointer(\(needAddress)\(escapeSwift(argref))\(optstorage)),"
// }
argSetup += " "
if refParameterIsOptional {
argSetup += "UnsafeRawPointer(\(escapeSwift(argref))?.handle ?? nil),"
} else {
argSetup += "UnsafeRawPointer(\(needAddress)\(escapeSwift(argref))\(optstorage)),"
}
}
argSetup += "]"
argSetup += varArgSetupInit
Expand All @@ -172,7 +237,9 @@ func methodGen (_ p: Printer, method: MethodDefinition, className: String, cdef:
}

let godotReturnType = method.returnValue?.type
let returnType = getGodotType (method.returnValue)
let godotReturnTypeIsReferenceType = classMap [godotReturnType ?? ""] != nil
let returnOptional = godotReturnTypeIsReferenceType && isReturnOptional(className: className, method: method.name)
let returnType = getGodotType (method.returnValue) + (returnOptional ? "?" : "")

if inline != "" {
p (inline)
Expand Down Expand Up @@ -204,7 +271,7 @@ func methodGen (_ p: Printer, method: MethodDefinition, className: String, cdef:
} else if godotReturnType == "String" {
p ("var _result = GString ()")
} else {
if classMap [godotReturnType ?? ""] != nil {
if godotReturnTypeIsReferenceType {
frameworkType = true
p ("var _result = UnsafeRawPointer (bitPattern: 0)")
} else {
Expand Down Expand Up @@ -273,7 +340,15 @@ func methodGen (_ p: Printer, method: MethodDefinition, className: String, cdef:
fatalError("Do not support this return type")
}
} else if frameworkType {
p ("return lookupObject (nativeHandle: _result!)")
//print ("OBJ RETURN: \(className) \(method.name)")
p ("guard let _result else") {
if returnOptional {
p ("return nil")
} else {
p ("fatalError (\"Unexpected nil return from a method that should never return nil\")")
}
}
p ("return lookupObject (nativeHandle: _result)")
} else if godotReturnType?.starts(with: "typedarray::") ?? false {
let defaultInit = makeDefaultInit(godotType: godotReturnType!, initCollection: "content: _result")

Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftGodot/Extensions/GD+Utils.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ extension GD {
///
/// - Parameter path: Path of the `Resource` to load.
/// - Returns: The loaded `Resource`.
public static func load(path: String) -> Resource {
public static func load(path: String) -> Resource? {
return ResourceLoader().load(path: path, cacheMode: .reuse)
}

Expand Down
8 changes: 6 additions & 2 deletions Sources/SwiftGodot/Variant.swift
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,11 @@ public class Variant: Hashable, Equatable, ExpressibleByStringLiteral {
Variant.fromTypeMap [GType.rid.rawValue] (&content, &value.content)
}

public init (_ value: Object) {
public init (_ value: Object?) {
guard let value else {
Variant.fromTypeMap [GType.object.rawValue] (&content, UnsafeMutableRawPointer(mutating: nil))
return
}
Variant.fromTypeMap [GType.object.rawValue] (&content, UnsafeMutableRawPointer (mutating: value.handle))
}

Expand Down Expand Up @@ -300,7 +304,7 @@ public class Variant: Hashable, Equatable, ExpressibleByStringLiteral {
///
/// Attempts to cast the Variant into a GodotObject, this requires that the Variant value be of type `.object`.
/// - Returns: nil on error, or the type on success
///
///
public func asObject<T:GodotObject> () -> T? {
guard gtype == .object else {
return nil
Expand Down

0 comments on commit 8b76a62

Please sign in to comment.