-
-
Notifications
You must be signed in to change notification settings - Fork 23.4k
Fix: Add Variant to type autocompletion #111878
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
base: master
Are you sure you want to change the base?
Conversation
| } | ||
| } | ||
|
|
||
| static void _add_variant_types(HashMap<String, ScriptLanguage::CodeCompletionOption> &r_result) { |
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.
To me, this seems like premature splitting. This is a special case, so there's no need to separate this, an inline block inside _list_available_types() would be sufficient. Also, the function name is confusing: types instead of type when this function adds only one Variant type, and not, for example, all built-in Variant types.
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.
That's a very helpful review, thanks! 🙏
It's my first time submitting a PR , so I really appreciate you catching these details.
I agree with you completely; this certainly looks like premature splitting. I'll follow your suggestion and move it back to an inline block within _list_available_types(), as the Variant is probably the only type needed for this change.
I apologize for the types plural—that was an oversight (and a lazy use of auto-complete!).
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 think the analysis of the problem is wrong. It's not that Variant is inherently missing, it's that variant is called Nil in the variant type enum names (I think Variant::NIL is supposed to be GDScripts Variant but if someone more knowledgeable could confirm this it would be great). In completion it appears under that name:
which is obviously wrong.
This PR adds Variant to the list without handling the wrong Nil option.
A fix should happen in _find_build_in_variants which is responsible for adding variant types.
Don't be confused by the special case for Nil in _find_build_in_variants which converts it to null. That's an erroneous codepath. This method should not be resposible for adding null. There is other code to handle this.
|
Please do not request another review without having made any changes, wait until you've resolved the issue that the reviewer has raised |
No, I don't think that's quite right. It's true that |
|
I guess it's more complicated then 😅, still Nil needs to be filtered out as well since it is currently no GDScript type. Also the docs for |
The editor interprets |
Fixes #111854
This PR addresses an issue where the Variant meta-type was missing from the autocompletion suggestions in various GDScript type-hinting contexts.
This PR adds a helper function
_add_variant_types()to properly include Variant in type completion contexts while excluding it from inheritance contexts where it's invalid.It is called conditionally within
_list_available_types()to ensure Variant is only listed as a type and never as an inheritance option.Tested and confirmed working in relevant type-hinting scenarios:
Variable Type Declaration: (var my_var: Variant)
Container Element Types: (Array[Variant], Dictionary[String, Variant])
Function Return Types: (func get() -> Variant)
The exclusion logic was also confirmed: Variant does not appear in the autocompletion list following the extends keyword.
I'd appreciate any advice on the changes.