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

Closed

Conversation

tracefree
Copy link
Contributor

@tracefree tracefree commented Oct 19, 2024

Edit: Superseded by #98400

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?

@rune-scape
Copy link
Contributor

it was probly copied over from my first try at find_cached_external_parser_for_class,, its there in the second definition of that func

sorry to say, u happened upon a part of the analyzer that requires some deep knowledge to fix correctly
it was easier for me to look at the change and fix it, it was a simple mistake i made, looking only at the first container type
i made a PR: #98400

searching through the gdscript cache like u are here could fix the problem, but it could cover up a bug later and make it hard to reproduce, i might be being a bit pedantic about this tho

@tracefree
Copy link
Contributor Author

Awesome, thank you for making a proper fix! Closing this in favor of your PR

@tracefree tracefree closed this Oct 22, 2024
@AThousandShips AThousandShips removed this from the 4.4 milestone Oct 22, 2024
@tracefree tracefree deleted the typed_dict_parser_bug branch October 22, 2024 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants