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

Analyze GDScript dependencies recursively when doing autocompletion #84266

Conversation

HolonProduction
Copy link
Member

Second part of fixing #78003
Related to #84264

Please correct me if my assumptions about how GDScript works are wrong. The code is not exactly simple to read.

To allow for cyclic references Godot does not analyze other scripts when references to them are found in a script, since this recursive approach would lead to problems with cyclic references.

This also means that other CLASS types are not necessarily completely resolved when they are encountered since they are basically a "shallow" reference. This is fine for running GDScript, but when doing autocompletion it means that we can't get the type for interfered type chains, since this type would be generated by the analyzer in conjunction with the interfered types from analyzing the dependencies.

My idea to solving this, is to change analyzer behavior when doing autocompletion, to recursively resolve dependencies. In edge cases with cyclic references this might fail, but this would only mean we can't provide autocompletion for it. And those cases wouldn't receive completion at the moment anyway.

That said I'm unsure about this implementation. I suspect it may lead to error spam in certain edge cases. I couldn't find one until now however.

@HolonProduction HolonProduction requested a review from a team as a code owner October 31, 2023 21:17
@AThousandShips AThousandShips added this to the 4.3 milestone Oct 31, 2023
@akien-mga akien-mga changed the title Analyze GDScript dpendencies recursively when doing autocompletion Analyze GDScript dependencies recursively when doing autocompletion Oct 31, 2023
@HolonProduction HolonProduction force-pushed the autocompletion-78003-the-hard-part branch from 4311612 to db7d74b Compare February 5, 2024 19:26
@vnen
Copy link
Member

vnen commented Apr 29, 2024

So, I don't really know how this is not causing other problems. The thing is that the analyzer does solve the dependencies, but only to the point where it's necessary. This staggered approach is made this way exactly to deal well with cyclic dependencies.

If perhaps the analyzer is not solving the dependencies enough for the completion to work, then it should probably be done after the script is analyzed, not during it (and especially not at the first depended parser request). I fear the way proposed here may break more things than it solves.

It could be done in the complete_code function. After analyzing the script, just ask for the depended parsers and raise their status recursively. This may be a problem in terms of time though, because if it needs to parse more stuff than it will take longer and autocomplete should be fast.

@HolonProduction
Copy link
Member Author

So, I don't really know how this is not causing other problems.

That's a good summary of how I feel about this 😅

If perhaps the analyzer is not solving the dependencies enough for the completion to work, then it should probably be done after the script is analyzed, not during it (and especially not at the first depended parser request).

That's what we do right now isn't it? And it will work well as long as only one level of types uses the analyzer. But without resolving certain types first, we can't encounter other depended parsers. If we only learn about the dependency on class B, after we analysed class A, we will never get completion on B if we don't analyse A before resolving the suite which uses it.

@vnen
Copy link
Member

vnen commented May 2, 2024

That's what we do right now isn't it?
I'm not sure it's done, at least not for completion. It is definitely done when compiling.

And it will work well as long as only one level of types uses the analyzer. But without resolving certain types first, we can't encounter other depended parsers. If we only learn about the dependency on class B, after we analysed class A, we will never get completion on B if we don't analyse A before resolving the suite which uses it.

I get that it needs to be recursive, but I don't think it matters when it's done, as long as it's before the completion start trying to guess stuff.

TBH I don't have a good solution for this, but I'm not confident on the solution proposed here.

@HolonProduction
Copy link
Member Author

Closing in favour of #91653
Which makes the analysing a responsibility of the autocompletion.

@AThousandShips AThousandShips removed this from the 4.3 milestone May 7, 2024
@HolonProduction HolonProduction deleted the autocompletion-78003-the-hard-part branch October 5, 2024 19:02
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.

3 participants