-
-
Notifications
You must be signed in to change notification settings - Fork 21.8k
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: Out of order member resolution #69471
Conversation
Does this address #61159? |
@MewPurPur i can check in a bit, but my instinct is no |
As is, this PR fixes #69479. I just tested it. |
} | ||
return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was annoyed that Variant and Object were the only ones to return a good result early
removing this actually fixes an annoying bug that allows the type Variant[int]
but chops off the [int]
part in the analyzer. same with Object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, deleted my comment after realizing that you can probably still fill out some stuff later, so you can do all these checks and assign partial information to result
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
85025ff
to
809696e
Compare
found = true; | ||
break; | ||
case GDScriptParser::ClassNode::Member::ENUM: | ||
result = member.m_enum->get_datatype(); | ||
result = member.get_datatype(); | ||
found = true; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can now let CLASS
fallthrough to ENUM
since the code is the same! Wonder if there's any other places where this could be done :)
@anvilfolk and @adamscott thank you for reviewing! |
Technically this could be expanded to allow the analyzer to resolve individual members of other scripts, allowing member dependencies both ways between script interfaces, but i'm not sure anyone has a use for that 🙂 |
Wouldn't this actually be really neat now that cyclic dependencies are possible? Could probably come up with a small interesting case like: # A.gd
var a := B.b_func()
static func a_func() -> int:
return 42
# B.gd
static func b_func() -> int:
return A.a_func() Something like that? |
809696e
to
d41ef9f
Compare
@anvilfolk exactly! i do kinda wanna see how difficult it is, so i'll put it on my todo list 😄 |
i added braces inside |
@MewPurPur no, this doesn't fix #61159, but i found the bug. |
d41ef9f
to
809696e
Compare
I cannot NOT underline the great teamwork between you both, @rune-scape and @anvilfolk. <3 |
30600f0
to
2144162
Compare
ok, it took a while to figure out, but tests are added and they helped me find a few bugs! they're finally done |
2144162
to
2dfc6d5
Compare
Nice! Look forward to seeing this merged so I can fix conflicts with the enum PR! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Thanks! |
@lufog Can you create an issue? |
@adamscott, to be honest, I don't even know how to properly formulate it. But I'll try. |
@lufog thank you for the project! i think i found the mistake |
@lufog You can formulate it like "Autoloads broken: cannot find members" and copy/paste what you wrote here in the issue. It will be easier for @rune-scape to create a PR that links to your issue. |
Done #70127 |
since this pr I'm also having some errors some of which was just lazy code on my part. My biggest issue that I'm having trouble isolating is from an autoload that loads a bunch of resources that create instances from a named base class. I get this one of the other issues that was easily fixed was a reference from an autoload to another autoload |
@dmaz Please create issues and link them to this PR. Commenting here doesn't really help fixing your issues. |
yeah for sure... just haven't been able to isolate class one into an example yet |
Implements out of order member resolution
-- this was allowed in some specific places, but this PR pulls it out into its own function to make the behavior consistent
-- and allow more complex ordering
class members are now resolved just before they are needed
-- This means class members will always have resolved types before used, else an error is pushed
same goes for members of external classes
-- This fixes alot of dependency errors.
also fixes a few minor bugs:
-- singleton datatypes now get properly resolved in extends
-- Object and Variant no longer allow array syntax (
Variant[int]
doesn't parse)-- pushes an error if a native enum doesn't exist when resolving a type
those were all just missing a line or 2
fixes #68069
fixes #69479
fixes #67085
fixes #69763
fixes #68017
may fix #44375 (didn't read the all the comments, but it addresses the problem in the most recent ones)