CodeLens support

[matklad]

For rust-lang/vscode-rust#347

[matklad]

This uses regex approach for finding tests as well. I have yet to see into leveraging save analysis for this. On one hand, this is "correct" and should work if, for example, tests are generated by macros. On the other hand, save analysis is not always available (I see "dependency failed to build" constantly when using RLS), and I think that it should be the goal of RLS to provide a minimal level of service no matter what. Perhaps the logic should be "if have save-analysis, use it, otherwise fallback to heuristics"?

[nrc]

WIP

let me know when this is ready to land, I assume it is blocking rust-lang/vscode-rust#347 ?

I have yet to see into leveraging save analysis for this

I'm not sure this is worthwhile - we'd search a little more accurately for #[test] but I think it would not help that much and it would be no better with custom test frameworks. It would be better to add API to Cargo to get all tests, but that should happen later as part of the CTF stuff.

I see "dependency failed to build" constantly when using RLS

If you see this where the dep should build and can reproduce, it would be great if you could file an issue with STR - these are really hard for me to track down, but once I have a reliable test case it is usually OK to fix them.

[matklad]

it would be great if you could file an issue with STR

I have no idea what STR is, but my problem should be fixed by rust-lang/vscode-rust#352 :-)

EDIT: a-ha, STR=Steps To Reproduce

[matklad]

Ready for the review.

There's a ton of stuff to make this work properly, but this alone should provide value as is.

[matklad]

The only thing I am unsure about is client/server version mismatch.

Currently, if you use this rls server with VS Code client which lacks rls.run command, the error pop-up will appear.

[Xanewok]

LGTM, thanks for working on this!

I am and still will be detached from the project for a week or two, so I'll leave merging and coordinating the rls-vscode PR merge to @nrc =)

The only thing I am unsure about is client/server version mismatch.

Currently, if you use this rls server with VS Code client which lacks rls.run command, the error pop-up > will appear.

To be fair that's something to keep in mind while we prepare for 1.0 - it'd be good to decide how we'll work around the plugin<>rls compatibility. Currently we just require minimum vscode version for the vscode plugin, but the plugin itself has no backwards compatibility guarantees wrt RLS itself.

One idea might be to mirror stable-beta-nightly release channels for the plugin or bake supported stable-beta-nightly channel triple in the plugin and warn when executed rls version is different, but that's a discussion for another topic.

[nrc]

STR = steps to reproduce

[nrc]

A comment inline about a nit.

I think to address the issue of erroring if the client doesn't have the run command, we should require the client to opt-in to code lens as a capability in the initialize message and the server should only provide code lens if the client has done that.

[nrc]

Could this be restored?

[matklad]

Oups, shouldn't have committed this, fixed!

[matklad]

I think to address the issue of erroring if the client doesn't have the run command, we should require the client to opt-in to code lens as a capability in the initialize message and the server should only provide code lens if the client has done that.

Implemented this. I am opting-in not into the code-lens as a whole, but only into rls.run command specifically. This I think is more general because I can imagine code lenses other than tests ("find impls", for example, could be a lens).

[matklad]

Not sure what's wrong with the test on windows: it prints six instead of five progress messages. Is there a way to write a test without listing all the initialization messages explicitly?

[nrc]

You could cfg for windows vs non-windows. If you want to avoid that, you can send messages to the server after setup but before you start expecting messages and manually clear the reply buffer. I don't think we've done that before, but it should be possible.

[matklad]

Let's ignore the test on windows for now?

It seems to me that the test infrastructure can be improved in a more principled way, by separating "testing protocol implementation" and "testing that analysis features work", but I'd want to look into that separately.

[matklad]

Hm, now the test failed on linux, but it does pass locally. Guess I'll need to understand where all this "progess" things actually come from :-)

[matklad]

Ok, looks like adding one more progress should fix the test :)

[nrc]

Thanks!

[alexheretic]

As an aside: I did add an ability to ignore progress messages to the test harness a while ago, but I scrapped it as the feature I was working on changed. Maybe I should re-add it though, I don't think all of our tests need to assert deterministic progress messaging.

[matklad]

@alexheretic you are doing the atom rls client, right? Does the proposed rls.run command make sense to you? Do you want to change anything to make it easier to support in Atom?

[alexheretic]

@matklad I don't think atom-ide-ui / atom-languageclient support codelens properly yet, so atom can't do this. I'll raise issues if this doesn't work when atom does support codelens.

[matklad]

Uguh... I've actually noticed a problem with VS Code implementation of code lens: you can't invoke lens action via a shortcut: microsoft/vscode#41579.

The issue suggest an interesting solution to that: provide the same action both as code lens and as a code action. I think we should do this for tests: you'll see both Run Test lens and, if the cursor is on the test fn identifier, you'll be able to run test with a ctrl+. shortcut. This is a nice pattern for IDEs: noisy visual indication to make the feature discoverable for new users + contexttual shortcut for power users, which you need to know about.

I think I'll add the code action soonish, and that should be usable from atom.

[alexheretic]

I think I'll add the code action soonish, and that should be usable from atom.

codeAction is currently only usable in atom-ide-ui when related to diagnostics. So not a good fit for running tests. I think this is correct too, it's how I understand the difference between action & lens. Actions are for fixing issues, lens is for doing optional stuff.

For example ide-rust can't really make use of the deglob functionality at the moment.

[matklad]

Actions are for fixing issues, lens is for doing optional stuff.

I think it's useful to look at code actions more broadly. At it's core, a code action is just a UI to execute some command, depending on the surrounding context. There are three general categories of things such UI might be useful for:

• Quick fixes for compiler errors and warnings, which are a reactions to red squiggles

• Micro refactoring like "swap two parameters in a function call", "apply de-morgan law to this boolean expression", "swap branches of this if". This are not diagnostics, because the are for "there are two ways to do it, and neither is inherently better than other" stuff.

• Just arbitrary actions, related to a particular code. For example, "run the test at cursor", or "flip this setting that is related to the code".

Code lens fundamentally are almost the same. The principal difference is that you need to explicitly invoke shortcut to see code actions, and code lenses pop up automatically.

Here's example of similar concepts from IntelliJ UI:

The context menu with actions is shown on Alt+Enter shortcut. "Convert/Move" entries are examples of micro-refactorings, and the "Run" entry is an example of arbitrary action. The "Play" icon on the left is analogous to Code Lens: it executes the same "Run" action, is more discoverable because of an icon, but requires a mouse click.

If atom-ide-ui has a restriction that code action is always a reaction to some error/warning, I think it makes sense to try to remove it, because code actions actually are much more useful than just a tool for fixing error.

Hope this is a useful perspective! :)

[alexheretic]

The ui distinction is up to the client though, I guess atom-ide-ui has different ideas to vscode. For example codeActions do pop up automatically in ide-rust for all diagnostics.

I don't have too much control of this myself in ide-rust, being at the mercy of upstream design in the way all framework consumers are.

In any case as long as rls follows the protocol in a reasonable way everything should be reasonably fine. For example, once atom-ide-ui & atom-languageclient support codelens this should work fine.

In atom having a codeAction in addition to a codeLens at the moment won't cause issue, as currently the action will be invisible due to the diagnostic requirement. So I don't have strong feelings about it. I'm not sure if it's the "done thing" to duplicate lens into action.