From 0813b357d28e9715dde4c91cc22441eb9822660b Mon Sep 17 00:00:00 2001 From: HolonProduction Date: Sun, 26 Apr 2026 19:02:11 +0200 Subject: [PATCH] LSP: Fix crash in `find_usages_in_file` by correctly resolving annotation names --- modules/gdscript/gdscript_editor.cpp | 2 +- .../gdscript_extend_parser.cpp | 70 ++++++++++++------- .../language_server/gdscript_extend_parser.h | 8 ++- .../gdscript_language_protocol.cpp | 10 +-- .../language_server/gdscript_workspace.cpp | 50 +++++++------ modules/gdscript/language_server/godot_lsp.h | 7 ++ modules/gdscript/tests/test_lsp.h | 33 +++++++++ 7 files changed, 126 insertions(+), 54 deletions(-) diff --git a/modules/gdscript/gdscript_editor.cpp b/modules/gdscript/gdscript_editor.cpp index cba1cd494f..b29b1cead2 100644 --- a/modules/gdscript/gdscript_editor.cpp +++ b/modules/gdscript/gdscript_editor.cpp @@ -4631,7 +4631,7 @@ static Error _lookup_symbol_from_base(const GDScriptParser::DataType &p_base, co } } break; case GDScriptParser::COMPLETION_ANNOTATION: { - const String annotation_symbol = "@" + p_symbol; + const String annotation_symbol = "@" + p_symbol.trim_prefix("@"); if (parser.annotation_exists(annotation_symbol)) { r_result.type = ScriptLanguage::LOOKUP_RESULT_CLASS_ANNOTATION; r_result.class_name = "@GDScript"; diff --git a/modules/gdscript/language_server/gdscript_extend_parser.cpp b/modules/gdscript/language_server/gdscript_extend_parser.cpp index 9d6333d4e7..21b6d8cb55 100644 --- a/modules/gdscript/language_server/gdscript_extend_parser.cpp +++ b/modules/gdscript/language_server/gdscript_extend_parser.cpp @@ -700,14 +700,29 @@ String ExtendGDScriptParser::get_text_for_lookup_symbol(const LSP::Position &p_c return longthing; } -String ExtendGDScriptParser::get_identifier_under_position(const LSP::Position &p_position, LSP::Range &r_range) const { +String ExtendGDScriptParser::get_symbol_name_under_position(const LSP::Position &p_position, LSP::Range &r_range) const { + r_range = LSP::Range(p_position, p_position); // Default for error macros. ERR_FAIL_INDEX_V(p_position.line, lines.size(), ""); + String line = lines[p_position.line]; if (line.is_empty()) { return ""; } + // Checks against line.size(), which includes a terminating NUL. This is to allow a cursor after the last character. ERR_FAIL_INDEX_V(p_position.character, line.size(), ""); + LSP::Position pos = p_position; + + // Cursor after last character. + if (pos.character >= line.length()) { + pos.character--; + } + + // If on the start of an annotation move the position into the identifier part. We account for "@" at the end. + if (line[pos.character] == '@' && pos.character + 1 < line.size() && is_unicode_identifier_start(line[pos.character + 1])) { + pos.character++; + } + // `p_position` cursor is BETWEEN chars, not ON chars. // -> // ```gdscript @@ -719,51 +734,54 @@ String ExtendGDScriptParser::get_identifier_under_position(const LSP::Position & // | // | cursor on `member`, pos on ` ` (space) // ``` - // -> Move position to previous character if: - // * Position not on valid identifier char. - // * Prev position is valid identifier char. - LSP::Position pos = p_position; - if ( - pos.character >= line.length() // Cursor at end of line. - || (!is_unicode_identifier_continue(line[pos.character]) // Not on valid identifier char. - && (pos.character > 0 // Not line start -> there is a prev char. - && is_unicode_identifier_continue(line[pos.character - 1]) // Prev is valid identifier char. - ))) { + if (!is_unicode_identifier_continue(line[pos.character])) { + if (pos.character == 0 || !is_unicode_identifier_continue(line[pos.character - 1])) { + // In between two non-identifier chars. + return ""; + } + // Move position to previous character if not on valid char and the previous char is valid. pos.character--; } - int start_pos = pos.character; + // Iterate forward till we have a start. Symbol starts require lookahead, so we save the latest valid start and track back to it once we can stop looking. + // E.g. ?0nly + // ^ + // | could be an identifier start (since 0 can't start identifiers), but if there was another symbol in front of 0 the identifier would be longer. + int last_valid_start_pos = -1; for (int c = pos.character; c >= 0; c--) { - start_pos = c; char32_t ch = line[c]; - bool valid_char = is_unicode_identifier_continue(ch); - if (!valid_char) { + if (is_unicode_identifier_start(ch)) { + last_valid_start_pos = c; + } + if (!is_unicode_identifier_continue(ch)) { break; } } - int end_pos = pos.character; + // Iterate backwards till we have an end. No lookahead required. Uses +1 since the end of a range is exclusive. + int end_pos = pos.character + 1; for (int c = pos.character; c < line.length(); c++) { char32_t ch = line[c]; - bool valid_char = is_unicode_identifier_continue(ch); - if (!valid_char) { + if (!is_unicode_identifier_continue(ch)) { break; } - end_pos = c; + end_pos = c + 1; } - if (!is_unicode_identifier_start(line[start_pos + 1])) { + if (last_valid_start_pos == -1) { return ""; } - if (start_pos < end_pos) { - r_range.start.line = r_range.end.line = pos.line; - r_range.start.character = start_pos + 1; - r_range.end.character = end_pos + 1; - return line.substr(start_pos + 1, end_pos - start_pos); + // For annotations we include the @, since it is included in the symbol name used by other parts of the LSP code. + int start_pos = last_valid_start_pos; + if (start_pos > 0 && line[start_pos - 1] == '@') { + start_pos -= 1; } - return ""; + r_range.start.character = start_pos; + r_range.end.character = end_pos; + + return line.substr(start_pos, end_pos - start_pos); } String ExtendGDScriptParser::get_uri() const { diff --git a/modules/gdscript/language_server/gdscript_extend_parser.h b/modules/gdscript/language_server/gdscript_extend_parser.h index ecc8b26492..d3ba003f63 100644 --- a/modules/gdscript/language_server/gdscript_extend_parser.h +++ b/modules/gdscript/language_server/gdscript_extend_parser.h @@ -139,7 +139,13 @@ public: String get_text_for_completion(const LSP::Position &p_cursor) const; String get_text_for_lookup_symbol(const LSP::Position &p_cursor, const String &p_symbol = "", bool p_func_required = false) const; - String get_identifier_under_position(const LSP::Position &p_position, LSP::Range &r_range) const; + /** + * Parses the symbol name at the given position. Returns that name and its full range. + * + * The returned name might be a false positive and not translate to an actual symbol e.g. it might be a language keyword. + * The results of this method are not equivalent to identifier AST nodes. Instead it returns results that are compatible with `LSP::DocumentSymbol::name` i.e. includes `@` for annotations. + */ + String get_symbol_name_under_position(const LSP::Position &p_position, LSP::Range &r_range) const; String get_uri() const; /** diff --git a/modules/gdscript/language_server/gdscript_language_protocol.cpp b/modules/gdscript/language_server/gdscript_language_protocol.cpp index a5538ddafb..230cfb39a0 100644 --- a/modules/gdscript/language_server/gdscript_language_protocol.cpp +++ b/modules/gdscript/language_server/gdscript_language_protocol.cpp @@ -631,12 +631,12 @@ void GDScriptLanguageProtocol::resolve_related_symbols(const LSP::TextDocumentPo return; } - String symbol_identifier; + String symbol_name; LSP::Range range; - symbol_identifier = parser->get_identifier_under_position(p_doc_pos.position, range); + symbol_name = parser->get_symbol_name_under_position(p_doc_pos.position, range); for (const KeyValue &E : workspace->native_members) { - if (const LSP::DocumentSymbol *const *symbol = E.value.getptr(symbol_identifier)) { + if (const LSP::DocumentSymbol *const *symbol = E.value.getptr(symbol_name)) { r_list.push_back(*symbol); } } @@ -644,13 +644,13 @@ void GDScriptLanguageProtocol::resolve_related_symbols(const LSP::TextDocumentPo for (const KeyValue &E : client->parse_results) { const ExtendGDScriptParser *scr = E.value; const ClassMembers &members = scr->get_members(); - if (const LSP::DocumentSymbol *const *symbol = members.getptr(symbol_identifier)) { + if (const LSP::DocumentSymbol *const *symbol = members.getptr(symbol_name)) { r_list.push_back(*symbol); } for (const KeyValue &F : scr->get_inner_classes()) { const ClassMembers *inner_class = &F.value; - if (const LSP::DocumentSymbol *const *symbol = inner_class->getptr(symbol_identifier)) { + if (const LSP::DocumentSymbol *const *symbol = inner_class->getptr(symbol_name)) { r_list.push_back(*symbol); } } diff --git a/modules/gdscript/language_server/gdscript_workspace.cpp b/modules/gdscript/language_server/gdscript_workspace.cpp index 50ecfe1ae1..9be68897f2 100644 --- a/modules/gdscript/language_server/gdscript_workspace.cpp +++ b/modules/gdscript/language_server/gdscript_workspace.cpp @@ -423,7 +423,7 @@ bool GDScriptWorkspace::can_rename(const LSP::TextDocumentPositionParams &p_doc_ String path = get_file_path(p_doc_pos.textDocument.uri); const ExtendGDScriptParser *parser = GDScriptLanguageProtocol::get_singleton()->get_parse_result(path); if (parser) { - _ALLOW_DISCARD_ parser->get_identifier_under_position(p_doc_pos.position, r_range); + _ALLOW_DISCARD_ parser->get_symbol_name_under_position(p_doc_pos.position, r_range); r_symbol = *reference_symbol; return true; } @@ -453,7 +453,7 @@ Vector GDScriptWorkspace::find_usages_in_file(const LSP::Document params.position.character = character; LSP::Range range; - String identifier_under_cursor = parser->get_identifier_under_position(params.position, range); + String identifier_under_cursor = parser->get_symbol_name_under_position(params.position, range); if (identifier_under_cursor == identifier) { const LSP::DocumentSymbol *other_symbol = resolve_symbol(params); @@ -468,7 +468,14 @@ Vector GDScriptWorkspace::find_usages_in_file(const LSP::Document } } - character = line.find(identifier, range.end.character); + if (identifier_under_cursor.length() < identifier.length()) { + // `get_symbol_name_under_position` is supposed to recognize all possible symbol names. Since a simple string search already confirmed + // the presence of `p_symbol.name` in the text, this case has to be a bug. + ERR_PRINT(vformat("LSP Bug, please report. \"get_symbol_name_under_position\" did not correctly resolve \"%s\"", identifier)); + character = line.find(identifier, character + 1); + } else { + character = line.find(identifier, range.end.character); + } } } } @@ -641,29 +648,30 @@ const LSP::DocumentSymbol *GDScriptWorkspace::resolve_symbol(const LSP::TextDocu const ExtendGDScriptParser *parser = GDScriptLanguageProtocol::get_singleton()->get_parse_result(path); if (parser) { - String symbol_identifier = p_symbol_name; - if (symbol_identifier.get_slice_count("(") > 0) { - symbol_identifier = symbol_identifier.get_slicec('(', 0); + String symbol_name = p_symbol_name; + if (symbol_name.get_slice_count("(") > 0) { + symbol_name = symbol_name.get_slicec('(', 0); } LSP::Position pos = p_doc_pos.position; - if (symbol_identifier.is_empty()) { + if (symbol_name.is_empty()) { LSP::Range range; - symbol_identifier = parser->get_identifier_under_position(p_doc_pos.position, range); + symbol_name = parser->get_symbol_name_under_position(p_doc_pos.position, range); pos.character = range.end.character; } - if (!symbol_identifier.is_empty()) { - if (ScriptServer::is_global_class(symbol_identifier)) { - String class_path = ScriptServer::get_global_class_path(symbol_identifier); + if (!symbol_name.is_empty()) { + if (ScriptServer::is_global_class(symbol_name)) { + String class_path = ScriptServer::get_global_class_path(symbol_name); symbol = get_script_symbol(class_path); } else { ScriptLanguage::LookupResult ret; - if (symbol_identifier == "new" && parser->get_lines()[p_doc_pos.position.line].remove_chars(" \t").contains("new(")) { - symbol_identifier = "_init"; + // TODO: `lookup_code` should already account for this. We might be able to simplify code here. + if (symbol_name == "new" && parser->get_lines()[p_doc_pos.position.line].remove_chars(" \t").contains("new(")) { + symbol_name = "_init"; } - if (OK == GDScriptLanguage::get_singleton()->lookup_code(parser->get_text_for_lookup_symbol(pos, symbol_identifier, p_func_required), symbol_identifier, path, nullptr, ret)) { + if (OK == GDScriptLanguage::get_singleton()->lookup_code(parser->get_text_for_lookup_symbol(pos, symbol_name, p_func_required), symbol_name, path, nullptr, ret)) { if (ret.location >= 0) { String target_script_path = path; if (ret.script.is_valid()) { @@ -674,13 +682,13 @@ const LSP::DocumentSymbol *GDScriptWorkspace::resolve_symbol(const LSP::TextDocu const ExtendGDScriptParser *target_parser = GDScriptLanguageProtocol::get_singleton()->get_parse_result(target_script_path); if (target_parser) { - symbol = target_parser->get_symbol_defined_at_line(LINE_NUMBER_TO_INDEX(ret.location), symbol_identifier); + symbol = target_parser->get_symbol_defined_at_line(LINE_NUMBER_TO_INDEX(ret.location), symbol_name); if (symbol) { switch (symbol->kind) { case LSP::SymbolKind::Function: { - if (symbol->name != symbol_identifier) { - symbol = get_parameter_symbol(symbol, symbol_identifier); + if (symbol->name != symbol_name) { + symbol = get_parameter_symbol(symbol, symbol_name); } } break; } @@ -688,15 +696,15 @@ const LSP::DocumentSymbol *GDScriptWorkspace::resolve_symbol(const LSP::TextDocu } } else { String member = ret.class_member; - if (member.is_empty() && symbol_identifier != ret.class_name) { - member = symbol_identifier; + if (member.is_empty() && symbol_name != ret.class_name) { + member = symbol_name; } symbol = get_native_symbol(ret.class_name, member); } } else { - symbol = get_local_symbol_at(parser, symbol_identifier, p_doc_pos.position); + symbol = get_local_symbol_at(parser, symbol_name, p_doc_pos.position); if (!symbol) { - symbol = parser->get_member_symbol(symbol_identifier); + symbol = parser->get_member_symbol(symbol_name); } } } diff --git a/modules/gdscript/language_server/godot_lsp.h b/modules/gdscript/language_server/godot_lsp.h index 0571920dde..5688d22099 100644 --- a/modules/gdscript/language_server/godot_lsp.h +++ b/modules/gdscript/language_server/godot_lsp.h @@ -110,6 +110,9 @@ struct Position { dict["character"] = character; return dict; } + + Position() = default; + Position(int p_line, int p_character) : line(p_line), character(p_character) {} }; /** @@ -160,6 +163,10 @@ struct Range { dict["end"] = end.to_json(); return dict; } + + Range() = default; + Range(Position p_start, Position p_end) : start(p_start), end(p_end) {} + Range(int p_start_line, int p_start_column, int p_end_line, int p_end_column) : start(p_start_line, p_start_column), end(p_end_line, p_end_column) {} }; /** diff --git a/modules/gdscript/tests/test_lsp.h b/modules/gdscript/tests/test_lsp.h index 33de0fac8f..3e1215056b 100644 --- a/modules/gdscript/tests/test_lsp.h +++ b/modules/gdscript/tests/test_lsp.h @@ -598,6 +598,39 @@ func f(): CHECK_EQ(LSP::marked_documentation("Class [Sprite2D] with [url=https://godotengine.org]link[/url]"), "Class `Sprite2D` with [link](https://godotengine.org)"); } + TEST_CASE("get_symbol_name_under_position") { + const String code = U"we_do suPPort Unicöde and @annotations, numb3rs \n0nly for-continuation #comment\n@start@\na\n b \n"; + const HashMap symbols = { + { "we_do", LSP::Range(0, 0, 0, 5) }, + { U"suPPort", LSP::Range(0, 6, 0, 13) }, + { U"Unicöde", LSP::Range(0, 14, 0, 21) }, + { U"and", LSP::Range(0, 23, 0, 26) }, + { "@annotations", LSP::Range(0, 27, 0, 39) }, + { U"numb3rs", LSP::Range(0, 41, 0, 48) }, + { U"nly", LSP::Range(1, 1, 1, 4) }, + { U"for", LSP::Range(1, 5, 1, 8) }, + { U"continuation", LSP::Range(1, 9, 1, 21) }, + { U"comment", LSP::Range(1, 23, 1, 30) }, + { U"@start", LSP::Range(2, 0, 2, 6) }, + { U"a", LSP::Range(3, 0, 3, 1) }, + { U"b", LSP::Range(4, 1, 4, 2) }, + }; + + ExtendGDScriptParser *parser = memnew(ExtendGDScriptParser); + parser->parse(code, "res://dummy.gd"); + + for (KeyValue symbol : symbols) { + for (int i = symbol.value.start.character; i <= symbol.value.end.character; i++) { + LSP::Range range; + const String symbol_name = parser->get_symbol_name_under_position(LSP::Position(symbol.value.start.line, i), range); + + CHECK_EQ(symbol.key, symbol_name); + CHECK_EQ(range, symbol.value); + } + } + + memdelete(parser); + } } } // namespace GDScriptTests