Migrate to new symbol index schema #37

olafurpg · 2017-11-18T16:18:59Z

This PR migrates to the persistable indexing schema explained in #35. It does not yet take advantage of the fact that we can persist the symbol index and load it on startup, we leave that for another PR. The major improvement is related to testing.

Testing

While doing the refactoring, I took the liberty to create a proper test fixture to spin up the server, and programmatically invoke "go to definition" requests. Tests are now written using utest. This makes it possible to assert key features including workspace semanticdb indexing and classpath ctags indexing (for both java and scala sources). Setting up the test fixture required making it easy to stub out several services and selectively disable certain features:

ServerConfig: bag of options to disable jdk indexing, classpath indexing or scalafmt classloading
Notifications: make it possible to programmatically load a SymbolIndex without an lsp server.
Formatter.empty: formatter that doesn't do anything
DocumentStore: abstract key/value store for URI -> Document, default implementation is in-memory but should be easy to create alternative store that persists to disk (e.g., sqlite).
SymbolIndexerMap: in-memory store over the new SymbolIndex protobuf message. Has methods to incrementally register definitions and references. Should come up with a better name.

While setting up the test infrastructure, I moved the test-workspace build into the main build so that edits in UserTest.scala trigger semanticdb recompilation and tests in metaserver to re-run.

Thanks to Monix TestScheduler, we have fine grained control over the execution context and can manually trigger all the indexing mechanism that normally runs asynchronously to run synchronously before initializing our tests.

gabro

Impressive work @olafurpg. I think the refactor is very worth it and it's also a step towards #6.

I would rather see this merged sooner than later, since it's already a bit long to review and incremental changes are going to be much easier to review.

I left some non-blocking comments :-)

gabro · 2017-11-19T04:55:31Z

metaserver/src/main/scala/org/langmeta/languageserver/InputEnrichments.scala


 object InputEnrichments {
  implicit class XtensionInputOffset(val input: Input) extends AnyVal {
+    def fromIndexRange(range: i.Range): Position = {


maybe toPosition for consistency?

gabro · 2017-11-19T04:57:44Z

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

@@ -10,6 +10,13 @@ abstract class Formatter {
  def format(code: String, configFile: String, filename: String): String
 }
 object Formatter extends LazyLogging {
+  def empty: Formatter = new Formatter {


micro-optimization, but this could be a SAM

def empty: Formatter = (code, _, _) => code

also maybe noop instead of empty?

Renamed to noop, realized there is actually another method to format using the default config so I added that one instead of converting to SAM. To double check it worked as expected I added FormatterTest

gabro · 2017-11-19T04:59:30Z

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

@@ -15,6 +15,7 @@ object Main extends LazyLogging {
    val out = new PrintStream(new FileOutputStream(logPath))
    val err = new PrintStream(new FileOutputStream(logPath))
    val cwd = AbsolutePath(workspace)
+    val config = ServerConfig(cwd, indexJDK = true)


isn't indexJDK = true already the default?

It is, removed!

gabro · 2017-11-19T05:05:10Z

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

+
+  def updated(symbol: String)(f: SymbolIndex => SymbolIndex): Unit = {
+    val value = symbols.getOrElseUpdate(symbol, newValue(symbol))
+    value.getAndUpdate(new UnaryOperator[SymbolIndex] {


This too can be a SAM, I think.

I haven't tried, but this may actually be as simple as:

value.getAndUpdate(f)

I can make the SAM conversion happen to the call to updated, however that makes IJ unhappy even if it compiles 😢

Problem seems to be caused by the type parameter.

I think my reluctance to use SAM is caused by fairly bad IJ support (and need to cross-build against 2.11 in other projects). I haven't yet made the switch to LSP+vscode/vim

gabro · 2017-11-19T05:06:52Z

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

+    new AtomicReference(SymbolIndex(symbol = symbol))
+  }
+
+  def index: Traversable[SymbolIndex] = new Traversable[SymbolIndex] {


def index: Traversable[SymbolIndex] = f => symbols.values.foreach(s => f(s.get))

The foreach signature is polymorphic [U](f: SymbolIndex => U) which makes SAM conversion fail 😢

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

metaserver/src/test/scala/tests/MegaSuite.scala

+ * - pretty multiline string diffing
+ * - FunSuite-style test("name") { => fun }
+ */
+class MegaSuite(implicit filename: sourcecode.File) extends TestSuite {


Didn't know what to call it, but it's mega good 😄

gabro · 2017-11-19T05:21:29Z

metaserver/src/test/scala/tests/index/SymbolIndexerTest.scala

+
+    "fallback" - {
+      "<<User>>(...)" - {
+        val term = indexer.findSymbol(path, 3, 17).get


how does this fail in case a symbol is not found?

Maybe it's worth to also check that term is defined?

val term = indexer.findSymbol(path, 3, 17) assert(term.isDefined) assertNoDiff(term.get.symbol, "_root_.a.User#") assert(term.get.definition.isDefined)

Or even a common utility as:

def assertSymbolFound(path: AbsolutePath, line: Int, column: Int, expected: String) = { val term = indexer.findSymbol(path, line, column) assert(term.isDefined, s"Symbol not found at $path:$line:$column") assertNoDiff(term.get.symbol, expected) assert(term.get.definition.isDefined, s"Definition not found for term $term") }

I like your style! Great idea, much cleaner now 💯

Now just need to figure out how to replace those pesky line+column magic numbers with something more readable

olafurpg

Thank you for the review @gabro I admit this PR is way too big, I should have started with the test fixture before migrating to the new schema. Let me try and complete the bullets before merging this, since currently the PR is removing features without any additional improvements.

olafurpg · 2017-11-19T12:09:05Z

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

@@ -10,6 +10,13 @@ abstract class Formatter {
  def format(code: String, configFile: String, filename: String): String
 }
 object Formatter extends LazyLogging {
+  def empty: Formatter = new Formatter {


Renamed to noop, realized there is actually another method to format using the default config so I added that one instead of converting to SAM. To double check it worked as expected I added FormatterTest

olafurpg · 2017-11-19T12:09:29Z

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

@@ -15,6 +15,7 @@ object Main extends LazyLogging {
    val out = new PrintStream(new FileOutputStream(logPath))
    val err = new PrintStream(new FileOutputStream(logPath))
    val cwd = AbsolutePath(workspace)
+    val config = ServerConfig(cwd, indexJDK = true)


It is, removed!

olafurpg · 2017-11-19T12:12:09Z

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


 }

 object SymbolIndexer {
-  val emptyDocument: Document = Document(Input.None, "", Nil, Nil, Nil, Nil)
+  def empty(cwd: AbsolutePath): SymbolIndexer = apply(
+    new Notifications {


Converted to SAM!

olafurpg · 2017-11-19T12:15:46Z

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

+    new AtomicReference(SymbolIndex(symbol = symbol))
+  }
+
+  def index: Traversable[SymbolIndex] = new Traversable[SymbolIndex] {


The foreach signature is polymorphic [U](f: SymbolIndex => U) which makes SAM conversion fail 😢

olafurpg · 2017-11-19T12:21:38Z

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

+
+  def updated(symbol: String)(f: SymbolIndex => SymbolIndex): Unit = {
+    val value = symbols.getOrElseUpdate(symbol, newValue(symbol))
+    value.getAndUpdate(new UnaryOperator[SymbolIndex] {


I can make the SAM conversion happen to the call to updated, however that makes IJ unhappy even if it compiles 😢

Problem seems to be caused by the type parameter.

I think my reluctance to use SAM is caused by fairly bad IJ support (and need to cross-build against 2.11 in other projects). I haven't yet made the switch to LSP+vscode/vim

olafurpg · 2017-11-19T12:22:44Z

metaserver/src/test/scala/tests/MegaSuite.scala

+ * - pretty multiline string diffing
+ * - FunSuite-style test("name") { => fun }
+ */
+class MegaSuite(implicit filename: sourcecode.File) extends TestSuite {


Didn't know what to call it, but it's mega good 😄

olafurpg · 2017-11-19T12:23:13Z

metaserver/src/test/scala/tests/ctags/BaseCtagsTest.scala

+  def check(filename: String, original: String, expected: String): Unit = {
+    test(filename) {
+      val obtained = Database(Ctags.index(filename, original) :: Nil)
+//      println(obtained)


These commented printlns are intentionally left behind, since they're useful when updating the snapshot tests

olafurpg · 2017-11-19T12:36:12Z

metaserver/src/test/scala/tests/index/SymbolIndexerTest.scala

+
+    "fallback" - {
+      "<<User>>(...)" - {
+        val term = indexer.findSymbol(path, 3, 17).get


I like your style! Great idea, much cleaner now 💯

Now just need to figure out how to replace those pesky line+column magic numbers with something more readable

olafurpg · 2017-11-19T12:37:05Z

metaserver/src/test/scala/tests/index/SymbolIndexerTest.scala

+      }
+    }
+
+    "bijection" - {


Moved all of the tricky logic here to an InverseSymbolIndexer utility in main, making this test a lot smaller.

It compiles, but lots of ???.

I didn't like how - the expected output was in a separate file - it was too easy to create huge snapshot files I think the ideal snapshot tests should be small and you should be able to see the expected output alongside the input. This should be possible by using the same trick as Jane Street does with "expect tests", where the test framework creates a sibling source file `X.check`, which is identical to the original test suite `X.scala` except with the multiline strings fixed. If you are happy with the expected output in the check file, you can overwrite the original file with the check file.

- Move everything to _root_.tests package - Remove relics of snapshot testing - Configure logback to not spam in tests - Print out when tests are ignored

This test asserts that we can reconstruct the database from symbol index. This commit removes the test-workspace build in favor of merging it into the main build.

- `test-workspace` now compiles before running `metaserver/test`. - updated contributing guide to reflect this

Previously, I manually created the SymbolIndexer. However, this made it difficult to be sure the tests were testing the actual server. Now, we use the server directly instead.

To speed up the edit/test/debug, use ServerConfig to disable several features like indexing the dependnecy classpath, JDK, or classload scalafmt.

Added a test suite to validate it works as expected, and realized that - we didn't expose the method to format without a config file - the format(String, String, String) signature was unnecessarily weakly typed.

- SAM types ftw - no implicit cwd: AbsolutePath, make it explicit

The IJ formatter will sometimes randomly reorder statements, even if that completely breaks a ton of code.

This separation from interface and implementation can make it easier for us to experiment with different approaches.

We were ignoring everything in the scala.meta.languageserver package.

Previously, exceptions would get lost (not appear in logs) when the server crashed. Now we at least log them so we can debug the problem.

This is the best we got until scalameta#36 gets fixed. I had to adapt the implementation to accommodate the new URI based indexing scheme.

olafurpg · 2017-11-19T17:46:51Z

OK, I've rebased on top of master and made the functionality on par with what we have in master. Jump to definition works for project sources and classpath entries.

I will leave it for another PR to actually persist the index on shutdown and load it on startup.

olafurpg · 2017-11-19T17:47:22Z

I must not do 1k loc PRs
I must not do 1k loc PRs
I must not do 1k loc PRs
I must not do 1k loc PRs
I must not do 1k loc PRs
I must not do 1k loc PRs
I must not do 1k loc PRs
I must not do 1k loc PRs
I must not do 1k loc PRs
I must not do 1k loc PRs
I must not do 1k loc PRs

😅

- missing docstrings - consistent blank lines - up-to-date contributing guide

olafurpg · 2017-11-20T19:51:53Z

Let's merge this PR as is and I am happy to address bugs and regressions in a followup PR

olafurpg changed the title ~~Migrate to new symbol index schema~~ [WIP] Migrate to new symbol index schema Nov 18, 2017

olafurpg force-pushed the snapshot branch from 33326d3 to 92b4e4f Compare November 18, 2017 21:22

gabro reviewed Nov 19, 2017

View reviewed changes

gabro mentioned this pull request Nov 19, 2017

Add a test suite #12

Closed

olafurpg mentioned this pull request Nov 19, 2017

Fixing symbols hierarchy #34

Merged

olafurpg commented Nov 19, 2017

View reviewed changes

olafurpg added 24 commits November 19, 2017 17:38

Move to new index format.

4c1a057

It compiles, but lots of ???.

At least tests pass

d85bd71

Clean up

a25e0b0

Implement go to definition

1618d21

Reorganize tests

e380f47

- Move everything to _root_.tests package - Remove relics of snapshot testing - Configure logback to not spam in tests - Print out when tests are ignored

Implmeent bijection test for symbol index.

8899389

This test asserts that we can reconstruct the database from symbol index. This commit removes the test-workspace build in favor of merging it into the main build.

Improve the build for nice edit/test/debug workflow

89469f5

- `test-workspace` now compiles before running `metaserver/test`. - updated contributing guide to reflect this

Update tag numbers in the metaserver.proto

d8beb98

Remove unnecessary parts

445a434

Add fallback tests for .apply

8df17b8

Fix broken classpath test

8996aad

Use ScalametaLanguageServer for testing.

b78a8fe

Previously, I manually created the SymbolIndexer. However, this made it difficult to be sure the tests were testing the actual server. Now, we use the server directly instead.

Get bijection test passing after refactoring to use server.

54d1fa2

To speed up the edit/test/debug, use ServerConfig to disable several features like indexing the dependnecy classpath, JDK, or classload scalafmt.

Add test for JavaCtags.

57d5395

Move Notifications to separate file

bdf176f

Pass around AbsolutePath explicitly

f160f24

Setup compilerconfig in CI

dddc88f

Address review comments on Formatter.

588f80d

Added a test suite to validate it works as expected, and realized that - we didn't expose the method to format without a config file - the format(String, String, String) signature was unnecessarily weakly typed.

s/fromIndexRange/toPosition

de7e527

Clean up SymbolIndexer constructors.

0a4db89

- SAM types ftw - no implicit cwd: AbsolutePath, make it explicit

Refactor fallback/classpath tests for less boilerplate.

2dbfc96

Move logic out of SymbolIndexerTest to index.InverseSymbolIndexer

6087be7

Move all index-related classes to search package.

210b57a

olafurpg added 9 commits November 19, 2017 17:39

Remove unused

124dcd3

Document the magnificent SymbolIndexer.

be56b21

Fix completely broken SymbolIndexTest from IJ "formatter"

17d6fd7

The IJ formatter will sometimes randomly reorder statements, even if that completely breaks a ton of code.

Note that SymbolIndexer is just a key/value store

c68995c

Refactor SymbolIndexer into trait.

ab3ef3c

This separation from interface and implementation can make it easier for us to experiment with different approaches.

Add missing import from broken rebase

ff192f3

Fix broken .scalafmt.conf

8cc3061

We were ignoring everything in the scala.meta.languageserver package.

Log uncaught exceptions from ScalametaLanguageServer.

51f7e28

Previously, exceptions would get lost (not appear in logs) when the server crashed. Now we at least log them so we can debug the problem.

Reinstante original "go to definition inside jars" hack.

6ca66d2

This is the best we got until scalameta#36 gets fixed. I had to adapt the implementation to accommodate the new URI based indexing scheme.

olafurpg force-pushed the snapshot branch from 5869b22 to 6ca66d2 Compare November 19, 2017 17:43

olafurpg changed the title ~~[WIP] Migrate to new symbol index schema~~ Migrate to new symbol index schema Nov 19, 2017

Clean up a few more things.

ed0e859

- missing docstrings - consistent blank lines - up-to-date contributing guide

olafurpg merged commit 984e42d into scalameta:master Nov 20, 2017

olafurpg deleted the snapshot branch November 20, 2017 19:56

This was referenced Nov 20, 2017

Benchmark symbol indexer #40

Closed

Server is not indexing the JDK #43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to new symbol index schema #37

Migrate to new symbol index schema #37

olafurpg commented Nov 18, 2017 •

edited

gabro left a comment

gabro Nov 19, 2017

gabro Nov 19, 2017

gabro Nov 19, 2017

olafurpg Nov 19, 2017

gabro Nov 19, 2017

olafurpg Nov 19, 2017

gabro Nov 19, 2017

olafurpg Nov 19, 2017

gabro Nov 19, 2017

olafurpg Nov 19, 2017

gabro Nov 19, 2017

olafurpg Nov 19, 2017

gabro Nov 19, 2017

olafurpg Nov 19, 2017

olafurpg left a comment

olafurpg Nov 19, 2017

olafurpg Nov 19, 2017

olafurpg Nov 19, 2017

olafurpg Nov 19, 2017

olafurpg Nov 19, 2017

olafurpg Nov 19, 2017

olafurpg Nov 19, 2017

olafurpg Nov 19, 2017

olafurpg Nov 19, 2017

olafurpg commented Nov 19, 2017

olafurpg commented Nov 19, 2017

olafurpg commented Nov 20, 2017

Migrate to new symbol index schema #37

Migrate to new symbol index schema #37

Conversation

olafurpg commented Nov 18, 2017 • edited

Testing

gabro left a 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

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

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 left a 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

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 19, 2017

olafurpg commented Nov 19, 2017

olafurpg commented Nov 20, 2017

olafurpg commented Nov 18, 2017 •

edited