Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions jsemanticdb/src/main/protobuf/mbt.proto
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ message IndexedDocument {
V7 = 7;
V8 = 8;
V9 = 9;
V10 = 10;
}

enum Source {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ case class IndexedDocument(
def toProto(): Mbt.IndexedDocument.Builder = {
val bloomFilterVersion = language match {
case Language.JAVA => Mbt.IndexedDocument.BloomFilterVersion.V7
case Language.PROTOBUF => Mbt.IndexedDocument.BloomFilterVersion.V9
case Language.PROTOBUF => Mbt.IndexedDocument.BloomFilterVersion.V10
case _ => Mbt.IndexedDocument.BloomFilterVersion.V6
}
Mbt.IndexedDocument
Expand Down Expand Up @@ -271,8 +271,8 @@ object IndexedDocument {
doc.getBloomFilterVersion().getNumber >= (doc.getLanguage() match {
case Language.JAVA => Mbt.IndexedDocument.BloomFilterVersion.V7
case Language.SCALA => Mbt.IndexedDocument.BloomFilterVersion.V6
// V9: Proto bloom filters include scanner fixes for option blocks.
case Language.PROTOBUF => Mbt.IndexedDocument.BloomFilterVersion.V9
// V10: Includes V9 option-block scanner fixes and Java packages for Turbine.
case Language.PROTOBUF => Mbt.IndexedDocument.BloomFilterVersion.V10
case _ => Mbt.IndexedDocument.BloomFilterVersion.V1
}).getNumber()
}
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ class MbtWorkspaceSymbolProvider(
clearAllProtobufCaches,
)

private def isProtoJavaPackageIndexingEnabled: Boolean =
protobufWorkspace.isJavaPackageIndexingEnabled ||
javaSymbolLoader().isTurbineClasspath

/**
* The Java outlines synthesized from the given `.proto` file (one per
* generated top-level class). Empty when the file isn't an indexed proto or
Expand Down Expand Up @@ -202,10 +206,7 @@ class MbtWorkspaceSymbolProvider(
)
)
.toList
} else if (
file.isProtoFilename &&
protobufWorkspace.isJavaPackageIndexingEnabled
) {
} else if (file.isProtoFilename && isProtoJavaPackageIndexingEnabled) {
// Include the Java outlines generated from proto files so that
// turbine can resolve references to proto-generated classes when it
// header-compiles the workspace. Without these, a Java method
Expand Down Expand Up @@ -547,7 +548,7 @@ class MbtWorkspaceSymbolProvider(
): Future[Unit] = try {
if (MbtIndexFilter.included(indexFilters, MbtFileCandidate(file))) {
val enableProtoJavaPackage =
file.isProtoFilename && protobufWorkspace.isJavaPackageIndexingEnabled
file.isProtoFilename && isProtoJavaPackageIndexingEnabled
val mdoc =
IndexedDocument.fromFile(
file,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class TurbineClasspathFileManager(
delegate: JavaFileManager,
workspaceClasspath: () => TurbineCompileResult,
listSourcepath: String => java.lang.Iterable[JavaFileObject],
listProtoBinaryNames: String => Set[String],
isDeleted: String => Boolean,
projectClasspath: ClassPath,
) extends ForwardingJavaFileManager[JavaFileManager](delegate) {
Expand Down Expand Up @@ -83,6 +84,20 @@ class TurbineClasspathFileManager(
val turbinePackageName = packageNames.mkString("/")
val objects = new ju.ArrayList[JavaFileObject]()
val cp = workspaceClasspath()
val isAddedBinaryName = new ju.HashSet[String]()
val protoBinaryNames = listProtoBinaryNames(packageName)
if (protoBinaryNames.nonEmpty) {
super.list(location, packageName, kinds, recurse).forEach { obj =>
val binaryName = inferBinaryName(location, obj).replace('.', '/')
val topLevelBinaryName = binaryName.takeWhile(_ != '$')
if (
protoBinaryNames.contains(topLevelBinaryName) &&
isAddedBinaryName.add(binaryName)
) {
objects.add(obj)
}
}
}
cp.symbolsByPackage.get(turbinePackageName) match {
case None =>
case Some(values) =>
Expand All @@ -94,7 +109,7 @@ class TurbineClasspathFileManager(
// or have a pending source on SOURCE_PATH (so javac uses the updated source)
if (!isDeleted(binaryName)) {
val bytes = cp.lowered.bytes().get(binaryName)
if (bytes != null) {
if (bytes != null && isAddedBinaryName.add(binaryName)) {
val obj = new TurbineClassfileObject(
binaryName,
() => bytes,
Expand All @@ -104,14 +119,13 @@ class TurbineClasspathFileManager(
}
}
}
val isAddedBinaryName = new ju.HashSet[String]()
for {
cp <- List(
// Prioritize the project classpath over the fallback classpath
projectClasspath,
cp.classpath,
)
} listPackageClasspath(cp, packageNames, isAddedBinaryName) { obj =>
classpath <- List(projectClasspath, cp.classpath)
} listPackageClasspath(
classpath,
packageNames,
isAddedBinaryName,
) { obj =>
objects.add(obj)
}
objects
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import scala.meta.internal.metals.PcQueryContext
import scala.meta.internal.metals.ReportContext
import scala.meta.internal.metals.Sleeper
import scala.meta.pc.ProgressBars
import scala.meta.pc.SemanticdbCompilationUnit

import com.google.common.collect.ImmutableList
import com.google.common.collect.ImmutableMap
Expand Down Expand Up @@ -329,17 +330,18 @@ class TurbineCompiler[T](
projectClasspathJars: ju.List[Path],
): JavaFileManager = {
val isGlobalClasspathEntry = this.classpath().toSet
val filteredProjectClasspath =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Had a quick LLM assisted look:

Dropping the filter also re-indexes every project jar (including ones already on the global CP) on each presentation-compiler create. In a large MBT classpath that can be a noticeable startup cost; the filter existed to avoid that.

the worry here seems legit, we will be radding jars, which were already added.

The project jars might also be outdated compared to what turbine has actually 🤔

Could we instead make it work with protobuf jars only?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done, kept isGlobalClasspathEntry and adjusted the project classpath with protobuf jars. Let me know if this is closer to what you had in mind

val projectClasspathEntries =
projectClasspathJars.asScala.filter(file =>
!isGlobalClasspathEntry(file) && TurbineCompiler.isJarFile(file)
)
val projectClasspath =
ClassPathBinder.bindClasspath(filteredProjectClasspath.asJava)
ClassPathBinder.bindClasspath(projectClasspathEntries.asJava)
onNewProjectClasspath(projectClasspath)
new TurbineClasspathFileManager(
underlying,
() => result,
listSourcepath = listCombinedSourcepath,
listProtoBinaryNames,
isDeleted,
projectClasspath,
)
Expand All @@ -362,6 +364,14 @@ class TurbineCompiler[T](
}
}

private def listProtoBinaryNames(packageName: String): Set[String] = {
val protoPackage = packageName.replace('.', '/') + "/"
listProtoJavaOutlinesForPackage(protoPackage).collect {
case file: SemanticdbCompilationUnit =>
file.binaryName().replace('.', '/')
}.toSet
}

private[mbt] def listSourcepath(
packageName: String
): java.lang.Iterable[JavaFileObject] = {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,35 @@
package tests.mbt

import java.net.URI
import java.nio.file.Files
import java.nio.file.Paths
import java.util.EnumSet
import java.util.zip.ZipEntry
import java.util.zip.ZipOutputStream
import javax.tools.JavaFileObject
import javax.tools.StandardLocation
import javax.tools.ToolProvider

import scala.collection.JavaConverters._
import scala.collection.parallel.mutable.ParArray
import scala.concurrent.Await
import scala.concurrent.ExecutionContext
import scala.concurrent.duration._
import scala.util.Using

import scala.meta.internal.metals.Configs
import scala.meta.internal.metals.EmptyWorkDoneProgress
import scala.meta.internal.metals.LoggerReportContext
import scala.meta.internal.metals.Sleeper
import scala.meta.internal.metals.mbt.IndexingStats
import scala.meta.internal.metals.mbt.MbtWorkspaceSymbolProvider
import scala.meta.internal.metals.mbt.TurbineCompileResult
import scala.meta.internal.metals.mbt.TurbineCompiler
import scala.meta.internal.metals.mbt.VirtualTextDocument
import scala.meta.io.AbsolutePath
import scala.meta.pc

import com.google.turbine.diag.SourceFile
import munit.AnyFixture
import munit.TestOptions
import org.eclipse.{lsp4j => l}
Expand Down Expand Up @@ -154,6 +172,27 @@ object Hello2 {
)
}

test("turbine-indexes-proto-java-package-with-protobuf-lsp-disabled") {
FileLayout.fromString(
"""
/example/Dependency.proto
syntax = "proto3";
package example;
option java_package = "generated.example";
message Dependency {}
""",
root = workspace(),
)
val provider = newProvider()
workspace.executeCommand("git init -b main")
workspace.gitCommitAllChanges()
assertEquals(
provider.onReindex().awaitBackgroundJobs(),
IndexingStats(totalFiles = 1, updatedFiles = 1),
)
assert(provider.listAllPackages().containsKey("generated/example/"))
}

test("exclude-module-info-java") {
FileLayout.fromString(
"""
Expand Down Expand Up @@ -227,3 +266,112 @@ module com.example {
)

}

class TurbineClasspathFileManagerSuite extends munit.FunSuite {
implicit val reportContext: LoggerReportContext.type = LoggerReportContext
implicit val executionContext: ExecutionContext = ExecutionContext.global

private val workspaceSource =
"package example; public class Dependency { public static class Builder { public void workspace() {} } }"
private val projectSource =
"package example; public class Dependency { public static class Builder { public void project() {} } }"

private def compile(source: String): TurbineCompileResult =
TurbineCompiler.compileClassfiles(
ParArray(source),
(text: String) => Seq(new SourceFile("Dependency.java", text)),
Nil,
EmptyWorkDoneProgress,
)

private def checkTargetClasspathPrecedence(isProtobuf: Boolean): Unit = {
val projectResult = compile(projectSource)
val jar = Files.createTempFile("metals-project-classpath", ".jar")
val output = new ZipOutputStream(Files.newOutputStream(jar))
try {
for ((name, bytes) <- projectResult.lowered.bytes().asScala) {
output.putNextEntry(new ZipEntry(s"$name.class"))
output.write(bytes)
output.closeEntry()
}
} finally output.close()

var fallbackClasspath = Seq.empty[java.nio.file.Path]
val protoOutline = VirtualTextDocument(
URI.create("file:///Dependency.java"),
pc.Language.JAVA,
workspaceSource,
Seq("example"),
Seq("example/Dependency#"),
)
val compiler = new TurbineCompiler[String](
() => ParArray(workspaceSource),
text => Seq(new SourceFile("Dependency.java", text)),
() => fallbackClasspath,
EmptyWorkDoneProgress,
() => Configs.TurbineRecompileDelayConfig.testing,
packageName =>
if (isProtobuf && packageName == "example/") Iterator(protoOutline)
else Iterator.empty,
Sleeper.TestingSleeper,
() => (),
_ => (),
)
compiler.doCompileNow()
val workspaceResult = compiler.result
fallbackClasspath = Seq(jar)

val standardFileManager = ToolProvider
.getSystemJavaCompiler()
.getStandardFileManager(null, null, null)
standardFileManager.setLocationFromPaths(
StandardLocation.CLASS_PATH,
List(jar).asJava,
)
val fileManager = compiler.createFileManager(
standardFileManager,
List(jar).asJava,
)
try {
val classfiles = fileManager
.list(
StandardLocation.CLASS_PATH,
"example",
EnumSet.of(JavaFileObject.Kind.CLASS),
false,
)
.asScala
.toList
val obtained = classfiles.map { classfile =>
val binaryName = fileManager
.inferBinaryName(StandardLocation.CLASS_PATH, classfile)
.replace('.', '/')
binaryName -> Using.resource(classfile.openInputStream())(
_.readAllBytes().toSeq
)
}.toMap
val expectedResult =
if (isProtobuf) projectResult
else workspaceResult
val expected = expectedResult.lowered
.bytes()
.asScala
.map { case (name, bytes) =>
name -> bytes.toSeq
}
.toMap
assertEquals(obtained, expected)
} finally {
fileManager.close()
Files.deleteIfExists(jar)
}
}

test("target-protobuf-classpath-before-workspace-headers") {
checkTargetClasspathPrecedence(isProtobuf = true)
}

test("workspace-headers-before-non-protobuf-target-classpath") {
checkTargetClasspathPrecedence(isProtobuf = false)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
Loading