Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort auto-completed exhaustive match'es by declaration order #1174

Merged
merged 12 commits into from Dec 16, 2019
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -21,6 +21,9 @@ class ClasspathOnlySymbolSearch(classpath: ClasspathSearch)

def definition(symbol: String): ju.List[Location] = ju.Collections.emptyList()

override def definitionSourceToplevels(symbol: String): ju.List[String] =
ju.Collections.emptyList()

override def search(
query: String,
buildTargetIdentifier: String,
Expand Down
Expand Up @@ -73,6 +73,17 @@ final class DefinitionProvider(
case Some(destination) => destination.locations
}

/**
* Returns VirtualFile that contains the definition of
* the given symbol (of semanticdb).
*/
def definitionPathInputFromSymbol(
sym: String
): Option[Input.VirtualFile] =
index
.definition(Symbol(sym))
.map(symDef => symDef.path.toInputFromBuffers(buffers))

def symbolOccurence(
source: AbsolutePath,
dirtyPosition: TextDocumentPositionParams
Expand Down
Expand Up @@ -173,6 +173,7 @@ class MetalsLanguageServer(
private var foldingRangeProvider: FoldingRangeProvider = _
private val packageProvider: PackageProvider =
new PackageProvider(buildTargets)
private var symbolSearch: MetalsSymbolSearch = _
private var compilers: Compilers = _
var tables: Tables = _
var statusBar: StatusBar = _
Expand Down Expand Up @@ -386,18 +387,19 @@ class MetalsLanguageServer(
interactiveSemanticdbs.toFileOnDisk
)
foldingRangeProvider = FoldingRangeProvider(trees, buffers, params)
symbolSearch = new MetalsSymbolSearch(
symbolDocs,
workspaceSymbols,
definitionProvider
)
compilers = register(
new Compilers(
workspace,
config,
() => userConfig,
buildTargets,
buffers,
new MetalsSymbolSearch(
symbolDocs,
workspaceSymbols,
definitionProvider
),
symbolSearch,
embedded,
statusBar,
sh,
Expand Down Expand Up @@ -1597,6 +1599,7 @@ class MetalsLanguageServer(
semanticDBIndexer.reset()
treeView.reset()
worksheetProvider.reset()
symbolSearch.reset()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear symbolSearch cache (for dependency sources) on indexing workspace

buildTargets.addWorkspaceBuildTargets(i.workspaceBuildTargets)
buildTargets.addScalacOptions(i.scalacOptions)
for {
Expand Down
Expand Up @@ -3,10 +3,14 @@ package scala.meta.internal.metals
import java.{util => ju}
import org.eclipse.lsp4j.Location
import java.util.Optional
import scala.collection.concurrent.TrieMap
import ch.epfl.scala.bsp4j.BuildTargetIdentifier
import scala.meta.pc.SymbolDocumentation
import scala.meta.pc.SymbolSearch
import scala.meta.pc.SymbolSearchVisitor
import scala.meta.internal.mtags.Mtags
import scala.meta.io.AbsolutePath
import scala.meta.internal.metals.MetalsEnrichments._

/**
* Implementation of SymbolSearch that delegates to WorkspaceSymbolProvider and SymbolDocumentationIndexer.
Expand All @@ -16,13 +20,61 @@ class MetalsSymbolSearch(
wsp: WorkspaceSymbolProvider,
defn: DefinitionProvider
) extends SymbolSearch {
// A cache for definitionSourceToplevels.
// The key is an absolutepath to the dependency source file, and
// the value is the list of symbols that the file contains.
private val dependencySourceCache =
new TrieMap[AbsolutePath, ju.List[String]]()

def reset(): Unit = {
dependencySourceCache.clear()
}

override def documentation(symbol: String): Optional[SymbolDocumentation] =
docs.documentation(symbol)

def definition(symbol: String): ju.List[Location] = {
defn.fromSymbol(symbol)
}

/**
* Returns a list of semanticdb symbols in a source file that contains the
* definition of the given symbol.
*/
override def definitionSourceToplevels(symbol: String): ju.List[String] = {
defn
.definitionPathInputFromSymbol(symbol)
.map(input => {
val path = AbsolutePath(input.path)
if (path.isWorkspaceSource(wsp.workspace)) {
// If the source file is a workspace source, retrieve its symbols from
// WorkspaceSymbolProvider so that metals server can reuse its cache.
wsp.inWorkspace
.get(path.toNIO)
.map(symInfo => {
symInfo.symbols
.sortBy(sym =>
(
sym.range.getStart().getLine(),
sym.range.getStart().getCharacter()
)
)
.map(_.symbol)
.asJava
})
.getOrElse(
ju.Collections.emptyList[String]()
)
} else {
dependencySourceCache.getOrElseUpdate(
path,
Mtags.toplevels(input).asJava
)
}
})
.getOrElse(ju.Collections.emptyList())
}

override def search(
query: String,
buildTargetIdentifier: String,
Expand Down
Expand Up @@ -19,6 +19,12 @@ public interface SymbolSearch {
*/
List<Location> definition(String symbol);

/**
* Returns the all symbols in the file where the given symbol is defined
* in declaration order, if any.
*/
List<String> definitionSourceToplevels(String symbol);

/**
* Runs fuzzy symbol search for the given query.
*
Expand Down
34 changes: 34 additions & 0 deletions mtags/src/main/scala/scala/meta/internal/pc/Completions.scala
Expand Up @@ -1297,7 +1297,41 @@ trait Completions { this: MetalsGlobal =>
val members = ListBuffer.empty[TextEditMember]
val importPos = autoImportPosition(pos, text)
val context = doLocateImportContext(pos, importPos)
val subclasses = ListBuffer.empty[Symbol]

tpe.typeSymbol.foreachKnownDirectSubClass { sym =>
subclasses += sym
}
val subclassesResult = subclasses.result()

// sort subclasses by declaration order
// see: https://github.com/scalameta/metals-feature-requests/issues/49
val sortedSubclasses =
if (subclassesResult.forall(_.pos.isDefined)) {
// if all the symbols of subclasses' position is defined
// we can sort those symbols by declaration order
// based on their position information quite cheaply
subclassesResult.sortBy(subclass =>
(subclass.pos.line, subclass.pos.column)
)
} else {
// Read all the symbols in the source that contains
// the definition of the symbol in declaration order
val defnSymbols = search
.definitionSourceToplevels(semanticdbSymbol(tpe.typeSymbol))
.asScala
if (defnSymbols.length > 0) {
val symbolIdx = defnSymbols.zipWithIndex.toMap
subclassesResult
.sortBy(sym => {
symbolIdx.getOrElse(semanticdbSymbol(sym), -1)
})
} else {
subclassesResult
}
}

sortedSubclasses.foreach { sym =>
val (shortName, edits) =
importPos match {
case Some(value) =>
Expand Down
Expand Up @@ -20,6 +20,10 @@ object EmptySymbolSearch extends SymbolSearch {
ju.Collections.emptyList()
}

override def definitionSourceToplevels(symbol: String): ju.List[String] = {
ju.Collections.emptyList()
}

override def documentation(symbol: String): Optional[SymbolDocumentation] =
Optional.empty()
}
61 changes: 61 additions & 0 deletions tests/cross/src/test/scala/tests/pc/CompletionMatchSuite.scala
Expand Up @@ -3,6 +3,10 @@ package tests.pc
import tests.BaseCompletionSuite

object CompletionMatchSuite extends BaseCompletionSuite {
override def beforeAll(): Unit = {
indexScalaLibrary()
}

check(
"match",
"""
Expand Down Expand Up @@ -103,6 +107,63 @@ object CompletionMatchSuite extends BaseCompletionSuite {
filter = _.contains("exhaustive")
)

checkEdit(
"exhaustive-sorting",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add another cross test case for 3rd party defined symbols? Cross tests have the ability to index library dependencies, see indexScalaLibrary()

"""package sort
|sealed abstract class TestTree
|case class Branch1(t1: TestTree) extends TestTree
|case class Leaf(v: Int) extends TestTree
|case class Branch2(t1: TestTree, t2: TestTree) extends TestTree
|object App {
| null.asInstanceOf[TestTree] matc@@
|}
|""".stripMargin,
"""|package sort
|sealed abstract class TestTree
|case class Branch1(t1: TestTree) extends TestTree
|case class Leaf(v: Int) extends TestTree
|case class Branch2(t1: TestTree, t2: TestTree) extends TestTree
|object App {
| null.asInstanceOf[TestTree] match {
|\tcase Branch1(t1) => $0
|\tcase Leaf(v) =>
|\tcase Branch2(t1, t2) =>
|}
|}
|""".stripMargin,
filter = _.contains("exhaustive")
)

if (!isScala211) {
checkEdit(
"exhaustive-sorting-scalalib",
"""package sort
|object App {
| Option(1) matc@@
|}
|""".stripMargin,
if (!isScala211)
"""package sort
|object App {
| Option(1) match {
|\tcase Some(value) => $0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, really excited to finally get the Some case at the top

|\tcase None =>
|}
|}
|""".stripMargin
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the else here? !isScala211 is already tested at the top

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, we don't need it. I meant like this 0d5f96f, thank you for pointing it out :)

"""package sort
|object App {
| Option(1) match {
|\tcase Some(x) => $0
|\tcase None =>
|}
|}
|""".stripMargin,
filter = _.contains("exhaustive")
)
}

check(
"inner-class",
"""
Expand Down
19 changes: 19 additions & 0 deletions tests/mtest/src/main/scala/tests/TestingSymbolSearch.scala
Expand Up @@ -3,6 +3,8 @@ package tests
import java.{util => ju}
import org.eclipse.lsp4j.Location
import java.util.Optional
import java.nio.file.Files
import scala.meta.inputs.Input
import scala.meta.internal.metals.ClasspathSearch
import scala.meta.internal.metals.Docstrings
import scala.meta.internal.metals.WorkspaceSymbolQuery
Expand All @@ -12,6 +14,7 @@ import scala.meta.pc.SymbolSearchVisitor
import scala.meta.internal.mtags.OnDemandSymbolIndex
import scala.meta.internal.mtags.GlobalSymbolIndex
import scala.meta.internal.mtags.Symbol
import scala.meta.internal.mtags.Mtags

/**
* Implementation of `SymbolSearch` for testing purposes.
Expand Down Expand Up @@ -46,6 +49,22 @@ class TestingSymbolSearch(
}
}

override def definitionSourceToplevels(symbol: String): ju.List[String] = {
index.definition(Symbol(symbol)) match {
case None =>
ju.Collections.emptyList()
case Some(value) =>
import scala.collection.JavaConverters._
val filename = value.path.toNIO.getFileName().toString()
val content = new String(Files.readAllBytes(value.path.toNIO))
val input = Input.VirtualFile(
filename,
content
)
Mtags.toplevels(input).asJava
}
}

override def search(
textQuery: String,
buildTargetIdentifier: String,
Expand Down
Expand Up @@ -11,4 +11,10 @@ object CompletionCrossLspSuite
testAsync("basic-213") {
basicTest(V.scala213)
}
testAsync("match-211") {
matchKeywordTest(V.scala213)
}
testAsync("match-213") {
matchKeywordTest(V.scala213)
}
}