-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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: Add error when exporting node in non Node
-derived classes
#82843
GDScript: Add error when exporting node in non Node
-derived classes
#82843
Conversation
Node
-derived classes
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.
Looks nice!
Thanks! |
@@ -5544,7 +5544,7 @@ Error GDScriptAnalyzer::analyze() { | |||
// Apply annotations. | |||
for (GDScriptParser::AnnotationNode *&E : parser->head->annotations) { | |||
resolve_annotation(E); | |||
E->apply(parser, parser->head); | |||
E->apply(parser, parser->head, nullptr); |
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.
parser->head
is a ClassNode *
, so this line should be:
E->apply(parser, parser->head, parser->head);
I have added a fix for this to #67777.
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.
No, p_class
is the class that p_target
is in. The root class is not in any other class. This is a subtle detail, but for example if an annotation is applied to an inner class (we don't have such a case right now), then p_target
will be the class, and p_class
will be its outer class. Therefore, if you do not need the context class, then it is important to use p_target
rather than p_class
, even this requires static_cast
.
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.
Thanks, I missed that detail before. I've updated my PR and add a comment in the abstract annotation apply method for why we are using p_target
instead of p_class
.
Maybe p_class
should be renamed to p_in_class
? Since it's the class the target is in.
Just a small note: if we are referring to some unspecified limitations, we should link to some place that explains what those are. Do we have an article that goes into such detail about Node exports? |
Fair point. I remember that there are issues with exporting nodes in |
Node
-derived class.@export
annotation documentation.p_node
top_target
so.cpp
matches.h
.