Language Server #11350

christianparpart · 2021-05-05T14:38:49Z

This PR contains the Language Server mode for solc. Closes #7763.

Patch for LSP TCP mode saved here.

libsolidity/lsp/LanguageServer.cpp

chriseth

Just a rough review, but we are getting there indeed!

libsolidity/lsp/LanguageServer.cpp

libsolidity/lsp/LanguageServer.h

libsolidity/lsp/LanguageServer.cpp

libsolidity/interface/FileReader.cpp

libsolidity/lsp/LanguageServer.cpp

solc/CMakeLists.txt

libsolidity/lsp/LanguageServer.cpp

libsolidity/interface/FileReader.cpp

libsolidity/interface/StandardCompiler.cpp

solc/CMakeLists.txt

solc/CommandLineInterface.cpp

libsolidity/lsp/Transport.cpp

liblangutil/SourceLocation.h

libsolidity/lsp/LanguageServer.h

libsolidity/lsp/LanguageServer.cpp

solc/CommandLineParser.cpp

scripts/test_solidity_lsp.py

chriseth · 2021-12-14T11:14:20Z

scripts/test_solidity_lsp.py

+            f"  Assertions: {self.assertions_passed} passed, {self.assertions_failed} failed\n"
+        )
+
+        return min(self.assertions_failed, 127)


Please also return nonzero if we have at least one failed test, regardless of the number of failed assertions.

chriseth · 2021-12-14T11:15:21Z

scripts/test_solidity_lsp.py

+        Prepares the solc LSP server by calling `initialize`,
+        and `initialized` methods.
+        """
+        project_root_uri = "file://" + self.project_root_dir


duplication

chriseth · 2021-12-14T11:19:20Z

scripts/test_solidity_lsp.py

+            'rootPath': self.project_root_dir,
+            'rootUri': project_root_uri,
+            'trace': 'off',
+            'workspaceFolders': [


We don't support this.

chriseth · 2021-12-14T11:20:40Z

scripts/test_solidity_lsp.py

+        if expose_project_root == False:
+            params['rootUri'] = None
+            params['rootPath'] = None
+        lsp.call_method('initialize', params)


Shouldn't this check the response?

chriseth · 2021-12-14T11:21:24Z

scripts/test_solidity_lsp.py

+            code = message['error']["code"]
+            text = message['error']['message']
+            raise RuntimeError(f"Error {code} received. {text}")
+        if not 'method' in message.keys():


Suggested change

if not 'method' in message.keys():

if 'method' not in message.keys():

?

scripts/test_solidity_lsp.py

chriseth · 2021-12-14T11:29:39Z

scripts/test_solidity_lsp.py

+
+        self.expect_equal(len(published_diagnostics), 2, "Diagnostic reports for 2 files")
+
+        # primary file:


Is the order specified?

The order is mandated by solc.

scripts/test_solidity_lsp.py

CMakeLists.txt

libsolidity/lsp/LanguageServer.cpp

chriseth · 2021-12-15T12:30:43Z

scripts/test_solidity_lsp.py

@@ -335,15 +335,24 @@ def expect_equal(self, actual, expected, description="Equality") -> None:
        print(prefix + SGR_STATUS_FAIL + 'FAILED' + SGR_RESET)
        raise ExpectationFailed(actual, expected)

-    def expect_diagnostic(self, diagnostic, code: int, lineNo: int, startEndColumns: Tuple[int, int]):
+    def expect_diagnostic(


In a future version we probably want to have factory functions for ranges.

.circleci/config.yml

libsolidity/lsp/FileRepository.h

cameel · 2021-12-16T13:41:38Z

test/libsolidity/lsp/publish_diagnostics_3.sol

@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity >=0.8.0;


Can we change the pragmas in these tests to pragma solidity *?

Does it matter? A change here will require updating all the locations in the expectations...

Not so much that it would stop the PR from being merged.

I just think that we won't be keeping them up to date so they will be useless at best and at worst can get in the way in case you want to extract test cases from an older version. The compiler version is implied by the tag on the revision you're taking the code from anyway.

cameel · 2021-12-16T13:51:11Z

libsolidity/lsp/Transport.cpp

+
+		string name = boost::to_lower_copy(line.substr(0, delimiterPos));
+		string value = line.substr(delimiterPos + 1);
+		headers[boost::trim_copy(name)] = boost::trim_copy(value);


I think it would be a good idea to at least add a comment in the source that the problem exists and we're deliberately ignoring it. No one will find this later in the PR :)

libsolidity/lsp/LanguageServer.cpp

cameel · 2021-12-16T14:19:49Z

libsolidity/lsp/LanguageServer.cpp

+	if (!checkServerInitialized(_id))
+		return;


It's probably not something to change right now but it strikes me that we would have been having an exception you can throw to report an error to the client. We have this big try block in run(). It could have a catch clause that catches the exception and calls error() with the message+code and continues. The calls to checkServerInitialized() would be much more natural - right now it looks as if it was only checking a condition but it actually immediately reports an error which was unexpected to me.

I'm actually not so sure about it. In the compiler this makes total sense since the control-flow is not that important, but here you might want to know if it gets further down where you answer the request or not.

That can be conveyed with naming. For example I'd usually name such a function with validate...() and by the fact that it does not return anything I'd naturally expect it to handle failed validation by throwing.

And you always have the option to use error() manually and not return for more fine-grained control. But for the most common case where you just want to error out and exit I think that exceptions actually make control flow clearer - you can see that the throw will just end the handler however deep you are and you don't have to worry about whether the caller will ignore your return code.

cameel · 2021-12-16T14:25:34Z

scripts/test_solidity_lsp.py

+from typing import Callable, List, Union, Any
+from deepdiff import DeepDiff
+
+# {{{ JsonRpcProcess


I still think it should have been a separate module, though maybe it's too late to change that for the MVP now.

cameel · 2021-12-16T14:29:07Z

scripts/test_solidity_lsp.py

+SGR_RESET = '\033[m'
+SGR_TRACE = '\033[1;36m'
+SGR_NOTICE = '\033[1;35m'
+SGR_TEST_BEGIN = '\033[1;33m'
+SGR_ASSERT_BEGIN = '\033[1;34m'
+SGR_STATUS_OKAY = '\033[1;32m'
+SGR_STATUS_FAIL = '\033[1;31m'


I think you could now replace these with Fore, Back and Style constants from colorama. That was part of the reason to switch to it after all.

The reason to use colorama for me was to actually make this compatible with Windows platform where no VT processing (Windows below version 10 (18H-something) was available. One could use names here, but It's more then foreground, the 1 means "bold", actually :)

This will all be removed when we move to pytest.

The reason to use colorama for me was to actually make this compatible with Windows platform

I mean this was another reason but for me it's also important that using colors is simple - I'd like to start using them more in our Python scripts and having to always repeat such a block of definitions would be too annoying and error-prone.

One could use names here, but It's more then foreground, the 1 means "bold", actually :)

Oh, right. I just noticed that your codes are semantic and not just simple color definitions. I guess it should still be possible to somehow map them to combinations of constants from colorama?

This will all be removed when we move to pytest.

True. This can be refactored while we move to a test framework.

But you mean unittest of course? pytest is a completely different test framework and I personally don't like it that much (too much magic :)).

scripts/test_solidity_lsp.py

cameel · 2021-12-16T14:49:03Z

libsolidity/lsp/Transport.h

+/**
+ * LSP Transport using JSON-RPC over iostreams.
+ */
+class JSONTransport: public Transport


Looks like #11350 (comment) has not been addressed.

I really think that this being named JSONTransport is confusing. Both it and Transport use JSON. The difference is really that the former uses iostreams and is a concrete implementation while the latter is an interface. I think that the names do not reflect that at all.

JSON objects, I agree, and this one is about is a JSON (RPC) transport. So Transport itself is just the interface. So the difference is, that this class is actually implementing this interface with respect to JSON-RPC. :)

Isn't JSON RPC already an assumption for Transport? Is it even meant to work with anything other than JSON RPC?

In any case, I'd include at least RPC in the name.

cameel · 2021-12-16T14:58:09Z

scripts/test_solidity_lsp.py

+        colorama.init()
+        args = create_cli_parser().parse_args()
+        self.solc_path = args.solc_path
+        self.project_root_dir = os.path.realpath(args.project_root_dir) + "/test/libsolidity/lsp"


The thing I mentioned in #11350 (comment) is still relevant here:

Seems a bit misleading to call the lsp dir "project dir". If I saw it, I'd assume it it was the main project dir.

Suggested change

self.project_root_dir = os.path.realpath(args.project_root_dir) + "/test/libsolidity/lsp"

self.lsp_test_dir = os.path.realpath(args.project_root_dir) + "/test/libsolidity/lsp"

this is all about lsp, i'm not sure lsp_test_dir is actually anything better here, because the reason it is named project_root_dir is, because that's what is passed as project root dir to the LSP server.

@christianparpart clarified that "project" here means the client's project that sources belong to, and not Solidity as a project. This was not obvious to me at all because I deal with our scripts a lot and the first thing that comes to mind when I see "project root" is our repo root. I still think it should be renamed because it's too ambiguous. How about this?

Suggested change

self.project_root_dir = os.path.realpath(args.project_root_dir) + "/test/libsolidity/lsp"

self.lsp_project_dir = os.path.realpath(args.solidity_repo_root) + "/test/libsolidity/lsp"

test_workspace_dir?

If workspace is synonymous with project for LSP (I'm not sure about the terminology) then yeah, that's much better.

christianparpart force-pushed the lsp branch from e63d8d0 to 24ae573 Compare May 5, 2021 14:43

chriseth reviewed May 5, 2021

View reviewed changes

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

chriseth reviewed May 5, 2021

View reviewed changes

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

chriseth reviewed May 5, 2021

View reviewed changes

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

chriseth reviewed May 5, 2021

View reviewed changes

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

chriseth reviewed May 5, 2021

View reviewed changes

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

chriseth reviewed May 5, 2021

View reviewed changes

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

chriseth suggested changes May 5, 2021

View reviewed changes

christianparpart force-pushed the lsp branch from 24ae573 to a664dd6 Compare May 5, 2021 15:29

aarlt reviewed May 5, 2021

View reviewed changes

solc/CMakeLists.txt Outdated Show resolved Hide resolved

christianparpart force-pushed the lsp branch 3 times, most recently from 24c65f9 to a2b5118 Compare May 17, 2021 13:46

mjlbach mentioned this pull request Aug 5, 2021

Solidity Support neovim/nvim-lspconfig#1102

Closed

fp-crypto mentioned this pull request Aug 29, 2021

LSP support vyperlang/vyper#2434

Closed

christianparpart force-pushed the lsp branch from 1f55be8 to 5db8185 Compare October 8, 2021 14:57

This comment has been minimized.

Sign in to view

stackenbotten reviewed Oct 8, 2021

View reviewed changes

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

cameel reviewed Oct 8, 2021

View reviewed changes

chriseth reviewed Oct 13, 2021

View reviewed changes

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

chriseth reviewed Oct 13, 2021

View reviewed changes

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

chriseth reviewed Oct 14, 2021

View reviewed changes

liblangutil/SourceLocation.h Outdated Show resolved Hide resolved

chriseth reviewed Oct 14, 2021

View reviewed changes

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

chriseth reviewed Oct 14, 2021

View reviewed changes

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

chriseth force-pushed the lsp branch 3 times, most recently from 0cb3403 to f6e23cc Compare October 14, 2021 16:31

stackenbotten reviewed Oct 19, 2021

View reviewed changes

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

cameel reviewed Oct 19, 2021

View reviewed changes

solc/CommandLineParser.cpp Outdated Show resolved Hide resolved

solc/CommandLineParser.cpp Show resolved Hide resolved

christianparpart force-pushed the lsp branch from 3cd35ed to 05becb1 Compare October 20, 2021 14:23

chriseth reviewed Dec 14, 2021

View reviewed changes

scripts/test_solidity_lsp.py Outdated Show resolved Hide resolved

chriseth reviewed Dec 14, 2021

View reviewed changes

scripts/test_solidity_lsp.py Outdated Show resolved Hide resolved

chriseth reviewed Dec 14, 2021

View reviewed changes

scripts/test_solidity_lsp.py Outdated Show resolved Hide resolved

chriseth reviewed Dec 14, 2021

View reviewed changes

scripts/test_solidity_lsp.py Outdated Show resolved Hide resolved

chriseth reviewed Dec 14, 2021

View reviewed changes

scripts/test_solidity_lsp.py Outdated Show resolved Hide resolved

chriseth reviewed Dec 14, 2021

View reviewed changes

scripts/test_solidity_lsp.py Outdated Show resolved Hide resolved

chriseth reviewed Dec 14, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

chriseth reviewed Dec 14, 2021

View reviewed changes

libsolidity/lsp/LanguageServer.cpp Show resolved Hide resolved

chriseth reviewed Dec 15, 2021

View reviewed changes

chriseth force-pushed the lsp branch from 0f6bf6f to f2637db Compare December 16, 2021 12:59

cameel reviewed Dec 16, 2021

View reviewed changes

cameel mentioned this pull request Dec 16, 2021

[CLI] Report errors for command line options that have no effect #11629

Closed

cameel previously approved these changes Dec 16, 2021

View reviewed changes

Initial implementation of Language Server

927b24d

chriseth dismissed cameel’s stale review via 927b24d December 16, 2021 16:53

chriseth force-pushed the lsp branch from f647ec1 to 927b24d Compare December 16, 2021 16:53

cameel approved these changes Dec 16, 2021

View reviewed changes

chriseth approved these changes Dec 16, 2021

View reviewed changes

chriseth merged commit 3193dfb into develop Dec 16, 2021

chriseth deleted the lsp branch December 16, 2021 17:54

cameel mentioned this pull request Dec 21, 2021

[pylint] Disable the opinionated too-many-xyz warnings #12444

Merged

cameel added the language server label Mar 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Language Server #11350

Language Server #11350

christianparpart commented May 5, 2021 •

edited

Loading

chriseth left a comment

This comment has been minimized.

chriseth Dec 14, 2021

chriseth Dec 14, 2021

chriseth Dec 14, 2021

chriseth Dec 14, 2021

chriseth Dec 14, 2021

chriseth Dec 14, 2021

christianparpart Dec 15, 2021

chriseth Dec 15, 2021

cameel Dec 16, 2021

chriseth Dec 16, 2021

cameel Dec 16, 2021 •

edited

Loading

cameel Dec 16, 2021

cameel Dec 16, 2021

chriseth Dec 16, 2021

cameel Dec 16, 2021

cameel Dec 16, 2021

cameel Dec 16, 2021

christianparpart Dec 16, 2021

chriseth Dec 16, 2021

cameel Dec 16, 2021

cameel Dec 16, 2021

christianparpart Dec 16, 2021

cameel Dec 16, 2021

cameel Dec 16, 2021

christianparpart Dec 16, 2021

cameel Dec 16, 2021 •

edited

Loading

chriseth Dec 16, 2021

cameel Dec 16, 2021

	if not 'method' in message.keys():
	if 'method' not in message.keys():


		self.expect_equal(len(published_diagnostics), 2, "Diagnostic reports for 2 files")

		# primary file:

		@@ -0,0 +1,10 @@
		// SPDX-License-Identifier: UNLICENSED
		pragma solidity >=0.8.0;

	self.project_root_dir = os.path.realpath(args.project_root_dir) + "/test/libsolidity/lsp"
	self.lsp_test_dir = os.path.realpath(args.project_root_dir) + "/test/libsolidity/lsp"

	self.project_root_dir = os.path.realpath(args.project_root_dir) + "/test/libsolidity/lsp"
	self.lsp_project_dir = os.path.realpath(args.solidity_repo_root) + "/test/libsolidity/lsp"

Language Server #11350

Language Server #11350

Conversation

christianparpart commented May 5, 2021 • edited Loading

chriseth left a comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianparpart commented May 5, 2021 •

edited

Loading

cameel Dec 16, 2021 •

edited

Loading

cameel Dec 16, 2021 •

edited

Loading