[READY] Add support for the Rust programming language

[jwilm]

racerd and the ycmd completer wrapper for racerd are finally ready for some peer review and testing. There are some blocking issues that need to get resolved before we can launch; these are enumerated later.

Demonstration

This gif shows semantic completion support (module paths and object members) and GoTo jumping (starting with BinaryHeap and further into the standard library). GoTo, GoToDefinition, and GoToDeclaration are all equivalent at this time.

Building ycmd with rust support

1. Install multirust if you don't already have it.

2. Add this branch to your local ycmd installation. For example,

   cd $YCMD_PATH
   git remote add jwilm https://github.com/jwilm/ycmd.git
   git fetch jwilm
   git checkout rust-support
   

3. Run the ycmd build.py script with an additional argument. If you only care about rust support, the build command might look like this.

   ./build.py --racerd-completer
   

Outstanding Issues

• Racerd ::racer::core::Session usage leaks memory. Need to patch racer + racerd to support a mutable, long-lived file cache. Until then, the FileCache's Arena grows indefinitely (can be witnessed by watching racerd's memory usage).
• Add tests for the rust completer in ycmd
• racerd HMAC support
• RustCompleter utilizes racerd hmac
• Support specifying rust source path as a ycm option (eg. via .vimrc for vim)
• Remove multirust dependency from build.py
• Tests pass on appveyor
• Add user option for overriding racerd path
• Add logging for racerd output
• Add synchronization for self._racerd_phandle (and friends) in RustCompleter
• Update racerd to handle PoisonError (Handle PoisonError when locking mutex jwilm/racerd#9)
• Eliminate OpenSSL dependencies (windows support)
• Rebase and include latest racerd

[vheon]

I made some random comments ;)

---

Reviewed 4 of 6 files at r1.
Review status: 4 of 6 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

build.py, line 172 [r1] (raw file):
why the blank line?

---

ycmd/completers/rust/rust_completer.py, line 55 [r1] (raw file):
Yes, personally I would prefer a new function in ycm_extra_conf.py where you will get all the options you want. Don't rust have a default path for this? Am I missing somethig?

---

ycmd/completers/rust/rust_completer.py, line 67 [r1] (raw file):
spaces inside the parenthesis

---

ycmd/completers/rust/rust_completer.py, line 68 [r1] (raw file):
You set this option but you don't have any log file (I suggest you to make them since it make it easier the debugging)

---

ycmd/completers/rust/rust_completer.py, line 90 [r1] (raw file):
Don't use the code directly. Use httplib.NO_CONTENT

---

ycmd/completers/rust/rust_completer.py, line 155 [r1] (raw file):
Missing a blank line here ( 2 blank lines between methods )

---

ycmd/completers/rust/rust_completer.py, line 164 [r1] (raw file):
couldn't you pass the port and the host as a parameter instead of parsing the output of racerd? To note that this would mean that you have to wait for racerd to start outputting the information instead of configuring it and let it be.

---

ycmd/completers/rust/rust_completer.py, line 179 [r1] (raw file):
Why pass some parameter to an API that all it does is tell you if the server is alive?

---

ycmd/completers/rust/rust_completer.py, line 198 [r1] (raw file):
This is not the API anymore, look #264 for the new API.

---

ycmd/completers/rust/rust_completer.py, line 242 [r1] (raw file):
This is never used and I believe is superseded by the StopServer

---

Comments from the review on Reviewable.io

[puremourning]

I took a swift look too.

---

Review status: 4 of 6 files reviewed at latest revision, 20 unresolved discussions, some commit checks failed.

---

ycmd/completers/rust/rust_completer.py, line 2 [r1] (raw file):
we don't normally put modelings in ycmd - while i would personally like it, @Valloric is of the opinion that there are too many editors in the world to start putting editor-specific configuration within the source files. He's probably right :)

---

ycmd/completers/rust/rust_completer.py, line 6 [r1] (raw file):
Are these the correct copyright headers? I suspect you can put `Copyright (C) 2015 ' or 'ycmd contributors'

---

ycmd/completers/rust/rust_completer.py, line 8 [r1] (raw file):
it's actually part of ycmd

---

ycmd/completers/rust/rust_completer.py, line 57 [r1] (raw file):
( src_key )

---

ycmd/completers/rust/rust_completer.py, line 60 [r1] (raw file):
[ src_key ]

I keep wondering if i should :imap ( ( and :imap [ [ etc....

---

ycmd/completers/rust/rust_completer.py, line 67 [r1] (raw file):
Is it really 'building' here? more 'initialising'. Also this might well be .debug level stuff? I think info is the default.

---

ycmd/completers/rust/rust_completer.py, line 68 [r1] (raw file):
yeah. i tried to use the normal stdout/stderr for tern, but this didn't work in the test suite. worked fine otherwise. quite sad.

---

ycmd/completers/rust/rust_completer.py, line 90 [r1] (raw file):
could 204 be httplib.NO_CONTENT ?

---

ycmd/completers/rust/rust_completer.py, line 106 [r1] (raw file):
( { and } )

---

ycmd/completers/rust/rust_completer.py, line 107 [r1] (raw file):
[ 'contents' ]

---

ycmd/completers/rust/rust_completer.py, line 140 [r1] (raw file):
should probably be debug, nor not at all ? this gets called a lot and is in the user's workflow critical path.

---

ycmd/completers/rust/rust_completer.py, line 164 [r1] (raw file):
We have the utils.GetUnusedLocalhostPort() to get a port to use in the ephemeral range. There's a small race that it might get used between selection and starting to listen, but this hasn't been noticed for ycmd or any other completer.

---

ycmd/completers/rust/rust_completer.py, line 196 [r1] (raw file):
If you're going to do this, you'll probably want to introduce a mutex to control the server status. I had problem with the tests and with YCM client in tern completer where it could try and start the server twice due to waitress request threading. It's fairly unlikely, so I'm not 100% sure how much it is needed, but synchronising these things certainly made the tests stop crashing :)

---

ycmd/completers/rust/rust_completer.py, line 241 [r1] (raw file):
2 empty lines

---

ycmd/completers/rust/rust_completer.py, line 242 [r1] (raw file):
I think this is a callback from Completer class ?

---

Comments from the review on Reviewable.io

[vheon]

Review status: 4 of 6 files reviewed at latest revision, 20 unresolved discussions, some commit checks failed.

---

ycmd/completers/rust/rust_completer.py, line 55 [r1] (raw file):
I know you're working on changing how to handle ycm_extra_conf.py in YCM but on the PR on your fork the contributor made me think that maybe using some static configuration option is better in this case. I mean you have to specify this in global wide, right? this isn't a per-project config, right?

---

ycmd/completers/rust/rust_completer.py, line 60 [r1] (raw file):
( If you use something like delimitMate there is a setting for the spaces. I don't use that kind of plugins, so I forget the spaces then I just use vim-surround ysi) )

---

ycmd/completers/rust/rust_completer.py, line 90 [r1] (raw file):
I was 100% sure that I made that comment too 😕

---

Comments from the review on Reviewable.io

[puremourning]

Reviewed 2 of 6 files at r1.
Review status: all files reviewed at latest revision, 20 unresolved discussions, some commit checks failed.

---

ycmd/completers/rust/rust_completer.py, line 60 [r1] (raw file):
yeah - i don't like plugins which try to write the code for me. Wait. YCM. hmm.

---

Comments from the review on Reviewable.io

[jwilm]

Is there a way to get flake8 to validate the spaces for parens and other delimiters?

[Valloric]

@jwilm Awesome work, thanks for doing this!

I gave this a light review since you're still working on it. I see both @puremourning and @vheon have provided excellent comments as usual, so you have feedback to go on. :)

---

Review status: all files reviewed at latest revision, 21 unresolved discussions, some commit checks failed.

---

build.py, line 262 [r1] (raw file):
Great as a starting point so that we can experiment with it. I'm even fine with landing an initial version that requires multirust, but we might want to hold off on publicizing the Rust support (or mark it experimental) until we have the full setup we talked about. From the email discussions:

check for multirust and use it if available (and beta is there etc). If it's not there, print a message saying you're going to use system cargo (if it's there) and that this might fail since you need non-stable rust to build. If neither is there, print a message recommending multirust (ideally with copy-pastable commands that will install everything necessary).

What you have is 👍 for now.

---

ycmd/completers/rust/rust_completer.py, line 2 [r1] (raw file):
Agreed with @puremourning, we don't put modelines in files.

---

ycmd/completers/rust/rust_completer.py, line 6 [r1] (raw file):
What @puremourning said. :)

Make sure you go through your files before you submit and check that the copyright headers are correct. It's one of those annoying bureaucratic hassles that sadly need to be done right. :(

---

ycmd/completers/rust/rust_completer.py, line 55 [r1] (raw file):
This sounds like it should be a YCM configuration option that ycmd then takes through the options file (see default_settings.json in ycmd source). We shouldn't have multiple ways of configuring ycmd (or its dependencies).

ycm_extra_conf should probably be used only for plugging in user-provided logic (like computing include paths for C++ etc).

I'm not sure does a normal rust installation include the full source for the std library, but if it does, we might want to figure this out automatically by probing the system. Maybe not for a v1 though.

---

Comments from the review on Reviewable.io

[homu]

☔ The latest upstream changes (presumably #272) made this pull request unmergeable. Please resolve the merge conflicts.

[jwilm]

Review status: 4 of 6 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

build.py, line 262 [r1] (raw file):
Agreed that we should be shipping the smarter build strategy as discussed. That's kind of at the bottom of my list at this point 😁; expect the patch after other features/issues are resolved.

---

ycmd/completers/rust/rust_completer.py, line 6 [r1] (raw file):
I've updating the files in this branch, and I am opening an issue to fix this for the rest of the project. The header was copied from another completer :).

---

ycmd/completers/rust/rust_completer.py, line 55 [r1] (raw file):
Rust doesn't distribute the source tree with the rustc and cargo executables; there is no default location for it.

This is definitely something we want configurable per project. I've got projects on my system that must be built with beta/nightly, and most of them are on stable. I would want completions/definitions to be found from the correct standard library. That said, one option would be configuring source trees for the various rust releases. The rust completer plugin could query rustc to see which version to use, but that incurs some runtime cost.

Options summary:

1. rust source trees configurable through .vimrc with completer detections of project rustc version. Maybe user has flags in .vimrc for g:ycm_rust_src_stable, g:ycm_rust_src_beta, etc. Pros: works per system and per project Cons: completer or racerd need to figure out rustc version for active project.
2. .ycm_extra_conf.py says where to look for the rust source. Pros: Per project rust source config. Cons: Will probably be right on just one system.
3. Look for environment variable RUST_SRC_PATH. Pros: Works per system. Cons: PITA with multiple projects

Option 1 seems to be the most versatile, but it does come with extra challenges.

---

ycmd/completers/rust/rust_completer.py, line 68 [r1] (raw file):
The completer plugin logs just end up in the ycmd stderr log. Do I need additional log files for racerd? I need to look at how other completers do this.

---

ycmd/completers/rust/rust_completer.py, line 164 [r1] (raw file):
I would prefer to keep the current strategy for getting the racerd listening address since it is guaranteed (insomuch as there are available ports) to be an unused port. A --host parameter should be added to racerd, though.

---

ycmd/completers/rust/rust_completer.py, line 196 [r1] (raw file):
Good to know - didn't realize the requests were threaded. I'll just add that before it bites us 😄.

---

ycmd/completers/rust/rust_completer.py, line 242 [r1] (raw file):
The base completer suggests that I should be using Shutdown. I had an issue with racerd processes "leaking" before adding this.

---

Comments from the review on Reviewable.io

[jwilm]

Whew.. reviewable has a bit of a learning curve!

[micbou]

Reviewed 1 of 6 files at r1.
Review status: 4 of 6 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

ycmd/completers/rust/rust_completer.py, line 242 [r1] (raw file):
This is called when ycmd is terminated (not on Windows unfortunately).

---

Comments from the review on Reviewable.io

[jwilm]

Review status: 4 of 6 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

ycmd/completers/rust/rust_completer.py, line 242 [r1] (raw file):
Should I leave RustCompleter's Shutdown() as is, then?

---

Comments from the review on Reviewable.io

[micbou]

Review status: 4 of 6 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

ycmd/completers/rust/rust_completer.py, line 242 [r1] (raw file):
Yes.

---

Comments from the review on Reviewable.io

[vheon]

Reviewed 1 of 2 files at r3.
Review status: 5 of 6 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

ycmd/completers/rust/rust_completer.py, line 55 [r1] (raw file):

rust source trees configurable through *.vimrc *

You have to consider that we're in the ycmd world where we don't know which editor is asking us to do completion or whatever so don't design this with vim in mind (I know that is not easy since I myself use vim and I don't know at all how other editor users would like to configure this). That being said, considering all the options you've set out I would go for option 2. For the Cons that you've mentioned is easily overcome by the fact that .ycm_extra_conf.py is nothing more than a python script.

---

ycmd/completers/rust/rust_completer.py, line 60 [r1] (raw file):
Well the different between YCM and delimitMate is that YCM let me chose if I want it to do the job while delimitMate just do it, so I don't use it 😏

---

ycmd/completers/rust/rust_completer.py, line 68 [r1] (raw file):
One thing is the completer plugin (this file) log and one thing is racerd log file.

---

ycmd/completers/rust/rust_completer.py, line 164 [r1] (raw file):
As I said on the PR on your fork using utils.GetUnusedLocalhostPort() it makes the code easier to read and the flow to understand. Every server that we start use that method.

---

ycmd/completers/rust/rust_completer.py, line 196 [r1] (raw file):
I didn't experience any problem without working on the JediHTTP, but I've just added it just in case ;)

---

Comments from the review on Reviewable.io

[jwilm]

Review status: 5 of 6 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

ycmd/completers/rust/rust_completer.py, line 55 [r1] (raw file):
Ah, right. Isn't there generally support for the user options across the different ycmd clients? Having spent time using this, the .vimrc option (for vim) is pretty appealing.

---

ycmd/completers/rust/rust_completer.py, line 164 [r1] (raw file):
If the code for reading the listening address from racerd doesn't read well, that feels like a separate problem. I'm very reluctant to move from a solution free from a race condition to one with a race condition. If you guys want me to switch, though, you are obviously welcome to override that.

---

Comments from the review on Reviewable.io

[puremourning]

Added some waffly thoughts - more discussion points than anything else :)

---

Review status: 5 of 6 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

build.py, line 262 [r1] (raw file):
TL;DR if the setup required for our rust completer to work is not present, should consider not providing rust semantic support from ycmd, so that existing workflows are not changed on upgrade? More of a discussion point than an "issue".

There's an existing rust plugin for Vim which provides an omnifunc. With tern completer I deliberately made sure that if the ycmd tern submodule had bot been "installed", that ycmd didn't create a Completer instance (in hook.py) and thus user's existing installations of the racer plugin continued to work as-is. IMO the omnifunc-based one is inferior due to not supporting LCS matching and a couple of other bits, but we don't want people to :PluginUpdate (or whatever) and for their existing workflow to be broken without a suitable replacement. In all likelihood, we'll just get bug reports saying "rust completion broken after upgrade. uninstalling and calling the feds." even if we stick something obvious in the README and even if the completer complains that it can't find rust source (or whatever). I may have inherited a tiny bit of Val's cynicism. :p

---

ycmd/completers/rust/hook.py, line 1 [r4] (raw file):
Ahem. Should probably update this too. Sorry :/

---

ycmd/completers/rust/rust_completer.py, line 55 [r1] (raw file):
I think I agree with @vheon with the additional qualification that a fallback to the env. var would help a chunk of users.

Here's my thinking:

• the existing rust completer plugin for vim uses this exact environment variable, IIRC so if we at least use it as a fallback, it should "just work" for existing users of that plugin... i think
• the .ycm_extra_conf.py would (could/should?) be submitted as part of the project source control, and as a result might not want to include hard-coded non-system paths. This is easily handled (as @vheon points out) by being arbitrary code and can (if it chooses) use any environment settings. the advantage of this is that users of a shared project need not configure anything in an ideal world (this is how I set this up at work - the extra conf works straight out of perforce, using our existing build system as the canonical source).
• i like to think that editor settings and project-settings are different tasks, so would like to think that the same project settings would work irrespective of editor. e.g. if i used 3 different editors all using ycmd (for whatever reason), then i should only have to configure ycmd to understand my project once (by putting something in my project's extra conf. file)

I think we're setting a precedent here, so it probably bears the discussion :)

---

ycmd/completers/rust/rust_completer.py, line 68 [r1] (raw file):
IMO appearing in ycmd's log file is ideal. You can see in :YcmToggleLogs and bug reports (should they follow CONTRIBUTING.md - admittedly unlikely) will include the downstream completer's log entries. However like I said I tried this with the tern completer, but got tripped up because our test suite (nose) mocks the stdout and stderr objects, so they aren't real "output streams" (or whatever python's equivalent is) and the tests just crashed. If it works for this completer, great! :) Maintaining the separate log files is just a pain and extra code to write.

---

ycmd/completers/rust/rust_completer.py, line 164 [r1] (raw file):
I was looking yesterday and found that we had issues with ycmd in the past where the output stream was buffered, leading to not getting the output containing the port. This also blocks the thread waiting for the server to start up and print this out, meaning the "server state" lock is held for much longer. There may be times when this blocks the GUI thread, though TBH i haven't investigated this in much detail.

---

ycmd/completers/rust/rust_completer.py, line 196 [r1] (raw file):
With the GIL, the threading model is (as i understand it) that there is only a single thread of concurrent execution, but your I/O operations yield if they block (such as requests.post) and other "requests" start executing (until they yield). A bit like co-operative multitasking or coroutines. This can mean that multiple OnFileReadyToParse events are "executing" (IO wait) simultaneously.

At least, that's what i surmised. I've said before that i'm no expert in this newfangled python whatsit.

---

ycmd/completers/rust/rust_completer.py, line 182 [r4] (raw file):
Out of curiosity, what are you thinking to do with request_data ? Normally the restart server commands just ignore the supplied data. Is there something cool you have in mind? :)

---

ycmd/completers/rust/rust_completer.py, line 194 [r4] (raw file):
Sorry to be a pain, but I changed this interface to add an args parameter which is everything after :YcmCompleter YourCommand

I just realise now, however, that that change was wholly unnecessary because I removed the 1 command which used it.

Please don't hate me.

---

Comments from the review on Reviewable.io

[puremourning]

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

Comments from the review on Reviewable.io

[vheon]

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

ycmd/completers/rust/rust_completer.py, line 194 [r4] (raw file):
so we have to add the args to every subcommand but no one use it? 😕

---

Comments from the review on Reviewable.io

[puremourning]

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

ycmd/completers/rust/rust_completer.py, line 194 [r4] (raw file):
Hey! I explicitly said "Please don't hate me". I even said "please". 😀

I originally had :YcmCompleter ConnectToServer <port> to connect to existing tern server, but I removed it as YAGNI.

I also intended to add :YcmCompleter FixItRename <name> but decided to leave it for version 2.

In theory, the API supports it so it's not such a big deal... maybe... hopefully...

I feel bad about this. I'm already punishing myself for it and grasping at justifications...

---

Comments from the review on Reviewable.io

[vheon]

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

ycmd/completers/rust/rust_completer.py, line 194 [r4] (raw file):

I feel bad about this. I'm already punishing myself for it and grasping at justifications...

Don't be. As you said it will be useful in the near future 👍

---

Comments from the review on Reviewable.io

[Valloric]

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

build.py, line 262 [r1] (raw file):
@puremourning I'm of the opinion that if YCM adds a new semantic completer and the user has a different plugin installed that provides semantic completion for the same filetype, then YCM should override the other plugin (as long as we're providing a better experience).

That seems possibly controversial at first glance. But YCM provides a superior completion experience than using vim's omnifunc and the user already clearly agrees otherwise they wouldn't have installed YCM. We also provide fuzzy matching, better perf, minimal Vim GUI thread blocking etc.

So if we add semantic completion for a new language, we should override specific plugins that used to provide such completion.

Note that we have plenty of experience doing this; YCM launched with only C-family completion. Everything else was slowly added over time, and we've always been overriding. And we've never had a complaint about it; you've seen our issue tracker and know that our users have absolutely no qualms about complaining. :)

We've also done this for diagnostics. At first, C-family diagnostics would come from Syntastic which would call compilers directly. Then we plugged ourselves into the Syntastic API and started returning the diagnostics we got from libclang. Eventually we replaced the Syntastic layer and just started showing diagnostics ourselves. In fact, we literally turn off Syntastic for c-family filetypes (see TurnOffSyntasticForCFamily()).

(And that reminds me, we should turn it off for other filetypes we provide diagnostics for.)

Again, we've never had a complaint about it so I'm pretty sure we're making users happy with this.

---

ycmd/completers/rust/rust_completer.py, line 6 [r1] (raw file):
Note that we don't want to remove previous copyright statements. So if an older completer had those lines, it should continue having them.

---

ycmd/completers/rust/rust_completer.py, line 55 [r1] (raw file):
@vheon Vimrc settings translate to ycmd options in the settings file, so the distinction doesn't matter too much.

I think we should go with vimrc setting + fallback to env var. We can add support for a function in the extra conf file later if necessary, but hopefully not. I see the extra conf file as a measure of last resort, not something we should be picking by default. It's executable code which means security issues (don't forget, we've had a terrible RCE vulnerability with it once), not everyone knows Python nor wants to learn it etc. Plenty of reasons why not to pick it.

---

ycmd/completers/rust/rust_completer.py, line 68 [r1] (raw file):
All logging from completers goes into ycmd's log file. If racerd as a server has its own logs, that's entirely separate (and their contents should not be included in ycmd logs).

---

Comments from the review on Reviewable.io

[Valloric]

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

ycmd/completers/rust/rust_completer.py, line 164 [r1] (raw file):
@jwilm The race condition is unfortunate, but to the best of our knowledge has never occurred in practice. That's not a stellar argument, I know. Can we easily get the port the server is running on without computing it ahead of time and passing it along? I don't know of a cross-platform way of doing it and we really must support telling the user which ports the other servers are using (through YcmDebugInfo etc). AFAIR this was the rationale for going with the hack that we have. If we can pass 0 and easily get the port afterwards, I'm all for it.

That sounds like a separate issue in ycmd though, and one that we'd solve across the codebase if we decided to tackle it. For now, you might want to go with utils.GetUnusedLocalhostPort() to make your life easier. If you wish to tackle the pass-0-and-fetch-port-later issue codebase-wide in a separate PR, I think that's a great idea! :)

---

Comments from the review on Reviewable.io

[jwilm]

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

build.py, line 262 [r1] (raw file):
What about the case of diagnostics for rust? I can accomplish the same thing as the rust.vim plugin, but I had initially decided against it since the experience is equivalent.

---

ycmd/completers/rust/rust_completer.py, line 6 [r1] (raw file):
Oh, right! Those were likely there since before ycmd was extracted from YouCompleteMe. D'oh.

---

ycmd/completers/rust/rust_completer.py, line 164 [r1] (raw file):
racerd prints a messages like this once the server is listening

racerd listening at 0.0.0.0:59382

The RustCompleter currently reads this line from racerd's stdout and uses it when making http requests. Unfortunately, it blocks while waiting for that output. In practice for racerd, it's a short amount of time. I haven't personally noticed any delay in the GUI, but that's obviously not representative of users as a whole.

It seems like the consensus is that we just want to specify the port ahead-of-time. I'll update this accordingly.

---

ycmd/completers/rust/rust_completer.py, line 194 [r4] (raw file):
Why not just grab the additional arguments from the request_data?

---

Comments from the review on Reviewable.io

[Valloric]

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

build.py, line 262 [r1] (raw file):
If we take that logic to the extreme, then the user needs YCM + 5 different plugins for 5 different languages if they develop in that many. The idea with YCM is that we provide as many quality code-comprehension services we reasonably can. And don't forget about other editors; rust.vim works with Vim and that's it. With ycmd, we have a backend that works with emacs, sublime text, atom etc.

For now, don't feel the need to focus on diagnostics, but we'll probably add them for Rust at some point.

---

Comments from the review on Reviewable.io

[jwilm]

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

build.py, line 262 [r1] (raw file):
😣 As a vim user, I keep forgetting that ycmd is editor agnostic, and clients exist for many editors. My mental model will surely update.. eventually 😀.

---

Comments from the review on Reviewable.io

[jwilm]

Added HMAC support :)

---

Review status: 3 of 6 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

Comments from the review on Reviewable.io

[vheon]

👍

---

Reviewed 1 of 5 files at r5.
Review status: 2 of 6 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

ycmd/completers/rust/rust_completer.py, line 99 [r5] (raw file):
Have you thought about using requests custom authentication? I used it for the JediHTTP completer https://github.com/vheon/ycmd/blob/feature-jedihttp/ycmd/completers/python/jedi_completer.py#L45. It would save you to dump the body manually and would not mix the request creation with its HMAC signing. Your call though ;)

---

Comments from the review on Reviewable.io

[micbou]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/tests/rust/subcommands_test.py, line 24 [r24] (raw file):
You are right about this. @jwilm You should take a look at the GoTo_all_test method in ycmd/tests/clang/subcommands_test.py for writing those tests.

---

Comments from the review on Reviewable.io

[vheon]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/tests/rust/subcommands_test.py, line 24 [r24] (raw file):
Why GoToDefinition and GoToDeclaration were added in the first place if in rust do not make sense to talk about declaration and definition? Why we don't just keep GoTo?

---

Comments from the review on Reviewable.io

[puremourning]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/tests/rust/subcommands_test.py, line 24 [r24] (raw file):
Just means that mappings work for any completer?

---

Comments from the review on Reviewable.io

[micbou]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/tests/rust/subcommands_test.py, line 24 [r24] (raw file):
Yes but we are not doing this for all completers. If we take this path, some uniformization needs to be done between completers:

• Typescript has GoToDefinition but no GoTo and GoToDeclaration;
• Javascript has no GoToDeclaration.
  At least, we should add the GoTo command to Typescript.

---

Comments from the review on Reviewable.io

[vheon]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/tests/rust/subcommands_test.py, line 24 [r24] (raw file):
IMHO if only one functionality is provided then it should be mapped to GoTo and then add the specific one if necessary. That way would be easier to be uniform.

---

Comments from the review on Reviewable.io

[Valloric]

Awesomesauce. :)

with minor comments.

@micbou Could you create an issue on the tracker so we don't forget the necessary work when Rust stable 1.7 goes out? You seem to have a solid understanding of the intricacies.

---

Reviewed 5 of 6 files at r22, 1 of 3 files at r23, 2 of 2 files at r24.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/tests/rust/subcommands_test.py, line 24 [r24] (raw file):
All of you have excellent points.

I'd lean towards us implementing GoTo, GoToDeclaration and GoToDefinition for languages for which declarations are definitions (and vice versa). They do indeed have declarations and definitions, they merely map to the same location. So logically all three of those should work.

Don't forget that we also document that GoTo will try to do the "most sensible" thing, so it should definitely work everywhere.

---

ycmd/tests/rust/subcommands_test.py, line 27 [r24] (raw file):
Solid point from @micbou, let's rename this.

---

Comments from the review on Reviewable.io

[homu]

☔ The latest upstream changes (presumably #246) made this pull request unmergeable. Please resolve the merge conflicts.

[jwilm]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

ycmd/tests/rust/subcommands_test.py, line 24 [r24] (raw file):
I've added tests using a generator as recommended and as in the clang subcommands test.

---

ycmd/tests/rust/subcommands_test.py, line 27 [r24] (raw file):
Done

---

Comments from the review on Reviewable.io

[vheon]

I believe that the tests wont start until you rebase against master.

---

Reviewed 1 of 1 files at r25.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

ycmd/tests/rust/subcommands_test.py, line 57 [r25] (raw file):
Since the command is the only thing you pass, maybe it would be easier to do

for command in [ 'GoTo', 'GoToDefinition', 'GoToDeclaration' ]:
  yield test, command

---

Comments from the review on Reviewable.io

[jwilm]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

ycmd/tests/rust/subcommands_test.py, line 57 [r25] (raw file):
I had considered that. For reasons of convention and maintenance, it seemed better to pass the dictionaries.

---

Comments from the review on Reviewable.io

[puremourning]

LGTM

---

Reviewed 1 of 1 files at r25, 3 of 3 files at r26.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from the review on Reviewable.io

[micbou]

@jwilm Tell us when you want this merged.

---

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from the review on Reviewable.io

[Valloric]

@jwilm Could you squash your commits? I see 62 commits here, and we'd rather not have that many in the history for one PR. :)

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[jwilm]

@Valloric I would love to clean up the commit log. Squashing it to a single commit would lose some potentially useful history (and my git activity!). However, there is a lot of incremental/fixup commits that don't add anything, and we can squash some history to tell the story more concisely.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[Valloric]

Feel free to preserve as much history as you feel is useful to have, just not 62 commits worth. :D If you could keep it under 10, that would be splendid.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[micbou]

I added minor comments.

---

Reviewed 1 of 1 files at r25, 3 of 3 files at r26.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

ycmd/completers/rust/rust_completer.py, line 173 [r23] (raw file):
Wrong indentation: 4 spaces instead of 2.

---

ycmd/completers/rust/rust_completer.py, line 198 [r23] (raw file):
You could do

return { 'location': location }

instead of these 3 lines.

---

ycmd/completers/rust/rust_completer.py, line 200 [r23] (raw file):
else branch is not needed. You can directly return.

---

Comments from the review on Reviewable.io

[jwilm]

After reading through the history, it was almost entirely incremental/fixup work on the feature without any real value. I squashed it into a single commit.

---

Review status: 13 of 15 files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[micbou]

---

Reviewed 2 of 2 files at r27.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[jwilm]

Hmm, maybe we should go ahead and merge this so the tests for my YCM PR will pass :).

[micbou]

@homu r+

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from the review on Reviewable.io

[homu]

📌 Commit 4e11be5 has been approved by micbou

[homu]

⚡ Test exempted - status