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

Make GDScriptAnalyzer aware of properties from other languages #85703

Conversation

TitanNano
Copy link
Contributor

GDScript's auto-completion is aware of properties from scripts in other languages (integrated via GDExtensions) but the GDScript analyzer will report the property as unknown.

The analyzer only ever checks the GDScript class and the native-type of an object for properties. The script type is currently ignored.

(this fix should also be compatible with at least 4.2 and 4.1)

@TitanNano TitanNano requested a review from a team as a code owner December 3, 2023 12:07
@dalexeev dalexeev added this to the 4.3 milestone Dec 3, 2023
@AThousandShips AThousandShips changed the title GDScriptAnalyzer is unaware of properties from other Languages Make GDScriptAnalyzer aware of properties from other languages Dec 3, 2023
@TitanNano TitanNano force-pushed the jovan/gdscript_foreign_script_properties branch from 1a7162e to 3159d14 Compare December 3, 2023 18:35
@TitanNano TitanNano force-pushed the jovan/gdscript_foreign_script_properties branch from 3159d14 to b96ccc9 Compare December 3, 2023 21:26
@vnen
Copy link
Member

vnen commented Dec 4, 2023

Looks good, but I think it should also look for methods and signals.

@TitanNano
Copy link
Contributor Author

Looks good, but I think it should also look for methods and signals.

@vnen Methods are already handled somewhere else:

MethodInfo info = base_script->get_method_info(function_name);

I can add signals, though. Should I also add constants here?

@vnen
Copy link
Member

vnen commented Dec 4, 2023

Methods are already handled somewhere else:

That is for direct calls. Methods can also be accessed like properties, which return a Callable.

I can add signals, though. Should I also add constants here?

Yes, constants should be included too.

@adamscott
Copy link
Member

@TitanNano Hi and welcome around! Thanks for your PR, by the way.

Could you join a simple test project? I'm wondering how we could test this PR before merging it.

@vnen
Copy link
Member

vnen commented Dec 4, 2023

I think the easiest way to test is with C#, since it's already integrated in Godot.

@TitanNano TitanNano force-pushed the jovan/gdscript_foreign_script_properties branch from b96ccc9 to 92d316e Compare December 8, 2023 23:51
@TitanNano
Copy link
Contributor Author

@TitanNano Hi and welcome around! Thanks for your PR, by the way.

Could you join a simple test project? I'm wondering how we could test this PR before merging it.

Do you have any pointers for me on how to get started with testing this? Especially with the mix of C# and GDScript.

@TitanNano TitanNano force-pushed the jovan/gdscript_foreign_script_properties branch from 92d316e to 124acf5 Compare December 12, 2023 21:27
@TitanNano TitanNano requested a review from a team as a code owner December 12, 2023 21:27
core/object/object.h Outdated Show resolved Hide resolved
@vnen
Copy link
Member

vnen commented Dec 13, 2023

Looks good to me. I did test this with a C# project and it seems to work well.

@TitanNano TitanNano force-pushed the jovan/gdscript_foreign_script_properties branch from 124acf5 to 7927b37 Compare December 15, 2023 23:25
Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for working on this!

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor style nits, looks good otherwise

modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
Co-authored-by: K. S. Ernest (iFire) Lee <fire@users.noreply.github.com>
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@TitanNano TitanNano force-pushed the jovan/gdscript_foreign_script_properties branch from c927b3b to 030aa41 Compare December 18, 2023 21:03
@TitanNano
Copy link
Contributor Author

@AThousandShips I applied your suggestions. Perhaps this can get merged then?

@YuriSizov YuriSizov merged commit 38d8ca0 into godotengine:master Dec 19, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

YuriSizov commented Dec 19, 2023

Thanks, and congrats on your first merged Godot contribution!

PS. For future PRs, please make sure to follow the preferred style for commit messages (starting with a verb, etc). The title of the PR right now is a good option. You also don't need to include reviewers as co-authors 🙂

@TitanNano TitanNano deleted the jovan/gdscript_foreign_script_properties branch December 19, 2023 13:26
@TitanNano
Copy link
Contributor Author

@YuriSizov thank you for the message. How do I backport / cherry-pick this for 4.2 and 4.1? Do I just open pull requests against these branches with a reference to this PR? I wasn't able to find specific documentation.

@YuriSizov
Copy link
Contributor

@TitanNano You don't need to do anything if it's cherry-pickable without conflicts, we'll handle it. That said, the team hasn't expressed yet if this should be cherry-picked.

@DavidVonDerau
Copy link

I'm not sure if this is related to this PR or not, but I was going through the commits that went into 4.3 and this seemed like a possible source of the following regression.


In 4.2.stable, the following works w/ type checking disabled:

image

where Multiply is just an example method defined as

image

If I enable type checking in 4.2.stable, I get the same warning that prompted this PR:

image

But if I use the 4.3.dev1 release containing this PR, I see the following error:

image

This is a parse error, so I can't just disable warnings.

I can get the code to parse correctly by using call, like so:

image

But this doesn't match the documentation on how cross-script method calls should work, and this is not backwards compatible with how it worked in 4.2 -- https://docs.godotengine.org/en/stable/tutorials/scripting/cross_language_scripting.html

@TitanNano
Copy link
Contributor Author

@DavidVonDerau there is #86259 which should fix this problem.

@vnen vnen added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 2, 2024
@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants