Merge pull request #41381 from vnen/gdscript-2-fixes

A few more GDScript fixes

modules/gdscript/gdscript_analyzer.cpp

@@ -33,6 +33,7 @@
 #include "core/class_db.h"
 #include "core/hash_map.h"
 #include "core/io/resource_loader.h"
+#include "core/os/file_access.h"
 #include "core/project_settings.h"
 #include "core/script_language.h"
 #include "gdscript.h"
@@ -1960,13 +1961,23 @@ void GDScriptAnalyzer::reduce_cast(GDScriptParser::CastNode *p_cast) {
 void GDScriptAnalyzer::reduce_dictionary(GDScriptParser::DictionaryNode *p_dictionary) {
 	bool all_is_constant = true;
 
+	HashMap<Variant, GDScriptParser::ExpressionNode *, VariantHasher, VariantComparator> elements;
+
 	for (int i = 0; i < p_dictionary->elements.size(); i++) {
 		const GDScriptParser::DictionaryNode::Pair &element = p_dictionary->elements[i];
 		if (p_dictionary->style == GDScriptParser::DictionaryNode::PYTHON_DICT) {
 			reduce_expression(element.key);
 		}
 		reduce_expression(element.value);
 		all_is_constant = all_is_constant && element.key->is_constant && element.value->is_constant;
+
+		if (element.key->is_constant) {
+			if (elements.has(element.key->reduced_value)) {
+				push_error(vformat(R"(Key "%s" was already used in this dictionary (at line %d).)", element.key->reduced_value, elements[element.key->reduced_value]->start_line), element.key);
+			} else {
+				elements[element.key->reduced_value] = element.value;
+			}
+		}
 	}
 
 	if (all_is_constant) {
@@ -2301,6 +2312,37 @@ void GDScriptAnalyzer::reduce_literal(GDScriptParser::LiteralNode *p_literal) {
 }
 
 void GDScriptAnalyzer::reduce_preload(GDScriptParser::PreloadNode *p_preload) {
+	if (!p_preload->path) {
+		return;
+	}
+
+	reduce_expression(p_preload->path);
+
+	if (!p_preload->path->is_constant) {
+		push_error("Preloaded path must be a constant string.", p_preload->path);
+		return;
+	}
+
+	if (p_preload->path->reduced_value.get_type() != Variant::STRING) {
+		push_error("Preloaded path must be a constant string.", p_preload->path);
+	} else {
+		p_preload->resolved_path = p_preload->path->reduced_value;
+		// TODO: Save this as script dependency.
+		if (p_preload->resolved_path.is_rel_path()) {
+			p_preload->resolved_path = parser->script_path.get_base_dir().plus_file(p_preload->resolved_path);
+		}
+		p_preload->resolved_path = p_preload->resolved_path.simplify_path();
+		if (!FileAccess::exists(p_preload->resolved_path)) {
+			push_error(vformat(R"(Preload file "%s" does not exist.)", p_preload->resolved_path), p_preload->path);
+		} else {
+			// TODO: Don't load if validating: use completion cache.
+			p_preload->resource = ResourceLoader::load(p_preload->resolved_path);
+			if (p_preload->resource.is_null()) {
+				push_error(vformat(R"(Could not p_preload resource file "%s".)", p_preload->resolved_path), p_preload->path);
+			}
+		}
+	}
+
 	p_preload->is_constant = true;
 	p_preload->reduced_value = p_preload->resource;
 	p_preload->set_datatype(type_from_variant(p_preload->reduced_value));
@@ -2476,7 +2518,7 @@ void GDScriptAnalyzer::reduce_subscript(GDScriptParser::SubscriptNode *p_subscri
 				// Check resulting type if possible.
 				result_type.builtin_type = Variant::NIL;
 				result_type.kind = GDScriptParser::DataType::BUILTIN;
-				result_type.type_source = GDScriptParser::DataType::INFERRED;
+				result_type.type_source = base_type.is_hard_type() ? GDScriptParser::DataType::ANNOTATED_INFERRED : GDScriptParser::DataType::INFERRED;
 
 				switch (base_type.builtin_type) {
 					// Can't index at all.

modules/gdscript/gdscript_parser.cpp

@@ -986,9 +986,15 @@ GDScriptParser::SignalNode *GDScriptParser::parse_signal() {
 	signal->identifier = parse_identifier();
 
 	if (match(GDScriptTokenizer::Token::PARENTHESIS_OPEN)) {
-		while (!check(GDScriptTokenizer::Token::PARENTHESIS_CLOSE) && !is_at_end()) {
+		do {
+			if (check(GDScriptTokenizer::Token::PARENTHESIS_CLOSE)) {
+				// Allow for trailing comma.
+				break;
+			}
+
 			ParameterNode *parameter = parse_parameter();
 			if (parameter == nullptr) {
+				push_error("Expected signal parameter name.");
 				break;
 			}
 			if (parameter->default_value != nullptr) {
@@ -1000,7 +1006,8 @@ GDScriptParser::SignalNode *GDScriptParser::parse_signal() {
 				signal->parameters_indices[parameter->identifier->name] = signal->parameters.size();
 				signal->parameters.push_back(parameter);
 			}
-		}
+		} while (match(GDScriptTokenizer::Token::COMMA) && !is_at_end());
+
 		consume(GDScriptTokenizer::Token::PARENTHESIS_CLOSE, R"*(Expected closing ")" after signal parameters.)*");
 	}
 
@@ -1022,6 +1029,8 @@ GDScriptParser::EnumNode *GDScriptParser::parse_enum() {
 	push_multiline(true);
 	consume(GDScriptTokenizer::Token::BRACE_OPEN, vformat(R"(Expected "{" after %s.)", named ? "enum name" : R"("enum")"));
 
+	HashMap<StringName, int> elements;
+
 	do {
 		if (check(GDScriptTokenizer::Token::BRACE_CLOSE)) {
 			break; // Allow trailing comma.
@@ -1033,7 +1042,9 @@ GDScriptParser::EnumNode *GDScriptParser::parse_enum() {
 			item.line = previous.start_line;
 			item.leftmost_column = previous.leftmost_column;
 
-			if (!named) {
+			if (elements.has(item.identifier->name)) {
+				push_error(vformat(R"(Name "%s" was already in this enum (at line %d).)", item.identifier->name, elements[item.identifier->name]), item.identifier);
+			} else if (!named) {
 				// TODO: Abstract this recursive member check.
 				ClassNode *parent = current_class;
 				while (parent != nullptr) {
@@ -1045,6 +1056,8 @@ GDScriptParser::EnumNode *GDScriptParser::parse_enum() {
 				}
 			}
 
+			elements[item.identifier->name] = item.line;
+
 			if (match(GDScriptTokenizer::Token::EQUAL)) {
 				ExpressionNode *value = parse_expression(false);
 				if (value == nullptr) {
@@ -1136,6 +1149,9 @@ GDScriptParser::FunctionNode *GDScriptParser::parse_function() {
 	if (match(GDScriptTokenizer::Token::FORWARD_ARROW)) {
 		make_completion_context(COMPLETION_TYPE_NAME_OR_VOID, function);
 		function->return_type = parse_type(true);
+		if (function->return_type == nullptr) {
+			push_error(R"(Expected return type or "void" after "->".)");
+		}
 	}
 
 	// TODO: Improve token consumption so it synchronizes to a statement boundary. This way we can get into the function body with unrecognized tokens.
@@ -2489,15 +2505,18 @@ GDScriptParser::ExpressionNode *GDScriptParser::parse_get_node(ExpressionNode *p
 		make_completion_context(COMPLETION_GET_NODE, get_node);
 		get_node->string = parse_literal();
 		return get_node;
-	} else if (check(GDScriptTokenizer::Token::IDENTIFIER)) {
+	} else if (current.is_node_name()) {
 		GetNodeNode *get_node = alloc_node<GetNodeNode>();
 		int chain_position = 0;
 		do {
 			make_completion_context(COMPLETION_GET_NODE, get_node, chain_position++);
-			if (!consume(GDScriptTokenizer::Token::IDENTIFIER, R"(Expect node identifer after "/".)")) {
+			if (!current.is_node_name()) {
+				push_error(R"(Expect node path after "/".)");
 				return nullptr;
 			}
-			IdentifierNode *identifier = parse_identifier();
+			advance();
+			IdentifierNode *identifier = alloc_node<IdentifierNode>();
+			identifier->name = previous.get_identifier();
 			get_node->chain.push_back(identifier);
 		} while (match(GDScriptTokenizer::Token::SLASH));
 		return get_node;
@@ -2521,29 +2540,6 @@ GDScriptParser::ExpressionNode *GDScriptParser::parse_preload(ExpressionNode *p_
 
 	if (preload->path == nullptr) {
 		push_error(R"(Expected resource path after "(".)");
-	} else if (preload->path->type != Node::LITERAL) {
-		push_error("Preloaded path must be a constant string.");
-	} else {
-		LiteralNode *path = static_cast<LiteralNode *>(preload->path);
-		if (path->value.get_type() != Variant::STRING) {
-			push_error("Preloaded path must be a constant string.");
-		} else {
-			preload->resolved_path = path->value;
-			// TODO: Save this as script dependency.
-			if (preload->resolved_path.is_rel_path()) {
-				preload->resolved_path = script_path.get_base_dir().plus_file(preload->resolved_path);
-			}
-			preload->resolved_path = preload->resolved_path.simplify_path();
-			if (!FileAccess::exists(preload->resolved_path)) {
-				push_error(vformat(R"(Preload file "%s" does not exist.)", preload->resolved_path));
-			} else {
-				// TODO: Don't load if validating: use completion cache.
-				preload->resource = ResourceLoader::load(preload->resolved_path);
-				if (preload->resource.is_null()) {
-					push_error(vformat(R"(Could not preload resource file "%s".)", preload->resolved_path));
-				}
-			}
-		}
 	}
 
 	pop_completion_call();

modules/gdscript/gdscript_tokenizer.cpp

@@ -168,6 +168,52 @@ bool GDScriptTokenizer::Token::is_identifier() const {
 	}
 }
 
+bool GDScriptTokenizer::Token::is_node_name() const {
+	// This is meant to allow keywords with the $ notation, but not as general identifiers.
+	switch (type) {
+		case IDENTIFIER:
+		case AND:
+		case AS:
+		case ASSERT:
+		case AWAIT:
+		case BREAK:
+		case BREAKPOINT:
+		case CLASS_NAME:
+		case CLASS:
+		case CONST:
+		case CONTINUE:
+		case ELIF:
+		case ELSE:
+		case ENUM:
+		case EXTENDS:
+		case FOR:
+		case FUNC:
+		case IF:
+		case IN:
+		case IS:
+		case MATCH:
+		case NAMESPACE:
+		case NOT:
+		case OR:
+		case PASS:
+		case PRELOAD:
+		case RETURN:
+		case SELF:
+		case SIGNAL:
+		case STATIC:
+		case SUPER:
+		case TRAIT:
+		case UNDERSCORE:
+		case VAR:
+		case VOID:
+		case WHILE:
+		case YIELD:
+			return true;
+		default:
+			return false;
+	}
+}
+
 String GDScriptTokenizer::get_token_name(Token::Type p_token_type) {
 	ERR_FAIL_INDEX_V_MSG(p_token_type, Token::TK_MAX, "<error>", "Using token type out of the enum.");
 	return token_names[p_token_type];

modules/gdscript/gdscript_tokenizer.h

@@ -169,6 +169,7 @@ class GDScriptTokenizer {
 
 		const char *get_name() const;
 		bool is_identifier() const;
+		bool is_node_name() const;
 		StringName get_identifier() const { return source; }
 
 		Token(Type p_type) {