Go-to-definition does not work after go-to-definition on an external source

[aiguofer]

I'll preface this saying I'm using Emacs and I'm not sure if this is an issue with KLS or with lsp-mode on Emacs, or some missbehavior between the two.

The Emacs lsp-mode offers a way to configure useKlsScheme. It defaults to true, and it says it's recommended, but this doesn't seem to work when I try to go-to-def: The following file kls:file:///Library/Java/JavaVirtualMachines/amazon-corretto-17.jdk/Contents/Home/lib/src.zip!/java.sql/javax/sql/DataSource.java?source=true is missing, ignoring from the results.

When I turn this off, go-to-def works by making some sort of temp file of the (jar extracted) decompiled class or source in /var. When it opens the file, it tries to load the file as a standalone script instead of knowing it was part of the project I navigated from. This means I can't go-to-def from that file since it does not have a classpath.

[aiguofer]

Ok, I did some digging and figured out how to add support for the kls scheme in Emacs: emacs-lsp/lsp-mode#4051

With the current implementation, I create the temp file in a .cache folder at the root of the project. I was hoping this would ensure that code navigation from Java dependencies would still work, but alas it still doesn't. It does seem to use the same kotlin-language-server, so it should work but it doesn't seem to...

When I navigate to a Java source and try to go-to-def on other objects there I see the following logs:

async2    Go-to-definition at .../logging/LoggingMiddleware.kt 21:51
async2    Found declaration descriptor Lazy Java class org.apache.arrow.flight.CallInfo
async2    Finished in 29 ms
async2    Go-to-definition at .../.cache/org.apache.arrow.flight.CallInfo.java 26:22
async2    Requested source file .../.cache/org.apache.arrow.flight.CallInfo.java is not on source path, this is most likely a bug. Adding it now temporarily...
async2    Adding temporary source file .../.cache/org.apache.arrow.flight.CallInfo.java to source path
async2    Finished in 59 ms
async2    Internal error: java.lang.IllegalArgumentException: Unable to find script compilation configuration for the script KtFile: <path_to_project>/.cache/org.apache.arrow.flight.CallInfo.java
java.util.concurrent.CompletionException: java.lang.IllegalArgumentException: Unable to find script compilation configuration for the script KtFile: <path_to_project>/.cache/org.apache.arrow.flight.CallInfo.java

[RenanKummer]

The same issue is happening in VS Code as well. Go-to-definition is not working for anything really, not even standard library stuff such as mapOf or Pair.

[fwcd]

Thanks for reporting this, I've found the issue and it seems to actually be a regression introduced with the class path cache in #337. Precisely the issue is here:

kotlin-language-server/shared/src/main/kotlin/org/javacs/kt/classpath/CachedClassPathResolver.kt

Line 72 in 8f96c29

sourceJar = it.sourceJar.toString()

Notice how we call .toString() on a nullable value (it.sourceJar of type Path?): This means absent source jars would have been stored as the string "null" in the database, rather than null as the schema would suggest:

kotlin-language-server/shared/src/main/kotlin/org/javacs/kt/classpath/CachedClassPathResolver.kt

Line 24 in 8f96c29

val sourceJar = varchar("sourcejar", length = MAX_PATH_LENGTH).nullable()

This then in turn caused

kotlin-language-server/server/src/main/kotlin/org/javacs/kt/externalsources/ClassPathSourceArchiveProvider.kt

Lines 6 to 11 in 8f96c29

	class ClassPathSourceArchiveProvider(
	private val cp: CompilerClassPath
	) : SourceArchiveProvider {
	override fun fetchSourceArchive(compiledArchive: Path): Path? =
	cp.classPath.firstOrNull { it.compiledJar == compiledArchive }?.sourceJar
	}

to return bogus results (paths containing the string "null"), which then led to an equally invalid resolvedUri in ClassContentProvider:

kotlin-language-server/server/src/main/kotlin/org/javacs/kt/externalsources/ClassContentProvider.kt

Line 38 in 8f96c29

val resolvedUri = sourceArchiveProvider.fetchSourceArchive(uri.archivePath)?.let(uri.withSource(true)::withArchivePath) ?: uri

Unfortunately, the database schema has now shipped, so we'll actually have to introduce some form of versioning to make sure user databases are rebuilt after fixing this (I've opened #503 for that, PR for the actual fix is underway...).

cc @daplf

[fwcd]

not even standard library stuff such as mapOf or Pair.

This is btw not a regression, simply something we haven't supported yet, though it's on the wishlist. See #308 too.

[daplf]

Apologies for the regression :/

Opened a PR to add database versioning: #504

Once that one is merged, I can also make a PR to fix the actual issue.

[fwcd]

Oh, no worries, that stuff happens. Thanks for the quick response, I'll have a look at #504!

We should probably add a few more tests for definition lookups on external libraries that cover all of the common cases (JDK, JAR with source, JAR without source, ...) to make sure that we don't regress on this in the future.