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.

Sign up for GitHub

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

Jump to bottom

GDScript: Add error when exporting node in non Node-derived classes #82843

Merged
merged 1 commit into from
Oct 5, 2023
Merged

GDScript: Add error when exporting node in non Node-derived classes #82843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion modules/gdscript/doc_classes/@GDScript.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,11 +285,36 @@
<description>
Mark the following property as exported (editable in the Inspector dock and saved to disk). To control the type of the exported property, use the type hint notation.
[codeblock]
extends Node

enum Direction {LEFT, RIGHT, UP, DOWN}

# Built-in types.
@export var string = ""
@export var int_number = 5
@export var float_number: float = 5

# Enums.
@export var type: Variant.Type
@export var format: Image.Format
@export var direction: Direction

# Resources.
@export var image: Image
[/codeblock]
@export var custom_resource: CustomResource

# Nodes.
@export var node: Node
@export var custom_node: CustomNode

# Typed arrays.
@export var int_array: Array[int]
@export var direction_array: Array[Direction]
@export var image_array: Array[Image]
@export var node_array: Array[Node]
[/codeblock]
[b]Note:[/b] Custom resources and nodes must be registered as global classes using [code]class_name[/code].
[b]Note:[/b] Node export is only supported in [Node]-derived classes and has a number of other limitations.
</description>
</annotation>
<annotation name="@export_category">
Expand Down
20 changes: 10 additions & 10 deletions modules/gdscript/gdscript_analyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -910,7 +910,7 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
for (GDScriptParser::AnnotationNode *&E : member_node->annotations) {
if (E->name == SNAME("@warning_ignore")) {
resolve_annotation(E);
E->apply(parser, member.variable);
E->apply(parser, member.variable, p_class);
}
}
for (GDScriptWarning::Code ignored_warning : member_node->ignored_warnings) {
Expand All @@ -933,7 +933,7 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
for (GDScriptParser::AnnotationNode *&E : member.variable->annotations) {
if (E->name != SNAME("@warning_ignore")) {
resolve_annotation(E);
E->apply(parser, member.variable);
E->apply(parser, member.variable, p_class);
}
}

Expand Down Expand Up @@ -985,7 +985,7 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
// Apply annotations.
for (GDScriptParser::AnnotationNode *&E : member.constant->annotations) {
resolve_annotation(E);
E->apply(parser, member.constant);
E->apply(parser, member.constant, p_class);
}
} break;
case GDScriptParser::ClassNode::Member::SIGNAL: {
Expand Down Expand Up @@ -1015,7 +1015,7 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
// Apply annotations.
for (GDScriptParser::AnnotationNode *&E : member.signal->annotations) {
resolve_annotation(E);
E->apply(parser, member.signal);
E->apply(parser, member.signal, p_class);
}
} break;
case GDScriptParser::ClassNode::Member::ENUM: {
Expand Down Expand Up @@ -1063,13 +1063,13 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
// Apply annotations.
for (GDScriptParser::AnnotationNode *&E : member.m_enum->annotations) {
resolve_annotation(E);
E->apply(parser, member.m_enum);
E->apply(parser, member.m_enum, p_class);
}
} break;
case GDScriptParser::ClassNode::Member::FUNCTION:
for (GDScriptParser::AnnotationNode *&E : member.function->annotations) {
resolve_annotation(E);
E->apply(parser, member.function);
E->apply(parser, member.function, p_class);
}
resolve_function_signature(member.function, p_source);
break;
Expand Down Expand Up @@ -1279,7 +1279,7 @@ void GDScriptAnalyzer::resolve_class_body(GDScriptParser::ClassNode *p_class, co
// Apply annotations.
for (GDScriptParser::AnnotationNode *&E : member.function->annotations) {
resolve_annotation(E);
E->apply(parser, member.function);
E->apply(parser, member.function, p_class);
}
resolve_function_body(member.function);
} else if (member.type == GDScriptParser::ClassNode::Member::VARIABLE && member.variable->property != GDScriptParser::VariableNode::PROP_NONE) {
Expand All @@ -1301,7 +1301,7 @@ void GDScriptAnalyzer::resolve_class_body(GDScriptParser::ClassNode *p_class, co
} else if (member.type == GDScriptParser::ClassNode::Member::GROUP) {
// Apply annotation (`@export_{category,group,subgroup}`).
resolve_annotation(member.annotation);
member.annotation->apply(parser, nullptr);
member.annotation->apply(parser, nullptr, p_class);
}
}

Expand Down Expand Up @@ -1837,7 +1837,7 @@ void GDScriptAnalyzer::resolve_suite(GDScriptParser::SuiteNode *p_suite) {
// Apply annotations.
for (GDScriptParser::AnnotationNode *&E : stmt->annotations) {
resolve_annotation(E);
E->apply(parser, stmt);
E->apply(parser, stmt, nullptr);
}

#ifdef DEBUG_ENABLED
Expand Down Expand Up @@ -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);
Copy link
Member

@aaronfranke aaronfranke Oct 6, 2023 •
edited
Loading

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.

Copy link
Member Author

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.

Copy link
Member

@aaronfranke aaronfranke Oct 6, 2023 •
edited
Loading

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.

}

resolve_interface();
Expand Down
64 changes: 35 additions & 29 deletions modules/gdscript/gdscript_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ void GDScriptParser::parse_program() {
if (annotation->applies_to(AnnotationInfo::SCRIPT)) {
// `@icon` needs to be applied in the parser. See GH-72444.
if (annotation->name == SNAME("@icon")) {
annotation->apply(this, head);
annotation->apply(this, head, nullptr);
} else {
head->annotations.push_back(annotation);
}
Expand Down Expand Up @@ -3795,12 +3795,12 @@ const GDScriptParser::SuiteNode::Local &GDScriptParser::SuiteNode::get_local(con
return empty;
}

bool GDScriptParser::AnnotationNode::apply(GDScriptParser *p_this, Node *p_target) {
bool GDScriptParser::AnnotationNode::apply(GDScriptParser *p_this, Node *p_target, ClassNode *p_class) {
if (is_applied) {
return true;
}
is_applied = true;
return (p_this->*(p_this->valid_annotations[name].apply))(this, p_target);
return (p_this->*(p_this->valid_annotations[name].apply))(this, p_target, p_class);
}

bool GDScriptParser::AnnotationNode::applies_to(uint32_t p_target_kinds) const {
Expand Down Expand Up @@ -3846,7 +3846,7 @@ bool GDScriptParser::validate_annotation_arguments(AnnotationNode *p_annotation)
return true;
}

bool GDScriptParser::tool_annotation(const AnnotationNode *p_annotation, Node *p_node) {
bool GDScriptParser::tool_annotation(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class) {
#ifdef DEBUG_ENABLED
if (this->_is_tool) {
push_error(R"("@tool" annotation can only be used once.)", p_annotation);
Expand All @@ -3857,15 +3857,15 @@ bool GDScriptParser::tool_annotation(const AnnotationNode *p_annotation, Node *p
return true;
}

bool GDScriptParser::icon_annotation(const AnnotationNode *p_annotation, Node *p_node) {
ERR_FAIL_COND_V_MSG(p_node->type != Node::CLASS, false, R"("@icon" annotation can only be applied to classes.)");
bool GDScriptParser::icon_annotation(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class) {
ERR_FAIL_COND_V_MSG(p_target->type != Node::CLASS, false, R"("@icon" annotation can only be applied to classes.)");
ERR_FAIL_COND_V(p_annotation->resolved_arguments.is_empty(), false);

ClassNode *p_class = static_cast<ClassNode *>(p_node);
ClassNode *class_node = static_cast<ClassNode *>(p_target);
String path = p_annotation->resolved_arguments[0];

#ifdef DEBUG_ENABLED
if (!p_class->icon_path.is_empty()) {
if (!class_node->icon_path.is_empty()) {
push_error(R"("@icon" annotation can only be used once.)", p_annotation);
return false;
}
Expand All @@ -3875,27 +3875,27 @@ bool GDScriptParser::icon_annotation(const AnnotationNode *p_annotation, Node *p
}
#endif // DEBUG_ENABLED

p_class->icon_path = path;
class_node->icon_path = path;

if (path.is_empty() || path.is_absolute_path()) {
p_class->simplified_icon_path = path.simplify_path();
class_node->simplified_icon_path = path.simplify_path();
} else if (path.is_relative_path()) {
p_class->simplified_icon_path = script_path.get_base_dir().path_join(path).simplify_path();
class_node->simplified_icon_path = script_path.get_base_dir().path_join(path).simplify_path();
} else {
p_class->simplified_icon_path = path;
class_node->simplified_icon_path = path;
}

return true;
}

bool GDScriptParser::onready_annotation(const AnnotationNode *p_annotation, Node *p_node) {
ERR_FAIL_COND_V_MSG(p_node->type != Node::VARIABLE, false, R"("@onready" annotation can only be applied to class variables.)");
bool GDScriptParser::onready_annotation(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class) {
ERR_FAIL_COND_V_MSG(p_target->type != Node::VARIABLE, false, R"("@onready" annotation can only be applied to class variables.)");

if (current_class && !ClassDB::is_parent_class(current_class->get_datatype().native_type, SNAME("Node"))) {
push_error(R"("@onready" can only be used in classes that inherit "Node".)", p_annotation);
}

VariableNode *variable = static_cast<VariableNode *>(p_node);
VariableNode *variable = static_cast<VariableNode *>(p_target);
if (variable->is_static) {
push_error(R"("@onready" annotation cannot be applied to a static variable.)", p_annotation);
return false;
Expand All @@ -3910,10 +3910,11 @@ bool GDScriptParser::onready_annotation(const AnnotationNode *p_annotation, Node
}

template <PropertyHint t_hint, Variant::Type t_type>
bool GDScriptParser::export_annotations(const AnnotationNode *p_annotation, Node *p_node) {
ERR_FAIL_COND_V_MSG(p_node->type != Node::VARIABLE, false, vformat(R"("%s" annotation can only be applied to variables.)", p_annotation->name));
bool GDScriptParser::export_annotations(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class) {
ERR_FAIL_COND_V_MSG(p_target->type != Node::VARIABLE, false, vformat(R"("%s" annotation can only be applied to variables.)", p_annotation->name));
ERR_FAIL_NULL_V(p_class, false);

VariableNode *variable = static_cast<VariableNode *>(p_node);
VariableNode *variable = static_cast<VariableNode *>(p_target);
if (variable->is_static) {
push_error(vformat(R"(Annotation "%s" cannot be applied to a static variable.)", p_annotation->name), p_annotation);
return false;
Expand Down Expand Up @@ -4128,7 +4129,12 @@ bool GDScriptParser::export_annotations(const AnnotationNode *p_annotation, Node
} break;
default:
push_error(R"(Export type can only be built-in, a resource, a node, or an enum.)", variable);
break;
return false;
}

if (variable->export_info.hint == PROPERTY_HINT_NODE_TYPE && !ClassDB::is_parent_class(p_class->base_type.native_type, SNAME("Node"))) {
push_error(vformat(R"(Node export is only supported in Node-derived classes, but the current class inherits "%s".)", p_class->base_type.to_string()), variable);
return false;
}

if (is_array) {
Expand Down Expand Up @@ -4173,7 +4179,7 @@ bool GDScriptParser::export_annotations(const AnnotationNode *p_annotation, Node
}

template <PropertyUsageFlags t_usage>
bool GDScriptParser::export_group_annotations(const AnnotationNode *p_annotation, Node *p_node) {
bool GDScriptParser::export_group_annotations(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class) {
AnnotationNode *annotation = const_cast<AnnotationNode *>(p_annotation);

if (annotation->resolved_arguments.is_empty()) {
Expand Down Expand Up @@ -4205,7 +4211,7 @@ bool GDScriptParser::export_group_annotations(const AnnotationNode *p_annotation
return true;
}

bool GDScriptParser::warning_annotations(const AnnotationNode *p_annotation, Node *p_node) {
bool GDScriptParser::warning_annotations(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class) {
#ifdef DEBUG_ENABLED
bool has_error = false;
for (const Variant &warning_name : p_annotation->resolved_arguments) {
Expand All @@ -4214,7 +4220,7 @@ bool GDScriptParser::warning_annotations(const AnnotationNode *p_annotation, Nod
push_error(vformat(R"(Invalid warning name: "%s".)", warning_name), p_annotation);
has_error = true;
} else {
p_node->ignored_warnings.push_back(warning);
p_target->ignored_warnings.push_back(warning);
}
}

Expand All @@ -4226,10 +4232,10 @@ bool GDScriptParser::warning_annotations(const AnnotationNode *p_annotation, Nod
#endif // DEBUG_ENABLED
}

bool GDScriptParser::rpc_annotation(const AnnotationNode *p_annotation, Node *p_node) {
ERR_FAIL_COND_V_MSG(p_node->type != Node::FUNCTION, false, vformat(R"("%s" annotation can only be applied to functions.)", p_annotation->name));
bool GDScriptParser::rpc_annotation(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class) {
ERR_FAIL_COND_V_MSG(p_target->type != Node::FUNCTION, false, vformat(R"("%s" annotation can only be applied to functions.)", p_annotation->name));

FunctionNode *function = static_cast<FunctionNode *>(p_node);
FunctionNode *function = static_cast<FunctionNode *>(p_target);
if (function->rpc_config.get_type() != Variant::NIL) {
push_error(R"(RPC annotations can only be used once per function.)", p_annotation);
return false;
Expand Down Expand Up @@ -4287,14 +4293,14 @@ bool GDScriptParser::rpc_annotation(const AnnotationNode *p_annotation, Node *p_
return true;
}

bool GDScriptParser::static_unload_annotation(const AnnotationNode *p_annotation, Node *p_target) {
bool GDScriptParser::static_unload_annotation(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class) {
ERR_FAIL_COND_V_MSG(p_target->type != Node::CLASS, false, vformat(R"("%s" annotation can only be applied to classes.)", p_annotation->name));
ClassNode *p_class = static_cast<ClassNode *>(p_target);
if (p_class->annotated_static_unload) {
ClassNode *class_node = static_cast<ClassNode *>(p_target);
if (class_node->annotated_static_unload) {
push_error(vformat(R"("%s" annotation can only be used once per script.)", p_annotation->name), p_annotation);
return false;
}
p_class->annotated_static_unload = true;
class_node->annotated_static_unload = true;
return true;
}

Expand Down
20 changes: 10 additions & 10 deletions modules/gdscript/gdscript_parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ class GDScriptParser {
bool is_resolved = false;
bool is_applied = false;

bool apply(GDScriptParser *p_this, Node *p_target);
bool apply(GDScriptParser *p_this, Node *p_target, ClassNode *p_class);
bool applies_to(uint32_t p_target_kinds) const;

AnnotationNode() {
Expand Down Expand Up @@ -1340,7 +1340,7 @@ class GDScriptParser {
bool in_lambda = false;
bool lambda_ended = false; // Marker for when a lambda ends, to apply an end of statement if needed.

typedef bool (GDScriptParser::*AnnotationAction)(const AnnotationNode *p_annotation, Node *p_target);
typedef bool (GDScriptParser::*AnnotationAction)(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class);
struct AnnotationInfo {
enum TargetKind {
NONE = 0,
Expand Down Expand Up @@ -1461,16 +1461,16 @@ class GDScriptParser {
bool register_annotation(const MethodInfo &p_info, uint32_t p_target_kinds, AnnotationAction p_apply, const Vector<Variant> &p_default_arguments = Vector<Variant>(), bool p_is_vararg = false);
bool validate_annotation_arguments(AnnotationNode *p_annotation);
void clear_unused_annotations();
bool tool_annotation(const AnnotationNode *p_annotation, Node *p_target);
bool icon_annotation(const AnnotationNode *p_annotation, Node *p_target);
bool onready_annotation(const AnnotationNode *p_annotation, Node *p_target);
bool tool_annotation(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class);
bool icon_annotation(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class);
bool onready_annotation(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class);
template <PropertyHint t_hint, Variant::Type t_type>
bool export_annotations(const AnnotationNode *p_annotation, Node *p_target);
bool export_annotations(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class);
template <PropertyUsageFlags t_usage>
bool export_group_annotations(const AnnotationNode *p_annotation, Node *p_target);
bool warning_annotations(const AnnotationNode *p_annotation, Node *p_target);
bool rpc_annotation(const AnnotationNode *p_annotation, Node *p_target);
bool static_unload_annotation(const AnnotationNode *p_annotation, Node *p_target);
bool export_group_annotations(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class);
bool warning_annotations(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class);
bool rpc_annotation(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class);
bool static_unload_annotation(const AnnotationNode *p_annotation, Node *p_target, ClassNode *p_class);
// Statements.
Node *parse_statement();
VariableNode *parse_variable(bool p_is_static);
Expand Down
8 changes: 8 additions & 0 deletions modules/gdscript/tests/scripts/analyzer/errors/export_node_in_non_node_derived_class_1.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# GH-82809

extends Resource

@export var node: Node

func test():
pass
2 changes: 2 additions & 0 deletions modules/gdscript/tests/scripts/analyzer/errors/export_node_in_non_node_derived_class_1.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
GDTEST_ANALYZER_ERROR
Node export is only supported in Node-derived classes, but the current class inherits "Resource".
9 changes: 9 additions & 0 deletions modules/gdscript/tests/scripts/analyzer/errors/export_node_in_non_node_derived_class_2.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# GH-82809

extends Node

class Inner:
@export var node: Node

func test():
pass
2 changes: 2 additions & 0 deletions modules/gdscript/tests/scripts/analyzer/errors/export_node_in_non_node_derived_class_2.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
GDTEST_ANALYZER_ERROR
Node export is only supported in Node-derived classes, but the current class inherits "RefCounted".
8 changes: 8 additions & 0 deletions modules/gdscript/tests/scripts/analyzer/errors/export_node_in_non_node_derived_class_3.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# GH-82809

extends Resource

@export var node_array: Array[Node]

func test():
pass
2 changes: 2 additions & 0 deletions modules/gdscript/tests/scripts/analyzer/errors/export_node_in_non_node_derived_class_3.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
GDTEST_ANALYZER_ERROR
Node export is only supported in Node-derived classes, but the current class inherits "Resource".
Loading
Loading