[READY] Migrate Rust completer to Rust Language Server #1224
Conversation
There was a problem hiding this comment.
Does it also solve ycm-core/YouCompleteMe#2935 and ycm-core/YouCompleteMe#3129 ?
Reviewed 43 of 43 files at r1.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @micbou)
build.py, line 126 at r1 (raw file):
return except OSError: try_number += 1
Why can we expect OSError to be raised n times but not n+1?
build.py, line 734 at r1 (raw file):
rustup_url = 'http://sh.rustup.rs' DownloadFileTo( rustup_url, rustup_init )
Is there a reason we are not downloading rustup only on CI? We were telling users to have rustup available before and we had no complaints about that.
ycmd/completers/rust/rust_completer.py, line 104 at r1 (raw file):
# Without LSP workspaces support, RLS relies on the rootUri to detect a # project. # TODO: add support for LSP workspaces to allow users to change project
How much work would support for workspaces be?
ycmd/tests/clangd/init.py, line 103 at r1 (raw file):
@functools.wraps( test ) def Wrapper( *args, **kwargs ): with IgnoreExtraConfOutsideTestsFolder():
Why was this changed?
ycmd/tests/rust/init.py, line 57 at r1 (raw file):
def StartRustCompleterServerInDirectory( app, directory ):
Don't we have a similar function for the jdt completer? Is there anything stopping us from having one function in test_utils.py?
There was a problem hiding this comment.
It fixes ycm-core/YouCompleteMe#3129 but not ycm-core/YouCompleteMe#2935.
Reviewed 6 of 43 files at r1.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)
build.py, line 126 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Why can we expect
OSErrorto be raised n times but not n+1?
This functions only exists to make rmtree more reliable on Windows. Removing a directory sometimes fails on this platform so we try several times.
build.py, line 734 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Is there a reason we are not downloading
rustuponly on CI? We were telling users to haverustupavailable before and we had no complaints about that.
This makes the instructions even simpler: just pass the --rust-completer option to the script.
ycmd/completers/rust/rust_completer.py, line 104 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
How much work would support for workspaces be?
I didn't look it in detail but it shouldn't be too difficult if we go with the strategy that each time a new .Cargo.toml is found, we have a new workspace.
ycmd/tests/clangd/init.py, line 103 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Why was this changed?
Extra confs are already ignored in IsolatedApp. I'll send a separate PR for that change.
ycmd/tests/rust/init.py, line 57 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Don't we have a similar function for the jdt completer? Is there anything stopping us from having one function in
test_utils.py?
The filepath differs. We could change the function to accept a filepath instead of a directory.
micbou
force-pushed
the
rust-language-server
branch
from
277577f
to
b677bb8
Compare
Codecov Report
@@ Coverage Diff @@
## master #1224 +/- ##
==========================================
+ Coverage 97.38% 97.38% +<.01%
==========================================
Files 96 96
Lines 7233 7119 -114
==========================================
- Hits 7044 6933 -111
+ Misses 189 186 -3 |
Codecov Report
@@ Coverage Diff @@
## master #1224 +/- ##
==========================================
+ Coverage 97.19% 97.42% +0.22%
==========================================
Files 95 95
Lines 7497 7389 -108
Branches 153 153
==========================================
- Hits 7287 7199 -88
+ Misses 169 149 -20
Partials 41 41 |
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)
ycmd/tests/clangd/init.py, line 103 at r1 (raw file):
Previously, micbou wrote…
Extra confs are already ignored in
IsolatedApp. I'll send a separate PR for that change.
See PR #1225.
micbou
force-pushed
the
rust-language-server
branch
from
b677bb8
to
2b58f63
Compare
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @micbou)
build.py, line 734 at r1 (raw file):
Previously, micbou wrote…
This makes the instructions even simpler: just pass the
--rust-completeroption to the script.
The counter argument is a larger ycmd directory and the bump to python 2.7.9 as the minimum required version. On the other hand I wouldn't expect any rust developer to not have rustup on his pc.
ycmd/completers/rust/rust_completer.py, line 104 at r1 (raw file):
Previously, micbou wrote…
I didn't look it in detail but it shouldn't be too difficult if we go with the strategy that each time a new
.Cargo.tomlis found, we have a new workspace.
That strategy seems reasonable to me. I was wondering about the generic LSP changes that would be needed to support this, but I could just look at the LSP specifications instead.
ycmd/tests/rust/init.py, line 57 at r1 (raw file):
Previously, micbou wrote…
The filepath differs. We could change the function to accept a filepath instead of a directory.
I think that would be nice, but won't block this PR because of it.
There was a problem hiding this comment.
Reviewed 22 of 43 files at r1, 2 of 2 files at r2.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)
build.py, line 734 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
The counter argument is a larger ycmd directory and the bump to python 2.7.9 as the minimum required version. On the other hand I wouldn't expect any rust developer to not have
rustupon his pc.
Downloading and installing programmatically rustup has no effect on the size of the ycmd directory (its folder is removed after the installation). What takes space is the toolchain. If you are suggesting to not put the toolchain in ycmd directory then we have the problem of polluting the user system by installing the toolchain and a bunch of components (rls, rust-analysis, and rust-src) in the ~/.cargo folder.
Considering Python 2 end-of-life, I think the version increase is acceptable.
ycmd/tests/rust/init.py, line 57 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
I think that would be nice, but won't block this PR because of it.
I'll look into it.
There was a problem hiding this comment.
Thanks for the PR.
Reviewable status: 1 of 2 LGTMs obtained
build.py, line 734 at r1 (raw file):
We definitely shouldn't pollute the user's home directory, so the components should be in the ycmd directory.
Considering Python 2 end-of-life, I think the version increase is acceptable.
I would agree with you, except that Ben is against dropping python2 when it reaches its EOL.
micbou
changed the title
micbou
force-pushed
the
rust-language-server
branch
12 times, most recently
from
8fe5710
to
18c04df
Compare
micbou
changed the title
There was a problem hiding this comment.
Improved tests robustness by waiting for RLS to finish the building and indexing of the project before running the test. PR is ready again.
Reviewed 13 of 43 files at r1, 3 of 3 files at r3, 8 of 8 files at r6, 2 of 2 files at r7, 4 of 4 files at r8, 2 of 2 files at r9.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale)
build.py, line 734 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
We definitely shouldn't pollute the user's home directory, so the components should be in the ycmd directory.
Considering Python 2 end-of-life, I think the version increase is acceptable.
I would agree with you, except that Ben is against dropping python2 when it reaches its EOL.
We now require rustup for Python versions older than 2.7.9. I think that's a good compromise.
There was a problem hiding this comment.
Great. Those few TODOs can be iterated over.
Reviewed 2 of 3 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 6 of 8 files at r6, 2 of 2 files at r7, 4 of 4 files at r8, 2 of 2 files at r9.
Reviewable status: 1 of 2 LGTMs obtained
build.py, line 734 at r1 (raw file):
Previously, micbou wrote…
We now require rustup for Python versions older than 2.7.9. I think that's a good compromise.
Sounds good to me.
micbou
force-pushed
the
rust-language-server
branch
3 times, most recently
from
e0af820
to
72c8da2
Compare
There was a problem hiding this comment.
Reviewed 27 of 43 files at r1, 1 of 2 files at r2, 3 of 8 files at r6, 2 of 2 files at r7, 1 of 4 files at r8, 2 of 2 files at r9, 4 of 4 files at r10, 5 of 6 files at r11, 1 of 1 files at r12.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @micbou)
.travis.yml, line 63 at r12 (raw file):
- $HOME/.cache/pip # Python packages from pip - $HOME/.npm # Node packages from npm - $HOME/.cargo # Cargo package deps
does this actually remove from the cache ? I seem to recall we had to use some API to empty the cache previously.
build.py, line 97 at r12 (raw file):
TSSERVER_VERSION = '3.3.3333' RUST_TOOLCHAIN = 'nightly-2019-03-23'
why this specific version? is there not a stable version we could use ?
ycmd/default_settings.json, line 30 at r12 (raw file):
"gocode_binary_path": "", "godef_binary_path": "", "rust_src_path": "",
Is this a compatibility issue? Should we be worried?
ycmd/completers/language_server/language_server_completer.py, line 1772 at r12 (raw file):
fixit = WorkspaceEditToFixIt( request_data, response[ 'result' ] ) if not fixit: raise RuntimeError( 'Cannot rename under cursor.' )
This error could be worded better
ycmd/completers/language_server/simple_language_server_completer.py, line 92 at r12 (raw file):
def GetServerEnvironment( self ):
Put this method at the top where the other methods you're supposed to override are.
ycmd/completers/rust/rust_completer.py, line 41 at r12 (raw file):
RUSTC_EXECUTABLE = utils.FindExecutable( os.path.join( RLS_BIN_DIR, 'rustc' ) ) RLS_EXECUTABLE = utils.FindExecutable( os.path.join( RLS_BIN_DIR, 'rls' ) ) RLS_VERSION = re.compile( r'^rls (?P<version>.*)$' )
better: RLS_VERSION_REGEX as it isn't the actual version.
ycmd/completers/rust/rust_completer.py, line 165 at r12 (raw file):
def HandleNotificationInPollThread( self, notification ): # TODO: the building status is currently displayed in the debug info. We
Any reason not to do what java completer does and return a DisplayMessageResponse (outside of the poll thread) as well (by handling ConvertNotificationToMessage)
ycmd/completers/rust/rust_completer.py, line 173 at r12 (raw file):
message = params[ 'title' ].lower() if 'done' in params: self._server_progress[ progress_id ] = message + ' done'
You should probably protect access to _server_progress with a lock, or use an event (like java does) to say whether or not the server is ready. This prevents possible races.
ycmd/completers/rust/rust_completer.py, line 174 at r12 (raw file):
if 'done' in params: self._server_progress[ progress_id ] = message + ' done' return
I think this callback you're supposed to call the base implementation no matter what. At least, that's what the java completer does.
There was a problem hiding this comment.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @micbou)
a discussion (no related file):
Just ran the installer and it said:
To get started you need Cargo's bin directory
(/var/folders/s3/v5q17n8532x05nn495s125yh0000gn/T/rustinstallEY3kgL/bin) in
your PATH environment variable. Next time you log in this will be done
automatically.
I'm not sure I want it to do that.
micbou
changed the title
|
☔ The latest upstream changes (presumably #1241) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Reviewed 4 of 4 files at r10, 5 of 6 files at r11, 1 of 1 files at r12.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @micbou and @puremourning)
.travis.yml, line 63 at r12 (raw file):
Previously, puremourning (Ben Jackson) wrote…
does this actually remove from the cache ? I seem to recall we had to use some API to empty the cache previously.
Doesn't matter any more, since we moved to Azure.
build.py, line 97 at r12 (raw file):
Previously, puremourning (Ben Jackson) wrote…
why this specific version? is there not a stable version we could use ?
RLS needs a nightly toolchain, if I'm not mistaken.
micbou
force-pushed
the
rust-language-server
branch
3 times, most recently
from
bbe20c7
to
0332663
Compare
Support following features: - server management through the RestartServer command; - semantic auto-completion; - jump to definition with the GoTo, GoToDefinition, and GoToDeclaration commands; - find implementations with the GoToImplementation command; - find references with the GoToReferences command; - rename symbols with the RefactorRename command; - return symbol documentation with the GetDoc command; - return symbol type with the GetType command; - format code with the Format command.
micbou
force-pushed
the
rust-language-server
branch
from
0332663
to
3145b69
Compare
micbou
changed the title
There was a problem hiding this comment.
It's ready again.
Reviewed 4 of 4 files at r10, 6 of 6 files at r11, 1 of 1 files at r12, 12 of 17 files at r13, 3 of 3 files at r14.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)
a discussion (no related file):
Previously, puremourning (Ben Jackson) wrote…
Just ran the installer and it said:
To get started you need Cargo's bin directory
(/var/folders/s3/v5q17n8532x05nn495s125yh0000gn/T/rustinstallEY3kgL/bin) in
your PATH environment variable. Next time you log in this will be done
automatically.I'm not sure I want it to do that.
Fixed by adding the --no-modify-path option to the rustup installer.
build.py, line 97 at r12 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
RLS needs a nightly toolchain, if I'm not mistaken.
This was the latest version at the time. Since RLS is still going under a lot of changes, I think we are better off pinning the version to the latest nightly.
ycmd/default_settings.json, line 30 at r12 (raw file):
Previously, puremourning (Ben Jackson) wrote…
Is this a compatibility issue? Should we be worried?
No because the Rust sources are installed along RLS so there is no need to specify the path manually.
ycmd/completers/language_server/language_server_completer.py, line 1772 at r12 (raw file):
Previously, puremourning (Ben Jackson) wrote…
This error could be worded better
What do you suggest? I am trying to be consistent with our "Cannot jump to location." error. Would the wording "Cannot rename the symbol under cursor." be better?
ycmd/completers/language_server/simple_language_server_completer.py, line 92 at r12 (raw file):
Previously, puremourning (Ben Jackson) wrote…
Put this method at the top where the other methods you're supposed to override are.
Done.
ycmd/completers/rust/rust_completer.py, line 41 at r12 (raw file):
Previously, puremourning (Ben Jackson) wrote…
better:
RLS_VERSION_REGEXas it isn't the actual version.
Done.
ycmd/completers/rust/rust_completer.py, line 165 at r12 (raw file):
Previously, puremourning (Ben Jackson) wrote…
Any reason not to do what java completer does and return a DisplayMessageResponse (outside of the poll thread) as well (by handling ConvertNotificationToMessage)
I don't think we want to display a message to the user each time there are changes in the current buffer.
ycmd/completers/rust/rust_completer.py, line 173 at r12 (raw file):
Previously, puremourning (Ben Jackson) wrote…
You should probably protect access to
_server_progresswith a lock, or use an event (like java does) to say whether or not the server is ready. This prevents possible races.
Done using _server_info_mutex lock.
ycmd/completers/rust/rust_completer.py, line 174 at r12 (raw file):
Previously, puremourning (Ben Jackson) wrote…
I think this callback you're supposed to call the base implementation no matter what. At least, that's what the java completer does.
Fixed.
There was a problem hiding this comment.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)
ycmd/completers/language_server/language_server_completer.py, line 1772 at r12 (raw file):
Previously, micbou wrote…
What do you suggest? I am trying to be consistent with our "Cannot jump to location." error. Would the wording "Cannot rename the symbol under cursor." be better?
That definitely sounds much better than the current error.
There was a problem hiding this comment.
- LSP full functionality
- Tests are passing
- Code looks good
- strong community support for the change
What's not to like ?
Reviewed 10 of 17 files at r13, 3 of 3 files at r14.
Reviewable status:complete! 2 of 2 LGTMs obtained (waiting on @bstaletic and @micbou)
ycmd/completers/language_server/language_server_completer.py, line 1772 at r12 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
That definitely sounds much better than the current error.
yep "Can't rename symbol" or "Symbol under cursor cannot be renamed" or, as you suggested something like "Cannot rename the requested symbol".
Basically the current sentence is missing a noun.
ycmd/completers/rust/rust_completer.py, line 165 at r12 (raw file):
Previously, micbou wrote…
I don't think we want to display a message to the user each time there are changes in the current buffer.
OK
There was a problem hiding this comment.
Reviewed 3 of 3 files at r15.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic and @micbou)
There was a problem hiding this comment.
Reviewable status:
ycmd/completers/language_server/language_server_completer.py, line 1772 at r12 (raw file):
Previously, puremourning (Ben Jackson) wrote…
yep "Can't rename symbol" or "Symbol under cursor cannot be renamed" or, as you suggested something like "Cannot rename the requested symbol".
Basically the current sentence is missing a noun.
we can fix this later if we need to
|
Thanks for sending a PR! |
ycm-core/ycmd#1275 Remap python GoTo* commands ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn ycm-core/ycmd#1239 Error ID in diagnostic messages ycm-core/ycmd#1267 Clangd extra conf support ycm-core/ycmd#1266 JDT update to 0.40.0 ycm-core/ycmd#1245 Generic (pluggable) LSP completer support ycm-core/ycmd#1263 Support JDT extensions ycm-core/ycmd#1261 Typescript update ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration ycm-core/ycmd#1262 Multi-user installation ycm-core/ycmd#1243 Migrate to GOPLS ycm-core/ycmd#1249 Support MSVC 16 (VS2019) ycm-core/ycmd#1224 Migrate to RLS
ycm-core/ycmd#1275 Remap python GoTo* commands ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn ycm-core/ycmd#1239 Error ID in diagnostic messages ycm-core/ycmd#1267 Clangd extra conf support ycm-core/ycmd#1266 JDT update to 0.40.0 ycm-core/ycmd#1245 Generic (pluggable) LSP completer support ycm-core/ycmd#1263 Support JDT extensions ycm-core/ycmd#1261 Typescript update ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration ycm-core/ycmd#1262 Multi-user installation ycm-core/ycmd#1243 Migrate to GOPLS ycm-core/ycmd#1249 Support MSVC 16 (VS2019) ycm-core/ycmd#1224 Migrate to RLS
ycm-core/ycmd#1275 Remap python GoTo* commands ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn ycm-core/ycmd#1239 Error ID in diagnostic messages ycm-core/ycmd#1267 Clangd extra conf support ycm-core/ycmd#1266 JDT update to 0.40.0 ycm-core/ycmd#1245 Generic (pluggable) LSP completer support ycm-core/ycmd#1263 Support JDT extensions ycm-core/ycmd#1261 Typescript update ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration ycm-core/ycmd#1262 Multi-user installation ycm-core/ycmd#1243 Migrate to GOPLS ycm-core/ycmd#1249 Support MSVC 16 (VS2019) ycm-core/ycmd#1224 Migrate to RLS
ycm-core/ycmd#1275 Remap python GoTo* commands ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn ycm-core/ycmd#1239 Error ID in diagnostic messages ycm-core/ycmd#1267 Clangd extra conf support ycm-core/ycmd#1266 JDT update to 0.40.0 ycm-core/ycmd#1245 Generic (pluggable) LSP completer support ycm-core/ycmd#1263 Support JDT extensions ycm-core/ycmd#1261 Typescript update ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration ycm-core/ycmd#1262 Multi-user installation ycm-core/ycmd#1243 Migrate to GOPLS ycm-core/ycmd#1249 Support MSVC 16 (VS2019) ycm-core/ycmd#1224 Migrate to RLS
ycm-core/ycmd#1275 Remap python GoTo* commands ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn ycm-core/ycmd#1239 Error ID in diagnostic messages ycm-core/ycmd#1267 Clangd extra conf support ycm-core/ycmd#1266 JDT update to 0.40.0 ycm-core/ycmd#1245 Generic (pluggable) LSP completer support ycm-core/ycmd#1263 Support JDT extensions ycm-core/ycmd#1261 Typescript update ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration ycm-core/ycmd#1262 Multi-user installation ycm-core/ycmd#1243 Migrate to GOPLS ycm-core/ycmd#1249 Support MSVC 16 (VS2019) ycm-core/ycmd#1224 Migrate to RLS
Migrate the Rust completer from racerd to the Rust Language Server. This new version of the completer supports the following features:
RestartServercommand;GoTo,GoToDefinition, andGoToDeclarationcommands;GoToImplementationcommand;GoToReferencescommand;RefactorRenamecommand;GetDoccommand;GetTypecommand;Formatcommand.Demo:
Planned features:
Closes #362.
Closes #849.
Closes #1105.
Closes ycm-core/YouCompleteMe#1912.
Closes ycm-core/YouCompleteMe#2462.
Closes ycm-core/YouCompleteMe#3129.
This change is