allow setting remappings on lsp server init #13423

chrisfarms · 2022-08-22T14:25:23Z

What

enables client's of the lsp server to configure remappings during the initialisation command.

we already allow configuring import-paths through the initialisation, so hopefully this is not controversial.

the format expected to be passed in from the initialise json is:

{ 
  "remappings": [
      {context: "", prefix: "foo", target: "bar"},
      ...
  ]
}

Why

Because without being able to set remappings, the lsp-server throws errors for any project that requires them.

@@ -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:


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.

cameel · 2022-10-21T20:22:16Z

libsolidity/lsp/LanguageServer.cpp

+				if (
+					jsonPath.isObject() &&
+					jsonPath.get("context", "").isString() &&
+					jsonPath.get("prefix", "").isString() &&
+					jsonPath.get("target", "").isString()
+				)


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

cameel · 2022-10-21T20:27:44Z

test/lsp.py

+        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")


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

cameel · 2022-10-21T20:31:18Z

test/lsp.py

+        )
+
+        # test file
+        file_with_remapped_import_uri = 'file:///remapped-include.sol'


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.

cameel · 2022-10-21T20:35:31Z

Does this fix #12603? If so, please add #Fixes #12603 in the PR description.

Also needs rebase on latest develop and approvals from @christianparpart and @leonardoalt.

nikola-matic · 2022-11-04T11:25:30Z

Rebased and resolved conflicts, should be in a more workable state now.

support remappings in the lsp server by allowing reading the required configuration during lsp "initialize". the format expected is: ``` { "remappings": [{context: "", prefix: "foo", target: "bar"}] } ```

nikola-matic · 2022-11-04T11:32:40Z

libsolidity/lsp/LanguageServer.cpp

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


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(),

);

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

nikola-matic · 2022-11-04T11:38:19Z

libsolidity/lsp/LanguageServer.cpp

+			{
+				if (
+					jsonPath.isObject() &&
+					jsonPath.get("context", "").isString() &&


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.

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.

nikola-matic · 2022-11-04T11:39:23Z

libsolidity/lsp/LanguageServer.cpp

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


Suggested change

NunoFilipeSantos · 2022-12-05T14:10:50Z

Right now, we're choosing to close this PR as we won't allocate any time to this particular area.

christianparpart self-assigned this Aug 22, 2022

Marenz added the external contribution ⭐ label Aug 22, 2022

Marenz added the language server label Aug 22, 2022

christianparpart suggested changes Aug 22, 2022

View reviewed changes

libsolidity/lsp/FileRepository.h Outdated Show resolved Hide resolved

libsolidity/lsp/LanguageServer.cpp Outdated Show resolved Hide resolved

libsolidity/lsp/LanguageServer.cpp Outdated Show resolved Hide resolved

libsolidity/lsp/LanguageServer.cpp Outdated Show resolved Hide resolved

christianparpart requested a review from Marenz August 22, 2022 14:46

Marenz reviewed Aug 23, 2022

View reviewed changes

test/lsp.py Outdated Show resolved Hide resolved

chrisfarms force-pushed the lsp-remappings branch 2 times, most recently from 309477c to 193d3b8 Compare August 25, 2022 07:35

Marenz force-pushed the lsp-remappings branch from 691feca to 47cb5d5 Compare August 29, 2022 12:43

Marenz force-pushed the lsp-remappings branch from 47cb5d5 to 4255653 Compare August 29, 2022 12:47

Marenz previously approved these changes Aug 29, 2022

View reviewed changes

Marenz enabled auto-merge August 29, 2022 12:48

leonardoalt reviewed Aug 29, 2022

View reviewed changes

libsolidity/lsp/LanguageServer.cpp Outdated Show resolved Hide resolved

leonardoalt reviewed Aug 29, 2022

View reviewed changes

libsolidity/lsp/LanguageServer.cpp Outdated Show resolved Hide resolved

Marenz disabled auto-merge August 29, 2022 12:56

leonardoalt suggested changes Aug 29, 2022

View reviewed changes

libsolidity/lsp/LanguageServer.cpp Outdated Show resolved Hide resolved

leonardoalt reviewed Aug 29, 2022

View reviewed changes

libsolidity/lsp/LanguageServer.cpp Outdated Show resolved Hide resolved

christianparpart dismissed Marenz’s stale review via 3c1ac60 September 5, 2022 10:24

Marenz previously approved these changes Sep 5, 2022

View reviewed changes

leonardoalt reviewed Sep 5, 2022

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

christianparpart dismissed Marenz’s stale review via 877e5b9 September 5, 2022 13:21

leonardoalt suggested changes Sep 5, 2022

View reviewed changes

libsolidity/lsp/LanguageServer.cpp Show resolved Hide resolved

christianparpart force-pushed the lsp-remappings branch from 877e5b9 to d66c0d9 Compare September 5, 2022 13:27

cameel added the selected for development It's on our short-term development label Sep 8, 2022

cameel removed the selected for development It's on our short-term development label Sep 8, 2022

Marenz force-pushed the lsp-remappings branch 2 times, most recently from d0b55ec to 73c097e Compare September 13, 2022 13:19

christianparpart requested a review from cameel September 19, 2022 15:13

cameel reviewed Oct 21, 2022

View reviewed changes

nikola-matic force-pushed the lsp-remappings branch from 73c097e to fed02e3 Compare November 4, 2022 11:25

lsp: support setting remappings on init

5cfb4b4

support remappings in the lsp server by allowing reading the required configuration during lsp "initialize". the format expected is: ``` { "remappings": [{context: "", prefix: "foo", target: "bar"}] } ```

nikola-matic force-pushed the lsp-remappings branch from fed02e3 to 5cfb4b4 Compare November 4, 2022 11:28

nikola-matic reviewed Nov 4, 2022

View reviewed changes

NunoFilipeSantos unassigned christianparpart Nov 30, 2022

NunoFilipeSantos closed this Dec 5, 2022

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


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

allow setting remappings on lsp server init #13423

allow setting remappings on lsp server init #13423

Uh oh!

Conversation

chrisfarms commented Aug 22, 2022

What

Why

Related

Uh oh!

christianparpart commented Aug 22, 2022

Uh oh!

christianparpart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chrisfarms commented Aug 22, 2022

Uh oh!

chrisfarms commented Aug 22, 2022

Uh oh!

Uh oh!

Marenz commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

christianparpart commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisfarms commented Sep 5, 2022

Uh oh!

Uh oh!

leonardoalt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

christianparpart commented Sep 19, 2022

Uh oh!

cameel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel commented Oct 21, 2022

Uh oh!

nikola-matic commented Nov 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NunoFilipeSantos commented Dec 5, 2022

Uh oh!

Uh oh!

Marenz commented Aug 29, 2022 •

edited

Loading

christianparpart commented Sep 5, 2022 •

edited

Loading