From d95ff2d2fb6920c2a2ff54661dbbca5b54275772 Mon Sep 17 00:00:00 2001 From: Tyler Wilding Date: Fri, 16 Sep 2022 20:28:44 -0400 Subject: [PATCH] lsp: improve LSP IR2 hovers (#1891) Some new hover features: docstrings (`@param/@return` syntax is not yet supported, nor is a syntax-highlighted preview, will do later) ![image](https://user-images.githubusercontent.com/13153231/190830490-d0ef774c-e7e5-4bb9-8007-b366be0f491e.png) better MIPs instruction descriptions ![image](https://user-images.githubusercontent.com/13153231/190830507-0bb35c13-7e88-4b74-a63b-b7fb3587b82e.png) numeric hovers convert to different bases / tell you the OpenGOAL method id (i can't tell you the amount of times ive done -16 / 4 manually myself...) ![image](https://user-images.githubusercontent.com/13153231/190830674-f66ed15a-f983-48ff-b251-259374dfbcac.png) Closes https://github.com/open-goal/opengoal-vscode/issues/33 Closes https://github.com/open-goal/opengoal-vscode/issues/31 Related to https://github.com/open-goal/opengoal-vscode/issues/29 --- lsp/handlers/lsp_router.cpp | 14 +-- lsp/handlers/text_document/go_to.h | 16 ++-- lsp/handlers/text_document/hover.h | 142 ++++++++++++++++++++++++++--- lsp/main.cpp | 2 +- lsp/state/workspace.cpp | 27 ++---- lsp/state/workspace.h | 4 +- 6 files changed, 158 insertions(+), 47 deletions(-) diff --git a/lsp/handlers/lsp_router.cpp b/lsp/handlers/lsp_router.cpp index e8baeda8a2..be98c0bfc8 100644 --- a/lsp/handlers/lsp_router.cpp +++ b/lsp/handlers/lsp_router.cpp @@ -62,12 +62,11 @@ std::optional> LSPRouter::route_message( const MessageBuffer& message_buffer, AppState& appstate) { const json& body = message_buffer.body(); - auto method = body["method"]; - lg::info(method); + auto& method = body["method"]; // If the workspace has not yet been initialized but the client sends a // message that doesn't have method "initialize" then we'll return an error - // as per LSP spec. + // as per the LSP spec. if (method != "initialize" && !appstate.workspace.is_initialized()) { auto error = { make_response(error_resp(ErrorCodes::ServerNotInitialized, "Server not yet initialized."))}; @@ -94,13 +93,14 @@ std::optional> LSPRouter::route_message( break; case LSPRouteType::REQUEST_RESPONSE: auto resp_body = route.m_request_handler(appstate.workspace, body["id"], body["params"]); + json resp; + resp["id"] = body["id"]; if (resp_body) { - json resp; - // TODO - this should be the job of the handler, not here! - resp["id"] = body["id"]; resp["result"] = resp_body.value(); - resp_bodies.push_back(resp); + } else { + resp["result"] = nullptr; } + resp_bodies.push_back(resp); break; } diff --git a/lsp/handlers/text_document/go_to.h b/lsp/handlers/text_document/go_to.h index 11eb59fb50..c57825393f 100644 --- a/lsp/handlers/text_document/go_to.h +++ b/lsp/handlers/text_document/go_to.h @@ -19,25 +19,25 @@ std::optional go_to_definition_handler(Workspace& workspace, int id, json if (!symbol_name) { lg::debug("GOTODEF - no symbol"); - return locations; + return {}; } lg::debug("GOTODEF - symbol - {}", symbol_name.value()); - auto symbol_info = - workspace.get_symbol_info_from_all_types(symbol_name.value(), tracked_file->m_all_types_uri); + auto symbol_info = workspace.get_definition_info_from_all_types(symbol_name.value(), + tracked_file->m_all_types_uri); if (!symbol_info) { lg::debug("GOTODEF - no symbol info"); - return locations; + return {}; } LSPSpec::Location location; location.m_uri = tracked_file->m_all_types_uri; - location.m_range.m_start = {(uint32_t)symbol_info->line_idx_to_display, - (uint32_t)symbol_info->pos_in_line}; - location.m_range.m_end = {(uint32_t)symbol_info->line_idx_to_display, - (uint32_t)symbol_info->pos_in_line}; + location.m_range.m_start = {(uint32_t)symbol_info.value().definition_info->line_idx_to_display, + (uint32_t)symbol_info.value().definition_info->pos_in_line}; + location.m_range.m_end = {(uint32_t)symbol_info.value().definition_info->line_idx_to_display, + (uint32_t)symbol_info.value().definition_info->pos_in_line}; locations.push_back(location); return locations; diff --git a/lsp/handlers/text_document/hover.h b/lsp/handlers/text_document/hover.h index 9f837791b6..f38f88d894 100644 --- a/lsp/handlers/text_document/hover.h +++ b/lsp/handlers/text_document/hover.h @@ -1,10 +1,19 @@ #include +#include #include "lsp/protocol/common_types.h" #include "lsp/protocol/hover.h" #include "lsp/state/data/mips_instructions.h" #include "lsp/state/workspace.h" +bool is_number(const std::string& s) { + return !s.empty() && std::find_if(s.begin(), s.end(), + [](unsigned char c) { return !std::isdigit(c); }) == s.end(); +} + +std::vector og_method_names = {"new", "delete", "print", "inspect", "length", + "asize-of", "copy", "relocate", "mem-usage"}; + std::optional hover_handler(Workspace& workspace, int id, json params) { auto converted_params = params.get(); auto tracked_file = workspace.get_tracked_ir_file(converted_params.m_textDocument.m_uri); @@ -13,32 +22,143 @@ std::optional hover_handler(Workspace& workspace, int id, json params) { return {}; } + // See if it's an OpenGOAL symbol or a MIPS mnemonic + auto symbol_name = tracked_file->get_symbol_at_position(converted_params.m_position); auto token_at_pos = tracked_file->get_mips_instruction_at_position(converted_params.m_position); + if (!symbol_name && !token_at_pos) { + return {}; + } + + // TODO - try specifying the range so it highlights everything, ie. `c.lt.s` LSPSpec::MarkupContent markup; markup.m_kind = "markdown"; + + // Prefer symbols + if (symbol_name) { + lg::debug("hover - symbol match - {}", symbol_name.value()); + auto symbol_info = workspace.get_definition_info_from_all_types(symbol_name.value(), + tracked_file->m_all_types_uri); + if (symbol_info && symbol_info.value().docstring.has_value()) { + std::string docstring = symbol_info.value().docstring.value(); + lg::debug("hover - symbol has docstring - {}", docstring); + // A docstring exists, print it! + // By convention, docstrings are assumed to be markdown, they support code-blocks everything + // the only thing extra we do, is replace [[]] with links if available + std::unordered_map symbol_replacements = {}; + std::smatch match; + + std::string::const_iterator searchStart(docstring.cbegin()); + while ( + std::regex_search(searchStart, docstring.cend(), match, std::regex("\\[{2}(.*)\\]{2}"))) { + // Have we already accounted for this symbol? + const auto& name = match[1].str(); + if (symbol_replacements.count(name) != 0) { + continue; + } + // Get this symbols info + auto symbol_info = + workspace.get_definition_info_from_all_types(name, tracked_file->m_all_types_uri); + if (!symbol_info) { + symbol_replacements[name] = fmt::format("_{}_", name); + } else { + // Construct path + auto symbol_uri = + fmt::format("{}#L{}%2C{}", tracked_file->m_all_types_uri, + symbol_info.value().definition_info->line_idx_to_display + 1, + symbol_info.value().definition_info->pos_in_line); + symbol_replacements[name] = fmt::format("[{}]({})", name, symbol_uri); + } + searchStart = match.suffix().first; + } + // Replace all symbol occurences + for (const auto& [key, val] : symbol_replacements) { + docstring = std::regex_replace(docstring, std::regex("\\[{2}" + key + "\\]{2}"), val); + } + + markup.m_value = docstring; + LSPSpec::Hover hover_resp; + hover_resp.m_contents = markup; + return hover_resp; + } else if (!token_at_pos) { + // Check if it's a number, and if so we'll do some numeric conversions + if (!is_number(symbol_name.value())) { + return {}; + } + lg::debug("hover - numeric match - {}", symbol_name.value()); + // Construct the body + std::string body = ""; + uint32_t num = std::atoi(symbol_name.value().data()); + // Assuming it comes in as Decimal + body += "| Base | Value |\n"; + body += "|---------|-------|\n"; + body += fmt::format("| Decimal | `{:d}` |\n", num); + body += fmt::format("| Hex | `{:X}` |\n", num); + // TODO - would be nice to format as groups of 4 + body += fmt::format("| Binary | `{:b}` |\n", num); + if (num >= 16 && (num - 16) % 4 == 0) { + uint32_t method_id = (num - 16) / 4; + std::string method_name = "not built-in"; + if (method_id <= 8) { + method_name = og_method_names.at(method_id); + } + body += fmt::format("| Method ID | `{}` - `{}` |\n", method_id, method_name); + } + body += fmt::format("| Octal | `{:o}` |\n", num); + + markup.m_value = body; + LSPSpec::Hover hover_resp; + hover_resp.m_contents = markup; + return hover_resp; + } + } + + // Otherwise, maybe it's a MIPS instruction if (token_at_pos) { - auto token = token_at_pos.value(); + lg::debug("hover - token match - {}", token_at_pos.value()); + auto& token = token_at_pos.value(); std::transform(token.begin(), token.end(), token.begin(), [](unsigned char c) { return std::tolower(c); }); // Find the instruction, there are some edge-cases here where they could be multiple - // TODO - handle those (print both is an easy fix?) // TODO - havn't addressed `bc` and such instructions! Those need to be prefixed matched + std::vector ee_instructions = {}; + std::vector vu_instructions = {}; for (const auto& instr : LSPData::MIPS_INSTRUCTION_LIST) { auto mnemonic_lower = instr.mnemonic; std::transform(mnemonic_lower.begin(), mnemonic_lower.end(), mnemonic_lower.begin(), [](unsigned char c) { return std::tolower(c); }); if (mnemonic_lower == token) { - markup.m_value = fmt::format("### {}\n{}", instr.mnemonic, instr.description); - break; + if (instr.type == "ee") { + ee_instructions.push_back(fmt::format("- _{}_\n\n", instr.description)); + } else { + vu_instructions.push_back(fmt::format("- _{}_\n\n", instr.description)); + } } } - } else { - return {}; - } - LSPSpec::Hover hover_resp; - hover_resp.m_contents = markup; - // TODO - try specifying the range so it highlights everything, ie. `c.lt.s` - return hover_resp; + // Construct the body + std::string body = ""; + if (!ee_instructions.empty()) { + body += "**EE Instructions**\n\n"; + for (const auto& instr : ee_instructions) { + body += instr; + } + body += "___\n\n"; + } + + if (!vu_instructions.empty()) { + body += "**VU Instructions**\n\n"; + for (const auto& instr : vu_instructions) { + body += instr; + } + body += "___\n\n"; + } + + markup.m_value = body; + LSPSpec::Hover hover_resp; + hover_resp.m_contents = markup; + return hover_resp; + } + + return {}; } diff --git a/lsp/main.cpp b/lsp/main.cpp index 96d7b4d201..7e4cce0e75 100644 --- a/lsp/main.cpp +++ b/lsp/main.cpp @@ -92,7 +92,7 @@ int main(int argc, char** argv) { for (const auto& response : responses.value()) { std::cout << response.c_str() << std::flush; if (appstate.verbose) { - lg::trace("<<< Sending message: {}", response); + lg::debug("<<< Sending message: {}", response); } else { lg::info("<<< Sending message of method '{}'", method_name); } diff --git a/lsp/state/workspace.cpp b/lsp/state/workspace.cpp index 57975c8e00..1d18e61602 100644 --- a/lsp/state/workspace.cpp +++ b/lsp/state/workspace.cpp @@ -61,7 +61,7 @@ std::optional Workspace::get_tracked_ir_file(const LSPSpec::URI return m_tracked_ir_files[file_uri]; } -std::optional Workspace::get_symbol_info_from_all_types( +std::optional Workspace::get_definition_info_from_all_types( const std::string& symbol_name, const LSPSpec::DocumentUri& all_types_uri) { if (m_tracked_all_types_files.count(all_types_uri) == 0) { @@ -71,7 +71,7 @@ std::optional Workspace::get_symbol_info_from_all_types if (dts.symbol_metadata_map.count(symbol_name) == 0) { return {}; } - return dts.symbol_metadata_map.at(symbol_name).definition_info; + return dts.symbol_metadata_map.at(symbol_name); } void Workspace::start_tracking_file(const LSPSpec::DocumentUri& file_uri, @@ -163,12 +163,11 @@ void WorkspaceIRFile::find_all_types_path(const std::string& line) { if (std::regex_search(line, matches, regex)) { if (matches.size() == 3) { - auto game_version = matches[1]; - auto all_types_path = matches[2]; + const auto& game_version = matches[1]; + const auto& all_types_path = matches[2]; lg::debug("Found DTS Path - {} : {}", game_version.str(), all_types_path.str()); auto all_types_uri = uri_from_path(fs::path(all_types_path.str())); lg::debug("DTS URI - {}", all_types_uri); - if (valid_game_version(game_version.str())) { m_game_version = game_name_to_version(game_version.str()); m_all_types_uri = all_types_uri; @@ -188,7 +187,7 @@ void WorkspaceIRFile::find_function_symbol(const uint32_t line_num_zero_based, if (std::regex_search(line, matches, regex)) { // NOTE - assumes we can only find 1 function per line if (matches.size() == 2) { - auto match = matches[1]; + const auto& match = matches[1]; lg::info("Adding Symbol - {}", match.str()); LSPSpec::DocumentSymbol new_symbol; new_symbol.m_name = match.str(); @@ -209,7 +208,6 @@ void WorkspaceIRFile::find_function_symbol(const uint32_t line_num_zero_based, std::regex end_function("^;; \\.endfunction\\s*$"); if (std::regex_match(line, end_function)) { - lg::debug("Found end of previous function on line - {}", line); // Set the previous symbols end-line if (!m_symbols.empty()) { m_symbols[m_symbols.size() - 1].m_range.m_end.m_line = line_num_zero_based - 1; @@ -234,8 +232,7 @@ void WorkspaceIRFile::identify_diagnostics(const uint32_t line_num_zero_based, if (std::regex_search(line, info_matches, info_regex)) { // NOTE - assumes we can only find 1 function per line if (info_matches.size() == 2) { - auto match = info_matches[1]; - lg::debug("Found info-level diagnostic - {}", match.str()); + const auto& match = info_matches[1]; LSPSpec::Diagnostic new_diag; new_diag.m_severity = LSPSpec::DiagnosticSeverity::Information; new_diag.m_message = match.str(); @@ -249,8 +246,7 @@ void WorkspaceIRFile::identify_diagnostics(const uint32_t line_num_zero_based, if (std::regex_search(line, warn_matches, warn_regex)) { // NOTE - assumes we can only find 1 function per line if (warn_matches.size() == 2) { - auto match = warn_matches[1]; - lg::debug("Found warn-level diagnostic - {}", match.str()); + const auto& match = warn_matches[1]; LSPSpec::Diagnostic new_diag; new_diag.m_severity = LSPSpec::DiagnosticSeverity::Warning; new_diag.m_message = match.str(); @@ -264,8 +260,7 @@ void WorkspaceIRFile::identify_diagnostics(const uint32_t line_num_zero_based, if (std::regex_search(line, error_matches, error_regex)) { // NOTE - assumes we can only find 1 function per line if (error_matches.size() == 2) { - auto match = error_matches[1]; - lg::debug("Found error-level diagnostic - {}", match.str()); + const auto& match = error_matches[1]; LSPSpec::Diagnostic new_diag; new_diag.m_severity = LSPSpec::DiagnosticSeverity::Error; new_diag.m_message = match.str(); @@ -285,8 +280,7 @@ std::optional WorkspaceIRFile::get_mips_instruction_at_position( std::regex regex("[\\w\\.]+"); if (std::regex_search(line, matches, regex)) { - auto match = matches[0]; - lg::info("hover first match - {}", match.str()); + const auto& match = matches[0]; auto match_start = matches.position(0); auto match_end = match_start + match.length(); if (position.m_character >= match_start && position.m_character <= match_end) { @@ -301,18 +295,15 @@ std::optional WorkspaceIRFile::get_symbol_at_position( const LSPSpec::Position position) { // Split the line on typical word boundaries std::string line = m_lines.at(position.m_line); - lg::debug("symbol checking - '{}'", line); std::smatch matches; std::regex regex("[\\w\\.\\-_!<>*]+"); std::regex_token_iterator rend; std::regex_token_iterator match(line.begin(), line.end(), regex); while (match != rend) { - lg::debug("match - '{}'", match->str()); auto match_start = std::distance(line.begin(), match->first); auto match_end = match_start + match->length(); if (position.m_character >= match_start && position.m_character <= match_end) { - lg::debug("returning"); return match->str(); } match++; diff --git a/lsp/state/workspace.h b/lsp/state/workspace.h index ae69f854be..2d7c18404a 100644 --- a/lsp/state/workspace.h +++ b/lsp/state/workspace.h @@ -69,9 +69,9 @@ class Workspace { void update_tracked_file(const LSPSpec::DocumentUri& file_uri, const std::string& content); void stop_tracking_file(const LSPSpec::DocumentUri& file_uri); std::optional get_tracked_ir_file(const LSPSpec::URI& file_uri); - std::optional get_symbol_info_from_all_types( + std::optional get_definition_info_from_all_types( const std::string& symbol_name, - const std::string& all_types_uri); + const LSPSpec::DocumentUri& all_types_uri); private: bool m_initialized = false;