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

Fix parser bug for typed dictionaries with custom classes as values #98336

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tracefree
Copy link
Contributor

Fixes #96881

I assume that this was an oversight during a larger refactorization since script_path was defined and initialized but never used. I added what I assume to be its originally intended use and it fixes the the bug for me.

@tracefree tracefree requested a review from a team as a code owner October 19, 2024 13:17
Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

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

Good catch!

@dalexeev
Copy link
Member

CC @godotengine/gdscript, @rune-scape

Copy link
Contributor

@rune-scape rune-scape left a comment

Choose a reason for hiding this comment

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

i should add a comment to this function, as its actually a very delicate piece of code,
theres a couple things wrong with this change

+it returns early, so the parser ref doesn't get cached
+it doesn't check to make sure the parser has the class, which is the point of the function

it needs to get the correct parser_ref that has the requested class bc after #94871 there can technically be 2 parser_refs floating around and the analyzer needs to find the correct one

get_depended_parser_for is intentionally not called in this function bc the parser its looking for may not be in the gdscript cache but it should be somewhere in the dependancy chain, and this function looks for it

if it cant find it there is probably a bug somewhere else

an alternate fix might be to look in the gdscript cache for it, without using get_depended_parser_for if it cant find it anywhere else

@tracefree
Copy link
Contributor Author

Thanks for the correction! I'm very new to this part of the codebase so I appreciate the insight. Do you know why the script_path variable was defined but not used (unless I missed something), and do you have a suggestion for where the real cause of the bug could be?

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