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

Conversation

@tanishiking
Copy link
Member

tanishiking commented Dec 9, 2019

https://github.com/scalameta/metals-feature-requests/issues/49

This PR enables metals to sort auto-completed exhaustive match cases by declaration order, instead of the order of Symbol#knownDirectSubclasses

  • if symbols of subclasses' position information are not available, load all the symbols of the file that contains the definition of the symbol (in declaration order), and sort subclasses by those symbols.
  • Otherwise, sort cases based on the position information without loading symbols.
    • This case happens if the definition of abstract sealed class ... is located in the same file with the target match ... statement.
    • In that case, we can sort subclasses quite cheaply.

Here's a test of match exhaustive completion for scala.meta.internal.docstrings.Block

/** A block-level element of text, such as a paragraph or code block. */
sealed abstract class Block
final case class Title(text: Inline, level: Int) extends Block
final case class Paragraph(text: Inline) extends Block
final case class Code(data: String) extends Block
final case class UnorderedList(items: Seq[Block]) extends Block
final case class OrderedList(items: Seq[Block], style: String) extends Block
final case class DefinitionList(items: SortedMap[Inline, Block]) extends Block
final case class HorizontalRule() extends Block
final case class Table(
header: Row,
columnOptions: Seq[ColumnOption],
rows: Seq[Row]
) extends Block

match


Thank you for giving a kind instruction for implementing this @olafurpg ! It's insightful and made me more comfortable to read through the metals' codebase :) https://github.com/scalameta/metals-feature-requests/issues/49#issuecomment-544236184

https://github.com/scalameta/metals-feature-requests/issues/49

This commit enable metals to sort auto-completed exhaustive match'es by
declaration order.
However, it caluculates their declaration positions from the infromation
retrieved from interactive compiler, and we cannot sort them if the
interactive compiler hasn't yet loaded the symbols.
* The content of the virtual file.
*/
String value();
}

This comment has been minimized.

Copy link
@tanishiking

tanishiking Dec 9, 2019

Author Member

Java version of Input.VirtualFile for defining a method that returns Input.VirtualFile equivalent information in SymbolSearch.java.

@tanishiking tanishiking force-pushed the tanishiking:sort-match-auto-completion branch from 924e398 to de9eb2c Dec 9, 2019
This commit enable metals to sort completed exhaustive match'es by
declaration order without stale: read the whole symbols of TextDocument
that contains the definition of the given symbol.
@tanishiking tanishiking force-pushed the tanishiking:sort-match-auto-completion branch from de9eb2c to 7b22279 Dec 9, 2019
Copy link
Member

gabro left a comment

Thank you for working on this @tanishiking!

I've given a cursory review and left a few minor comments.

I'm not super familiar with these parts of the codebase, so I'll let @olafurpg or @tgodzik review this more thoroughly.

Also, I've moved the referenced issue from the feature request repo to the main issue tracker, so that it's clear it's beeing worked on.

@@ -103,6 +103,33 @@ object CompletionMatchSuite extends BaseCompletionSuite {
filter = _.contains("exhaustive")
)

checkEdit(
"Sort auto-completed exhaustive match keywords by declaration order",

This comment has been minimized.

Copy link
@gabro

gabro Dec 9, 2019

Member

we generally use short meaningful names for the tests (e.g. exhaustive-sorting), so that they can be used as targets for testOnly.

case None =>
ju.Optional.empty()
case Some(value) =>
import java.nio.file.Files

This comment has been minimized.

Copy link
@gabro

gabro Dec 9, 2019

Member

nit: move this in the toplevel import block, for consistency

import scala.meta.pc.VirtualFile
import scala.meta.inputs.Input

case class MetalsVirtualFile(

This comment has been minimized.

Copy link
@gabro

gabro Dec 9, 2019

Member

I believe we use the Impl suffix for similar instances, so this would be VirtualFileImpl, for consistency.

path: String,
value: String
) extends VirtualFile
object MetalsVirtualFile {

This comment has been minimized.

Copy link
@gabro

gabro Dec 9, 2019

Member

nit: add a newline in between class and object

@gabro gabro requested review from olafurpg and tgodzik Dec 9, 2019
@tanishiking

This comment has been minimized.

Copy link
Member Author

tanishiking commented Dec 10, 2019

Thank you for taking a look at this @gabro ! I just added some commits based on your feedback :)

Copy link
Member

olafurpg left a comment

Thank you for this contribution! Great job stitching all the pieces together @tanishiking 👏

I have only one concern with performance, it would be nice to cache the Mtags.toplevels method call between completions.

/**
* Returns the file where the symbol is defined, if any.
*/
Optional<VirtualFile> definitionSource(String symbol);

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 13, 2019

Member

I'm wondering if we should change this API to directly return the toplevels 🤔

Suggested change
Optional<VirtualFile> definitionSource(String symbol);
List<String> definitionSourceToplevels(String symbol);

This would allow the Metals server to potentially cache the results between different invocations. The Mtags.toplevels method is on the other hand very fast (~900k lines/sec) I have a feeling that a lot of match (exhaustive) completions are on the same types (for example scala/Option#).

This comment has been minimized.

Copy link
@tanishiking

tanishiking Dec 14, 2019

Author Member

I'm wondering if we should change this API to directly return the toplevels 🤔

Sure! That makes more sense, and now we can remove the java version of VirtualFile and VirtualFileImpl 😄

This comment has been minimized.

Copy link
@tanishiking

tanishiking Dec 14, 2019

Author Member

Thank you so much for your input @olafurpg !

I just added a straightforward implementation of the cache for definitionSourceToplevels in a70886e

Though it definitely makes sense to introduce cache for definitionSourceToplevels 👍, I have one concern about the cache invalidation:
In a70886e the cache in MetalsSymbolSearch will be cleared only when the workspace is updated.
If we add some new subclasses to a sealed abstract class X, metals server would be unable to reflect the changes until the workspace is updated.


To avoid this, it might be better to do:

  • cache definitionSourceToplevels result by the symbol of definition's (absolute) path
    • private val cache = new TrieMap[AbsolutePath, ju.List[String]]()
  • Everytime the project is compiled, clear the cache for definitionSourceToplevels at Compilers#didCompile before restart the PresentationCompiler instance.
    • so the metals server can reflect the change to the completion items.
    • to do this, we have to add cache cleanup method to SymbolSearch
  • The cache items we have to clear are the items whose key (AbsolutePath) is located under the project's workspace.
    • We don't need to clear the symbol cache from the 3rd party definitions, because they won't change without updating the workspace.

What do you think about this?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 14, 2019

Member

Cache invalidation is a good question. Can we maybe reuse the toplevels indexing for WorkspaceSymbolProvider? That index is already updated on file save.

@@ -103,6 +103,33 @@ object CompletionMatchSuite extends BaseCompletionSuite {
filter = _.contains("exhaustive")
)

checkEdit(
"exhaustive-sorting",

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 13, 2019

Member

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

@@ -125,4 +160,60 @@ abstract class BaseCompletionLspSuite(name: String) extends BaseLspSuite(name) {
)
} yield ()
}

def matchKeywordTest(scalaVersion: String): Future[Unit] = {

This comment has been minimized.

Copy link
@olafurpg
@tanishiking tanishiking force-pushed the tanishiking:sort-match-auto-completion branch from c67ac37 to ad787b0 Dec 14, 2019
Copy link
Member

olafurpg left a comment

A few more comments, this PR is getting very close to ready!

* Returns VirtualFile that contains the definition of
* the given symbol (of semanticdb).
*/
private[internal] def definitionPathInputFromSymbol(

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 15, 2019

Member

nit: there's no need to write private[internal], the package is already named "internal"

@@ -16,13 +18,27 @@ class MetalsSymbolSearch(
wsp: WorkspaceSymbolProvider,
defn: DefinitionProvider
) extends SymbolSearch {
// cache for definitionSourceTopLevels
private val cache = new TrieMap[String, ju.List[String]]()

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 15, 2019

Member

We can reuse WorkspaceSymbolProvider.inWorkspace to avoid caching workspace toplevels, see

val inWorkspace: TrieMap[Path, WorkspaceSymbolsIndex] =

For library dependency sources, we can keep this cache but it should be cleared during re-indexing

This comment has been minimized.

Copy link
@tanishiking

tanishiking Dec 15, 2019

Author Member

done in a68027a and now the completion seems much faster (especially for workspace sources)!

if (defnSymbols.length > 0)
subclassesResult
.sortBy(sym => {
defnSymbols.indexOf(semanticdbSymbol(sym))

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 15, 2019

Member

To insure ourselves against bad performance in pathological cases, we can cache the result of indexOf like this

@ List("z", "a", "b").iterator.zipWithIndex.toMap
res1: Map[String, Int] = Map("z" -> 0, "a" -> 1, "b" -> 2)
@ List("b", "z").sortBy(res1.get)
res3: List[String] = List("z", "b")

indexOf is linear and the callback to sortBy gets repeatedly called for each element.

This comment has been minimized.

Copy link
@tanishiking

tanishiking Dec 15, 2019

Author Member

Thanks! 👍 707a24f

"""package sort
|object App {
| Option(1) match {
|\tcase Some(value) => $0

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 15, 2019

Member

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

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Dec 15, 2019

Feel free to ignore the unrelated test failure, I'm hoping #1186 fixes that

tanishiking added 3 commits Dec 15, 2019
Since the cache for AbsolutePath -> SymbolInformation in WorkspaceSymbolProvider
is updated on file save, definitionSourceToplevels can reflect the file
changes while caching the symbols.
@@ -1597,6 +1599,7 @@ class MetalsLanguageServer(
semanticDBIndexer.reset()
treeView.reset()
worksheetProvider.reset()
symbolSearch.reset()

This comment has been minimized.

Copy link
@tanishiking

tanishiking Dec 15, 2019

Author Member

Clear symbolSearch cache (for dependency sources) on indexing workspace

Copy link
Member

olafurpg left a comment

LGTM 👍 Thank you for addressing all comments @tanishiking ! I'm looking forward to use this feature 😄

Copy link
Collaborator

tgodzik left a comment

Just one comment from me, otherwise looks good to go.

|}
|}
|""".stripMargin
else

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 16, 2019

Collaborator

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

This comment has been minimized.

Copy link
@tanishiking

tanishiking Dec 16, 2019

Author Member

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

Copy link
Collaborator

tgodzik left a comment

Thanks @tanishiking Looks awesome!

@olafurpg olafurpg changed the title Sort auto-completed exhaustive match'es by declaration order Sort auto-completed exhaustive match'es by declaration order Dec 16, 2019
@olafurpg olafurpg merged commit 902ca7b into scalameta:master Dec 16, 2019
11 checks passed
11 checks passed
ubuntu-latest unit tests
Details
windows-latest unit tests
Details
macOS-latest unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Pants integration
Details
LSP integration tests
Details
Scala cross tests
Details
Scalafmt/Scalafix/Docs
Details
@tanishiking tanishiking deleted the tanishiking:sort-match-auto-completion branch Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.