Removes local variables from outline #594

[aldonogueira]

This change removes local variables from outline making it much more useful, at least in Atom IDE.

[nrc]

Is this something clients expect? (I'm honestly not sure). I thought that symbols search should include every def in a file, and I think (though I'm not 100% sure) this is what VSCode does for other languages.

Could you filter on the client side using the symbol kind? Perhaps we could tweak the symbol kinds to make this possible if it is not.

[aldonogueira]

Look what I have found:

• Although the animation on https://ide.atom.io/ shows local variables on the outline for javascript...
• There isn't a local variable kind on language server spec. Search for "export namespace SymbolKind" there.
• Atom IDE doesn't show local variables for Java. However I couldn't determine if they are removed at the server or at the atom client
• As far as I know, VSCode doesn't have an outline view (or I couldn't make it work), it only has a symbol search, so people don't care much about the presence of local variables there

[nrc]

There isn't a local variable kind on language server spec. Search for "export namespace SymbolKind" there.

Variable is 13 there

As far as I know, VSCode doesn't have an outline view (or I couldn't make it work), it only has a symbol search, so people don't care much about the presence of local variables there

Yeah, I think it doesn't have an outline view. There is only one message in the LSP for both symbol search and outline, and that is documentSymbol, so I think you must filter on the client if you want to have local variables in symbol search.

[aldonogueira]

Exactly. That's why this line on rls/src/lsp_data.rs
"DefKind::Local | DefKind::Static | DefKind::Const | DefKind::Field => SymbolKind::Variable,"
All these symbols are translated to Variable. How can we separate local variables after this? What do you mean by "tweak the symbol kinds"?

[nrc]

What do you mean by "tweak the symbol kinds"?

We could use Field for fields and Constant for statics and consts, and then Variable would only be used by locals

[Yanpas]

Both C# and C++ language servers filter local variables. Moreover both these LS are developed by Microsoft, so it may treated as de-facto standard.

I think this nuance should be menthioned in the standard too despite it is not appliable to all languages. Created an issue microsoft/language-server-protocol#414

[aldonogueira]

If we follow microsoft's interpretation for static typed languages here, then the problem is kind of solved by this patch.

On the other hand, I see there is a problem with the translation of Racer kinds to Language Server kinds. For example, Eclipse's Java Language Server translation is a little bit different. They map IJavaElement.FIELD to SymbolKind.Field.

If we do as @nrc suggested, we can filter local variables on the client for the outline view but still make them reachable through the symbol search.

Another issue is hierarchy. If we had hierarchy #86, clients could achieve the same effect by removing variables that are under a function symbol.

[Yanpas]

I've also opened issue similar to #86 : microsoft/language-server-protocol#327

[aldonogueira]

I reverted the previous commit and changed the mapping from Rust kinds to the spec kinds according to the comments before. Should I change the title of this PR or create a new one?

We should probably ask @DSpeckhals his opinion on this mapping as well since he added TupleVariant and StructVariant. I'm not sure what they mean. Aren't they enum members?

I have also noticed that the SymbolKind on languageserver-types is outdated

[DSpeckhals]

Yes, TupleVariant and StructVariant are enum members. There's a little more history in rust-lang/vscode-rust#140

[aldonogueira]

I read the PR comments, but I can't say that I understood it completely. Is there a reason TupleVariant must be mapped to SymbolKind::Constant and StructVariant to SymbolKind::Class? Can't they just be SymbolKind::Field?
I also checked with rustc -Zsave-analysis just now and saw that unit-like enum members are classified as TupleVariant as well.

[DSpeckhals]

It was just to differentiate between the different types of tuple variants. I believe I looked at the TypeScript mappings, and tried to do what it did with enum variants. Field is okay, but I think that the new EnumMember would be best once languageserver-types is updated.

[nrc]

Thanks for the update!