Comments (18)
@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)
from metals.
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.
from metals.
I think we can actually share code without changing the output data structure by writing a custom Traverser, I can give this a swing.
from metals.
@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.
from metals.
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.
from metals.
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.
from metals.
@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)
from metals.
This discussion really gets to the differences between LSP and Kythe. Thanks for the link.
from metals.
@ShaneDelmore See also quoted/linked comments here: facebookarchive/atom-ide-ui#121 (comment)
from metals.
@laughedelic is this fixed with #93 ?
from metals.
#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..
from metals.
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.
from metals.
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.
from metals.
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
inclass Foo { def bar = 2 }
encloses the full template{ def bar = 2 }
then that means clicking anywhere inside that template will go to definition onFoo
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?
from metals.
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
.
from metals.
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]
.
from metals.
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]
.
from metals.
I see, thanks for explanation
from metals.
Related Issues (20)
- "Implement missing members" bad placements in braceless syntax
- '#' character in millw.bat script causes error HOT 1
- Formatting (i.e. using Scalafmt) is unnecessarily slow HOT 1
- Indexing runs forever in VSCode probably caused by an unexpected symbol HOT 9
- Auto-import is not suggested for classes with long name HOT 4
- Extract selection as method doesn't infer correct return type HOT 1
- Auto-import is not working for classes with the same name
- Class renaming is broken when you have other tab opened with import
- False error when splicing a polymorphic function HOT 1
- Memory leak - PC + scala 2 HOT 13
- "could not find main class" error when assigning a key to "run" lense for a main class HOT 13
- Type information errors when classloading in macros with `Selectable` HOT 4
- Metals starts a bsp server for project ref, when added as the first workspace folder in VSCode
- Worksheet doesn't show correct Array Output HOT 6
- Worksheet output of previous lines is wrongly updated HOT 1
- Сonstant Scalafix errors HOT 8
- Java source indexing looks to indexing has innefficient file I/O HOT 7
- Metals creates a `.semanticdb` file when opening a single Scala file in VS Cdoe
- OOM while running tests HOT 3
- java.nio.file.NoSuchFileException for a path with @ HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from metals.