From ba3d1d06ae6a343cd9dd855b6a07b2e0de23383e Mon Sep 17 00:00:00 2001 From: JohnnyMorganz Date: Thu, 26 Dec 2024 12:33:53 +0000 Subject: [PATCH 1/6] Remove weakly_canonical and replace with Luau's normalisePath weakly_canonical performs filesystem calls to check if all components of the path exist, and resolve symlinks. We don't need these features. We replace with Luau's normalisePath. Note that weakly_canonical would also make relative file paths absolute by appending them to the current working directory. This cause relative string requires to break. We resolve this by making paths absolute in string requires - we shouldn't be relying on the current directory in the first place --- CHANGELOG.md | 2 + src/Utils.cpp | 134 ++++++++++++++++++++++++ src/Workspace.cpp | 4 +- src/include/LSP/Utils.hpp | 6 ++ src/platform/LSPPlatform.cpp | 6 +- src/platform/roblox/RobloxSourcemap.cpp | 15 +-- tests/WorkspaceFileResolver.test.cpp | 12 +-- 7 files changed, 153 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4ba17b5..2c859205 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). 0.24s) ([#749](https://github.com/JohnnyMorganz/luau-lsp/issues/749)) - These improvements heavily depend on the amount of code you have matching ignore globs. Workspace diagnostics improvements depends on the performance of Luau typechecking. +- Optimised the resolution of relative string requires by remove filesystem + calls ([#856](https://github.com/JohnnyMorganz/luau-lsp/issues/856)) ## [1.37.0] - 2024-12-14 diff --git a/src/Utils.cpp b/src/Utils.cpp index ff0c5c9d..d36ba9a1 100644 --- a/src/Utils.cpp +++ b/src/Utils.cpp @@ -214,3 +214,137 @@ void replaceAll(std::string& str, const std::string& from, const std::string& to start_pos += to.length(); } } + +bool isAbsolutePath(std::string_view path) +{ +#ifdef _WIN32 + // Must either begin with "X:/", "X:\", "/", or "\", where X is a drive letter + return (path.size() >= 3 && isalpha(path[0]) && path[1] == ':' && (path[2] == '/' || path[2] == '\\')) || + (path.size() >= 1 && (path[0] == '/' || path[0] == '\\')); +#else + // Must begin with '/' + return path.size() >= 1 && path[0] == '/'; +#endif +} + +// Returns the normal/canonical form of a path (e.g. "../subfolder/../module.luau" -> "../module.luau") +std::string normalizePath(std::string_view path) +{ + return resolvePath(path, ""); +} + +// Takes a path that is relative to the file at baseFilePath and returns the path explicitly rebased onto baseFilePath. +// For absolute paths, baseFilePath will be ignored, and this function will resolve the path to a canonical path: +// (e.g. "/Users/.././Users/johndoe" -> "/Users/johndoe"). +std::string resolvePath(std::string_view path, std::string_view baseFilePath) +{ + std::vector pathComponents; + std::vector baseFilePathComponents; + + // Dependent on whether the final resolved path is absolute or relative + // - if relative (when path and baseFilePath are both relative), resolvedPathPrefix remains empty + // - if absolute (if either path or baseFilePath are absolute), resolvedPathPrefix is "C:\", "/", etc. + std::string resolvedPathPrefix; + bool isResolvedPathRelative = false; + + if (isAbsolutePath(path)) + { + // path is absolute, we use path's prefix and ignore baseFilePath + size_t afterPrefix = path.find_first_of("\\/") + 1; + resolvedPathPrefix = path.substr(0, afterPrefix); + pathComponents = splitPath(path.substr(afterPrefix)); + } + else + { + size_t afterPrefix = baseFilePath.find_first_of("\\/") + 1; + baseFilePathComponents = splitPath(baseFilePath.substr(afterPrefix)); + if (isAbsolutePath(baseFilePath)) + { + // path is relative and baseFilePath is absolute, we use baseFilePath's prefix + resolvedPathPrefix = baseFilePath.substr(0, afterPrefix); + } + else + { + // path and baseFilePath are both relative, we do not set a prefix (resolved path will be relative) + isResolvedPathRelative = true; + } + pathComponents = splitPath(path); + } + + // Remove filename from components + if (!baseFilePathComponents.empty()) + baseFilePathComponents.pop_back(); + + // Resolve the path by applying pathComponents to baseFilePathComponents + int numPrependedParents = 0; + for (std::string_view component : pathComponents) + { + if (component == "..") + { + if (baseFilePathComponents.empty()) + { + if (isResolvedPathRelative) + numPrependedParents++; // "../" will later be added to the beginning of the resolved path + } + else if (baseFilePathComponents.back() != "..") + { + baseFilePathComponents.pop_back(); // Resolve cases like "folder/subfolder/../../file" to "file" + } + } + else if (component != "." && !component.empty()) + { + baseFilePathComponents.push_back(component); + } + } + + // Create resolved path prefix for relative paths + if (isResolvedPathRelative) + { + if (numPrependedParents > 0) + { + resolvedPathPrefix.reserve(numPrependedParents * 3); + for (int i = 0; i < numPrependedParents; i++) + { + resolvedPathPrefix += "../"; + } + } + else + { + resolvedPathPrefix = "./"; + } + } + + // Join baseFilePathComponents to form the resolved path + std::string resolvedPath = resolvedPathPrefix; + for (auto iter = baseFilePathComponents.begin(); iter != baseFilePathComponents.end(); ++iter) + { + if (iter != baseFilePathComponents.begin()) + resolvedPath += "/"; + + resolvedPath += *iter; + } + if (resolvedPath.size() > resolvedPathPrefix.size() && resolvedPath.back() == '/') + { + // Remove trailing '/' if present + resolvedPath.pop_back(); + } + return resolvedPath; +} + +std::vector splitPath(std::string_view path) +{ + std::vector components; + + size_t pos = 0; + size_t nextPos = path.find_first_of("\\/", pos); + + while (nextPos != std::string::npos) + { + components.push_back(path.substr(pos, nextPos - pos)); + pos = nextPos + 1; + nextPos = path.find_first_of("\\/", pos); + } + components.push_back(path.substr(pos)); + + return components; +} \ No newline at end of file diff --git a/src/Workspace.cpp b/src/Workspace.cpp index ff40c1dd..5c335794 100644 --- a/src/Workspace.cpp +++ b/src/Workspace.cpp @@ -180,11 +180,11 @@ bool WorkspaceFolder::isIgnoredFileForAutoImports(const std::filesystem::path& p bool WorkspaceFolder::isDefinitionFile(const std::filesystem::path& path, const std::optional& givenConfig) { auto config = givenConfig ? *givenConfig : client->getConfiguration(rootUri); - auto canonicalised = std::filesystem::weakly_canonical(path); + auto canonicalised = normalizePath(path.generic_string()); for (auto& file : config.types.definitionFiles) { - if (std::filesystem::weakly_canonical(resolvePath(file)) == canonicalised) + if (normalizePath(resolvePath(file).generic_string()) == canonicalised) { return true; } diff --git a/src/include/LSP/Utils.hpp b/src/include/LSP/Utils.hpp index 8e137190..1239f7d1 100644 --- a/src/include/LSP/Utils.hpp +++ b/src/include/LSP/Utils.hpp @@ -46,3 +46,9 @@ inline bool contains(const std::map& map, const K& value) { return map.find(value) != map.end(); } + +// TODO: taken from FileUtils, use that instead? +bool isAbsolutePath(std::string_view path); +std::string normalizePath(std::string_view path); +std::string resolvePath(std::string_view relativePath, std::string_view baseFilePath); +std::vector splitPath(std::string_view path); diff --git a/src/platform/LSPPlatform.cpp b/src/platform/LSPPlatform.cpp index b7c9faec..ba283686 100644 --- a/src/platform/LSPPlatform.cpp +++ b/src/platform/LSPPlatform.cpp @@ -118,7 +118,7 @@ std::optional LSPPlatform::resolveStringRequire(const Luau::Mo return std::nullopt; std::filesystem::path basePath = contextPath->parent_path(); - auto filePath = basePath / requiredString; + auto filePath = workspaceFolder->rootUri.fsPath() / basePath / requiredString; auto luauConfig = fileResolver->getConfig(context->name); if (auto aliasedPath = resolveAlias(requiredString, luauConfig)) @@ -142,10 +142,10 @@ std::optional LSPPlatform::resolveStringRequire(const Luau::Mo } } - std::error_code ec; - filePath = std::filesystem::weakly_canonical(filePath, ec); + filePath = normalizePath(filePath.generic_string()); // Handle "init.luau" files in a directory + std::error_code ec; if (std::filesystem::is_directory(filePath, ec)) { filePath /= "init"; diff --git a/src/platform/roblox/RobloxSourcemap.cpp b/src/platform/roblox/RobloxSourcemap.cpp index 2830a62e..88c03d15 100644 --- a/src/platform/roblox/RobloxSourcemap.cpp +++ b/src/platform/roblox/RobloxSourcemap.cpp @@ -505,16 +505,12 @@ std::optional RobloxPlatform::getSourceNodeFromVirtualPath(const std::optional RobloxPlatform::getSourceNodeFromRealPath(const std::string& name) const { - std::error_code ec; - auto canonicalName = std::filesystem::weakly_canonical(name, ec); - if (ec.value() != 0) - canonicalName = name; + auto canonicalName = normalizePath(name); // URI-ify the file path so that its normalised (in particular, the drive letter) canonicalName = Uri::parse(Uri::file(canonicalName).toString()).fsPath(); - auto strName = canonicalName.generic_string(); - if (realPathsToSourceNodes.find(strName) == realPathsToSourceNodes.end()) + if (realPathsToSourceNodes.find(canonicalName) == realPathsToSourceNodes.end()) return std::nullopt; - return realPathsToSourceNodes.at(strName); + return realPathsToSourceNodes.at(canonicalName); } Luau::ModuleName RobloxPlatform::getVirtualPathFromSourceNode(const SourceNodePtr& sourceNode) @@ -529,10 +525,7 @@ std::optional RobloxPlatform::getRealPathFromSourceNode(c // TODO: make sure this is correct once we make sourcemap.json generic if (auto filePath = sourceNode->getScriptFilePath()) { - std::error_code ec; - auto canonicalName = std::filesystem::weakly_canonical(fileResolver->rootUri.fsPath() / *filePath, ec); - if (ec.value() != 0) - canonicalName = *filePath; + auto canonicalName = normalizePath((fileResolver->rootUri.fsPath() / *filePath).generic_string()); // URI-ify the file path so that its normalised (in particular, the drive letter) return Uri::parse(Uri::file(canonicalName).toString()).fsPath(); } diff --git a/tests/WorkspaceFileResolver.test.cpp b/tests/WorkspaceFileResolver.test.cpp index 7df092f3..c44c3ed0 100644 --- a/tests/WorkspaceFileResolver.test.cpp +++ b/tests/WorkspaceFileResolver.test.cpp @@ -238,12 +238,8 @@ TEST_CASE_FIXTURE(Fixture, "resolve_alias_supports_tilde_expansion") TEST_CASE_FIXTURE(Fixture, "string require doesn't add file extension if already exists") { - WorkspaceFileResolver fileResolver; - LSPPlatform platform{&fileResolver}; - fileResolver.platform = &platform; - Luau::ModuleInfo baseContext{}; - auto resolved = platform.resolveStringRequire(&baseContext, "Module.luau"); + auto resolved = workspace.platform->resolveStringRequire(&baseContext, "Module.luau"); REQUIRE(resolved.has_value()); CHECK(endsWith(resolved->name, "/Module.luau")); @@ -251,12 +247,8 @@ TEST_CASE_FIXTURE(Fixture, "string require doesn't add file extension if already TEST_CASE_FIXTURE(Fixture, "string require doesn't replace a non-luau/lua extension") { - WorkspaceFileResolver fileResolver; - LSPPlatform platform{&fileResolver}; - fileResolver.platform = &platform; - Luau::ModuleInfo baseContext{}; - auto resolved = platform.resolveStringRequire(&baseContext, "Module.mod"); + auto resolved = workspace.platform->resolveStringRequire(&baseContext, "Module.mod"); REQUIRE(resolved.has_value()); CHECK(endsWith(resolved->name, "/Module.mod.lua")); From ec771c0b9eeaa0f5b6956854cff3c389dd9c8734 Mon Sep 17 00:00:00 2001 From: JohnnyMorganz Date: Thu, 26 Dec 2024 12:42:31 +0000 Subject: [PATCH 2/6] Fix compilation on windows --- src/platform/roblox/RobloxSourcemap.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/roblox/RobloxSourcemap.cpp b/src/platform/roblox/RobloxSourcemap.cpp index 88c03d15..083ecf58 100644 --- a/src/platform/roblox/RobloxSourcemap.cpp +++ b/src/platform/roblox/RobloxSourcemap.cpp @@ -507,7 +507,7 @@ std::optional RobloxPlatform::getSourceNodeFromRealPath(const std { auto canonicalName = normalizePath(name); // URI-ify the file path so that its normalised (in particular, the drive letter) - canonicalName = Uri::parse(Uri::file(canonicalName).toString()).fsPath(); + canonicalName = Uri::parse(Uri::file(canonicalName).toString()).fsPath().generic_string(); if (realPathsToSourceNodes.find(canonicalName) == realPathsToSourceNodes.end()) return std::nullopt; return realPathsToSourceNodes.at(canonicalName); From f35c8ee9b28fc0dc490c6e615746ffec5fa00864 Mon Sep 17 00:00:00 2001 From: JohnnyMorganz Date: Thu, 26 Dec 2024 12:47:02 +0000 Subject: [PATCH 3/6] Use fileResolver root URI rather than workspaceFolder --- src/platform/LSPPlatform.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/LSPPlatform.cpp b/src/platform/LSPPlatform.cpp index ba283686..708a98e8 100644 --- a/src/platform/LSPPlatform.cpp +++ b/src/platform/LSPPlatform.cpp @@ -118,7 +118,7 @@ std::optional LSPPlatform::resolveStringRequire(const Luau::Mo return std::nullopt; std::filesystem::path basePath = contextPath->parent_path(); - auto filePath = workspaceFolder->rootUri.fsPath() / basePath / requiredString; + auto filePath = fileResolver->rootUri.fsPath() / basePath / requiredString; auto luauConfig = fileResolver->getConfig(context->name); if (auto aliasedPath = resolveAlias(requiredString, luauConfig)) From 63b1f6bfd59bef8fa294a8fe49f98c16fe2bc484 Mon Sep 17 00:00:00 2001 From: JohnnyMorganz Date: Thu, 26 Dec 2024 17:29:28 +0000 Subject: [PATCH 4/6] Fix handling of file aliases We need to apply the rootUri prefix for file aliases too in case they were relative --- src/platform/LSPPlatform.cpp | 4 +-- tests/WorkspaceFileResolver.test.cpp | 48 ++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/platform/LSPPlatform.cpp b/src/platform/LSPPlatform.cpp index 708a98e8..aa811df7 100644 --- a/src/platform/LSPPlatform.cpp +++ b/src/platform/LSPPlatform.cpp @@ -118,7 +118,7 @@ std::optional LSPPlatform::resolveStringRequire(const Luau::Mo return std::nullopt; std::filesystem::path basePath = contextPath->parent_path(); - auto filePath = fileResolver->rootUri.fsPath() / basePath / requiredString; + auto filePath = basePath / requiredString; auto luauConfig = fileResolver->getConfig(context->name); if (auto aliasedPath = resolveAlias(requiredString, luauConfig)) @@ -142,7 +142,7 @@ std::optional LSPPlatform::resolveStringRequire(const Luau::Mo } } - filePath = normalizePath(filePath.generic_string()); + filePath = normalizePath((fileResolver->rootUri.fsPath() / filePath).generic_string()); // Handle "init.luau" files in a directory std::error_code ec; diff --git a/tests/WorkspaceFileResolver.test.cpp b/tests/WorkspaceFileResolver.test.cpp index c44c3ed0..35829a72 100644 --- a/tests/WorkspaceFileResolver.test.cpp +++ b/tests/WorkspaceFileResolver.test.cpp @@ -4,6 +4,7 @@ #include "Platform/RobloxPlatform.hpp" #include "Luau/Ast.h" #include "Luau/FileResolver.h" +#include "TempDir.h" TEST_SUITE_BEGIN("WorkspaceFileResolverTests"); @@ -264,4 +265,51 @@ TEST_CASE_FIXTURE(Fixture, "string_require_resolves_relative_to_file") CHECK_EQ(Luau::toString(requireType(getModule(moduleName), "other")), "number"); } +TEST_CASE_FIXTURE(Fixture, "string_require_supports_relative_file_aliases") +{ + client->globalConfig.require.fileAliases = { + {"@file", "path/to/source.luau"}, + }; + + Luau::ModuleInfo baseContext{}; + auto resolved = workspace.platform->resolveStringRequire(&baseContext, "@file"); + REQUIRE(resolved); + CHECK_EQ(resolved->name, workspace.rootUri.fsPath() / "path/to/source.luau"); +} + +TEST_CASE_FIXTURE(Fixture, "string_require_supports_absolute_file_aliases") +{ +#ifdef _WIN32 + auto basePath = "C:/Users/test/folder"; +#else + auto basePath = "/home/folder"; +#endif + + auto fullPath = (std::filesystem::path(basePath) / "path/to/source.luau").generic_string(); + + client->globalConfig.require.fileAliases = { + {"@file", fullPath}, + }; + + Luau::ModuleInfo baseContext{}; + auto resolved = workspace.platform->resolveStringRequire(&baseContext, "@file"); + REQUIRE(resolved); + CHECK_EQ(resolved->name, fullPath); +} + +TEST_CASE_FIXTURE(Fixture, "string_require_supports_absolute_file_aliases_with_tilde_expansion") +{ + client->globalConfig.require.fileAliases = { + {"@file", "~/path/to/source.luau"}, + }; + + auto home = getHomeDirectory(); + REQUIRE(home); + + Luau::ModuleInfo baseContext{}; + auto resolved = workspace.platform->resolveStringRequire(&baseContext, "@file"); + REQUIRE(resolved); + CHECK_EQ(resolved->name, home.value() / "path/to/source.luau"); +} + TEST_SUITE_END(); From 88d02fae486d4dd193448e3dc61b16c18e0aba87 Mon Sep 17 00:00:00 2001 From: JohnnyMorganz Date: Thu, 26 Dec 2024 23:28:34 +0000 Subject: [PATCH 5/6] Add tests for auto imports --- CMakeLists.txt | 1 + tests/AutoImports.test.cpp | 617 +++++++++++++++++++++ tests/testdata/standard_definitions.d.luau | 8 +- 3 files changed, 625 insertions(+), 1 deletion(-) create mode 100644 tests/AutoImports.test.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 9b68c1bd..c018c045 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -76,6 +76,7 @@ target_sources(Luau.LanguageServer.Test PRIVATE tests/Fixture.cpp tests/TempDir.cpp tests/Autocomplete.test.cpp + tests/AutoImports.test.cpp tests/MagicFunctions.test.cpp tests/Documentation.test.cpp tests/TextDocument.test.cpp diff --git a/tests/AutoImports.test.cpp b/tests/AutoImports.test.cpp new file mode 100644 index 00000000..591d7fd7 --- /dev/null +++ b/tests/AutoImports.test.cpp @@ -0,0 +1,617 @@ +#include "doctest.h" +#include "Fixture.h" +#include "TempDir.h" +#include "Platform/RobloxPlatform.hpp" +#include "LSP/IostreamHelpers.hpp" + +static std::optional getItem(const std::vector& items, const std::string& label) +{ + for (const auto& item : items) + if (item.label == label) + return item; + + return std::nullopt; +} + +static std::vector filterAutoImports(const std::vector& items, const std::string& moduleName = "") +{ + std::vector results; + + for (const auto& item : items) + { + if (item.kind == lsp::CompletionItemKind::Module) + if (moduleName.empty() || item.label == moduleName) + results.emplace_back(item); + } + + return results; +} + +TEST_SUITE_BEGIN("AutoImports"); + +TEST_CASE_FIXTURE(Fixture, "services_show_up_in_auto_import") +{ + client->globalConfig.completion.imports.enabled = true; + auto [source, marker] = sourceWithMarker(R"( + | + )"); + + auto uri = newDocument("foo.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + + auto serviceImport = getItem(result, "ReplicatedStorage"); + REQUIRE(serviceImport); + + CHECK_EQ(serviceImport->label, "ReplicatedStorage"); + CHECK_EQ(serviceImport->insertText, "ReplicatedStorage"); + CHECK_EQ(serviceImport->detail, "Auto-import"); + CHECK_EQ(serviceImport->kind, lsp::CompletionItemKind::Class); + REQUIRE_EQ(serviceImport->additionalTextEdits.size(), 1); + CHECK_EQ(serviceImport->additionalTextEdits[0].newText, "local ReplicatedStorage = game:GetService(\"ReplicatedStorage\")\n"); + CHECK_EQ(serviceImport->additionalTextEdits[0].range, lsp::Range{{0, 0}, {0, 0}}); + + CHECK(getItem(result, "ServerScriptService")); + CHECK(getItem(result, "Workspace")); +} + +TEST_CASE_FIXTURE(Fixture, "services_do_not_show_up_in_autocomplete_if_imports_is_disabled") +{ + client->globalConfig.completion.imports.enabled = false; + auto [source, marker] = sourceWithMarker(R"( + | + )"); + + auto uri = newDocument("foo.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + CHECK_EQ(getItem(result, "ReplicatedStorage"), std::nullopt); +} + +TEST_CASE_FIXTURE(Fixture, "services_do_not_show_up_in_autocomplete_if_suggest_services_is_disabled") +{ + client->globalConfig.completion.imports.enabled = true; + client->globalConfig.completion.imports.suggestServices = false; + auto [source, marker] = sourceWithMarker(R"( + | + )"); + + auto uri = newDocument("foo.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + CHECK_EQ(getItem(result, "ReplicatedStorage"), std::nullopt); +} + +TEST_CASE_FIXTURE(Fixture, "service_does_not_show_up_in_autocomplete_if_already_exists") +{ + client->globalConfig.completion.imports.enabled = true; + auto [source, marker] = sourceWithMarker(R"( + local ReplicatedStorage = game:GetService("ReplicatedStorage") + | + )"); + + auto uri = newDocument("foo.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + + // item will exist as variable import, but it shouldn't be an auto import + auto item = getItem(result, "ReplicatedStorage"); + CHECK_NE(item->detail, "Auto-import"); + CHECK_EQ(item->additionalTextEdits.size(), 0); +} + +TEST_CASE_FIXTURE(Fixture, "service_auto_imports_are_inserted_alphabetically") +{ + client->globalConfig.completion.imports.enabled = true; + auto [source, marker] = sourceWithMarker(R"( + local ServerScriptService = game:GetService("ServerScriptService") + | + )"); + + auto uri = newDocument("foo.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + + auto beforeImport = getItem(result, "ReplicatedStorage"); + REQUIRE_EQ(beforeImport->additionalTextEdits.size(), 1); + CHECK_EQ(beforeImport->additionalTextEdits[0].range, lsp::Range{{1, 0}, {1, 0}}); + + auto afterImport = getItem(result, "Workspace"); + REQUIRE_EQ(afterImport->additionalTextEdits.size(), 1); + CHECK_EQ(afterImport->additionalTextEdits[0].range, lsp::Range{{2, 0}, {2, 0}}); +} + +TEST_CASE_FIXTURE(Fixture, "service_auto_imports_are_inserted_after_hot_comments") +{ + client->globalConfig.completion.imports.enabled = true; + auto [source, marker] = sourceWithMarker(R"( + --!strict + | + )"); + + auto uri = newDocument("foo.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + + auto import = getItem(result, "ReplicatedStorage"); + REQUIRE(import); + REQUIRE_EQ(import->additionalTextEdits.size(), 1); + CHECK_EQ(import->additionalTextEdits[0].range, lsp::Range{{2, 0}, {2, 0}}); +} + +TEST_CASE_FIXTURE(Fixture, "module_script_shows_up_in_auto_imports") +{ + client->globalConfig.completion.imports.enabled = true; + loadSourcemap(R"( + { + "name": "Game", + "className": "DataModel", + "children": [ + { + "name": "ReplicatedStorage", + "className": "ReplicatedStorage", + "children": [ + { + "name": "Folder", + "className": "Folder", + "children": [{ "name": "Module", "className": "ModuleScript" }] + } + ] + } + ] + } + )"); + + auto [source, marker] = sourceWithMarker(R"( + | + )"); + + auto uri = newDocument("foo.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + auto imports = filterAutoImports(result, "Module"); + + REQUIRE_EQ(imports.size(), 1); + CHECK_EQ(imports[0].label, "Module"); + CHECK_EQ(imports[0].insertText, "Module"); + CHECK_EQ(imports[0].kind, lsp::CompletionItemKind::Module); + + REQUIRE_EQ(imports[0].additionalTextEdits.size(), 2); + CHECK_EQ(imports[0].additionalTextEdits[0].newText, "local ReplicatedStorage = game:GetService(\"ReplicatedStorage\")\n"); + CHECK_EQ(imports[0].additionalTextEdits[0].range, lsp::Range{{0, 0}, {0, 0}}); + CHECK_EQ(imports[0].additionalTextEdits[1].newText, "local Module = require(ReplicatedStorage.Folder.Module)\n"); + CHECK_EQ(imports[0].additionalTextEdits[1].range, lsp::Range{{0, 0}, {0, 0}}); +} + +TEST_CASE_FIXTURE(Fixture, "module_script_does_not_show_up_in_autocomplete_if_imports_is_disabled") +{ + client->globalConfig.completion.imports.enabled = false; + loadSourcemap(R"( + { + "name": "Game", + "className": "DataModel", + "children": [ + { + "name": "ReplicatedStorage", + "className": "ReplicatedStorage", + "children": [ + { + "name": "Folder", + "className": "Folder", + "children": [{ "name": "Module", "className": "ModuleScript" }] + } + ] + } + ] + } + )"); + + auto [source, marker] = sourceWithMarker(R"( + | + )"); + + auto uri = newDocument("foo.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + + CHECK_EQ(filterAutoImports(result, "Module").size(), 0); +} + +TEST_CASE_FIXTURE(Fixture, "module_script_does_not_show_up_in_autocomplete_if_require_imports_is_disabled") +{ + client->globalConfig.completion.imports.enabled = true; + client->globalConfig.completion.imports.suggestRequires = false; + loadSourcemap(R"( + { + "name": "Game", + "className": "DataModel", + "children": [ + { + "name": "ReplicatedStorage", + "className": "ReplicatedStorage", + "children": [ + { + "name": "Folder", + "className": "Folder", + "children": [{ "name": "Module", "className": "ModuleScript" }] + } + ] + } + ] + } + )"); + + auto [source, marker] = sourceWithMarker(R"( + | + )"); + + auto uri = newDocument("foo.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + + CHECK_EQ(filterAutoImports(result, "Module").size(), 0); +} + +TEST_CASE_FIXTURE(Fixture, "auto_import_reuses_service_if_already_defined") +{ + client->globalConfig.completion.imports.enabled = true; + loadSourcemap(R"( + { + "name": "Game", + "className": "DataModel", + "children": [ + { + "name": "ReplicatedStorage", + "className": "ReplicatedStorage", + "children": [ + { + "name": "Folder", + "className": "Folder", + "children": [{ "name": "Module", "className": "ModuleScript" }] + } + ] + } + ] + } + )"); + + auto [source, marker] = sourceWithMarker(R"( + local ReplicatedStorage = game:GetService("ReplicatedStorage") + | + )"); + + auto uri = newDocument("foo.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + auto imports = filterAutoImports(result, "Module"); + + REQUIRE_EQ(imports.size(), 1); + REQUIRE_EQ(imports[0].additionalTextEdits.size(), 1); + CHECK_EQ(imports[0].additionalTextEdits[0].newText, "local Module = require(ReplicatedStorage.Folder.Module)\n"); + CHECK_EQ(imports[0].additionalTextEdits[0].range, lsp::Range{{2, 0}, {2, 0}}); +} + +TEST_CASE_FIXTURE(Fixture, "auto_import_separates_new_service_and_require_with_line") +{ + client->globalConfig.completion.imports.enabled = true; + client->globalConfig.completion.imports.separateGroupsWithLine = true; + loadSourcemap(R"( + { + "name": "Game", + "className": "DataModel", + "children": [ + { + "name": "ReplicatedStorage", + "className": "ReplicatedStorage", + "children": [ + { + "name": "Folder", + "className": "Folder", + "children": [{ "name": "Module", "className": "ModuleScript" }] + } + ] + } + ] + } + )"); + + auto [source, marker] = sourceWithMarker(R"( + | + )"); + + auto uri = newDocument("foo.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + auto imports = filterAutoImports(result, "Module"); + + REQUIRE_EQ(imports.size(), 1); + REQUIRE_EQ(imports[0].additionalTextEdits.size(), 2); + CHECK_EQ(imports[0].additionalTextEdits[0].newText, "local ReplicatedStorage = game:GetService(\"ReplicatedStorage\")\n\n"); + CHECK_EQ(imports[0].additionalTextEdits[0].range, lsp::Range{{0, 0}, {0, 0}}); + CHECK_EQ(imports[0].additionalTextEdits[1].newText, "local Module = require(ReplicatedStorage.Folder.Module)\n"); + CHECK_EQ(imports[0].additionalTextEdits[1].range, lsp::Range{{0, 0}, {0, 0}}); +} + +TEST_CASE_FIXTURE(Fixture, "auto_import_separates_existing_service_and_new_require_with_line") +{ + client->globalConfig.completion.imports.enabled = true; + client->globalConfig.completion.imports.separateGroupsWithLine = true; + loadSourcemap(R"( + { + "name": "Game", + "className": "DataModel", + "children": [ + { + "name": "ReplicatedStorage", + "className": "ReplicatedStorage", + "children": [ + { + "name": "Folder", + "className": "Folder", + "children": [{ "name": "Module", "className": "ModuleScript" }] + } + ] + } + ] + } + )"); + + auto [source, marker] = sourceWithMarker(R"( + local ReplicatedStorage = game:GetService("ReplicatedStorage") + | + )"); + + auto uri = newDocument("foo.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + auto imports = filterAutoImports(result, "Module"); + + REQUIRE_EQ(imports.size(), 1); + REQUIRE_EQ(imports[0].additionalTextEdits.size(), 1); + CHECK_EQ(imports[0].additionalTextEdits[0].newText, "\nlocal Module = require(ReplicatedStorage.Folder.Module)\n"); + CHECK_EQ(imports[0].additionalTextEdits[0].range, lsp::Range{{2, 0}, {2, 0}}); +} + +TEST_CASE_FIXTURE(Fixture, "auto_imports_will_prefer_relative_over_absolute_import_for_sibling_modules") +{ + client->globalConfig.completion.imports.enabled = true; + loadSourcemap(R"( + { + "name": "Game", + "className": "DataModel", + "children": [ + { + "name": "ReplicatedStorage", + "className": "ReplicatedStorage", + "children": [ + { + "name": "Module", + "className": "ModuleScript", + "filePaths": ["source.luau"] + }, + { + "name": "SiblingModule", + "className": "ModuleScript" + } + ] + } + ] + } + )"); + + auto [source, marker] = sourceWithMarker(R"( + | + )"); + + auto uri = newDocument("source.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + auto imports = filterAutoImports(result, "SiblingModule"); + + REQUIRE_EQ(imports.size(), 1); + CHECK_EQ(imports[0].label, "SiblingModule"); + REQUIRE_EQ(imports[0].additionalTextEdits.size(), 1); + CHECK_EQ(imports[0].additionalTextEdits[0].newText, "local SiblingModule = require(script.Parent.SiblingModule)\n"); +} + +TEST_CASE_FIXTURE(Fixture, "auto_imports_will_force_absolute_import_depending_on_setting") +{ + client->globalConfig.completion.imports.enabled = true; + client->globalConfig.completion.imports.requireStyle = ImportRequireStyle::AlwaysAbsolute; + loadSourcemap(R"( + { + "name": "Game", + "className": "DataModel", + "children": [ + { + "name": "ReplicatedStorage", + "className": "ReplicatedStorage", + "children": [ + { + "name": "Module", + "className": "ModuleScript", + "filePaths": ["source.luau"] + }, + { + "name": "SiblingModule", + "className": "ModuleScript" + } + ] + } + ] + } + )"); + + auto [source, marker] = sourceWithMarker(R"( + | + )"); + + auto uri = newDocument("source.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + auto imports = filterAutoImports(result, "SiblingModule"); + + REQUIRE_EQ(imports.size(), 1); + CHECK_EQ(imports[0].label, "SiblingModule"); + REQUIRE_EQ(imports[0].additionalTextEdits.size(), 2); + CHECK_EQ(imports[0].additionalTextEdits[0].newText, "local ReplicatedStorage = game:GetService(\"ReplicatedStorage\")\n"); + CHECK_EQ(imports[0].additionalTextEdits[1].newText, "local SiblingModule = require(ReplicatedStorage.SiblingModule)\n"); +} + +TEST_CASE_FIXTURE(Fixture, "auto_imports_will_prefer_absolute_import_over_relative_import_for_further_away_modules") +{ + client->globalConfig.completion.imports.enabled = true; + loadSourcemap(R"( + { + "name": "Game", + "className": "DataModel", + "children": [ + { + "name": "ReplicatedStorage", + "className": "ReplicatedStorage", + "children": [ + { + "name": "Module", + "className": "ModuleScript", + "filePaths": ["source.luau"] + }, + { + "name": "Folder", + "className": "Folder", + "children": [ + { "name": "OtherModule", "className": "ModuleScript" } + ] + } + ] + } + ] + } + )"); + + auto [source, marker] = sourceWithMarker(R"( + | + )"); + + auto uri = newDocument("source.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + auto imports = filterAutoImports(result, "OtherModule"); + + REQUIRE_EQ(imports.size(), 1); + CHECK_EQ(imports[0].label, "OtherModule"); + REQUIRE_EQ(imports[0].additionalTextEdits.size(), 2); + CHECK_EQ(imports[0].additionalTextEdits[0].newText, "local ReplicatedStorage = game:GetService(\"ReplicatedStorage\")\n"); + CHECK_EQ(imports[0].additionalTextEdits[1].newText, "local OtherModule = require(ReplicatedStorage.Folder.OtherModule)\n"); +} + +TEST_CASE_FIXTURE(Fixture, "auto_imports_will_force_relative_import_depending_on_setting") +{ + client->globalConfig.completion.imports.enabled = true; + client->globalConfig.completion.imports.requireStyle = ImportRequireStyle::AlwaysRelative; + loadSourcemap(R"( + { + "name": "Game", + "className": "DataModel", + "children": [ + { + "name": "ReplicatedStorage", + "className": "ReplicatedStorage", + "children": [ + { + "name": "Module", + "className": "ModuleScript", + "filePaths": ["source.luau"] + }, + { + "name": "Folder", + "className": "Folder", + "children": [ + { "name": "OtherModule", "className": "ModuleScript" } + ] + } + ] + } + ] + } + )"); + + auto [source, marker] = sourceWithMarker(R"( + | + )"); + + auto uri = newDocument("source.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + auto imports = filterAutoImports(result, "OtherModule"); + + REQUIRE_EQ(imports.size(), 1); + CHECK_EQ(imports[0].label, "OtherModule"); + REQUIRE_EQ(imports[0].additionalTextEdits.size(), 1); + CHECK_EQ(imports[0].additionalTextEdits[0].newText, "local OtherModule = require(script.Parent.Folder.OtherModule)\n"); +} + +TEST_SUITE_END(); diff --git a/tests/testdata/standard_definitions.d.luau b/tests/testdata/standard_definitions.d.luau index f7b1a192..62a43c20 100644 --- a/tests/testdata/standard_definitions.d.luau +++ b/tests/testdata/standard_definitions.d.luau @@ -1,4 +1,4 @@ ---#METADATA#{"CREATABLE_INSTANCES":["TextLabel","Part"], "SERVICES": ["ReplicatedStorage"]} +--#METADATA#{"CREATABLE_INSTANCES":["TextLabel","Part"], "SERVICES": ["ReplicatedStorage","ServerScriptService","Workspace"]} declare class Enum function GetEnumItems(self): { any } @@ -82,6 +82,12 @@ end declare class ReplicatedStorage extends Instance end +declare class ServerScriptService extends Instance +end + +declare class Workspace extends Instance +end + declare class ServiceProvider extends Instance function GetService(self, className: string): Instance end From 52e3924637ea58f3444ec6120e2e63ab2b8b7b64 Mon Sep 17 00:00:00 2001 From: JohnnyMorganz Date: Thu, 26 Dec 2024 23:29:55 +0000 Subject: [PATCH 6/6] Include require path in auto import details to help disambiguation --- CHANGELOG.md | 6 ++++ src/platform/roblox/RobloxCompletion.cpp | 9 ++--- tests/AutoImports.test.cpp | 46 ++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 165f35b7..2c5e9b7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## [Unreleased] +### Added + +- Auto-import requires will now show the require path in the details section rather than just "Auto-import". This will + help to disambiguate between multiple modules with the same name but at different + paths. ([#593](https://github.com/JohnnyMorganz/luau-lsp/issues/593)) + ### Changed - The server no longer computes `relatedDocuments` in a `textDocument/diagnostic` request unnecessarily if the client diff --git a/src/platform/roblox/RobloxCompletion.cpp b/src/platform/roblox/RobloxCompletion.cpp index 791098f5..50282cfe 100644 --- a/src/platform/roblox/RobloxCompletion.cpp +++ b/src/platform/roblox/RobloxCompletion.cpp @@ -80,8 +80,8 @@ static lsp::TextEdit createRequireTextEdit(const std::string& name, const std::s return {range, importText}; } -static lsp::CompletionItem createSuggestRequire( - const std::string& name, const std::vector& textEdits, const char* sortText, const std::string& path) +static lsp::CompletionItem createSuggestRequire(const std::string& name, const std::vector& textEdits, const char* sortText, + const std::string& path, const std::string& requirePath) { std::string documentation; for (const auto& edit : textEdits) @@ -90,7 +90,7 @@ static lsp::CompletionItem createSuggestRequire( lsp::CompletionItem item; item.label = name; item.kind = lsp::CompletionItemKind::Module; - item.detail = "Auto-import"; + item.detail = requirePath; item.documentation = {lsp::MarkupKind::Markdown, codeBlock("luau", documentation) + "\n\n" + path}; item.insertText = name; item.sortText = sortText; @@ -433,7 +433,8 @@ void RobloxPlatform::handleSuggestImports(const TextDocument& textDocument, cons textEdits.emplace_back(createRequireTextEdit(node->name, require, lineNumber, prependNewline)); - items.emplace_back(createSuggestRequire(name, textEdits, isRelative ? SortText::AutoImports : SortText::AutoImportsAbsolute, path)); + items.emplace_back( + createSuggestRequire(name, textEdits, isRelative ? SortText::AutoImports : SortText::AutoImportsAbsolute, path, require)); } } } diff --git a/tests/AutoImports.test.cpp b/tests/AutoImports.test.cpp index 591d7fd7..a14e7164 100644 --- a/tests/AutoImports.test.cpp +++ b/tests/AutoImports.test.cpp @@ -614,4 +614,50 @@ TEST_CASE_FIXTURE(Fixture, "auto_imports_will_force_relative_import_depending_on CHECK_EQ(imports[0].additionalTextEdits[0].newText, "local OtherModule = require(script.Parent.Folder.OtherModule)\n"); } +TEST_CASE_FIXTURE(Fixture, "auto_imports_of_modules_show_path_name") +{ + client->globalConfig.completion.imports.enabled = true; + loadSourcemap(R"( + { + "name": "Game", + "className": "DataModel", + "children": [ + { + "name": "ReplicatedStorage", + "className": "ReplicatedStorage", + "children": [ + { + "name": "Folder1", + "className": "Folder", + "children": [{ "name": "Module", "className": "ModuleScript" }] + }, + { + "name": "Folder2", + "className": "Folder", + "children": [{ "name": "Module", "className": "ModuleScript" }] + } + ] + } + ] + } + )"); + + auto [source, marker] = sourceWithMarker(R"( + | + )"); + + auto uri = newDocument("foo.luau", source); + + lsp::CompletionParams params; + params.textDocument = lsp::TextDocumentIdentifier{uri}; + params.position = marker; + + auto result = workspace.completion(params); + auto imports = filterAutoImports(result, "Module"); + + REQUIRE_EQ(imports.size(), 2); + CHECK_EQ(imports[1].detail, "ReplicatedStorage.Folder1.Module"); + CHECK_EQ(imports[0].detail, "ReplicatedStorage.Folder2.Module"); +} + TEST_SUITE_END();