Add nullable static types to GDScript. - #1335
Open
mcdubhghlas wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (42)
📒 Files selected for processing (54)
🚧 Files skipped from review as they are similar to previous changes (51)
WalkthroughNullable type syntax, flow-sensitive non-null narrowing, nullable-aware bytecode execution, unsafe-access warnings, and analyzer and runtime tests are added across GDScript. ChangesNullable GDScript support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GDScriptParser
participant GDScriptAnalyzer
participant GDScriptCompiler
participant GDScriptVM
GDScriptParser->>GDScriptAnalyzer: provide nullable type information
GDScriptAnalyzer->>GDScriptAnalyzer: narrow identifiers and validate nullable access
GDScriptAnalyzer->>GDScriptCompiler: provide checked nullable types
GDScriptCompiler->>GDScriptVM: emit nullable assignment and return metadata
GDScriptVM->>GDScriptVM: accept NIL for nullable targets or enforce validation
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/gdscript/gdscript_analyzer.cpp`:
- Around line 970-975: Update the nested-enum resolution paths for
Variant.SomeEnum and built-in nested enums so they pass through the existing
nullable finalization block guarded by p_type->is_nullable, allowing annotations
such as Vector2.Axis? to set result.is_nullable. Preserve the current Variant
rejection behavior, and add a regression test covering nullable nested enums.
- Around line 2090-2098: Update the IF handling in the non-null narrowing logic
to use the absence of an exiting false branch rather than requiring false_block
to be null. Replace the false_block-null condition in the true_exits branch with
the corresponding !false_exits check, preserving the existing symmetric branch
and retaining the non-null fact after an else whose path does not exit.
In `@modules/gdscript/gdscript_function.h`:
- Around line 85-87: Complete nullable handling across all listed sites: in
modules/gdscript/gdscript_function.h:85-87, accept nullable Variant::NIL before
switching on GDScriptDataType::kind; in
modules/gdscript/gdscript_analyzer.cpp:4971-4975 and :5098-5102, emit
nullable-access warnings for non-meta object/class bases in attribute and index
access; in :6386-6398, reject nullable sources assigned to non-nullable targets
before object polymorphism checks while preserving nil acceptance for nullable
targets. Add regressions covering Node? and script-class assignment,
parameter/return transfers, null values, and member access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eb70ba44-2981-44d8-bd62-91969edb0ad2
⛔ Files ignored due to path filters (40)
modules/gdscript/tests/scripts/analyzer/errors/nullable_array_element.outis excluded by!**/*.outmodules/gdscript/tests/scripts/analyzer/errors/nullable_dictionary_key_element.outis excluded by!**/*.outmodules/gdscript/tests/scripts/analyzer/errors/nullable_dictionary_value_element.outis excluded by!**/*.outmodules/gdscript/tests/scripts/analyzer/errors/nullable_variant.outis excluded by!**/*.outmodules/gdscript/tests/scripts/analyzer/warnings/nullable_and_false_no_narrow.outis excluded by!**/*.outmodules/gdscript/tests/scripts/analyzer/warnings/nullable_break_guard_no_narrow.outis excluded by!**/*.outmodules/gdscript/tests/scripts/analyzer/warnings/nullable_compound_assignment.outis excluded by!**/*.outmodules/gdscript/tests/scripts/analyzer/warnings/nullable_continue_guard_no_narrow.outis excluded by!**/*.outmodules/gdscript/tests/scripts/analyzer/warnings/nullable_narrowing_limits.outis excluded by!**/*.outmodules/gdscript/tests/scripts/analyzer/warnings/nullable_or_true_no_narrow.outis excluded by!**/*.outmodules/gdscript/tests/scripts/analyzer/warnings/nullable_unsafe_operations.outis excluded by!**/*.outmodules/gdscript/tests/scripts/analyzer/warnings/nullable_unsafe_transfer.outis excluded by!**/*.outmodules/gdscript/tests/scripts/analyzer/warnings/unsafe_nullable_access.outis excluded by!**/*.outmodules/gdscript/tests/scripts/parser/errors/nullable_void_return.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/errors/nullable_enum_null_transfer.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/errors/nullable_null_reaches_argument.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/errors/nullable_null_reaches_typed_local.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/errors/nullable_null_use.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/errors/nullable_return_null_from_nonnull.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/errors/nullable_typed_array_assign_mismatch.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/errors/nullable_typed_array_reassign_mismatch.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/errors/nullable_typed_dictionary_assign_mismatch.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/errors/nullable_typed_dictionary_reassign_mismatch.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/errors/nullable_typed_dictionary_return_mismatch.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/errors/nullable_typed_return_mismatch.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_empty_callable.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_enum_raw_integer.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_enum_transfer.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_freed_object.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_multivar_and.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_multivar_or_guard.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_narrowing.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_narrowing_mutation.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_not_compound_or.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_renarrow_after_reassign.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_self_reference_assignment.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_typed_container_returns.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_types.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_variant_null_return.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_while_body_mutation.outis excluded by!**/*.out
📒 Files selected for processing (51)
modules/gdscript/gdscript_analyzer.cppmodules/gdscript/gdscript_analyzer.hmodules/gdscript/gdscript_byte_codegen.cppmodules/gdscript/gdscript_compiler.cppmodules/gdscript/gdscript_disassembler.cppmodules/gdscript/gdscript_function.hmodules/gdscript/gdscript_parser.cppmodules/gdscript/gdscript_parser.hmodules/gdscript/gdscript_vm.cppmodules/gdscript/gdscript_warning.cppmodules/gdscript/gdscript_warning.hmodules/gdscript/tests/scripts/analyzer/errors/nullable_array_element.gdmodules/gdscript/tests/scripts/analyzer/errors/nullable_dictionary_key_element.gdmodules/gdscript/tests/scripts/analyzer/errors/nullable_dictionary_value_element.gdmodules/gdscript/tests/scripts/analyzer/errors/nullable_variant.gdmodules/gdscript/tests/scripts/analyzer/warnings/nullable_and_false_no_narrow.gdmodules/gdscript/tests/scripts/analyzer/warnings/nullable_break_guard_no_narrow.gdmodules/gdscript/tests/scripts/analyzer/warnings/nullable_compound_assignment.gdmodules/gdscript/tests/scripts/analyzer/warnings/nullable_continue_guard_no_narrow.gdmodules/gdscript/tests/scripts/analyzer/warnings/nullable_narrowing_limits.gdmodules/gdscript/tests/scripts/analyzer/warnings/nullable_or_true_no_narrow.gdmodules/gdscript/tests/scripts/analyzer/warnings/nullable_unsafe_operations.gdmodules/gdscript/tests/scripts/analyzer/warnings/nullable_unsafe_transfer.gdmodules/gdscript/tests/scripts/analyzer/warnings/unsafe_nullable_access.gdmodules/gdscript/tests/scripts/parser/errors/nullable_void_return.gdmodules/gdscript/tests/scripts/runtime/errors/nullable_enum_null_transfer.gdmodules/gdscript/tests/scripts/runtime/errors/nullable_null_reaches_argument.gdmodules/gdscript/tests/scripts/runtime/errors/nullable_null_reaches_typed_local.gdmodules/gdscript/tests/scripts/runtime/errors/nullable_null_use.gdmodules/gdscript/tests/scripts/runtime/errors/nullable_return_null_from_nonnull.gdmodules/gdscript/tests/scripts/runtime/errors/nullable_typed_array_assign_mismatch.gdmodules/gdscript/tests/scripts/runtime/errors/nullable_typed_array_reassign_mismatch.gdmodules/gdscript/tests/scripts/runtime/errors/nullable_typed_dictionary_assign_mismatch.gdmodules/gdscript/tests/scripts/runtime/errors/nullable_typed_dictionary_reassign_mismatch.gdmodules/gdscript/tests/scripts/runtime/errors/nullable_typed_dictionary_return_mismatch.gdmodules/gdscript/tests/scripts/runtime/errors/nullable_typed_return_mismatch.gdmodules/gdscript/tests/scripts/runtime/features/nullable_empty_callable.gdmodules/gdscript/tests/scripts/runtime/features/nullable_enum_raw_integer.gdmodules/gdscript/tests/scripts/runtime/features/nullable_enum_transfer.gdmodules/gdscript/tests/scripts/runtime/features/nullable_freed_object.gdmodules/gdscript/tests/scripts/runtime/features/nullable_multivar_and.gdmodules/gdscript/tests/scripts/runtime/features/nullable_multivar_or_guard.gdmodules/gdscript/tests/scripts/runtime/features/nullable_narrowing.gdmodules/gdscript/tests/scripts/runtime/features/nullable_narrowing_mutation.gdmodules/gdscript/tests/scripts/runtime/features/nullable_not_compound_or.gdmodules/gdscript/tests/scripts/runtime/features/nullable_renarrow_after_reassign.gdmodules/gdscript/tests/scripts/runtime/features/nullable_self_reference_assignment.gdmodules/gdscript/tests/scripts/runtime/features/nullable_typed_container_returns.gdmodules/gdscript/tests/scripts/runtime/features/nullable_types.gdmodules/gdscript/tests/scripts/runtime/features/nullable_variant_null_return.gdmodules/gdscript/tests/scripts/runtime/features/nullable_while_body_mutation.gd
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/gdscript/gdscript_analyzer.cpp (1)
2079-2103: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the complete narrowing state, not only its size.
These scopes snapshot
narrowed_non_nullby length and later callresize(), butinvalidate_narrowing()can erase facts that existed before the snapshot. Resizing cannot restore those removed entries, so a nested assignment or boolean expression can permanently lose an outer non-null fact and produce falseUNSAFE_NULLABLE_ACCESSwarnings afterward. Save and restore the vector contents (or use an equivalent scoped snapshot), and add a regression covering an outer null check with a nested branch that reassigns the narrowed variable.Also applies to: 2284-2392, 3015-3029, 3254-3260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/gdscript/gdscript_analyzer.cpp` around lines 2079 - 2103, Restore the full narrowed_non_null contents after each scoped narrowing operation, rather than only resizing to the saved length. Update the snapshot/cleanup logic around the shown suite loop and the corresponding scopes near the other referenced locations, preserving facts removed by invalidate_narrowing() during nested branches or expressions. Add a regression covering an outer null check whose nested branch reassigns the narrowed variable, ensuring the outer non-null fact remains available afterward.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@modules/gdscript/gdscript_analyzer.cpp`:
- Around line 2079-2103: Restore the full narrowed_non_null contents after each
scoped narrowing operation, rather than only resizing to the saved length.
Update the snapshot/cleanup logic around the shown suite loop and the
corresponding scopes near the other referenced locations, preserving facts
removed by invalidate_narrowing() during nested branches or expressions. Add a
regression covering an outer null check whose nested branch reassigns the
narrowed variable, ensuring the outer non-null fact remains available afterward.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a888371c-2741-4562-86b0-9680d6e02ded
⛔ Files ignored due to path filters (2)
modules/gdscript/tests/scripts/runtime/features/nullable_narrowing_else.outis excluded by!**/*.outmodules/gdscript/tests/scripts/runtime/features/nullable_nested_enum.outis excluded by!**/*.out
📒 Files selected for processing (4)
doc/classes/ProjectSettings.xmlmodules/gdscript/gdscript_analyzer.cppmodules/gdscript/tests/scripts/runtime/features/nullable_narrowing_else.gdmodules/gdscript/tests/scripts/runtime/features/nullable_nested_enum.gd
Member
|
LGTM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Title.
Summary by CodeRabbit
?) for values, containers, enums, callables, and return types.nullwhen not explicitly initialized.