From cd993c5600494bd3a79b4288a22dd9ebccef3a63 Mon Sep 17 00:00:00 2001 From: Katarzyna Marek Date: Thu, 11 Jan 2024 11:04:00 +0100 Subject: [PATCH] bugfix: show import renames in hover (#5982) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * bugfix: show using renames in hover * show import renames in hover Scala 3 * show import renames in hover Scala 2 * scala 3 fix * put used used rename in history * change context for Scala 3 hover Co-Authored-By: Jakub Cieśluk <72950514+jkciesluk@users.noreply.github.com> --------- Co-authored-by: Jakub Cieśluk <72950514+jkciesluk@users.noreply.github.com> --- .../scala/meta/internal/pc/HoverMarkup.scala | 9 ++- .../scala/meta/internal/pc/ScalaHover.scala | 25 +++++++- .../meta/internal/pc/HoverProvider.scala | 20 +++++-- .../scala/meta/internal/pc/MetalsGlobal.scala | 13 ++-- .../scala/meta/internal/pc/Signatures.scala | 15 ++++- .../meta/internal/pc/HoverProvider.scala | 32 +++++----- .../internal/pc/printer/DotcPrinter.scala | 8 ++- .../internal/pc/printer/MetalsPrinter.scala | 2 + .../internal/pc/printer/ShortenedNames.scala | 9 +++ .../scala/tests/hover/HoverDefnSuite.scala | 8 +-- .../scala/tests/hover/HoverTermSuite.scala | 59 +++++++++++++++++++ .../tests/pc/InsertInferredTypeSuite.scala | 30 ++++++++++ 12 files changed, 190 insertions(+), 40 deletions(-) diff --git a/mtags-shared/src/main/scala/scala/meta/internal/pc/HoverMarkup.scala b/mtags-shared/src/main/scala/scala/meta/internal/pc/HoverMarkup.scala index c3291903bf9..9a7e2add956 100644 --- a/mtags-shared/src/main/scala/scala/meta/internal/pc/HoverMarkup.scala +++ b/mtags-shared/src/main/scala/scala/meta/internal/pc/HoverMarkup.scala @@ -18,9 +18,16 @@ object HoverMarkup { expressionType: String, optSymbolSignature: Option[String], docstring: String, - forceExpressionType: Boolean = false + forceExpressionType: Boolean = false, + contextInfo: List[String] = Nil ): String = { val markdown = new StringBuilder() + if (contextInfo.nonEmpty) { + markdown + .append("```scala\n") + .append(contextInfo.mkString("\n")) + .append("\n```\n\n") + } if (forceExpressionType || optSymbolSignature.isEmpty) { markdown .append( diff --git a/mtags-shared/src/main/scala/scala/meta/internal/pc/ScalaHover.scala b/mtags-shared/src/main/scala/scala/meta/internal/pc/ScalaHover.scala index ab3c2e16d2d..d4125bb1fec 100644 --- a/mtags-shared/src/main/scala/scala/meta/internal/pc/ScalaHover.scala +++ b/mtags-shared/src/main/scala/scala/meta/internal/pc/ScalaHover.scala @@ -12,9 +12,26 @@ case class ScalaHover( symbolSignature: Option[String] = None, docstring: Option[String] = None, forceExpressionType: Boolean = false, - range: Option[lsp4j.Range] = None + range: Option[lsp4j.Range] = None, + contextInfo: List[String] // e.g. info about rename imports ) extends HoverSignature { + def this( + expressionType: Option[String], + symbolSignature: Option[String], + docstring: Option[String], + forceExpressionType: Boolean, + range: Option[lsp4j.Range] + ) = + this( + expressionType, + symbolSignature, + docstring, + forceExpressionType, + range, + contextInfo = Nil + ) + def signature(): Optional[String] = symbolSignature.asJava def toLsp(): lsp4j.Hover = { val markdown = @@ -22,7 +39,8 @@ case class ScalaHover( expressionType.getOrElse(""), symbolSignature, docstring.getOrElse(""), - forceExpressionType + forceExpressionType, + contextInfo ) new lsp4j.Hover(markdown.toMarkupContent, range.orNull) } @@ -35,7 +53,8 @@ case class ScalaHover( symbolSignature, docstring, forceExpressionType, - Some(range) + Some(range), + contextInfo ) } diff --git a/mtags/src/main/scala-2/scala/meta/internal/pc/HoverProvider.scala b/mtags/src/main/scala-2/scala/meta/internal/pc/HoverProvider.scala index ad2cb19384c..76803cf52af 100644 --- a/mtags/src/main/scala-2/scala/meta/internal/pc/HoverProvider.scala +++ b/mtags/src/main/scala-2/scala/meta/internal/pc/HoverProvider.scala @@ -217,13 +217,19 @@ class HoverProvider(val compiler: MetalsGlobal, params: OffsetParams)(implicit pos.start != pos.end && (symbol == null || symbol == NoSymbol || symbol.isErroneous) ) { val context = doLocateContext(pos) + val re: scala.collection.Map[Symbol, Name] = renamedSymbols(context) val history = new ShortenedNames( - lookupSymbol = name => context.lookupSymbol(name, _ => true) :: Nil + lookupSymbol = name => context.lookupSymbol(name, _ => true) :: Nil, + renames = re ) val prettyType = metalsToLongString(tpe.widen.finalResultType, history) val lspRange = if (range.isRange) Some(range.toLsp) else None Some( - new ScalaHover(expressionType = Some(prettyType), range = lspRange) + new ScalaHover( + expressionType = Some(prettyType), + range = lspRange, + contextInfo = history.getUsedRenamesInfo() + ) ) } else if (symbol == null || tpe.typeSymbol.isAnonymousClass) None else if (symbol.hasPackageFlag || symbol.hasModuleFlag) { @@ -231,13 +237,16 @@ class HoverProvider(val compiler: MetalsGlobal, params: OffsetParams)(implicit new ScalaHover( expressionType = Some( s"${symbol.javaClassSymbol.keyString} ${symbol.fullName}" - ) + ), + contextInfo = Nil ) ) } else { val context = doLocateContext(pos) + val re: scala.collection.Map[Symbol, Name] = renamedSymbols(context) val history = new ShortenedNames( - lookupSymbol = name => context.lookupSymbol(name, _ => true) :: Nil + lookupSymbol = name => context.lookupSymbol(name, _ => true) :: Nil, + renames = re ) val symbolInfo = if (seenFrom.isErroneous) symbol.info @@ -273,7 +282,8 @@ class HoverProvider(val compiler: MetalsGlobal, params: OffsetParams)(implicit docstring = Some(docstring), forceExpressionType = pos.start != pos.end || !prettySignature.endsWith(prettyType), - range = if (range.isRange) Some(range.toLsp) else None + range = if (range.isRange) Some(range.toLsp) else None, + contextInfo = history.getUsedRenamesInfo() ) ) } diff --git a/mtags/src/main/scala-2/scala/meta/internal/pc/MetalsGlobal.scala b/mtags/src/main/scala-2/scala/meta/internal/pc/MetalsGlobal.scala index b5ca5fb3970..037da67021f 100644 --- a/mtags/src/main/scala-2/scala/meta/internal/pc/MetalsGlobal.scala +++ b/mtags/src/main/scala-2/scala/meta/internal/pc/MetalsGlobal.scala @@ -311,7 +311,7 @@ class MetalsGlobal( !metalsConfig.isDefaultSymbolPrefixes || hasConflictingMembersInScope if (shouldRenamePrefix) { - val existingRename = history.renames.get(ownerSym) + val existingRename = history.rename(ownerSym) existingRename.isEmpty && history.tryShortenName( ShortName(rename, ownerSymbol) ) @@ -329,9 +329,8 @@ class MetalsGlobal( args.map(arg => loop(arg, None)) ) case _ => - history.renames.get(sym) match { - case Some(rename) - if history.nameResolvesToSymbol(rename, sym) => + history.rename(sym) match { + case Some(rename) => TypeRef( NoPrefix, sym.newErrorSymbol(rename), @@ -411,7 +410,7 @@ class MetalsGlobal( // to make sure we always use renamed package // what is saved in renames is actually companion module of a package val renamedOwnerIndex = - owners.indexWhere(s => history.renames.contains(s.companionModule)) + owners.indexWhere(s => history.rename(s.companionModule).nonEmpty) if (renamedOwnerIndex < 0 && history.tryShortenName(name)) NoPrefix else { val prefix = @@ -434,8 +433,8 @@ class MetalsGlobal( .reverse .map(s => m.Term.Name( - history.renames - .get(s.companionModule) + history + .rename(s.companionModule) .map(_.toString()) .getOrElse(s.nameSyntax) ) diff --git a/mtags/src/main/scala-2/scala/meta/internal/pc/Signatures.scala b/mtags/src/main/scala-2/scala/meta/internal/pc/Signatures.scala index 02c64b054c3..fb1395d76e0 100644 --- a/mtags/src/main/scala-2/scala/meta/internal/pc/Signatures.scala +++ b/mtags/src/main/scala-2/scala/meta/internal/pc/Signatures.scala @@ -102,9 +102,22 @@ trait Signatures { compiler: MetalsGlobal => val history: mutable.Map[Name, ShortName] = mutable.Map.empty, val lookupSymbol: Name => List[NameLookup] = _ => Nil, val config: collection.Map[Symbol, Name] = Map.empty, - val renames: collection.Map[Symbol, Name] = Map.empty, + renames: collection.Map[Symbol, Name] = Map.empty, val owners: collection.Set[Symbol] = Set.empty ) { + + private val lookedUpRenames = mutable.Set[Symbol]() + + def rename(sym: Symbol): Option[Name] = { + lookedUpRenames.add(sym) + renames.get(sym) + } + + def getUsedRenamesInfo(): List[String] = + lookedUpRenames.flatMap { key => + renames.get(key).map(v => s"type $v = ${key.nameString}") + }.toList + def this(context: Context) = this(lookupSymbol = { name => context.lookupSymbol(name, _ => true) :: Nil diff --git a/mtags/src/main/scala-3/scala/meta/internal/pc/HoverProvider.scala b/mtags/src/main/scala-3/scala/meta/internal/pc/HoverProvider.scala index 29349df2dca..39cd0264215 100644 --- a/mtags/src/main/scala-3/scala/meta/internal/pc/HoverProvider.scala +++ b/mtags/src/main/scala-3/scala/meta/internal/pc/HoverProvider.scala @@ -32,16 +32,19 @@ object HoverProvider: val uri = params.uri val sourceFile = CompilerInterfaces.toSource(params.uri, params.text) driver.run(uri, sourceFile) + val unit = driver.compilationUnits.get(uri) - given ctx: Context = driver.currentCtx + given ctx: Context = + val ctx = driver.currentCtx + unit.map(ctx.fresh.setCompilationUnit).getOrElse(ctx) val pos = driver.sourcePosition(params) - val trees = driver.openedTrees(uri) + val path = unit + .map(unit => Interactive.pathTo(unit.tpdTree, pos.span)) + .getOrElse(Interactive.pathTo(driver.openedTrees(uri), pos)) val indexedContext = IndexedContext(ctx) - def typeFromPath(path: List[Tree]) = if path.isEmpty then NoType else path.head.tpe - val path = Interactive.pathTo(trees, pos) val tp = typeFromPath(path) val tpw = tp.widenTermRefExpr // For expression we need to find all enclosing applies to get the exact generic type @@ -67,14 +70,16 @@ object HoverProvider: |path: |${path .map(tree => s"""|```scala - |$tree + |${tree.show} |``` |""".stripMargin) .mkString("\n")} |trees: - |${trees + |${unit + .map(u => List(u.tpdTree)) + .getOrElse(driver.openedTrees(uri).map(_.tree)) .map(tree => s"""|```scala - |$tree + |${tree.show} |``` |""".stripMargin) .mkString("\n")} @@ -89,16 +94,9 @@ object HoverProvider: else val skipCheckOnName = !pos.isPoint // don't check isHoveringOnName for RangeHover - - val printerContext = - driver.compilationUnits.get(uri) match - case Some(unit) => - val newctx = - ctx.fresh.setCompilationUnit(unit) - MetalsInteractive.contextOfPath(enclosing)(using newctx) - case None => ctx + val printerCtx = MetalsInteractive.contextOfPath(path) val printer = MetalsPrinter.standard( - IndexedContext(printerContext), + IndexedContext(printerCtx), search, includeDefaultParam = MetalsPrinter.IncludeDefaultParam.Include, ) @@ -152,6 +150,7 @@ object HoverProvider: symbolSignature = Some(hoverString), docstring = Some(docString), forceExpressionType = forceExpressionType, + contextInfo = printer.getUsedRenamesInfo(), ) ) case _ => @@ -185,6 +184,7 @@ object HoverProvider: new ScalaHover( expressionType = Some(tpeString), symbolSignature = Some(s"$valOrDef $name$tpeString"), + contextInfo = printer.getUsedRenamesInfo(), ) ) case RefinedType(parent, _, _) => diff --git a/mtags/src/main/scala-3/scala/meta/internal/pc/printer/DotcPrinter.scala b/mtags/src/main/scala-3/scala/meta/internal/pc/printer/DotcPrinter.scala index 58d331a4f75..565bf6521f2 100644 --- a/mtags/src/main/scala-3/scala/meta/internal/pc/printer/DotcPrinter.scala +++ b/mtags/src/main/scala-3/scala/meta/internal/pc/printer/DotcPrinter.scala @@ -91,9 +91,15 @@ object DotcPrinter: case tp: AppliedType => tp.tycon match case p: PrettyType => - Str(p.toString) ~ "[" ~ toText(tp.args, ", ") ~ "]" + val args: List[Text] = + tp.args.map { + case _: TypeBounds => "?" + case arg => toText(arg) + } + Str(p.toString) ~ "[" ~ Text(args, ", ") ~ "]" case other => super.toText(tp) case other => super.toText(tp) + end Std /** diff --git a/mtags/src/main/scala-3/scala/meta/internal/pc/printer/MetalsPrinter.scala b/mtags/src/main/scala-3/scala/meta/internal/pc/printer/MetalsPrinter.scala index 6e713b466d9..0d3345a6634 100644 --- a/mtags/src/main/scala-3/scala/meta/internal/pc/printer/MetalsPrinter.scala +++ b/mtags/src/main/scala-3/scala/meta/internal/pc/printer/MetalsPrinter.scala @@ -55,6 +55,8 @@ class MetalsPrinter( def shortenedNames: List[ShortName] = names.namesToImport + def getUsedRenamesInfo(): List[String] = names.getUsedRenamesInfo + def expressionType(tpw: Type)(using Context): Option[String] = tpw match case t: PolyType => diff --git a/mtags/src/main/scala-3/scala/meta/internal/pc/printer/ShortenedNames.scala b/mtags/src/main/scala-3/scala/meta/internal/pc/printer/ShortenedNames.scala index e7cfc77a95f..b0a2db93457 100644 --- a/mtags/src/main/scala-3/scala/meta/internal/pc/printer/ShortenedNames.scala +++ b/mtags/src/main/scala-3/scala/meta/internal/pc/printer/ShortenedNames.scala @@ -29,6 +29,13 @@ class ShortenedNames( private val history = collection.mutable.Map.empty[Name, ShortName] + private val foundRenames = collection.mutable.Map.empty[Symbol, String] + + def getUsedRenamesInfo(using Context): List[String] = + foundRenames.map { (from, to) => + s"type $to = ${from.showName}" + }.toList + /** * Returns a list of shortened names */ @@ -137,6 +144,7 @@ class ShortenedNames( // for example, we have `import java.lang.{Boolean => JBoolean}` and // complete `java.lang.Boolean`. See `CompletionOverrideSuite`'s `rename'. case Some(rename) => + foundRenames += (h -> rename) PrettyType( (rename :: prev.map(_.name)).mkString(".") ) @@ -147,6 +155,7 @@ class ShortenedNames( processOwners(sym, Nil, sym.ownersIterator.toList) renames.get(sym.owner) match case Some(rename) => + foundRenames += (sym.owner -> rename) val short = ShortName(Names.termName(rename), sym.owner) if tryShortenName(short) then PrettyType(s"$rename.${sym.name.show}") diff --git a/tests/cross/src/test/scala/tests/hover/HoverDefnSuite.scala b/tests/cross/src/test/scala/tests/hover/HoverDefnSuite.scala index ce1eb2b4ac9..4a685460103 100644 --- a/tests/cross/src/test/scala/tests/hover/HoverDefnSuite.scala +++ b/tests/cross/src/test/scala/tests/hover/HoverDefnSuite.scala @@ -171,7 +171,7 @@ class HoverDefnSuite extends BaseHoverSuite { ) check( - "package", + "package".tag(IgnoreForScala3CompilerPC), """package b.p@@kg |object Main |""".stripMargin, @@ -179,11 +179,7 @@ class HoverDefnSuite extends BaseHoverSuite { |package b.pkg |``` |""".stripMargin, - automaticPackage = false, - compat = Map( - // TODO hover doesn't show information on package - "3" -> "".hover - ) + automaticPackage = false ) check( diff --git a/tests/cross/src/test/scala/tests/hover/HoverTermSuite.scala b/tests/cross/src/test/scala/tests/hover/HoverTermSuite.scala index 93280c6a070..5aaea463977 100644 --- a/tests/cross/src/test/scala/tests/hover/HoverTermSuite.scala +++ b/tests/cross/src/test/scala/tests/hover/HoverTermSuite.scala @@ -621,4 +621,63 @@ class HoverTermSuite extends BaseHoverSuite { |""".stripMargin ) + check( + "import-rename".tag(IgnoreForScala3CompilerPC), + """|import scala.collection.{AbstractMap => AB} + |import scala.collection.{Set => S} + | + |object Main { + | def test(): AB[Int, String] = ??? + | <> + |} + |""".stripMargin, + """|```scala + |type AB = AbstractMap + |``` + | + |```scala + |val tt: AB[Int, String] + |```""".stripMargin, + compat = Map( + "2" -> + """|```scala + |type AB = AbstractMap + |``` + | + |```scala + |val tt: AB[Int,String] + |``` + |""".stripMargin + ) + ) + + check( + "import-rename2".tag(IgnoreForScala3CompilerPC), + """|import scala.collection.{AbstractMap => AB} + |import scala.collection.{Set => S} + | + |object Main { + | <> + |} + |""".stripMargin, + """|```scala + |type S = Set + |type AB = AbstractMap + |``` + | + |```scala + |def test(d: S[Int], f: S[Char]): AB[Int, String] + |```""".stripMargin, + compat = Map( + "2" -> """|```scala + |type AB = AbstractMap + |type S = Set + |``` + | + |```scala + |def test(d: S[Int], f: S[Char]): AB[Int,String] + |```""".stripMargin + ) + ) + } diff --git a/tests/cross/src/test/scala/tests/pc/InsertInferredTypeSuite.scala b/tests/cross/src/test/scala/tests/pc/InsertInferredTypeSuite.scala index cc6b615de81..d01e7566454 100644 --- a/tests/cross/src/test/scala/tests/pc/InsertInferredTypeSuite.scala +++ b/tests/cross/src/test/scala/tests/pc/InsertInferredTypeSuite.scala @@ -908,6 +908,36 @@ class InsertInferredTypeSuite extends BaseCodeActionSuite { |""".stripMargin ) + checkEdit( + "rename-import", + """|package a + |import scala.collection.{AbstractMap => AB} + | + |object Main { + | def test(): AB[Int, String] = ??? + | val <> = test() + |} + |""".stripMargin, + """|package a + |import scala.collection.{AbstractMap => AB} + | + |object Main { + | def test(): AB[Int, String] = ??? + | val x: AB[Int,String] = test() + |} + |""".stripMargin, + compat = Map( + "3" -> """|package a + |import scala.collection.{AbstractMap => AB} + | + |object Main { + | def test(): AB[Int, String] = ??? + | val x: AB[Int, String] = test() + |} + |""".stripMargin + ) + ) + def checkEdit( name: TestOptions, original: String,