-
-
Notifications
You must be signed in to change notification settings - Fork 21.8k
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
[Draft] Add support for nullable types in GDScript #76843
base: master
Are you sure you want to change the base?
[Draft] Add support for nullable types in GDScript #76843
Conversation
118ab45
to
7a9bbfd
Compare
Will be fixed by #76842 in a few minutes (rebasing this PR now on |
I set the PR to draft, as discussed in the chat! |
This is amazing. Could you clarify what's the result of a nullable access operator when the variable is null? Null, I guess, but just in case. |
@RandomShaper Yep, null! class B:
var cool_property := "inner"
class A:
var b: B?
func _run():
var a: A? = null
# Try to get "cool_property" or return a default value otherwise
print(a?.b?.cool_property ?? "default") |
7a9bbfd
to
7fa3481
Compare
Absolutely amazing PR description like I said in the chat! 🔥🔥🔥 I haven't been able to follow the dev chat fully lately, so this might have already been discussed there. I do think it bears having that discussion in this more public way though, so here we go! :) As I've said in the chat, I personally generally dislike the implicit narrowing that happens as a side-effect of using control flow statements like var x : String? = null
case x:
null: # Return / do error handling / perform default behavior?
String: # use x as if it was a non-nullable string One-liner variants of this could also work, e.g. The var x : String? = ""
if x: # Cannot distinguish between x == "" or x == null
print("not empty")
else:
print("empty")
var y : int? = 10
while y: # Cannot distinguish from y == null or y == 0
y = y - 1
# might do something here that results in y == null
print(y) And then you have to have expression analysis be aware that the expression, depending on context, may contain implicit narrowing syntax. For example, what happens with the example below? Somewhere in the middle of it there's a lone var y : int? = 10
while y > 0 and y*x < x + 4 and y != null and other_weird_function(y):
do_stuff_with_y_that_must_be_non_null(y) Doesn't that result in code that's harder to maintain compared to having a statement that explicitly handles nullability (and nothing else like boolean/booleanable expressions)? I'm worried that newer folks coming into Godot/programming for the first time are gonna get either bit or just very confused by these subtle side-effects of program control flow statements. Maybe it's just me though! And if the consensus is that there are no issues with user friendliness and code-maintainability, then I'm happy :) |
Definitely a valid concern @anvilfolk! I think there are two points I feel strongly of on the implicit narrowing subject:
Just my two cents. |
@anvilfolk Forgot to address the |
You need to sync GDExtension with these changes I believe: godot/core/extension/gdextension_interface.h Line 137 in cf8ad12
|
@AThousandShips I knew I was missing something, even asked on #gdextension, thank you! |
@AThousandShips |
Oh sorry missed that, now that you mention it I did see it, then this must be due to lack of sync in godot-cpp, got myself confused looking at the main thread and forgot |
Where the PR would truly shine is with dictionaries. Unfortunately, |
So In that case I would need to have a PR adding the coalesce operator to the godot-cpp merged before merging this one? |
That's my confusion, it does seem a very strange situation as neither would work without the other, I might be missing something elsewhere, that's why I got confused as I assumed it must be possible to make changes that doesn't immediately work with godot-cpp |
Indeed... |
Looking at how the godot-cpp test is executed I think I really need to get the new operator merged upstream on godot-cpp before this PR can be merged: # Dump GDExtension interface and API
- name: Dump GDExtension interface and API for godot-cpp build
if: ${{ matrix.godot-cpp-test }}
run: |
${{ matrix.bin }} --headless --dump-gdextension-interface --dump-extension-api
cp -f gdextension_interface.h godot-cpp/gdextension/
cp -f extension_api.json godot-cpp/gdextension/ |
@ronsiv8 it's a draft, it's not done or ready, please have patience, people work as best they can on their contributions, and people can't always put in the time they want, or there's something they haven't solved yet |
oh i didnt notice the TODO |
This PR looks amazing. I have three question-suggestions.
class A:
var cool_value := "cool!"
func _run():
var nullable: A? = null
print(nullable!.cool_value) # Usafe but wont cause a parse error
var foo: int? = null
if var bar ?= foo:
print(bar) # Bar will only be printed if foo is not null and it will be non-nullable (int) This way you take advantage of both For clarity
data = data ?? 123
data ??= 123 # equivalent to the above Thank you, great work! |
|
||
// Special case for container types, forbid assigning a nullable typed array to a non-nullable one (Like assigning an "Array[int?]" to an "Array[int]") | ||
if (p_origin->get_datatype().has_container_element_type() && p_assigned_value->get_datatype().has_container_element_type()) { | ||
GDScriptParser::DataType identifier_data_type = p_origin->get_datatype().get_container_element_type(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the correct source for the datatype, p_origin->get_datatype()
? this block only seems to catch:
var a:Array[int?] = [1]
var b:Array[int] = a # analyzer error
and not the statement:
b = a # runtime error, no analyzer error
I changed all the p_origin->get_datatype()
references to identifier->get_datatype()
in this block and it sorta works?
It catches the other error instead now lol.
if (p_match_pattern == nullptr) { | ||
return; | ||
} | ||
|
||
#define SET_NULLABLE(value) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know Godot's style guide stance on lambdas but if ever there was a case for one...
GDScriptParser::DataType non_nullable_left_type = left_type; | ||
non_nullable_left_type.is_nullable = false; | ||
if (!is_type_compatible(non_nullable_left_type, right_type, false, p_coalesce_op)) { | ||
push_error(vformat(R"(Invalid operands "%s" and "%s" for "??" operator.)", left_type.to_string(), right_type.to_string()), p_coalesce_op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be configurable rather than a hard error since Variant
catches everything that gets through the type system. With it being configurable different projects can utilize whatever style suits their productivity.
I set the datatype to GDScriptParser::DataType::VARIANT
here and commented out the error for testing and it feels good to use rather than being told off.
@@ -1041,6 +1041,8 @@ void Variant::_register_variant_operators() { | |||
|
|||
register_op<OperatorEvaluatorObjectHasPropertyString>(Variant::OP_IN, Variant::STRING, Variant::OBJECT); | |||
register_op<OperatorEvaluatorObjectHasPropertyStringName>(Variant::OP_IN, Variant::STRING_NAME, Variant::OBJECT); | |||
|
|||
register_op<OperatorEvaluatorObjectHasPropertyStringName>(Variant::OP_IN, Variant::STRING_NAME, Variant::OBJECT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a merge error? I don't understand the duplication.
@Macksaur |
I'm giving it a spin on a live project trying to simplify the codebase with it. It's a fun PR, you've done great! However I find myself wanting class A:
var prop1:int
#var prop2:int
var obj:Variant = some_obj_a()
var which = obj.prop2 ?? 123 # undefined ?? 123 => 123 I will try to add any other comments on things as I encounter them during use of the PR. Take your time, thank you! |
This one might be out of scope and off-topic for the issue but I view the distinction of this arbitrary (even though it follows suit of C#) and would like to field some thoughts.
func _on_area_entered(area:Area2D) -> void:
(area.owner as Entity)?.on_long_grass += 1
However if I rewrote it as: func _on_area_entered(area:Area2D) -> void:
(area.owner as Entity)?.set_on_long_grass(1) ...it would be ok? Given that operators are syntactic sugar for function calls I'd expect to be able to edit: I made this modification locally and it's very pleasant to use. I also combined it with (These functions are attached to the basic object type in my game and can be overridden/customized in derived classes or left with good (sane) defaults most of the time.) |
should_skip = true; | ||
} else if (src->get_type() == Variant::OBJECT) { | ||
Object *obj = src->get_validated_object_with_check(should_skip); | ||
should_skip |= obj == nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this simplify to should_skip = (src->get_validated_object() == null);
? OPCODE_EXIT_IF_NULL
is the same.
Can you clarify the following error? As far as I understand it the null-conditional is a short-circuiting operator and the expression should terminate early with Taking a page from C# https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/member-access-operators#null-conditional-operators--and-: Am I misunderstanding the implemented behaviour or have I made an error in my rebase? |
The nullable access operator in this PR was inspired by JavaScript's, where it behaves as if you were conditionally accessing an inner property, based on whether the "parent" is or isn't falsy and no short-circuit is assumed. This could be changed however... it should be possible to implement the short-circuiting behavior C#'s null conditional operator has. |
Yeah in hindsight I don't see a good reason to disallow assignment to a nullable access. I'll do some tests and get back on it |
Huh! I didn't know that JS didn't short-circuit on chains! I admire their basic coalescing behaviours for truthy-values otherwise but I think not having short-circuiting here is a missed opportunity to express simpler (and faster) code. Since the GDScript VM isn't jitted/compiled to any machine code it is perhaps disadvantageous to inflict users with multiple redundant null-checks in the general syntax. Is there an advantage to the behaviour producing the following errors in JS that would be worth the distinction on every chain? |
I've pushed my hacky WIP branch master...Macksaur:godot:nullable_types where I've gave a go at a implementing some of these things to play with. You're welcome to yank any of it. I've included the weird |
Hi, could it be possible to change the position of the var foo: ?Vector2 = null It helps, among other things, spotting the nullable arguments when reading documentations |
This has been raised above, you should use the reactions on that particular reply to indicate your support or not for that syntax. I will however say that it is a non-traditional syntax and doesn't make sense in the way that you're suggesting. If you have other languages to compare syntaxes with, I'd like to read about them if you could link examples. |
I just wanted to leave my support and gratitude for your work. I'm very excited for when this becomes a standard feature. |
} | ||
|
||
if (p_should_be_false) { | ||
return { static_cast<GDScriptParser::IdentifierNode *>(unary_op->operand) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This crashes as there's no guarantees that not x
is always an identifier, it could be not self?.some_fn()
. This could be fixed by delegating to another call of return deduce_nullable_narrowing(unary_op->operand)
.
return false; | ||
} | ||
|
||
Vector<GDScriptParser::IdentifierNode *> GDScriptAnalyzer::deduce_nullable_narrowing(GDScriptParser::ExpressionNode *p_node, bool p_should_be_false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does p_should_be_false
mean? I took a stab in the dark and renamed it to p_current_op_is_wide
, which is a double-negative-ish of p_current_op_is_not_narrowed
. Is this the correct understanding?
Should it be named and simplified? i.e. p_should_be_false
-> p_current_op_is_narrowed
?
Might be worth to test with the new typed |
This looks like an exciting PR! I have a question about what “This PR is also not about making Reference Types (Like Objects) no longer implicitly null by default. Their behavior remains unchanged.” means. Does it mean that the declarations Even if there were no nullable annotations at all, the new syntactic sugar for dealing with nulls would be plenty exciting on its own. I would like to float the idea of adding a unary postfix operator which generalizes the C# has all of the syntactic sugar described in this PR (null coalescing operators, null access operators, and ENNS syntax). These features all exist to safely mitigate the so-called “pyramid of doom,” where you have to write many lines of code to check if a single operation can be called safely. In my experience, these features are great but frequently still insufficient to avoid the problem. After all these features are implemented, by far the most common null-related pain point for me is functions with non-nullable parameters. Imagine I’m designing a strategy game with the following class:
Notice that neither the However, suppose I want to call this function in a context where I can't know for sure that I will have a valid target (for example, if the target dodged my attack by moving to another space, or if another attack defeated the target earlier in the turn) or even if I’ll have a weapon equipped. In that case, depending on the implementation of ENNS (here represented by
or two ugly lines:
In my experience, this sort of boilerplate is ubiquitous and annoying in C#, and it creates a temptation to avoid declaring functions with non-nullable parameters even when they would be most appropriate. A
In this picture, each line is numbered according to the order in which the expressions will evaluate. The expected type of each expression's parameters, if any, is written in parentheses after the expression's name. The return type of the expression follows the First, it makes it clear how the The tree representation also shows what I mean when I say that As with I’ve never seen another language with a feature like the First, it effectively precludes the possibility of implementing the Another potential objection to I can imagine places where people might use |
The addition of the nullable access and nullable coalescing operators are exciting! Are they something that could be added separate from this PR? They'd work on any values that could be I'm with @unfallible in being uncertain how these lines differ: var my_node: Node2D = $Child
var my_node: Node2D? = $Child I assume "inclusion of nullity safety guards" means that only variables declared as nullable (the second line) will cause parse errors when they're not null-checked. (That safety check sounds awesome!) Are there other differences? Could I pass both of them to a function expecting |
I personally hadn't considered separating the two ideas but this is definitely a proposal worth considering! The operators don't rely on type information to work, they just check the variant for
In semantics there is only What that means is that
This works. The other way around however... that will produce lots of errors at compile/parse time! |
Because arrays are passed by reference and mutable, passing
Also, there was a much smaller discussion in proposal #11054, and the consensus there seemed to be that until |
This will definitely need lots of test cases like the above 😆 If nullable makes it into arrays hopefully we can figure out a clean way to cast/copy that's not too verbose, otherwise nullable types end up poisoning the call graph rather than extending type support. |
Nullable Types in GDScript
This PR aims to add a feature-set to GDScript tailored towards the safe usage of potentially-null expressions.
It was initially based off godotengine/godot-proposals#162 and through several discussions over RocketChat it grew to be what it is currently. Further feedback is wholeheartedly appreciated!
TODOs
Convert nullable errors to(Won't consider for now)UNSAFE_*
warningsPROPERTY_USAGE_NULLABLE
to thePropertyUsageFlags
Merge https://github.com/godotengine/godot-cpp/pull/1110`Re-enable "Build godot-cpp test extension" before mergingWhat it is
This PR is:
The addition of the nullable qualifier, an optional type qualifier that indicates whether a given type is nullable (which can be interpreted as a union type of the type itself + null, like:
String?
, which would be a union type ofString | Null
). Usages of the nullable qualifier are:The inclusion of nullity safety guards on several expressions to ensure nullable types are handled in a safe manner. These safety guards were added to the following expressions:
The implicit narrowing of nullable identifiers, which ensures that nullable identifiers can be safely handled (without safety errors) if properly checked beforehand. This currently applies to the following expressions:
The addition of the nullable access operator (
?.
and?[]
), which ensures subscript accesses are safe even on null values by only accessing the right-hand attribute if the left-hand base is not null, otherwise by returning null:The inclusion of the nullable coalescing operator (
??
), which allows a nullable expression to be coalesced into another expression, essentially allowing nullable expressions to have default values. Both operands must have the same type (but not necessarily the same nullity) and Resource Types (Objects) are also supported:What it is not
This PR is not about making the core null-aware or even updating the API docs to reflect instances where null might be a valid argument/return.
This PR is also not about making Reference Types (Like Objects) no longer implicitly null by default. Their behavior remains unchanged.
Next steps
Next steps for improvements included:
if bar ?= foo
syntax:for bar in foo
:Converting the nullable related errors toUNSAFE_*
warnings as suggested by @dalexeev. (Will do next)Closes godotengine/godot-proposals#162
Closes godotengine/godot-proposals#1321