From 0e2ab0500092dfb4c2ca6990b3a31e573be991b4 Mon Sep 17 00:00:00 2001 From: Christian Parpart Date: Mon, 13 Jun 2022 15:50:59 +0200 Subject: [PATCH 1/8] libsolutil: Adding findFilesRecursively() helper to find files recursively. --- libsolutil/CommonIO.h | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/libsolutil/CommonIO.h b/libsolutil/CommonIO.h index e6cd0c161..56f62f0ac 100644 --- a/libsolutil/CommonIO.h +++ b/libsolutil/CommonIO.h @@ -48,6 +48,46 @@ inline std::ostream& operator<<(std::ostream& os, bytes const& _bytes) namespace util { +namespace detail +{ + +template +struct RecursiveFileCollector +{ + Predicate predicate; + std::vector result {}; + + RecursiveFileCollector& operator()(boost::filesystem::path const& _directory) + { + if (!boost::filesystem::is_directory(_directory)) + return *this; + auto iterator = boost::filesystem::directory_iterator(_directory); + auto const iteratorEnd = boost::filesystem::directory_iterator(); + + while (iterator != iteratorEnd) + { + if (boost::filesystem::is_directory(iterator->status())) + (*this)(iterator->path()); + + if (predicate(iterator->path())) + result.push_back(iterator->path()); + + ++iterator; + } + return *this; + } +}; + +template +RecursiveFileCollector(Predicate) -> RecursiveFileCollector; +} + +template +std::vector findFilesRecursively(boost::filesystem::path const& _rootDirectory, Predicate _predicate) +{ + return detail::RecursiveFileCollector{_predicate}(_rootDirectory).result; +} + /// Retrieves and returns the contents of the given file as a std::string. /// If the file doesn't exist, it will throw a FileNotFound exception. /// If the file exists but is not a regular file, it will throw NotAFile exception. From b6ba43234e497ced4e14356207d8c621d3803c04 Mon Sep 17 00:00:00 2001 From: Christian Parpart Date: Mon, 13 Jun 2022 15:56:55 +0200 Subject: [PATCH 2/8] lsp: Always load all solidity files from project for analyzing. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kamil Śliwak --- Changelog.md | 1 + libsolidity/lsp/FileRepository.cpp | 6 ++ libsolidity/lsp/LanguageServer.cpp | 65 ++++++++++++++++- libsolidity/lsp/LanguageServer.h | 22 ++++++ libsolidity/lsp/Transport.cpp | 3 + libsolutil/CommonIO.h | 40 ----------- .../lsp/analyze-full-project/C.sol | 6 ++ .../lsp/analyze-full-project/D.sol | 6 ++ .../lsp/analyze-full-project/E.sol | 6 ++ test/lsp.py | 70 ++++++++++++++++++- 10 files changed, 181 insertions(+), 44 deletions(-) create mode 100644 test/libsolidity/lsp/analyze-full-project/C.sol create mode 100644 test/libsolidity/lsp/analyze-full-project/D.sol create mode 100644 test/libsolidity/lsp/analyze-full-project/E.sol diff --git a/Changelog.md b/Changelog.md index dfa237e2d..f0f8b8485 100644 --- a/Changelog.md +++ b/Changelog.md @@ -9,6 +9,7 @@ Language Features: Compiler Features: * Code Generator: More efficient overflow checks for multiplication. * Yul Optimizer: Simplify the starting offset of zero-length operations to zero. + * Language Server: Analyze all files in a project by default (can be customized by setting ``'file-load-strategy'`` to ``'directly-opened-and-on-import'`` in LSP settings object). Bugfixes: diff --git a/libsolidity/lsp/FileRepository.cpp b/libsolidity/lsp/FileRepository.cpp index 6ba583c3a..58bc0bf8d 100644 --- a/libsolidity/lsp/FileRepository.cpp +++ b/libsolidity/lsp/FileRepository.cpp @@ -17,6 +17,7 @@ // SPDX-License-Identifier: GPL-3.0 #include +#include #include #include @@ -25,11 +26,14 @@ #include #include #include +#include #include #include +#include + using namespace std; using namespace solidity; using namespace solidity::lsp; @@ -84,6 +88,7 @@ string FileRepository::sourceUnitNameToUri(string const& _sourceUnitName) const string FileRepository::uriToSourceUnitName(string const& _path) const { + lspAssert(boost::algorithm::starts_with(_path, "file://"), ErrorCode::InternalError, "URI must start with file://"); return stripFileUriSchemePrefix(_path); } @@ -92,6 +97,7 @@ void FileRepository::setSourceByUri(string const& _uri, string _source) // This is needed for uris outside the base path. It can lead to collisions, // but we need to mostly rewrite this in a future version anyway. auto sourceUnitName = uriToSourceUnitName(_uri); + lspDebug(fmt::format("FileRepository.setSourceByUri({}): {}", _uri, _source)); m_sourceUnitNamesToUri.emplace(sourceUnitName, _uri); m_sourceCodes[sourceUnitName] = std::move(_source); } diff --git a/libsolidity/lsp/LanguageServer.cpp b/libsolidity/lsp/LanguageServer.cpp index 5465406c0..3020900c0 100644 --- a/libsolidity/lsp/LanguageServer.cpp +++ b/libsolidity/lsp/LanguageServer.cpp @@ -32,6 +32,7 @@ #include #include +#include #include #include @@ -42,6 +43,8 @@ #include #include +#include + using namespace std; using namespace std::string_literals; using namespace std::placeholders; @@ -118,7 +121,7 @@ LanguageServer::LanguageServer(Transport& _transport): {"cancelRequest", [](auto, auto) {/*nothing for now as we are synchronous */}}, {"exit", [this](auto, auto) { m_state = (m_state == State::ShutdownRequested ? State::ExitRequested : State::ExitWithoutShutdown); }}, {"initialize", bind(&LanguageServer::handleInitialize, this, _1, _2)}, - {"initialized", [](auto, auto) {}}, + {"initialized", bind(&LanguageServer::handleInitialized, this, _1, _2)}, {"$/setTrace", [this](auto, Json::Value const& args) { setTrace(args["value"]); }}, {"shutdown", [this](auto, auto) { m_state = State::ShutdownRequested; }}, {"textDocument/definition", GotoDefinition(*this) }, @@ -147,6 +150,26 @@ Json::Value LanguageServer::toJson(SourceLocation const& _location) void LanguageServer::changeConfiguration(Json::Value const& _settings) { + // The settings item: "file-load-strategy" (enum) defaults to "project-directory" if not (or not correctly) set. + // It can be overridden during client's handshake or at runtime, as usual. + // + // If this value is set to "project-directory" (default), all .sol files located inside the project directory or reachable through symbolic links will be subject to operations. + // + // Operations include compiler analysis, but also finding all symbolic references or symbolic renaming. + // + // If this value is set to "directly-opened-and-on-import", then only currently directly opened files and + // those files being imported directly or indirectly will be included in operations. + if (_settings["file-load-strategy"]) + { + auto const text = _settings["file-load-strategy"].asString(); + if (text == "project-directory") + m_fileLoadStrategy = FileLoadStrategy::ProjectDirectory; + else if (text == "directly-opened-and-on-import") + m_fileLoadStrategy = FileLoadStrategy::DirectlyOpenedAndOnImported; + else + lspAssert(false, ErrorCode::InvalidParams, "Invalid file load strategy: " + text); + } + m_settingsObject = _settings; Json::Value jsonIncludePaths = _settings["include-paths"]; @@ -173,6 +196,23 @@ void LanguageServer::changeConfiguration(Json::Value const& _settings) } } +vector LanguageServer::allSolidityFilesFromProject() const +{ + namespace fs = boost::filesystem; + + std::vector collectedPaths{}; + + // We explicitly decided against including all files from include paths but leave the possibility + // open for a future PR to enable such a feature to be optionally enabled (default disabled). + + auto directoryIterator = fs::recursive_directory_iterator(m_fileRepository.basePath(), fs::symlink_option::recurse); + for (fs::directory_entry const& dirEntry: directoryIterator) + if (dirEntry.path().extension() == ".sol") + collectedPaths.push_back(dirEntry.path()); + + return collectedPaths; +} + void LanguageServer::compile() { // For files that are not open, we have to take changes on disk into account, @@ -181,6 +221,18 @@ void LanguageServer::compile() FileRepository oldRepository(m_fileRepository.basePath(), m_fileRepository.includePaths()); swap(oldRepository, m_fileRepository); + // Load all solidity files from project. + if (m_fileLoadStrategy == FileLoadStrategy::ProjectDirectory) + for (auto const& projectFile: allSolidityFilesFromProject()) + { + lspDebug(fmt::format("adding project file: {}", projectFile.generic_string())); + m_fileRepository.setSourceByUri( + m_fileRepository.sourceUnitNameToUri(projectFile.generic_string()), + util::readFileAsString(projectFile) + ); + } + + // Overwrite all files as opened by the client, including the ones which might potentially have changes. for (string const& fileName: m_openFiles) m_fileRepository.setSourceByUri( fileName, @@ -269,6 +321,7 @@ bool LanguageServer::run() { string const methodName = (*jsonMessage)["method"].asString(); id = (*jsonMessage)["id"]; + lspDebug(fmt::format("received method call: {}", methodName)); if (auto handler = util::valueOrDefault(m_handlers, methodName)) handler(id, (*jsonMessage)["params"]); @@ -278,6 +331,10 @@ bool LanguageServer::run() else m_client.error({}, ErrorCode::ParseError, "\"method\" has to be a string."); } + catch (Json::Exception const&) + { + m_client.error(id, ErrorCode::InvalidParams, "JSON object access error. Most likely due to a badly formatted JSON request message."s); + } catch (RequestError const& error) { m_client.error(id, error.code(), error.comment() ? *error.comment() : ""s); @@ -347,6 +404,12 @@ void LanguageServer::handleInitialize(MessageID _id, Json::Value const& _args) m_client.reply(_id, move(replyArgs)); } +void LanguageServer::handleInitialized(MessageID, Json::Value const&) +{ + if (m_fileLoadStrategy == FileLoadStrategy::ProjectDirectory) + compileAndUpdateDiagnostics(); +} + void LanguageServer::semanticTokensFull(MessageID _id, Json::Value const& _args) { auto uri = _args["textDocument"]["uri"]; diff --git a/libsolidity/lsp/LanguageServer.h b/libsolidity/lsp/LanguageServer.h index ee4f06957..a05bec497 100644 --- a/libsolidity/lsp/LanguageServer.h +++ b/libsolidity/lsp/LanguageServer.h @@ -36,6 +36,23 @@ namespace solidity::lsp class RenameSymbol; enum class ErrorCode; +/** + * Enum to mandate what files to take into consideration for source code analysis. + */ +enum class FileLoadStrategy +{ + /// Takes only those files into consideration that are explicitly opened and those + /// that have been directly or indirectly imported. + DirectlyOpenedAndOnImported = 0, + + /// Takes all Solidity (.sol) files within the project root into account. + /// Symbolic links will be followed, even if they lead outside of the project directory + /// (`--allowed-paths` is currently ignored by the LSP). + /// + /// This resembles the closest what other LSPs should be doing already. + ProjectDirectory = 1, +}; + /** * Solidity Language Server, managing one LSP client. * This implements a subset of LSP version 3.16 that can be found at: @@ -68,6 +85,7 @@ private: /// Reports an error and returns false if not. void requireServerInitialized(); void handleInitialize(MessageID _id, Json::Value const& _args); + void handleInitialized(MessageID _id, Json::Value const& _args); void handleWorkspaceDidChangeConfiguration(Json::Value const& _args); void setTrace(Json::Value const& _args); void handleTextDocumentDidOpen(Json::Value const& _args); @@ -82,6 +100,9 @@ private: /// Compile everything until after analysis phase. void compile(); + + std::vector allSolidityFilesFromProject() const; + using MessageHandler = std::function; Json::Value toRange(langutil::SourceLocation const& _location); @@ -100,6 +121,7 @@ private: /// Set of source unit names for which we sent diagnostics to the client in the last iteration. std::set m_nonemptyDiagnostics; FileRepository m_fileRepository; + FileLoadStrategy m_fileLoadStrategy = FileLoadStrategy::ProjectDirectory; frontend::CompilerStack m_compilerStack; diff --git a/libsolidity/lsp/Transport.cpp b/libsolidity/lsp/Transport.cpp index aa85fd6b1..90c0b20a7 100644 --- a/libsolidity/lsp/Transport.cpp +++ b/libsolidity/lsp/Transport.cpp @@ -16,6 +16,7 @@ */ // SPDX-License-Identifier: GPL-3.0 #include +#include #include #include @@ -205,11 +206,13 @@ std::string StdioTransport::getline() { std::string line; std::getline(std::cin, line); + lspDebug(fmt::format("Received: {}", line)); return line; } void StdioTransport::writeBytes(std::string_view _data) { + lspDebug(fmt::format("Sending: {}", _data)); auto const bytesWritten = fwrite(_data.data(), 1, _data.size(), stdout); solAssert(bytesWritten == _data.size()); } diff --git a/libsolutil/CommonIO.h b/libsolutil/CommonIO.h index 56f62f0ac..e6cd0c161 100644 --- a/libsolutil/CommonIO.h +++ b/libsolutil/CommonIO.h @@ -48,46 +48,6 @@ inline std::ostream& operator<<(std::ostream& os, bytes const& _bytes) namespace util { -namespace detail -{ - -template -struct RecursiveFileCollector -{ - Predicate predicate; - std::vector result {}; - - RecursiveFileCollector& operator()(boost::filesystem::path const& _directory) - { - if (!boost::filesystem::is_directory(_directory)) - return *this; - auto iterator = boost::filesystem::directory_iterator(_directory); - auto const iteratorEnd = boost::filesystem::directory_iterator(); - - while (iterator != iteratorEnd) - { - if (boost::filesystem::is_directory(iterator->status())) - (*this)(iterator->path()); - - if (predicate(iterator->path())) - result.push_back(iterator->path()); - - ++iterator; - } - return *this; - } -}; - -template -RecursiveFileCollector(Predicate) -> RecursiveFileCollector; -} - -template -std::vector findFilesRecursively(boost::filesystem::path const& _rootDirectory, Predicate _predicate) -{ - return detail::RecursiveFileCollector{_predicate}(_rootDirectory).result; -} - /// Retrieves and returns the contents of the given file as a std::string. /// If the file doesn't exist, it will throw a FileNotFound exception. /// If the file exists but is not a regular file, it will throw NotAFile exception. diff --git a/test/libsolidity/lsp/analyze-full-project/C.sol b/test/libsolidity/lsp/analyze-full-project/C.sol new file mode 100644 index 000000000..d08ba140e --- /dev/null +++ b/test/libsolidity/lsp/analyze-full-project/C.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.0; + +contract C +{ +} diff --git a/test/libsolidity/lsp/analyze-full-project/D.sol b/test/libsolidity/lsp/analyze-full-project/D.sol new file mode 100644 index 000000000..93c8e92fc --- /dev/null +++ b/test/libsolidity/lsp/analyze-full-project/D.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.0; + +contract D +{ +} diff --git a/test/libsolidity/lsp/analyze-full-project/E.sol b/test/libsolidity/lsp/analyze-full-project/E.sol new file mode 100644 index 000000000..e0e5d1988 --- /dev/null +++ b/test/libsolidity/lsp/analyze-full-project/E.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.0; + +contract E +{ +} diff --git a/test/lsp.py b/test/lsp.py index 543a7ca43..98c16d3f5 100755 --- a/test/lsp.py +++ b/test/lsp.py @@ -537,6 +537,18 @@ class TestParser: """ return self.current_line_tuple is None +class FileLoadStrategy(Enum): + Undefined = auto() + ProjectDirectory = auto() + DirectlyOpenedAndOnImport = auto() + + def lsp_name(self): + if self == FileLoadStrategy.ProjectDirectory: + return 'project-directory' + elif self == FileLoadStrategy.DirectlyOpenedAndOnImport: + return 'directly-opened-and-on-import' + return None + class FileTestRunner: """ Runs all tests in a given file. @@ -898,18 +910,27 @@ class SolidityLSPTestSuite: # {{{ return min(max(self.test_counter.failed, self.assertion_counter.failed), 127) - def setup_lsp(self, lsp: JsonRpcProcess, expose_project_root=True): + def setup_lsp( + self, + lsp: JsonRpcProcess, + expose_project_root=True, + file_load_strategy: FileLoadStrategy=FileLoadStrategy.DirectlyOpenedAndOnImport, + project_root_subdir=None + ): """ Prepares the solc LSP server by calling `initialize`, and `initialized` methods. """ + project_root_uri_with_maybe_subdir = self.project_root_uri + if project_root_subdir is not None: + project_root_uri_with_maybe_subdir = self.project_root_uri + '/' + project_root_subdir params = { 'processId': None, - 'rootUri': self.project_root_uri, + 'rootUri': project_root_uri_with_maybe_subdir, # Enable traces to receive the amount of expected diagnostics before # actually receiving them. 'trace': 'messages', - 'initializationOptions': {}, + # 'initializationOptions': {}, 'capabilities': { 'textDocument': { 'publishDiagnostics': {'relatedInformation': True} @@ -923,6 +944,9 @@ class SolidityLSPTestSuite: # {{{ } } } + if file_load_strategy != FileLoadStrategy.Undefined: + params['initializationOptions'] = {} + params['initializationOptions']['file-load-strategy'] = file_load_strategy.lsp_name() if not expose_project_root: params['rootUri'] = None lsp.call_method('initialize', params) @@ -1059,6 +1083,14 @@ class SolidityLSPTestSuite: # {{{ ) return self.wait_for_diagnostics(solc_process) + def expect_true( + self, + actual, + description="Expected True value", + part=ExpectationFailed.Part.Diagnostics + ) -> None: + self.expect_equal(actual, True, description, part) + def expect_equal( self, actual, @@ -1295,6 +1327,38 @@ class SolidityLSPTestSuite: # {{{ # }}} # {{{ actual tests + def test_analyze_all_project_files1(self, solc: JsonRpcProcess) -> None: + """ + Tests the option (default) to analyze all .sol project files even when they have not been actively + opened yet. This is how other LSPs (at least for C++) work too and it makes cross-unit tasks + actually correct (e.g. symbolic rename, find all references, ...). + + In this test, we simply open up a custom project and ensure we're receiving the diagnostics + for all existing files in that project (while having none of these files opened). + """ + SUBDIR = 'analyze-full-project' + self.setup_lsp( + solc, + file_load_strategy=FileLoadStrategy.ProjectDirectory, + project_root_subdir=SUBDIR + ) + published_diagnostics = self.wait_for_diagnostics(solc) + self.expect_equal(len(published_diagnostics), 3, "Diagnostic reports for 3 files") + + # C.sol + report = published_diagnostics[0] + self.expect_equal(report['uri'], self.get_test_file_uri('C', SUBDIR), "Correct file URI") + self.expect_equal(len(report['diagnostics']), 0, "no diagnostics") + + # D.sol + report = published_diagnostics[1] + self.expect_equal(report['uri'], self.get_test_file_uri('D', SUBDIR), "Correct file URI") + self.expect_equal(len(report['diagnostics']), 0, "no diagnostics") + + # E.sol + report = published_diagnostics[2] + self.expect_equal(report['uri'], self.get_test_file_uri('E', SUBDIR), "Correct file URI") + self.expect_equal(len(report['diagnostics']), 0, "no diagnostics") def test_publish_diagnostics_errors_multiline(self, solc: JsonRpcProcess) -> None: self.setup_lsp(solc) From 122fbc6ff70ddd4a7f21616e1807a2506dbe912b Mon Sep 17 00:00:00 2001 From: Christian Parpart Date: Mon, 15 Aug 2022 12:36:47 +0200 Subject: [PATCH 3/8] Adds include-paths-nested test case. --- .../lsp/include-paths-nested/A/B/C/foo.sol | 6 +++++ .../lsp/include-paths-nested/A/B/foo.sol | 6 +++++ .../lsp/include-paths-nested/A/foo.sol | 6 +++++ .../lsp/include-paths-nested/foo.sol | 6 +++++ test/lsp.py | 23 +++++++++++++++++++ 5 files changed, 47 insertions(+) create mode 100644 test/libsolidity/lsp/include-paths-nested/A/B/C/foo.sol create mode 100644 test/libsolidity/lsp/include-paths-nested/A/B/foo.sol create mode 100644 test/libsolidity/lsp/include-paths-nested/A/foo.sol create mode 100644 test/libsolidity/lsp/include-paths-nested/foo.sol diff --git a/test/libsolidity/lsp/include-paths-nested/A/B/C/foo.sol b/test/libsolidity/lsp/include-paths-nested/A/B/C/foo.sol new file mode 100644 index 000000000..d08ba140e --- /dev/null +++ b/test/libsolidity/lsp/include-paths-nested/A/B/C/foo.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.0; + +contract C +{ +} diff --git a/test/libsolidity/lsp/include-paths-nested/A/B/foo.sol b/test/libsolidity/lsp/include-paths-nested/A/B/foo.sol new file mode 100644 index 000000000..0ab2f9c09 --- /dev/null +++ b/test/libsolidity/lsp/include-paths-nested/A/B/foo.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.0; + +contract B +{ +} diff --git a/test/libsolidity/lsp/include-paths-nested/A/foo.sol b/test/libsolidity/lsp/include-paths-nested/A/foo.sol new file mode 100644 index 000000000..1e31dbd99 --- /dev/null +++ b/test/libsolidity/lsp/include-paths-nested/A/foo.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.0; + +contract A +{ +} diff --git a/test/libsolidity/lsp/include-paths-nested/foo.sol b/test/libsolidity/lsp/include-paths-nested/foo.sol new file mode 100644 index 000000000..c589b6ae2 --- /dev/null +++ b/test/libsolidity/lsp/include-paths-nested/foo.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.0; + +contract RootContract +{ +} diff --git a/test/lsp.py b/test/lsp.py index 98c16d3f5..3e98bffc9 100755 --- a/test/lsp.py +++ b/test/lsp.py @@ -1360,6 +1360,29 @@ class SolidityLSPTestSuite: # {{{ self.expect_equal(report['uri'], self.get_test_file_uri('E', SUBDIR), "Correct file URI") self.expect_equal(len(report['diagnostics']), 0, "no diagnostics") + def test_analyze_all_project_files2(self, solc: JsonRpcProcess) -> None: + """ + Same as first test on that matter but with deeper nesting levels. + """ + SUBDIR = 'include-paths-nested' + EXPECTED_FILES = [ + "A/B/C/foo", + "A/B/foo", + "A/foo", + "foo", + ] + EXPECTED_URIS = [self.get_test_file_uri(x, SUBDIR) for x in EXPECTED_FILES] + self.setup_lsp( + solc, + file_load_strategy=FileLoadStrategy.ProjectDirectory, + project_root_subdir=SUBDIR + ) + published_diagnostics = self.wait_for_diagnostics(solc) + self.expect_equal(len(published_diagnostics), len(EXPECTED_FILES), "Test number of files analyzed.") + for report in published_diagnostics: + self.expect_true(report['uri'] in EXPECTED_URIS, "Correct file URI") + self.expect_equal(len(report['diagnostics']), 0, "no diagnostics") + def test_publish_diagnostics_errors_multiline(self, solc: JsonRpcProcess) -> None: self.setup_lsp(solc) TEST_NAME = 'publish_diagnostics_3' From d31e4dcc0ada4d232c52ef29c918592dbf31a1e1 Mon Sep 17 00:00:00 2001 From: Christian Parpart Date: Mon, 15 Aug 2022 14:53:38 +0200 Subject: [PATCH 4/8] lsp: Finishing last missing test wrt complex nested project directory structure and specifying custom includes, while using some (one) of them. --- .../lsp/include-paths-nested-2/A/B/C/foo.sol | 8 ++ .../lsp/include-paths-nested-2/A/B/foo.sol | 6 ++ .../lsp/include-paths-nested-2/A/foo.sol | 6 ++ .../lsp/include-paths-nested-2/foo.sol | 6 ++ .../lsp/other-include-dir/otherlib/second.sol | 7 ++ test/lsp.py | 80 +++++++++++++++++++ 6 files changed, 113 insertions(+) create mode 100644 test/libsolidity/lsp/include-paths-nested-2/A/B/C/foo.sol create mode 100644 test/libsolidity/lsp/include-paths-nested-2/A/B/foo.sol create mode 100644 test/libsolidity/lsp/include-paths-nested-2/A/foo.sol create mode 100644 test/libsolidity/lsp/include-paths-nested-2/foo.sol create mode 100644 test/libsolidity/lsp/other-include-dir/otherlib/second.sol diff --git a/test/libsolidity/lsp/include-paths-nested-2/A/B/C/foo.sol b/test/libsolidity/lsp/include-paths-nested-2/A/B/C/foo.sol new file mode 100644 index 000000000..d53d359b1 --- /dev/null +++ b/test/libsolidity/lsp/include-paths-nested-2/A/B/C/foo.sol @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.0; + +import "otherlib/second.sol"; + +contract C +{ +} diff --git a/test/libsolidity/lsp/include-paths-nested-2/A/B/foo.sol b/test/libsolidity/lsp/include-paths-nested-2/A/B/foo.sol new file mode 100644 index 000000000..0ab2f9c09 --- /dev/null +++ b/test/libsolidity/lsp/include-paths-nested-2/A/B/foo.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.0; + +contract B +{ +} diff --git a/test/libsolidity/lsp/include-paths-nested-2/A/foo.sol b/test/libsolidity/lsp/include-paths-nested-2/A/foo.sol new file mode 100644 index 000000000..1e31dbd99 --- /dev/null +++ b/test/libsolidity/lsp/include-paths-nested-2/A/foo.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.0; + +contract A +{ +} diff --git a/test/libsolidity/lsp/include-paths-nested-2/foo.sol b/test/libsolidity/lsp/include-paths-nested-2/foo.sol new file mode 100644 index 000000000..c589b6ae2 --- /dev/null +++ b/test/libsolidity/lsp/include-paths-nested-2/foo.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.0; + +contract RootContract +{ +} diff --git a/test/libsolidity/lsp/other-include-dir/otherlib/second.sol b/test/libsolidity/lsp/other-include-dir/otherlib/second.sol new file mode 100644 index 000000000..2f32daac2 --- /dev/null +++ b/test/libsolidity/lsp/other-include-dir/otherlib/second.sol @@ -0,0 +1,7 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.0; + +library Second +{ + function f(uint n) public pure returns (uint) { return n + 1; } +} diff --git a/test/lsp.py b/test/lsp.py index 3e98bffc9..e19a591a9 100755 --- a/test/lsp.py +++ b/test/lsp.py @@ -915,6 +915,7 @@ class SolidityLSPTestSuite: # {{{ lsp: JsonRpcProcess, expose_project_root=True, file_load_strategy: FileLoadStrategy=FileLoadStrategy.DirectlyOpenedAndOnImport, + custom_include_paths: list[str] = [], project_root_subdir=None ): """ @@ -944,11 +945,19 @@ class SolidityLSPTestSuite: # {{{ } } } + if file_load_strategy != FileLoadStrategy.Undefined: params['initializationOptions'] = {} params['initializationOptions']['file-load-strategy'] = file_load_strategy.lsp_name() + + if len(custom_include_paths) != 0: + if params['initializationOptions'] is None: + params['initializationOptions'] = {} + params['initializationOptions']['include-paths'] = custom_include_paths + if not expose_project_root: params['rootUri'] = None + lsp.call_method('initialize', params) lsp.send_notification('initialized') @@ -1383,6 +1392,40 @@ class SolidityLSPTestSuite: # {{{ self.expect_true(report['uri'] in EXPECTED_URIS, "Correct file URI") self.expect_equal(len(report['diagnostics']), 0, "no diagnostics") + def test_analyze_all_project_files3(self, solc: JsonRpcProcess) -> None: + """ + Same as first test on that matter but with deeper nesting levels. + """ + SUBDIR = 'include-paths-nested-2' + EXPECTED_FILES = [ + "A/B/C/foo", + "A/B/foo", + "A/foo", + "foo", + ] + IMPLICITLY_LOADED_FILE_COUNT = 1 + EXPECTED_URIS = [self.get_test_file_uri(x, SUBDIR) for x in EXPECTED_FILES] + self.setup_lsp( + solc, + file_load_strategy=FileLoadStrategy.ProjectDirectory, + project_root_subdir=SUBDIR, + custom_include_paths=[f"{self.project_root_dir}/other-include-dir"] + ) + published_diagnostics = self.wait_for_diagnostics(solc) + self.expect_equal(len(published_diagnostics), len(EXPECTED_FILES) + IMPLICITLY_LOADED_FILE_COUNT, "Test number of files analyzed.") + + # All but the last report should be from expected files + for report in published_diagnostics[:-IMPLICITLY_LOADED_FILE_COUNT]: + self.expect_true(report['uri'] in EXPECTED_URIS, "Correct file URI") + self.expect_equal(len(report['diagnostics']), 0, "no diagnostics") + + # Check last report (should be the custom imported lib). + # This file is analyzed because it was imported via "A/B/C/foo.sol". + report = published_diagnostics[len(EXPECTED_URIS)] + self.expect_equal(report['uri'], f"{self.project_root_uri}/other-include-dir/otherlib/second.sol", "Correct file URI") + self.expect_equal(len(report['diagnostics']), 0, "no diagnostics") + + def test_publish_diagnostics_errors_multiline(self, solc: JsonRpcProcess) -> None: self.setup_lsp(solc) TEST_NAME = 'publish_diagnostics_3' @@ -1495,6 +1538,43 @@ class SolidityLSPTestSuite: # {{{ self.expect_equal(len(diagnostics), 1, "no diagnostics") self.expect_diagnostic(diagnostics[0], code=2018, lineNo=5, startEndColumns=(4, 62)) + def test_custom_includes_with_full_project(self, solc: JsonRpcProcess) -> None: + """ + Tests loading all all project files while having custom include directories configured. + In such a scenario, all project files should be analyzed and those being included via search path + but not those include files that are not directly nor indirectly included. + """ + self.setup_lsp( + solc, + expose_project_root=True, + project_root_subdir='' + ) + solc.send_notification( + 'workspace/didChangeConfiguration', { + 'settings': { + 'include-paths': [ + f"{self.project_root_dir}/other-include-dir" + ] + } + } + ) + published_diagnostics = self.open_file_and_wait_for_diagnostics(solc, 'include-paths/using-custom-includes') + + self.expect_equal(len(published_diagnostics), 2, "Diagnostic reports for 2 files") + + # test file + report = published_diagnostics[0] + self.expect_equal(report['uri'], self.get_test_file_uri('using-custom-includes', 'include-paths')) + diagnostics = report['diagnostics'] + self.expect_equal(len(diagnostics), 0, "no diagnostics") + + # imported file + report = published_diagnostics[1] + self.expect_equal(report['uri'], f"{self.project_root_uri}/other-include-dir/otherlib/otherlib.sol") + diagnostics = report['diagnostics'] + self.expect_equal(len(diagnostics), 1, "no diagnostics") + self.expect_diagnostic(diagnostics[0], code=2018, lineNo=5, startEndColumns=(4, 62)) + def test_didChange_in_A_causing_error_in_B(self, solc: JsonRpcProcess) -> None: # Reusing another test but now change some file that generates an error in the other. self.test_textDocument_didOpen_with_relative_import(solc) From d0854cb485ad89b0fb8647b728642666b74f97b6 Mon Sep 17 00:00:00 2001 From: Christian Parpart Date: Mon, 22 Aug 2022 11:09:08 +0200 Subject: [PATCH 5/8] Applying CI-reported fixes. --- test/lsp.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/test/lsp.py b/test/lsp.py index e19a591a9..eb28e2e55 100755 --- a/test/lsp.py +++ b/test/lsp.py @@ -1,6 +1,7 @@ #!/usr/bin/env python3 # pragma pylint: disable=too-many-lines # test line 1 +from __future__ import annotations # See: https://github.com/PyCQA/pylint/issues/3320 import argparse import fnmatch import functools @@ -422,7 +423,7 @@ class TestParser: self.next_line() - def parseDiagnostics(self) -> Diagnostics: + def parseDiagnostics(self) -> TestParser.Diagnostics: """ Parse diagnostic expectations specified in the file. Returns a named tuple instance of "Diagnostics" @@ -454,7 +455,7 @@ class TestParser: return self.Diagnostics(**diagnostics) - def parseRequestAndResponse(self) -> RequestAndResponse: + def parseRequestAndResponse(self) -> TestParser.RequestAndResponse: RESPONSE_START = "// <- " REQUEST_END = "// }" COMMENT_PREFIX = "// " @@ -915,7 +916,7 @@ class SolidityLSPTestSuite: # {{{ lsp: JsonRpcProcess, expose_project_root=True, file_load_strategy: FileLoadStrategy=FileLoadStrategy.DirectlyOpenedAndOnImport, - custom_include_paths: list[str] = [], + custom_include_paths: list[str] = None, project_root_subdir=None ): """ @@ -950,7 +951,7 @@ class SolidityLSPTestSuite: # {{{ params['initializationOptions'] = {} params['initializationOptions']['file-load-strategy'] = file_load_strategy.lsp_name() - if len(custom_include_paths) != 0: + if custom_include_paths is not None and len(custom_include_paths) != 0: if params['initializationOptions'] is None: params['initializationOptions'] = {} params['initializationOptions']['include-paths'] = custom_include_paths @@ -1412,7 +1413,11 @@ class SolidityLSPTestSuite: # {{{ custom_include_paths=[f"{self.project_root_dir}/other-include-dir"] ) published_diagnostics = self.wait_for_diagnostics(solc) - self.expect_equal(len(published_diagnostics), len(EXPECTED_FILES) + IMPLICITLY_LOADED_FILE_COUNT, "Test number of files analyzed.") + self.expect_equal( + len(published_diagnostics), + len(EXPECTED_FILES) + IMPLICITLY_LOADED_FILE_COUNT, + "Test number of files analyzed." + ) # All but the last report should be from expected files for report in published_diagnostics[:-IMPLICITLY_LOADED_FILE_COUNT]: From b22d149e3c68eb353a4141e148f65f9b7d631cc3 Mon Sep 17 00:00:00 2001 From: Christian Parpart Date: Mon, 22 Aug 2022 14:53:15 +0200 Subject: [PATCH 6/8] Adds extra check to only consider regular files (e.g. not directories / device files) for inclusion. --- libsolidity/lsp/LanguageServer.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/libsolidity/lsp/LanguageServer.cpp b/libsolidity/lsp/LanguageServer.cpp index 3020900c0..e0c7dca8c 100644 --- a/libsolidity/lsp/LanguageServer.cpp +++ b/libsolidity/lsp/LanguageServer.cpp @@ -207,8 +207,13 @@ vector LanguageServer::allSolidityFilesFromProject() co auto directoryIterator = fs::recursive_directory_iterator(m_fileRepository.basePath(), fs::symlink_option::recurse); for (fs::directory_entry const& dirEntry: directoryIterator) - if (dirEntry.path().extension() == ".sol") - collectedPaths.push_back(dirEntry.path()); + { + if ( + dirEntry.status().type() == fs::file_type::regular_file && + dirEntry.path().extension() == ".sol" + ) + collectedPaths.push_back(dirEntry.path()); + } return collectedPaths; } From 3fc7debbef5f1fbff4f1668cd272e458ccf2ed02 Mon Sep 17 00:00:00 2001 From: Christian Parpart Date: Wed, 24 Aug 2022 10:31:16 +0200 Subject: [PATCH 7/8] lsp: Code-review fixups. --- libsolidity/lsp/LanguageServer.cpp | 27 +++++++++++----- test/lsp.py | 49 ++++++++++++------------------ 2 files changed, 39 insertions(+), 37 deletions(-) diff --git a/libsolidity/lsp/LanguageServer.cpp b/libsolidity/lsp/LanguageServer.cpp index e0c7dca8c..6ea60e0cf 100644 --- a/libsolidity/lsp/LanguageServer.cpp +++ b/libsolidity/lsp/LanguageServer.cpp @@ -53,9 +53,24 @@ using namespace solidity::lsp; using namespace solidity::langutil; using namespace solidity::frontend; +namespace fs = boost::filesystem; + namespace { +bool resolvesToRegularFile(boost::filesystem::path _path) +{ + fs::file_status fileStatus = fs::status(_path); + + while (fileStatus.type() == fs::file_type::symlink_file) + { + _path = boost::filesystem::read_symlink(_path); + fileStatus = fs::status(_path); + } + + return fileStatus.type() == fs::file_type::regular_file; +} + int toDiagnosticSeverity(Error::Type _errorType) { // 1=Error, 2=Warning, 3=Info, 4=Hint @@ -198,22 +213,18 @@ void LanguageServer::changeConfiguration(Json::Value const& _settings) vector LanguageServer::allSolidityFilesFromProject() const { - namespace fs = boost::filesystem; - - std::vector collectedPaths{}; + vector collectedPaths{}; // We explicitly decided against including all files from include paths but leave the possibility // open for a future PR to enable such a feature to be optionally enabled (default disabled). auto directoryIterator = fs::recursive_directory_iterator(m_fileRepository.basePath(), fs::symlink_option::recurse); for (fs::directory_entry const& dirEntry: directoryIterator) - { if ( - dirEntry.status().type() == fs::file_type::regular_file && - dirEntry.path().extension() == ".sol" + dirEntry.path().extension() == ".sol" && + (dirEntry.status().type() == fs::file_type::regular_file || resolvesToRegularFile(dirEntry.path())) ) - collectedPaths.push_back(dirEntry.path()); - } + collectedPaths.push_back(dirEntry.path()); return collectedPaths; } diff --git a/test/lsp.py b/test/lsp.py index eb28e2e55..2e097dab0 100755 --- a/test/lsp.py +++ b/test/lsp.py @@ -539,16 +539,9 @@ class TestParser: return self.current_line_tuple is None class FileLoadStrategy(Enum): - Undefined = auto() - ProjectDirectory = auto() - DirectlyOpenedAndOnImport = auto() - - def lsp_name(self): - if self == FileLoadStrategy.ProjectDirectory: - return 'project-directory' - elif self == FileLoadStrategy.DirectlyOpenedAndOnImport: - return 'directly-opened-and-on-import' - return None + Undefined = None + ProjectDirectory = 'project-directory' + DirectlyOpenedAndOnImport = 'directly-opened-and-on-import' class FileTestRunner: """ @@ -932,7 +925,6 @@ class SolidityLSPTestSuite: # {{{ # Enable traces to receive the amount of expected diagnostics before # actually receiving them. 'trace': 'messages', - # 'initializationOptions': {}, 'capabilities': { 'textDocument': { 'publishDiagnostics': {'relatedInformation': True} @@ -949,7 +941,7 @@ class SolidityLSPTestSuite: # {{{ if file_load_strategy != FileLoadStrategy.Undefined: params['initializationOptions'] = {} - params['initializationOptions']['file-load-strategy'] = file_load_strategy.lsp_name() + params['initializationOptions']['file-load-strategy'] = file_load_strategy.value if custom_include_paths is not None and len(custom_include_paths) != 0: if params['initializationOptions'] is None: @@ -1337,7 +1329,7 @@ class SolidityLSPTestSuite: # {{{ # }}} # {{{ actual tests - def test_analyze_all_project_files1(self, solc: JsonRpcProcess) -> None: + def test_analyze_all_project_files_flat(self, solc: JsonRpcProcess) -> None: """ Tests the option (default) to analyze all .sol project files even when they have not been actively opened yet. This is how other LSPs (at least for C++) work too and it makes cross-unit tasks @@ -1370,18 +1362,18 @@ class SolidityLSPTestSuite: # {{{ self.expect_equal(report['uri'], self.get_test_file_uri('E', SUBDIR), "Correct file URI") self.expect_equal(len(report['diagnostics']), 0, "no diagnostics") - def test_analyze_all_project_files2(self, solc: JsonRpcProcess) -> None: + def test_analyze_all_project_files_nested(self, solc: JsonRpcProcess) -> None: """ Same as first test on that matter but with deeper nesting levels. """ SUBDIR = 'include-paths-nested' - EXPECTED_FILES = [ + EXPECTED_FILES = { "A/B/C/foo", "A/B/foo", "A/foo", "foo", - ] - EXPECTED_URIS = [self.get_test_file_uri(x, SUBDIR) for x in EXPECTED_FILES] + } + EXPECTED_URIS = {self.get_test_file_uri(x, SUBDIR) for x in EXPECTED_FILES} self.setup_lsp( solc, file_load_strategy=FileLoadStrategy.ProjectDirectory, @@ -1389,23 +1381,22 @@ class SolidityLSPTestSuite: # {{{ ) published_diagnostics = self.wait_for_diagnostics(solc) self.expect_equal(len(published_diagnostics), len(EXPECTED_FILES), "Test number of files analyzed.") - for report in published_diagnostics: - self.expect_true(report['uri'] in EXPECTED_URIS, "Correct file URI") - self.expect_equal(len(report['diagnostics']), 0, "no diagnostics") + self.expect_equal({report['uri'] for report in published_diagnostics}, EXPECTED_URIS) + self.expect_equal([len(report['diagnostics']) for report in published_diagnostics], [0] * len(EXPECTED_URIS)) - def test_analyze_all_project_files3(self, solc: JsonRpcProcess) -> None: + def test_analyze_all_project_files_nested_with_include_paths(self, solc: JsonRpcProcess) -> None: """ Same as first test on that matter but with deeper nesting levels. """ SUBDIR = 'include-paths-nested-2' - EXPECTED_FILES = [ + EXPECTED_FILES = { "A/B/C/foo", "A/B/foo", "A/foo", "foo", - ] + } IMPLICITLY_LOADED_FILE_COUNT = 1 - EXPECTED_URIS = [self.get_test_file_uri(x, SUBDIR) for x in EXPECTED_FILES] + EXPECTED_URIS = {self.get_test_file_uri(x, SUBDIR) for x in EXPECTED_FILES} self.setup_lsp( solc, file_load_strategy=FileLoadStrategy.ProjectDirectory, @@ -1426,9 +1417,9 @@ class SolidityLSPTestSuite: # {{{ # Check last report (should be the custom imported lib). # This file is analyzed because it was imported via "A/B/C/foo.sol". - report = published_diagnostics[len(EXPECTED_URIS)] - self.expect_equal(report['uri'], f"{self.project_root_uri}/other-include-dir/otherlib/second.sol", "Correct file URI") - self.expect_equal(len(report['diagnostics']), 0, "no diagnostics") + last_report = published_diagnostics[len(EXPECTED_URIS)] + self.expect_equal(last_report['uri'], self.get_test_file_uri('second', 'other-include-dir/otherlib'), "Correct file URI") + self.expect_equal(len(last_report['diagnostics']), 0, "no diagnostics") def test_publish_diagnostics_errors_multiline(self, solc: JsonRpcProcess) -> None: @@ -1545,7 +1536,7 @@ class SolidityLSPTestSuite: # {{{ def test_custom_includes_with_full_project(self, solc: JsonRpcProcess) -> None: """ - Tests loading all all project files while having custom include directories configured. + Tests loading all project files while having custom include directories configured. In such a scenario, all project files should be analyzed and those being included via search path but not those include files that are not directly nor indirectly included. """ @@ -1577,7 +1568,7 @@ class SolidityLSPTestSuite: # {{{ report = published_diagnostics[1] self.expect_equal(report['uri'], f"{self.project_root_uri}/other-include-dir/otherlib/otherlib.sol") diagnostics = report['diagnostics'] - self.expect_equal(len(diagnostics), 1, "no diagnostics") + self.expect_equal(len(diagnostics), 1) self.expect_diagnostic(diagnostics[0], code=2018, lineNo=5, startEndColumns=(4, 62)) def test_didChange_in_A_causing_error_in_B(self, solc: JsonRpcProcess) -> None: From c8074d2c6e5eaa228e6d37be9a682b4c5a393162 Mon Sep 17 00:00:00 2001 From: Christian Parpart Date: Wed, 24 Aug 2022 15:46:08 +0200 Subject: [PATCH 8/8] lsp: Limit resolvesToRegularFile()'s recursion depth to 10. --- libsolidity/lsp/LanguageServer.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libsolidity/lsp/LanguageServer.cpp b/libsolidity/lsp/LanguageServer.cpp index 6ea60e0cf..65eeaf3d5 100644 --- a/libsolidity/lsp/LanguageServer.cpp +++ b/libsolidity/lsp/LanguageServer.cpp @@ -58,14 +58,15 @@ namespace fs = boost::filesystem; namespace { -bool resolvesToRegularFile(boost::filesystem::path _path) +bool resolvesToRegularFile(boost::filesystem::path _path, int maxRecursionDepth = 10) { fs::file_status fileStatus = fs::status(_path); - while (fileStatus.type() == fs::file_type::symlink_file) + while (fileStatus.type() == fs::file_type::symlink_file && maxRecursionDepth > 0) { _path = boost::filesystem::read_symlink(_path); fileStatus = fs::status(_path); + maxRecursionDepth--; } return fileStatus.type() == fs::file_type::regular_file;