Custom data structure for the index about metals

[olafurpg]

@laughedelic I'm not sure if it's worth sharing code between outline and scala mtags since abstracting over things will likely hurt readability. The skeleton for an outline traverser is not that big

class OutlineTraverser extends Traverser {
  override def apply(tree: Tree): Unit = {
    def continue(): Unit = super.apply(tree)
    def todo(): Unit = ???
    tree match {
      case t: Source => continue()
      case t: Template => continue()
      case t: Pkg => todo(); continue()
      case t: Pkg.Object => todo(); continue()
      case t: Defn.Class => todo(); continue()
      case t: Defn.Trait => todo(); continue()
      case t: Defn.Object => todo(); continue()
      case t: Defn.Def => todo()
      case t: Decl.Def => todo()
      case Defn.Val(_, Pat.Var(name) :: Nil, _, _) => todo()
      case Decl.Val(_, Pat.Var(name) :: Nil, _) => todo()
      case Defn.Var(_, Pat.Var(name) :: Nil, _, _) => todo()
      case Decl.Var(_, Pat.Var(name) :: Nil, _) => todo()
      case _ => ()
    }
  }
}
new OutlineTraverser.apply(tree)

[laughedelic]

If mtags stored the range position of the whole definition then "go to definition" anywhere in the definition body will jump to the name, which is a bit weird.

I didn't get this. go-to-definition in the definition body itself? I think go-to-definition should be triggered only on symbols, not on arbitrary blocks of code.

[olafurpg]

I think we can actually share code without changing the output data structure by writing a custom Traverser, I can give this a swing.

[laughedelic]

@olafurpg I'm fine with it. This is how I was going to do it, but then I thought that we're traversing in both cases for the same symbols and doing it for outline could be updating the index "by the way".

Anyway, if you think it's not worth it now, feel free to close this. I'm going to rework outline with the traverser later.

[olafurpg]

When I think about it, we can probably use the end position of the enclosing trait/class/object but start position of the name. Go to definition jumps to the exact line+column so it would be weird if that jumps to the keyword class, but the end position is ignored I think.

[ShaneDelmore]

I have use for an accurate end position. Quick look definition (Without going to it) is one of my favorite features. I would prefer to augment the data structures as needed vs make them inaccurate.

[laughedelic]

@olafurpg one reason why such range wouldn't be good is the definition preview mentioned by @ShaneDelmore, although I think it shows complete lines, so most of the time it won't be noticeable), but another thing which is definitely noticeable is the highlighting in the interface.

For example in VSCode outline: when you select a symbol in the outline, the whole corresponding range gets highlighted in the editor:

In the LSP repo there is a separate long discussion on this exact topic: microsoft/language-server-protocol#119

TL;DR summary of that thread: microsoft/language-server-protocol#119 (comment)

[ShaneDelmore]

This discussion really gets to the differences between LSP and Kythe. Thanks for the link.

[laughedelic]

@ShaneDelmore See also quoted/linked comments here: facebookarchive/atom-ide-ui#121 (comment)

[olafurpg]

@laughedelic is this fixed with #93 ?

[laughedelic]

#93 addressed the problem from another angle and I guess we just don't need this at the moment. Feel free to close it.

P.S. Now I remember that we also started a discussion here about storing position range for the whole definition vs. only name and didn't come to any conclusion..

[olafurpg]

P.S. Now I remember that we also started a discussion here about storing position range for the whole definition vs. only name and didn't come to any conclusion..

I think the conclusion is that mtags stores only position of the name and outline stores whole range of definition. This is how semanticdb expects things to be at least. If mtags stored the range position of the whole definition then "go to definition" anywhere in the definition body will jump to the name, which is a bit weird.

[olafurpg]

Go to definition is implemented on top of semanticdb. If semanticdb says the position of Foo in class Foo { def bar = 2 } encloses the full template { def bar = 2 } then that means clicking anywhere inside that template will go to definition on Foo. My general point is that outline != mtags since they have different needs. I think the current solution that we have is good.

[laughedelic]

My general point is that outline != mtags since they have different needs. I think the current solution that we have is good.

I get this point and I think that current solution is fine, but I just want to clarify this:

If semanticdb says the position of Foo in class Foo { def bar = 2 } encloses the full template { def bar = 2 } then that means clicking anywhere inside that template will go to definition on Foo

I don't think so. The only "clickable" place in this { ... } block is the bar identifier (which should lead to itself). When the server handles textDocument/definition request, it checks symbol at the given cursor position to lookup its information. If you call textDocument/definition on any empty space or keyword inside of a code block nothing will happen, server will reply with an empty response. Is this correct or are we talking about different things?

[olafurpg]

Is this correct or are we talking about different things?

We are probably talking about different things. I'm referring to how the positions of definitions in mtags influence the implementation of resolveName https://github.com/scalameta/language-server/blob/40b36722340b529c1dafd11c5af62a8ef73772c4/metaserver/src/main/scala/scala/meta/languageserver/search/SymbolIndex.scala#L64-L71 If mtags used the full position of the class then we could click anywhere in {...} to jump to Foo.

[laughedelic]

Ah! Now I see! Of course, ResolvedName should store the position of the name not the whole definition. It makes sense.

So then a question about Scalameta: would it make sense to add definition range to ResolvedName? i.e. instead of isDefinition: Boolean store definition: Option[Position].

[olafurpg]

So then a question about Scalameta: would it make sense to add definition range to ResolvedName? i.e. instead of isDefinition: Boolean store definition: Option[Position].

That information is already persisted as part of Document.contents since it can be inferred from the parsed AST. isDefinition is also redundant for the same reason, but we added it since we found it made indexing a whole lot simpler. I'm not sure we'll get the same value for definition: Option[Position].

[laughedelic]

I see, thanks for explanation