Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected non-optional #63

Closed
nvanfleet opened this issue May 24, 2023 · 6 comments
Closed

Unexpected non-optional #63

nvanfleet opened this issue May 24, 2023 · 6 comments

Comments

@nvanfleet
Copy link
Contributor

nvanfleet commented May 24, 2023

I noticed that there are some variables in SwiftGodot which are shown as non optional.

For example GeometryInstance3D.materialOverride. In Godot this is generally nil unless you assign it but it appears as if SwiftGodot assumes it's populated? In the code of GeometryInstance3D it appears to be force unwrapping it instead of just making it an optional value. I kind of think this might crash if you ever check it when there is no value present.

There are numerous things that are optional in Godot and don't have an default value and should just be nil.

This also works for MeshInstance3D. getSurfaceOverrideMaterial() or even MeshInstance3D.mesh

@migueldeicaza
Copy link
Owner

Agreed, this information is sadly not present in Godot. I suspect the only correct thing to do today is to flag both returns and parameters as optional and slowly introduce an annotation system to flag their nullability

@migueldeicaza
Copy link
Owner

this is not a new problem, sadly:

godotengine/godot-proposals#2241

@migueldeicaza
Copy link
Owner

As I ponder this challenge, I think that it would be relatively innocuous to make all arguments optional, from an API usability, it might not be bad, but we would get some exceptions at runtime - just like C# or GDScript would.

The return values are a bit more complicated, there are some 545 of those spread across functions and properties.

The properties we might be able to write a script to instantiate every object type in Godot, and attempt to set/read the value to null, and those that abort, we can safely flag as non-Optional, the rest we flag as Optional.

Maybe a script to track every constructor that initializes 'Ref' fields beyond the default might be useful to determine if nulls are allowed or not.

@migueldeicaza
Copy link
Owner

Update, I did some measurements:

  • We have some 114 properties that would need to be audited
  • 432 method returns that would need to be audited

@migueldeicaza
Copy link
Owner

Ok, I have settled on a plan.

The baseline is that all parameters that are object references can be optional, this matches the C# behavior, and will produce runtime errors as opposed to compile time errors for those cases.

The same applies for return values.

That said, I now have a list of custom return values and parameters that can be manually be extended to flag "This return type is never nil" or "this parameter can never be nil" that we can grow over time.

The bad news is that this will likely produce some churn on the code that relies on this, but it is also the right thing to do.

migueldeicaza added a commit that referenced this issue May 25, 2023
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
@nvanfleet
Copy link
Contributor Author

Neat thanks very much.

migueldeicaza added a commit that referenced this issue Sep 21, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants