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

Fixed wrong base class when using extends Base.Subclass #66781

Closed

Conversation

rune-scape
Copy link
Contributor

@rune-scape rune-scape commented Oct 2, 2022

Fixes #65953

Also prevents a crash on startup when opening this project, which has weird almost cyclic inheritance.
godot-rune-wrong-subclass-base-proj.zip
Edit: no longer prevents the crash on startup

@rune-scape rune-scape requested a review from a team as a code owner October 2, 2022 19:25
@akien-mga akien-mga added this to the 4.0 milestone Oct 3, 2022
@bitbrain
Copy link
Contributor

bitbrain commented Oct 3, 2022

inner-classes-refcount-errors.zip

@rune-scape could you try out this project? On current master branch it gives me this error:

Capture

as it is unable to detect that ParentClass is a base class of SubClass

@bitbrain
Copy link
Contributor

bitbrain commented Oct 3, 2022

Capture

Just tested it with your change and compared to current master it gives me now just a "Parser error" but still fails. (no console errors/stacktrace whatsoever)

@rune-scape
Copy link
Contributor Author

rune-scape commented Oct 4, 2022

Thanks for the input!
I've tested with that last push that both projects work (and would appreciate someone else confirming that),
but this should all really be applied after #65752, and then resolving inner classes of other scripts can be moved into _gdtype_from_datatype, where it belongs.

@bitbrain
Copy link
Contributor

bitbrain commented Oct 5, 2022

The latest commit of this PR now gives me a different error:

Capture

@rune-scape are you saying that once #65752 is merged that your PR should work without any issues in this regard? Otherwise I might write up a different issue after both the PRs are being merged.

@rune-scape
Copy link
Contributor Author

rune-scape commented Oct 6, 2022

Ah, yes, I minced my words. That is a separate issue that should (hopefully) be resolved after #65752 is merged AND the fix I mentioned in my last comment is applied.

@bitbrain
Copy link
Contributor

bitbrain commented Oct 6, 2022

@rune-scape looks like the formatting is failing at the moment:

https://github.com/godotengine/godot/actions/runs/3185280223/jobs/5194668640

@rune-scape
Copy link
Contributor Author

rune-scape commented Oct 6, 2022

Ya, but... its messed up, the formatting isn't wrong like it says. I didn't change any of the code its crying about.

@adamscott
Copy link
Member

Ya, but... its messed up, the formatting isn't wrong like it says. I didn't change any of the code its crying about.

@rune-scape I suggest you run clang-format -i modules/gdscript/gdscript.cpp.

@rune-scape
Copy link
Contributor Author

rune-scape commented Oct 9, 2022

Here's a screenshot of the diff, I have to assume its a bug in the formatter.
image

@rune-scape
Copy link
Contributor Author

There also seems to be 1 or 2 more places where extending an inner class is not taken into account.
I now think that extends should be changed in the parser to an identifier/preload and subscript expression because it simplifies everything.
It's mentioned in the analyzer:

// TODO: Extends could use identifier nodes. That way errors can be pointed out properly and it can be used here.

I feel like anything else is just chasing bugs as they pop up.
I'm working on it and figuring out whether or not it needs #65752.

@rune-scape rune-scape closed this Oct 9, 2022
@akien-mga
Copy link
Member

Here's a screenshot of the diff, I have to assume its a bug in the formatter. image

This is a known bug indeed, the same code is having to use /* clang-format off */ in the Mono module.

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.

GDScript 2.0: Incorrect base class when using extends Class.Subclass
4 participants