Fixing symbols hierarchy #34

laughedelic · 2017-11-12T22:21:24Z

WIP fix for #33.

Here's how it looks with 379a735:

The problem is, it shows only one level of nesting, which is wrong. I compared the responses with other servers (TypeScript and Python) and didn't find any difference: containerName just matches the corresponding symbol's name field, nothing special.

olafurpg

Change looks good, I propose we let this wait until #31 is merged to avoid merge conflicts for @gabro

olafurpg · 2017-11-12T22:42:17Z

metaserver/src/main/scala/scala/meta/languageserver/SymbolIndexer.scala

@@ -29,19 +30,25 @@ class SymbolIndexer(

  def documentSymbols(
      path: RelativePath
-  ): Seq[(Position.Range, Denotation)] =
+  ): Seq[SymbolInformation] = {


olafurpg · 2017-11-12T22:44:24Z

metaserver/src/main/scala/scala/meta/languageserver/SymbolIndexer.scala

+      denotation.name,
+      denotation.symbolKind,
+      path.toAbsolute.toLocation(position),
+      Option(denotations.get(symbol.owner)).map(_.name)


How about using symbol.owner.syntax? .owner is likely a Symbol.Global which has a .signature.name we can use

See #33 (comment). Also I didn't get you about .signature.name.

laughedelic · 2017-11-12T22:50:19Z

Yes, I thought that it will need a rebase on top of #31, just wanted to show what I have now. The change is little so it will be easy to adapt it to #31 once it's in a usable state.

laughedelic · 2017-11-14T01:46:54Z

Regarding broken nested hierarchy: facebookarchive/atom-ide-ui#121

laughedelic · 2017-11-14T02:13:23Z

@olafurpg how can we get the range of symbol's definition body? i.e. class/method/etc. with everything inside the braces {}

olafurpg · 2017-11-14T16:38:42Z

@laughedelic you mean the source file positions? You would have to parse the file and find the tree node that defines that symbol and then use Tree.pos.{start,end} to get it's position. Is that necessary for the outline feature? I feel like you should be able to somewhat infer it from the Symbol structure

a -> line 1
a.b -> line 2
a.b.c -> line 3
a.c -> line 4
d -> line 5
d.e -> line 6

laughedelic · 2017-11-14T17:35:23Z

you mean the source file positions?

Yes. Location range where the symbol starts and where its definition ends.

Is that necessary for the outline feature?

There are two things:

LSP doesn't allow you to rely on containerName for the hierarchy (and it just doesn't work as you see above). But you can use nested location ranges to nest the tree nodes. See Nested symbols hierarchy facebookarchive/atom-ide-ui#121 for details.
Generally speaking, it's not necessary and VS Code doesn't even have this tree representation, it shows symbols in a flat list: Feature request: symbols tree view microsoft/vscode#5605. Yet it would be nice to support if it's possible to do.

You would have to parse the file and find the tree node that defines that symbol and then use Tree.pos.{start,end} to get it's position.

Sounds like too much extra work for this basic feature. I hoped that it's something that's already available in semanticdb, but just needs some digging..

I feel like you should be able to somewhat infer it from the Symbol structure

This is a good idea for a workaround. I'll see how to do it.

gabro · 2017-11-14T17:36:53Z

Generally speaking, it's not necessary and VS Code doesn't even have this tree representation, it shows symbols in a flat list: microsoft/vscode#5605. Yet it would be nice to support if it's possible to do.

I agree it would be nice to support it. I believe eventually also VSCode will support a tree outline

gabro · 2017-11-14T17:42:54Z

Also, apparently some extensions are already providing it. E.g. https://marketplace.visualstudio.com/items?itemName=patrys.vscode-code-outline

laughedelic · 2017-11-14T18:00:34Z

Nice! And it also relies on nested ranges. It even selects the region when you click on the symbol.

gabro · 2017-11-14T18:07:24Z

Yep, I've tested it with TypeScript and it works quite well!

olafurpg · 2017-11-14T18:24:38Z

Cool! Let's do this then 💪 parsing the tree and finding the tree node of the symbol definition should not be too hard, which message needs this information?

Rough sketch

import scala.meta._, contrib._
val code: String = ???
def findSymbol(pos: Position) = ???
code.parse[Source].get.collect { 
  case node @ Defn.Object(_, name, _, _) =>
    findSymbol(name.pos) -> node.pos
// ... same for Defn.Trait and Defn.Class
}

laughedelic · 2017-11-14T18:51:24Z

@olafurpg thanks! I'll try to implement it. I was just concerned about reparsing source on every such request.

olafurpg · 2017-11-14T18:54:00Z

Parsing is cheap, we use the parser to index all the *.scala source files of your dependencies at 30k LOC/s 😄 How often is this message called?

laughedelic · 2017-11-14T19:15:02Z

I have to check, but I think it's called every time you open the outline panel and if it's open, on every file change 😓

olafurpg · 2017-11-14T19:19:11Z

We actually don't even need the compiler for this, once #23 is merged we can use the ctags indexer to build the semanticdbs on the fly. It's not prohibitive to run on every file change IMO, and if it turns out to be slow we can probably optimize it somehow. I agree this is a useful feature!

Its containerName field should be the symbol.owner's name, not its type

It searches a Member node in the parsed tree that has the same name and same startLine. This widens position range to the symbol definition, which is necessary for correct symbols hierarchy nesting.

laughedelic · 2017-11-15T05:37:49Z

I rebased this branch on master (after #23 was merged) and here's what I got

First some crazy example with different nested things and awful formatting (on purpose):
And an example of different SymbolKinds (some icons don't make much sense to me, but that's what atom-ide-ui has to offer):

There are some more SymbolKinds such as String, Number, Boolean, Array. I was doubting if it's worth implementing them and how exactly. I think they should be used for literals, but literals normally don't float around and don't appear in this outline..

Notice that I didn't want to nest everything under the package symbol, because it's always top-level and wouldn't give you any additional information. Even with more than one package in the same file it's fine, because they kind of work like "separators". This is discussable, of course.

About implementation: after some experimenting, I figured out how to do it using only parsing and traversing the tree. It's fast and updates immediately when you're editing (btw, how do you guys make those nice GIFs?). It was a good exercise for me to familiarize myself with the Tree 🌳 and some other things in Scalameta.

Unsolved problem: requests are sent on edits and responses are immediate, so when you edit to a non-parseable state, the outline is empty. This could be solved by storing the last generated symbol list and sending it until it can be updated. Any better ideas?

gabro · 2017-11-15T08:07:59Z

Wow, that looks great! I personally use RecordIt for the gifs, but I think there's many options available.
For the state, one may think of using an observable and only return the latest valid state for that buffer.

laughedelic · 2017-11-15T10:35:18Z

@gabro thanks, I will try it.

olafurpg

Nice work! Falling back to last outline makes sense, you can probably get away with a regular map keyed by the uri instead of using Observable.

olafurpg · 2017-11-15T10:45:34Z

metaserver/src/main/scala/scala/meta/languageserver/ScalametaLanguageServer.scala

+        Some(name)
+      case Term.Select(qual, name) =>
+        qualifiedName(qual).map { prefix => s"${prefix}.${name}" }
+      case Pkg(sel: Term.Select, _) =>


What about package a?

It falls into the Member case, so it will be just its name a.
This whole qualifiedName method is here only for the packages with Select, because their default name is just the last part and I think it's useful to see the full name in the outline.

I see, nice!

olafurpg · 2017-11-15T10:45:49Z

metaserver/src/main/scala/scala/meta/languageserver/ScalametaLanguageServer.scala

+        qualifiedName(qual).map { prefix => s"${prefix}.${name}" }
+      case Pkg(sel: Term.Select, _) =>
+        qualifiedName(sel)
+      case m: Member =>


Glad you found this! Took me a while to learn about it :)

What do you mean? 😅

I wasn't aware of Member until quite recently, always found it frustrating to handle all val/val/def/class/trait/object cases

Ah, yes, it's very helpful. I just searched for whatever with name, when I wanted to match it with symbols from the indexer 😄

olafurpg · 2017-11-15T10:50:00Z

@laughedelic I use GIPHY on OSX.

olafurpg

I just realized this is not too different from the recently added ScalaCtags https://github.com/scalameta/language-server/blob/8bbef8eb20e4cef9c8586cecf33b0093a09c6905/metaserver/src/main/scala/scala/meta/languageserver/ctags/ScalaCtags.scala

The ctags indexers will return a more useful schema after #35 which will probably make it more convenient to re-use for features like outline.

However, this PR is immediately useful and the feature looks really cool! I will keep my eyes open for sharing the implementation between ctags and outline if possible later down the road, but we should not let it block us now.

LGTM 👍 Nice work @laughedelic ! Feel free to merge.

olafurpg · 2017-11-15T21:08:20Z

metaserver/src/main/scala/scala/meta/languageserver/ScalametaLanguageServer.scala

+    val contents = buffers.read(path)
+    for {
+      tree <- contents.parse[Source].toOption.toList
+      node <- tree.collect {


FYI scala.meta.contrib._ adds a .filter on Tree

Thanks, I was looking for it but found after I wrote this and then forgot to change it.

laughedelic · 2017-11-15T21:34:48Z

@olafurpg thanks for the review, but please don't merge it yet. I tested it today with some real projects and found a couple of corner-cases which should be handled more carefully.
I will also take a closer at the ctags code.

gabro · 2017-11-19T05:31:09Z

@laughedelic what do you think about merging this and addressing the corner cases in a separate issue/PR? The more this PR stays open the more conflicts we risk to introduce.

olafurpg · 2017-11-19T11:43:58Z

I agree we should merge this to avoid conflicts with #37 👍

laughedelic · 2017-11-19T16:31:07Z

OK, I just pushed a couple of little fixes and I'm going to merge it. I'll submit another PR later with other improvements.

olafurpg

🎉

olafurpg · 2017-11-19T16:32:25Z

metaserver/src/main/scala/scala/meta/languageserver/ScalametaEnrichments.scala

-      case Defn.Object(_) => SymbolKind.Namespace
-      case Pkg.Object(_)  => SymbolKind.Module
+      case Defn.Object(_) => SymbolKind.Module
+      case Pkg.Object(_)  => SymbolKind.Namespace


Strictly speaking, package object are modules like regular objects. But having a different icon in the UI to distinguish package objects from objects makes sense.

Yep, that's what I thought

olafurpg · 2017-11-19T16:43:24Z

Nice! This PR fixes one of the TODOs in #37 😄

laughedelic · 2017-11-19T16:48:55Z

I didn't have time yet to check all the big changes in #37, but I just noticed that merging this introduced some merge conflicts there

olafurpg · 2017-11-19T16:50:59Z

I've fixed those, the conflicts were only related to document symbol provider which I had commented out

gabro · 2017-11-20T14:00:15Z

yay! Nice job @laughedelic!

laughedelic self-assigned this Nov 12, 2017

olafurpg reviewed Nov 12, 2017

View reviewed changes

laughedelic mentioned this pull request Nov 14, 2017

Nested symbols hierarchy facebookarchive/atom-ide-ui#121

Closed

laughedelic added 3 commits November 14, 2017 22:10

Changed SymbolIndexer.documentSymbols to return SymbolInformation

795e819

Its containerName field should be the symbol.owner's name, not its type

Naive implementation of symbol definition lookup

ec714db

It searches a Member node in the parsed tree that has the same name and same startLine. This widens position range to the symbol definition, which is necessary for correct symbols hierarchy nesting.

documentSymbols implementation without SymbolIndexer

d471baa

laughedelic force-pushed the symbols-hierarchy branch from 379a735 to d471baa Compare November 15, 2017 04:01

Added conversion of Tree nodes to SymbolKind

e866db9

laughedelic added 2 commits November 15, 2017 06:51

Minor comments

616bc5b

Removed WIP artifacts

77e9b50

olafurpg reviewed Nov 15, 2017

View reviewed changes

olafurpg approved these changes Nov 15, 2017

View reviewed changes

laughedelic added 2 commits November 16, 2017 01:16

Fixed package/package object symbol kinds

1a8eb9c

Match funtion kind only by val/var type, not rhs

d0b84c8

A little fix for determining parent member tree node

f22b970

laughedelic merged commit 641dfb8 into scalameta:master Nov 19, 2017

laughedelic deleted the symbols-hierarchy branch November 19, 2017 16:32

olafurpg reviewed Nov 19, 2017

View reviewed changes

olafurpg mentioned this pull request Nov 20, 2017

Symbols hierarchy #33

Closed

laughedelic mentioned this pull request Dec 4, 2017

Symbols outline rework #93

Merged

Fixing symbols hierarchy #34

Fixing symbols hierarchy #34

Conversation

laughedelic commented Nov 12, 2017

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laughedelic Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

laughedelic commented Nov 12, 2017

laughedelic commented Nov 14, 2017

laughedelic commented Nov 14, 2017

olafurpg commented Nov 14, 2017

laughedelic commented Nov 14, 2017

gabro commented Nov 14, 2017

gabro commented Nov 14, 2017

laughedelic commented Nov 14, 2017

gabro commented Nov 14, 2017

olafurpg commented Nov 14, 2017

laughedelic commented Nov 14, 2017

olafurpg commented Nov 14, 2017 • edited Loading

laughedelic commented Nov 14, 2017

olafurpg commented Nov 14, 2017

laughedelic commented Nov 15, 2017 • edited Loading

gabro commented Nov 15, 2017

laughedelic commented Nov 15, 2017

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laughedelic Nov 15, 2017 • 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

olafurpg commented Nov 15, 2017

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laughedelic commented Nov 15, 2017 • edited Loading

gabro commented Nov 19, 2017

olafurpg commented Nov 19, 2017

laughedelic commented Nov 19, 2017

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg commented Nov 19, 2017

laughedelic commented Nov 19, 2017

olafurpg commented Nov 19, 2017

gabro commented Nov 20, 2017

laughedelic Nov 12, 2017 •

edited

Loading

olafurpg commented Nov 14, 2017 •

edited

Loading

laughedelic commented Nov 15, 2017 •

edited

Loading

laughedelic Nov 15, 2017 •

edited

Loading

laughedelic commented Nov 15, 2017 •

edited

Loading