From 82738ecdd8c503e6161533bf28c6b4d9648d5eef 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 | 77 ++++++++++--------- .../internal/tvp/MetalsTreeViewProvider.scala | 24 +++--- .../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 | 3 +- 9 files changed, 107 insertions(+), 62 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 a7d242aa151..1adbff56ee0 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..5bcacc03c1e 100644 --- a/metals/src/main/scala/scala/meta/internal/tvp/IndexedSymbols.scala +++ b/metals/src/main/scala/scala/meta/internal/tvp/IndexedSymbols.scala @@ -84,10 +84,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 +142,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 +202,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..0c79327c5d6 100644 --- a/metals/src/main/scala/scala/meta/internal/tvp/MetalsTreeViewProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/tvp/MetalsTreeViewProvider.scala @@ -198,8 +198,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 +243,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] @@ -275,7 +276,10 @@ class FolderTreeViewProvider( path.filename }, _.toString, - () => buildTargets.allSourceJars, + () => { + scribe.info(s"libraries: ${buildTargets.allSourceJars.toList}") + buildTargets.allSourceJars + }, (path, symbol) => { val dialect = ScalaVersions.dialectForDependencyJar(path.filename) classpath.jarSymbols(path, symbol, dialect) @@ -308,8 +312,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,7 +408,7 @@ class FolderTreeViewProvider( closestSymbol: SymbolOccurrence, ): Option[List[String]] = { if (path.isDependencySource(folder.path) || path.isJarFileSystem) { - def jdkSources = JdkSources(userJavaHome.map(_.toString())).toOption + def jdkSources = JdkSources(userConfig().javaHome).toOption .collect { case sources if sources.toString == path.toNIO.getFileSystem().toString() || 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..3f99d0dcf1c 100644 --- a/tests/unit/src/test/scala/tests/TreeViewLspSuite.scala +++ b/tests/unit/src/test/scala/tests/TreeViewLspSuite.scala @@ -16,6 +16,7 @@ import scala.meta.io.AbsolutePath */ class TreeViewLspSuite extends BaseLspSuite("tree-view") { + scribe.info("Using for tests" + Paths.get(Properties.javaHome).toString()) private val javaVersion = JdkVersion .fromReleaseFileString(AbsolutePath(Paths.get(Properties.javaHome))) @@ -160,7 +161,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#",