From 55ba37ca2fb836c6f9e542e147e7e54dab795ed2 Mon Sep 17 00:00:00 2001 From: Tomasz Godzik Date: Wed, 25 Oct 2023 15:43:14 +0200 Subject: [PATCH] refactor: Inline symbolsForPackage method --- .../scala/meta/internal/metals/Indexer.scala | 1 + .../internal/metals/MetalsLspService.scala | 2 +- .../meta/internal/tvp/ClasspathTreeView.scala | 5 +- .../meta/internal/tvp/IndexedSymbols.scala | 82 ++++++++++--------- .../internal/tvp/MetalsTreeViewProvider.scala | 32 +++++--- .../meta/internal/metals/JdkSources.scala | 8 +- .../src/main/scala/tests/QuickBuild.scala | 2 +- .../src/main/scala/tests/TestingServer.scala | 47 +++++++++-- .../test/scala/tests/TreeViewLspSuite.scala | 7 +- 9 files changed, 120 insertions(+), 66 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/Indexer.scala b/metals/src/main/scala/scala/meta/internal/metals/Indexer.scala index e6396948819..81313211899 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Indexer.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Indexer.scala @@ -488,6 +488,7 @@ final case class Indexer( val jdkSources = JdkSources(userConfig().javaHome) jdkSources match { case Right(zip) => + scribe.info(s"Indexing JDK sources from $zip") usedJars += zip addSourceJarSymbols(zip) case Left(notFound) => diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala index ba4adca5d69..22e08c5ab8e 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala @@ -811,7 +811,7 @@ class MetalsLspService( buildTargets, () => buildClient.ongoingCompilations(), definitionIndex, - optJavaHome, + () => userConfig, scalaVersionSelector, classpathTreeIndex, ) diff --git a/metals/src/main/scala/scala/meta/internal/tvp/ClasspathTreeView.scala b/metals/src/main/scala/scala/meta/internal/tvp/ClasspathTreeView.scala index a7fee7bb82b..6a81132e02f 100644 --- a/metals/src/main/scala/scala/meta/internal/tvp/ClasspathTreeView.scala +++ b/metals/src/main/scala/scala/meta/internal/tvp/ClasspathTreeView.scala @@ -111,8 +111,9 @@ class ClasspathTreeView[Value, Key]( case k.CLASS => "symbol-class" case k.INTERFACE => "symbol-interface" case k.CONSTRUCTOR => "symbol-method" - case k.METHOD | k.MACRO if (child.properties.isVal) => "symbol-field" - case k.METHOD | k.MACRO if (child.properties.isVar) => "symbol-variable" + case k.METHOD | k.MACRO if (child.properties.isVal) => "symbol-field" + case k.METHOD | k.MACRO if (child.properties.isVar) => + "symbol-variable" case k.METHOD | k.MACRO => "symbol-method" case k.FIELD if (child.properties.isEnum) => "symbol-enum-member" case k.FIELD => "symbol-field" diff --git a/metals/src/main/scala/scala/meta/internal/tvp/IndexedSymbols.scala b/metals/src/main/scala/scala/meta/internal/tvp/IndexedSymbols.scala index 7513287ce2e..6b806ecd695 100644 --- a/metals/src/main/scala/scala/meta/internal/tvp/IndexedSymbols.scala +++ b/metals/src/main/scala/scala/meta/internal/tvp/IndexedSymbols.scala @@ -12,7 +12,6 @@ import scala.meta.internal.metals.SemanticdbFeatureProvider import scala.meta.internal.metals.Time import scala.meta.internal.metals.Timer import scala.meta.internal.mtags.Mtags -import scala.meta.internal.mtags.SemanticdbPath import scala.meta.internal.mtags.Symbol import scala.meta.internal.mtags.SymbolDefinition import scala.meta.internal.semanticdb.SymbolInformation @@ -65,8 +64,8 @@ class IndexedSymbols(isStatisticsEnabled: Boolean)(implicit rc: ReportContext) workspaceCache.put(path, all.toArray) } - override def onDelete(path: SemanticdbPath): Unit = { - workspaceCache.remove(path.absolutePath) + override def onDelete(path: AbsolutePath): Unit = { + workspaceCache.remove(path) } def reset(): Unit = { @@ -84,10 +83,10 @@ class IndexedSymbols(isStatisticsEnabled: Boolean)(implicit rc: ReportContext) } /** - * We load all symbols for workspace eagerly + * We load all symbols for workspace when semanticdb files are produced. * - * @param in the input file to index - * @param dialect dialect to parse the file with + * @param in the input file to get symbols for + * @param symbol we are looking for * @return list of tree view symbols within the file */ def workspaceSymbols( @@ -142,20 +141,50 @@ class IndexedSymbols(isStatisticsEnabled: Boolean)(implicit rc: ReportContext) } val parsedSymbol = Symbol(symbol) - // if it's a package we'll collect all the children - if (parsedSymbol.isPackage) { + // root package doesn't need to calculate any members, they will be calculated lazily + if (parsedSymbol.isRootPackage) { jarSymbols.values .collect { - // root package doesn't need to calculate any members, they will be calculated lazily - case Left(defn) if parsedSymbol.isRootPackage => + case Left(defn) => Array(toTreeView(defn)) - case Right(list) if parsedSymbol.isRootPackage => list - case cached => - symbolsForPackage(cached, dialect, jarSymbols, parsedSymbol) + case Right(list) => list } .flatten .iterator - } else { + } // If it's a package we'll collect all the children + else if (parsedSymbol.isPackage) { + jarSymbols + .collect { + /* If the package we are looking for is the parent of the current symbol we + * need to check if we have grandchildren and the nodes are exapandable + * on the UI + */ + case (_, Left(toplevel)) + if (toplevel.definitionSymbol.owner == parsedSymbol) => + val children = + members(toplevel.path, dialect).map(toTreeView) + jarSymbols.put( + toplevel.definitionSymbol.value, + Right(children), + ) + children + + /* If this is further down we don't need to resolve it yet as + * as we will check that later when resolving parent package + */ + case (toplevelSymbol, Left(toplevel)) + if toplevelSymbol.startsWith(symbol) => + Array(toTreeView(toplevel)) + + // If it's already calculated then we can just return it + case (toplevelSymbol, Right(allSymbols)) + if toplevelSymbol.startsWith(symbol) => + allSymbols + case _ => Array.empty[TreeViewSymbolInformation] + } + .flatten + .iterator + } else { // if we are looking for a particular symbol then we need to resolve it properly jarSymbols.get(toplevelOwner(Symbol(symbol)).value) match { case Some(Left(toplevelOnly)) => val allSymbols = members(toplevelOnly.path, dialect).map(toTreeView) @@ -172,31 +201,6 @@ class IndexedSymbols(isStatisticsEnabled: Boolean)(implicit rc: ReportContext) } } - private def symbolsForPackage( - cached: Either[TopLevel, AllSymbols], - dialect: Dialect, - jarSymbols: TrieMap[String, Either[TopLevel, AllSymbols]], - symbol: Symbol, - ): Array[TreeViewSymbolInformation] = - cached match { - case Left(toplevel) - if toplevel.definitionSymbol.value.startsWith(symbol.value) => - // we need to check if we have grandchildren and the nodes are exapandable - if (toplevel.definitionSymbol.owner == symbol) { - val children = - members(toplevel.path, dialect).map(toTreeView) - jarSymbols.put( - toplevel.definitionSymbol.value, - Right(children), - ) - children - } else { - Array(toTreeView(toplevel)) - } - case Right(allSymbols) => allSymbols - case _ => Array.empty[TreeViewSymbolInformation] - } - private def members( path: AbsolutePath, dialect: Dialect, diff --git a/metals/src/main/scala/scala/meta/internal/tvp/MetalsTreeViewProvider.scala b/metals/src/main/scala/scala/meta/internal/tvp/MetalsTreeViewProvider.scala index 47df8c9e79c..9351dead4a8 100644 --- a/metals/src/main/scala/scala/meta/internal/tvp/MetalsTreeViewProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/tvp/MetalsTreeViewProvider.scala @@ -22,6 +22,7 @@ import scala.meta.io.AbsolutePath import ch.epfl.scala.bsp4j.BuildTarget import ch.epfl.scala.bsp4j.BuildTargetIdentifier import org.eclipse.{lsp4j => l} +import java.nio.file.Paths class MetalsTreeViewProvider( getFolderTreeViewProviders: () => List[FolderTreeViewProvider], @@ -198,8 +199,8 @@ class MetalsTreeViewProvider( override def onVisibilityDidChange( params: TreeViewVisibilityDidChangeParams ): Unit = { - val trees = getFolderTreeViewProviders() - trees.foreach{ + val trees = getFolderTreeViewProviders() + trees.foreach { _.setVisible(params.viewId, params.visible) } if (params.visible) { @@ -243,15 +244,16 @@ class FolderTreeViewProvider( buildTargets: BuildTargets, compilations: () => TreeViewCompilations, definitionIndex: GlobalSymbolIndex, - userJavaHome: Option[AbsolutePath], + userConfig: () => UserConfiguration, scalaVersionSelector: ScalaVersionSelector, classpath: IndexedSymbols, ) { def dialectOf(path: AbsolutePath): Option[Dialect] = scalaVersionSelector.dialectFromBuildTarget(path) - private val maybeUsedJdkVersion = - userJavaHome.flatMap { path => - JdkVersion.fromReleaseFileString(path) + private def maybeUsedJdkVersion = + JdkSources.defaultJavaHome(userConfig().javaHome).headOption.flatMap { + path => + JdkVersion.fromReleaseFileString(path) } private val isVisible = TrieMap.empty[String, Boolean].withDefaultValue(false) private val isCollapsedTarget = TrieMap.empty[BuildTargetIdentifier, Boolean] @@ -265,7 +267,7 @@ class FolderTreeViewProvider( folder, identity, _.toURI.toString(), - _.toAbsolutePath, + _.toAbsolutePath(followSymlink = false), path => { if (path.filename == JdkSources.zipFileName) { maybeUsedJdkVersion @@ -308,8 +310,8 @@ class FolderTreeViewProvider( }, ) - def setVisible(viewId: String, visibility: Boolean): Unit ={ - isVisible(viewId) = visibility + def setVisible(viewId: String, visibility: Boolean): Unit = { + isVisible(viewId) = visibility } def flushPendingProjectUpdates(): Option[Array[TreeViewNode]] = { @@ -404,12 +406,20 @@ class FolderTreeViewProvider( closestSymbol: SymbolOccurrence, ): Option[List[String]] = { if (path.isDependencySource(folder.path) || path.isJarFileSystem) { - def jdkSources = JdkSources(userJavaHome.map(_.toString())).toOption + def pathJar = AbsolutePath( + Paths.get(path.toNIO.getFileSystem().toString()) + ).dealias + def jdkSources = JdkSources(userConfig().javaHome).toOption .collect { case sources - if sources.toString == path.toNIO.getFileSystem().toString() || + if sources.dealias == pathJar || !path.isJarFileSystem && path.isSrcZipInReadonlyDirectory(folder.path) => libraries.toUri(sources, closestSymbol.symbol).parentChain + case other => + scribe.warn(other.toString()) + scribe.warn(other.dealias.toString) + scribe.warn(" pathJar " + pathJar) + Nil } val result = buildTargets diff --git a/mtags/src/main/scala/scala/meta/internal/metals/JdkSources.scala b/mtags/src/main/scala/scala/meta/internal/metals/JdkSources.scala index 855c62f072c..0568af2835c 100644 --- a/mtags/src/main/scala/scala/meta/internal/metals/JdkSources.scala +++ b/mtags/src/main/scala/scala/meta/internal/metals/JdkSources.scala @@ -27,7 +27,9 @@ object JdkSources { ): Either[NoSourcesAvailable, AbsolutePath] = { val paths = candidates(userJavaHome) paths.find(_.isFile) match { - case Some(value) => Right(value) + case Some(value) => + logger.info("Found JDK sources: " + value) + Right(value) case None => Left(NoSourcesAvailable(paths)) } } @@ -40,9 +42,7 @@ object JdkSources { s"Failed to parse java home path $str: ${exception.getMessage}" ) None - case Success(value) => - if (value.toString().contains("unit")) throw new Exception - Some(value) + case Success(value) => Some(value) } } } diff --git a/tests/unit/src/main/scala/tests/QuickBuild.scala b/tests/unit/src/main/scala/tests/QuickBuild.scala index f0278935be3..52fd928cb38 100644 --- a/tests/unit/src/main/scala/tests/QuickBuild.scala +++ b/tests/unit/src/main/scala/tests/QuickBuild.scala @@ -208,7 +208,7 @@ case class QuickBuild( val javaHome = Option(platformJavaHome) .map(Paths.get(_)) .orElse(Option(Properties.jdkHome).map(Paths.get(_))) - + val tags = if (isTest) Tag.Test :: Nil else Nil val scalaCompiler = diff --git a/tests/unit/src/main/scala/tests/TestingServer.scala b/tests/unit/src/main/scala/tests/TestingServer.scala index 1b3021ea79d..0780258fea1 100644 --- a/tests/unit/src/main/scala/tests/TestingServer.scala +++ b/tests/unit/src/main/scala/tests/TestingServer.scala @@ -1821,7 +1821,7 @@ final case class TestingServer( ) } - val label = parents.iterator + val labelsMap = parents.iterator .flatMap { r => r.nodes.iterator.map { n => val icon = Option(n.icon) match { @@ -1835,6 +1835,24 @@ final case class TestingServer( .toMap .updated("root", "root") + def label(uri: String, default: String): String = + labelsMap.get(uri) match { + /* fallback for JDK sources since we seem to have issues on CI + * with multiple JDK homes and this is just for the label + */ + // case _ if uri.contains("src.zip") => + // val last = uri.split("src.zip").last + // labelsMap + // .collectFirst { + // case (k, v) if k.endsWith(last) && k.contains("src.zip") => v + // } + // .getOrElse(default) + case None => + scribe.warn(s"Cannot find label for $uri") + scribe.warn(labelsMap.mkString("\n")) + default + case Some(value) => value + } val tree = parents .zip(reveal.uriChain :+ "root") .foldLeft(PrettyPrintTree.empty) { case (child, (parent, uri)) => @@ -1842,9 +1860,9 @@ final case class TestingServer( if (uri.contains("-sources.jar")) uri else uri.replace(".jar", "-sources.jar") PrettyPrintTree( - label.getOrElse(realUri.toLowerCase, realUri), + label(realUri.toLowerCase, realUri), parent.nodes - .map(n => PrettyPrintTree(label(n.nodeUri.toLowerCase))) + .map(n => PrettyPrintTree(label(n.nodeUri.toLowerCase, n.nodeUri))) .filterNot(t => isIgnored(t.value)) .toList :+ child, ) @@ -1879,12 +1897,27 @@ final case class TestingServer( Assertions.assertNoDiff(obtained, expected) } - def treeViewVisibilityDidChange(viewId: String, isVisible: Boolean): Future[Unit] = { - fullServer.treeViewVisibilityDidChange(TreeViewVisibilityDidChangeParams(viewId, isVisible)).asScala + def treeViewVisibilityDidChange( + viewId: String, + isVisible: Boolean, + ): Future[Unit] = { + fullServer + .treeViewVisibilityDidChange( + TreeViewVisibilityDidChangeParams(viewId, isVisible) + ) + .asScala } - def treeViewNodeCollapseDidChange(viewId: String, nodeId: String, isCollapsed: Boolean): Future[Unit] = { - fullServer.treeViewNodeCollapseDidChange(TreeViewNodeCollapseDidChangeParams(viewId, nodeId, isCollapsed)).asScala + def treeViewNodeCollapseDidChange( + viewId: String, + nodeId: String, + isCollapsed: Boolean, + ): Future[Unit] = { + fullServer + .treeViewNodeCollapseDidChange( + TreeViewNodeCollapseDidChangeParams(viewId, nodeId, isCollapsed) + ) + .asScala } def findTextInDependencyJars( diff --git a/tests/unit/src/test/scala/tests/TreeViewLspSuite.scala b/tests/unit/src/test/scala/tests/TreeViewLspSuite.scala index f14cf086389..e9654c6c904 100644 --- a/tests/unit/src/test/scala/tests/TreeViewLspSuite.scala +++ b/tests/unit/src/test/scala/tests/TreeViewLspSuite.scala @@ -7,6 +7,7 @@ import scala.util.Properties import scala.meta.internal.metals.InitializationOptions import scala.meta.internal.metals.JdkVersion +import scala.meta.internal.metals.UserConfiguration import scala.meta.internal.tvp.TreeViewProvider import scala.meta.io.AbsolutePath @@ -20,10 +21,14 @@ class TreeViewLspSuite extends BaseLspSuite("tree-view") { JdkVersion .fromReleaseFileString(AbsolutePath(Paths.get(Properties.javaHome))) .getOrElse("") + private val jdkSourcesName = s"jdk-$javaVersion-sources" override protected def initializationOptions: Option[InitializationOptions] = Some(TestingServer.TestDefault) + override def userConfig: UserConfiguration = + UserConfiguration(javaHome = Some(Properties.javaHome)) + /** * The libraries we expect to find for tests in this file. */ @@ -160,7 +165,7 @@ class TreeViewLspSuite extends BaseLspSuite("tree-view") { } _ = assertEquals( server.client.workspaceTreeViewChanges, - s"metalsPackages projects-$folder:${server.buildTarget("a")}!/_root_/" + s"metalsPackages projects-$folder:${server.buildTarget("a")}!/_root_/", ) _ = server.assertTreeViewChildren( s"projects-$folder:${server.buildTarget("a")}!/_empty_/Zero#",