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

GDScript 2.0: Incorrect base class when using extends Class.Subclass #65953

Closed
rune-scape opened this issue Sep 17, 2022 · 13 comments · Fixed by #68374
Closed

GDScript 2.0: Incorrect base class when using extends Class.Subclass #65953

rune-scape opened this issue Sep 17, 2022 · 13 comments · Fixed by #68374

Comments

@rune-scape
Copy link
Contributor

Godot version

20d6672

System information

Windows 10 x64

Issue description

When using 'extends BaseClass.Subclass' the class extended is 'BaseClass'. Kinda.
All super calls will go to A, not B, but Analyzer still knows B is the intended super class.

The GDScriptDataType::script_type for the identifier is set wrong in i think 'resolve_inheritance' or 'reduce_identifier_from_base' in GDScriptAnalyser. Or left unset?
Something to do with the subclass class member not getting its script type resolved.
GDScriptDataType::script_type is not set right by the time it gets to GDScriptCompiler when it sets the 'base' ptr in GDScript, where all super calls go.
Tested on Beta 1

Steps to reproduce

Use the dot notation in an extends clause, like (extends A.B).
All super calls will go to A, not B, but Analyzer still knows B is the intended super class

Minimal reproduction project

GodotInheritanceBugProject.zip
Running the project will print:

TestClass._init
BaseClass.test

When it should print:

TestClass._init
SubClass.test
@Calinou Calinou changed the title [GDScript] Incorrect base class when using 'extends Class.Subclass' GDScript 2.0: Incorrect base class when using extends Class.Subclass Sep 17, 2022
@Calinou Calinou added this to the 4.0 milestone Sep 17, 2022
@Calinou Calinou moved this to To Assess in 4.x Priority Issues Sep 17, 2022
@bitbrain
Copy link
Contributor

Another one that might be related to #65752 (not sure if it will fix it though, as this one is not about class inheritance per-se but more about the script type)

@adamscott
Copy link
Member

@bitbrain #65752 will not fix this issue, I just tested.

@rune-scape
Copy link
Contributor Author

I had to stop programming for my mental health, but if anyone wants to pick up where I left off, my WIP branch is here:
https://github.com/rune-scape/godot/tree/rune-simplify-extends

@bitbrain
Copy link
Contributor

bitbrain commented Oct 20, 2022

I had to stop programming for my mental health, but if anyone wants to pick up where I left off, my WIP branch is here: https://github.com/rune-scape/godot/tree/rune-simplify-extends

@adamscott staring at you for no reason whatsoever.

@rune-scape get well soon. 🖤

@adamscott
Copy link
Member

I had to stop programming for my mental health

Take care, @rune-scape

@adamscott
Copy link
Member

adamscott commented Oct 26, 2022

@rune-scape I think I fixed your issue with my new cyclic references PR #67714 !

@rune-scape
Copy link
Contributor Author

rune-scape commented Oct 30, 2022

@adamscott I found a lot of edge cases where subclasses aren't considered when I was trying to switch extends to use resolve_datatype() rather than trusting every use of extends_path to correctly find the subclasses.. There was alot of redundant code, and fully qualified path names were different between parser and compiler making them not a unique id
I almost got it to a testable state too. I'd feel much better if that was the fix to this issue. It would also fix the what @bitbrain mentioned in #66781.
its built on #67714

@adamscott
Copy link
Member

@rune-scape You mean that it was built on #65752, I completely redid my PR from scratch.

I'll try to take a look to your PR one more time, to see what you did in resolve datatype.

@adamscott
Copy link
Member

@rune-scape Unfortunately, there's too many changes for me, in your branch, to be confortable to integrate it in #67714, especially as it just been approved by a reviewer.

If you create a PR, I'll make sure to work with you to help you refactor resolve_datatype().

@rune-scape
Copy link
Contributor Author

I completely redid my PR from scratch.

Ah

there's too many changes for me, in your branch, to be comfortable to integrate it in #67714
If you create a PR, I'll make sure to work with you to help you refactor resolve_datatype().

Sounds good, ill start in a few days

@rune-scape
Copy link
Contributor Author

rune-scape commented Oct 31, 2022

So I did it Again and I have to stop programming, but @adamscott, #68086. not working yet, but it runs
now on you're newer pr

@adamscott
Copy link
Member

@rune-scape Take your time. Especially for yourself. 💜

@adamscott
Copy link
Member

Note to the Godot Maintainers: my PR (#67714) shouldn't mark this issue as resolved. That honour should go to @rune-scape 's PR #68374 as it's solution is way more deep that mine could ever be.

Repository owner moved this from To Assess to Done in 4.x Priority Issues Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment