Skip to content

allow setting remappings on lsp server init #13423

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Compiler Features:
* Standard JSON: Add a boolean field `settings.metadata.appendCBOR` that skips CBOR metadata from getting appended at the end of the bytecode.
* Yul Optimizer: Allow replacing the previously hard-coded cleanup sequence by specifying custom steps after a colon delimiter (``:``) in the sequence string.
* Language Server: Add basic document hover support.
* Language Server: Add configuration option to apply custom remappings via ``remappings`` in the LSP settings object.


Bugfixes:
Expand Down
11 changes: 9 additions & 2 deletions libsolidity/lsp/FileRepository.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
// SPDX-License-Identifier: GPL-3.0

#include <libsolidity/interface/ImportRemapper.h>
#include <libsolidity/lsp/FileRepository.h>
#include <libsolidity/lsp/Transport.h>
#include <libsolidity/lsp/Utils.h>
Expand Down Expand Up @@ -43,9 +44,10 @@ using solidity::util::readFileAsString;
using solidity::util::joinHumanReadable;
using solidity::util::Result;

FileRepository::FileRepository(boost::filesystem::path _basePath, std::vector<boost::filesystem::path> _includePaths):
FileRepository::FileRepository(boost::filesystem::path _basePath, std::vector<boost::filesystem::path> _includePaths, vector<frontend::ImportRemapper::Remapping> _remappings):
m_basePath(std::move(_basePath)),
m_includePaths(std::move(_includePaths))
m_includePaths(std::move(_includePaths)),
m_remappings(std::move(_remappings))
{
}

Expand All @@ -54,6 +56,11 @@ void FileRepository::setIncludePaths(std::vector<boost::filesystem::path> _paths
m_includePaths = std::move(_paths);
}

void FileRepository::setRemappings(vector<frontend::ImportRemapper::Remapping> _remappings)
{
m_remappings = std::move(_remappings);
}

string FileRepository::sourceUnitNameToUri(string const& _sourceUnitName) const
{
regex const windowsDriveLetterPath("^[a-zA-Z]:/");
Expand Down
9 changes: 8 additions & 1 deletion libsolidity/lsp/FileRepository.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#pragma once

#include <libsolidity/interface/FileReader.h>
#include <libsolidity/interface/ImportRemapper.h>
#include <libsolutil/Result.h>

#include <string>
Expand All @@ -29,11 +30,14 @@ namespace solidity::lsp
class FileRepository
{
public:
FileRepository(boost::filesystem::path _basePath, std::vector<boost::filesystem::path> _includePaths);
FileRepository(boost::filesystem::path _basePath, std::vector<boost::filesystem::path> _includePaths, std::vector<frontend::ImportRemapper::Remapping> _remappings);

std::vector<boost::filesystem::path> const& includePaths() const noexcept { return m_includePaths; }
void setIncludePaths(std::vector<boost::filesystem::path> _paths);

std::vector<frontend::ImportRemapper::Remapping> const& remappings() const noexcept { return m_remappings; }
void setRemappings(std::vector<frontend::ImportRemapper::Remapping> _remappings);

boost::filesystem::path const& basePath() const { return m_basePath; }

/// Translates a compiler-internal source unit name to an LSP client path.
Expand Down Expand Up @@ -64,6 +68,9 @@ class FileRepository
/// Additional directories used for resolving relative paths in imports.
std::vector<boost::filesystem::path> m_includePaths;

/// Remappings of imports.
std::vector<frontend::ImportRemapper::Remapping> m_remappings;

/// Mapping of source unit names to their URIs as understood by the client.
StringMap m_sourceUnitNamesToUri;

Expand Down
42 changes: 39 additions & 3 deletions libsolidity/lsp/LanguageServer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <libsolidity/ast/AST.h>
#include <libsolidity/ast/ASTUtils.h>
#include <libsolidity/ast/ASTVisitor.h>
#include <libsolidity/interface/ImportRemapper.h>
#include <libsolidity/interface/ReadFile.h>
#include <libsolidity/interface/StandardCompiler.h>
#include <libsolidity/lsp/LanguageServer.h>
Expand Down Expand Up @@ -151,7 +152,7 @@ LanguageServer::LanguageServer(Transport& _transport):
{"textDocument/semanticTokens/full", bind(&LanguageServer::semanticTokensFull, this, _1, _2)},
{"workspace/didChangeConfiguration", bind(&LanguageServer::handleWorkspaceDidChangeConfiguration, this, _2)},
},
m_fileRepository("/" /* basePath */, {} /* no search paths */),
m_fileRepository("/" /* basePath */, {} /* no search paths */, {} /* no remappings */),
m_compilerStack{m_fileRepository.reader()}
{
}
Expand Down Expand Up @@ -212,6 +213,40 @@ void LanguageServer::changeConfiguration(Json::Value const& _settings)
if (typeFailureCount)
m_client.trace("Invalid JSON configuration passed. \"include-paths\" must be an array of strings.");
}

Json::Value jsonRemappings = _settings["remappings"];

if (jsonRemappings)
{
bool typeFailures = false;
if (jsonRemappings.isArray())
{
vector<frontend::ImportRemapper::Remapping> remappings;
for (Json::Value const& jsonPath: jsonRemappings)
{
if (
jsonPath.isObject() &&
jsonPath.get("context", "").isString() &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New type checking helpers were added and recently merged in this pull request, so these should be changed to use isOfType<T> or isOfTypeIfExists<T>, whichever one is more appropriate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd hold off a bit with using these helpers everywhere. I wanted to get them merged not to stall the asm import PR but in the end we may and up removing them (#13579 (comment)). Depends on what we end up with after #13579. I mean, it's fine to use them but for now I'd also not force that.

jsonPath.get("prefix", "").isString() &&
jsonPath.get("target", "").isString()
)
Comment on lines +227 to +232
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we report an error on unexpected keys?

Comment on lines +227 to +232
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a validation that target is not empty. solc won't accept remappings with an empty target on the CLI.

remappings.emplace_back(ImportRemapper::Remapping{
jsonPath.get("context", "").asString(),
jsonPath.get("prefix", "").asString(),
jsonPath.get("target", "").asString(),
});
Comment on lines +233 to +237
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
remappings.emplace_back(ImportRemapper::Remapping{
jsonPath.get("context", "").asString(),
jsonPath.get("prefix", "").asString(),
jsonPath.get("target", "").asString(),
});
remappings.emplace_back(
jsonPath.get("context", "").asString(),
jsonPath.get("prefix", "").asString(),
jsonPath.get("target", "").asString(),
);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, these gets can also be replaced with getOrDefault<string>(jsonPath["prefix"], "").

else
typeFailures = true;
}
m_fileRepository.setRemappings(std::move(remappings));
}
else
typeFailures = true;

if (typeFailures)
m_client.trace("Invalid JSON configuration passed. \"remappings\" should be of form [{\"context\":\"\", \"prefix\":\"foo\", \"target\":\"bar\"}].");

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

}
}

vector<boost::filesystem::path> LanguageServer::allSolidityFilesFromProject() const
Expand All @@ -237,7 +272,7 @@ void LanguageServer::compile()
// For files that are not open, we have to take changes on disk into account,
// so we just remove all non-open files.

FileRepository oldRepository(m_fileRepository.basePath(), m_fileRepository.includePaths());
FileRepository oldRepository(m_fileRepository.basePath(), m_fileRepository.includePaths(), m_fileRepository.remappings());
swap(oldRepository, m_fileRepository);

// Load all solidity files from project.
Expand All @@ -261,6 +296,7 @@ void LanguageServer::compile()
// TODO: optimize! do not recompile if nothing has changed (file(s) not flagged dirty).

m_compilerStack.reset(false);
m_compilerStack.setRemappings(m_fileRepository.remappings());
m_compilerStack.setSources(m_fileRepository.sourceUnits());
m_compilerStack.compile(CompilerStack::State::AnalysisPerformed);
}
Expand Down Expand Up @@ -404,7 +440,7 @@ void LanguageServer::handleInitialize(MessageID _id, Json::Value const& _args)
if (_args["trace"])
setTrace(_args["trace"]);

m_fileRepository = FileRepository(rootPath, {});
m_fileRepository = FileRepository(rootPath, {}, {});
if (_args["initializationOptions"].isObject())
changeConfiguration(_args["initializationOptions"]);

Expand Down
71 changes: 71 additions & 0 deletions test/lsp.py
Original file line number Diff line number Diff line change
Expand Up @@ -1085,6 +1085,13 @@ def open_file_and_wait_for_diagnostics(
)
return self.wait_for_diagnostics(solc_process)

def expect_report(self, published_diagnostics, uri):
for report in published_diagnostics:
if report['uri'] == uri:
return report
self.expect_true(False, f"expected ${uri} in published_diagnostics")
return None

def expect_true(
self,
actual,
Expand Down Expand Up @@ -1534,6 +1541,70 @@ def test_custom_includes(self, solc: JsonRpcProcess) -> None:
self.expect_equal(len(diagnostics), 1, "no diagnostics")
self.expect_diagnostic(diagnostics[0], code=2018, lineNo=5, startEndColumns=(4, 62))

def test_remapping_wrong_paramateres(self, solc: JsonRpcProcess) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def test_remapping_wrong_paramateres(self, solc: JsonRpcProcess) -> None:
def test_remapping_wrong_parameters(self, solc: JsonRpcProcess) -> None:

Also, something like test_remapping_wrong_parameter_types would be more precise.

self.setup_lsp(solc, expose_project_root=False)
solc.send_notification(
'workspace/didChangeConfiguration', {
'settings': {
'remappings': [
{
'context': False,
'prefix': 1,
'target': f"{self.project_root_dir}/other-include-dir/otherlib/"
}
]
}
}
)

response = solc.receive_message()
self.expect_equal(response["method"], "$/logTrace")

if not response["params"]["message"].startswith("Invalid JSON configuration passed."):
raise Exception("Expected JSON error.")

def test_remapping(self, solc: JsonRpcProcess) -> None:
self.setup_lsp(solc, expose_project_root=False)
solc.send_notification(
'workspace/didChangeConfiguration', {
'settings': {
'remappings': [
{
'context': '',
'prefix': '@other/',
'target': f"{self.project_root_dir}/other-include-dir/otherlib/"
}
]
}
}
)

# test file
file_with_remapped_import_uri = 'file:///remapped-include.sol'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be best to set this to something that would match the remapping if used in an import to show that these URIs are not remapped.

solc.send_message('textDocument/didOpen', {
'textDocument': {
'uri': file_with_remapped_import_uri,
'languageId': 'Solidity',
'version': 1,
'text': ''.join([
'// SPDX-License-Identifier: UNLICENSED\n',
'pragma solidity >=0.8.0;\n',
'import "@other/otherlib.sol";\n',
])
}
})
published_diagnostics = self.wait_for_diagnostics(solc)
self.expect_equal(len(published_diagnostics), 2, "expected reports for two files")

report = self.expect_report(published_diagnostics, file_with_remapped_import_uri)
diagnostics = report['diagnostics']
self.expect_equal(len(diagnostics), 0, "no diagnostics")

report = self.expect_report(published_diagnostics, f"{self.project_root_uri}/other-include-dir/otherlib/otherlib.sol")
diagnostics = report['diagnostics']
self.expect_equal(len(diagnostics), 1, "no diagnostics")
Comment on lines +1601 to +1605
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that one of these "no diagnostics" messages does not match the number :)

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 project files while having custom include directories configured.
Expand Down