From e9b3bf24a00af6961658bf156b187da6b00117fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C4=99drzej=20Rochala?= <48657087+rochala@users.noreply.github.com> Date: Fri, 10 Nov 2023 11:46:45 +0100 Subject: [PATCH] Completion assert diffs will now show completion source (#18890) Previously failed assertions showed a side by side diff of expected vs obtained completions: Screenshot 2023-11-09 at 18 28 46 And now: Screenshot 2023-11-09 at 18 29 31 This is extremely useful when debugging the completions, as we have multiple sources and finding what specific completion comes from is just a waste of time. There is also a chance that we'd want to not include this info in data, but this is minimal trade-off for a significant boost when working on PC. [Cherry-picked e196dec866f81d523bfc8bdcb9c1cf029333cc0c] --- .../pc/completions/CompletionProvider.scala | 5 +- .../pc/completions/CompletionValue.scala | 57 +++++++++++++---- .../tools/pc/base/BaseCompletionSuite.scala | 17 ++++-- .../dotty/tools/pc/base/BasePCSuite.scala | 10 ++- .../pc/base/BaseSignatureHelpSuite.scala | 6 +- .../dotty/tools/pc/utils/PcAssertions.scala | 61 +++++++++++++------ .../pc/utils/TestingWorkspaceSearch.scala | 6 +- 7 files changed, 113 insertions(+), 49 deletions(-) diff --git a/presentation-compiler/src/main/dotty/tools/pc/completions/CompletionProvider.scala b/presentation-compiler/src/main/dotty/tools/pc/completions/CompletionProvider.scala index 13a6e7cdb7cb..0b7b3d691bda 100644 --- a/presentation-compiler/src/main/dotty/tools/pc/completions/CompletionProvider.scala +++ b/presentation-compiler/src/main/dotty/tools/pc/completions/CompletionProvider.scala @@ -181,9 +181,8 @@ class CompletionProvider( item.setAdditionalTextEdits((completion.additionalEdits ++ additionalEdits).asJava) completion.insertMode.foreach(item.setInsertTextMode) - completion - .completionData(buildTargetIdentifier) - .foreach(data => item.setData(data.toJson)) + val data = completion.completionData(buildTargetIdentifier) + item.setData(data.toJson) item.setTags(completion.lspTags.asJava) diff --git a/presentation-compiler/src/main/dotty/tools/pc/completions/CompletionValue.scala b/presentation-compiler/src/main/dotty/tools/pc/completions/CompletionValue.scala index 0cdd4eb257c4..def023320d0d 100644 --- a/presentation-compiler/src/main/dotty/tools/pc/completions/CompletionValue.scala +++ b/presentation-compiler/src/main/dotty/tools/pc/completions/CompletionValue.scala @@ -16,6 +16,24 @@ import org.eclipse.lsp4j.InsertTextMode import org.eclipse.lsp4j.Range import org.eclipse.lsp4j.TextEdit +enum CompletionSource: + case Empty + case OverrideKind + case ImplementAllKind + case CompilerKind + case KeywordKind + case ScopeKind + case WorkspaceKind + case ExtensionKind + case NamedArgKind + case AutoFillKind + case FileSystemMemberKind + case IvyImportKind + case InterpolatorKind + case MatchCompletionKind + case CaseKeywordKind + case DocumentKind + sealed trait CompletionValue: def label: String def insertText: Option[String] = None @@ -24,11 +42,12 @@ sealed trait CompletionValue: def range: Option[Range] = None def filterText: Option[String] = None def completionItemKind(using Context): CompletionItemKind + def completionItemDataKind: Integer = CompletionItemData.None def description(printer: ShortenedTypePrinter)(using Context): String = "" def insertMode: Option[InsertTextMode] = None def completionData(buildTargetIdentifier: String)( using Context - ): Option[CompletionItemData] = None + ): CompletionItemData = CompletionItemData("", buildTargetIdentifier, kind = completionItemDataKind) def command: Option[String] = None /** @@ -44,17 +63,15 @@ object CompletionValue: sealed trait Symbolic extends CompletionValue: def symbol: Symbol def isFromWorkspace: Boolean = false - def completionItemDataKind = CompletionItemData.None + override def completionItemDataKind = CompletionItemData.None override def completionData( buildTargetIdentifier: String - )(using Context): Option[CompletionItemData] = - Some( - CompletionItemData( - SemanticdbSymbols.symbolName(symbol), - buildTargetIdentifier, - kind = completionItemDataKind - ) + )(using Context): CompletionItemData = + CompletionItemData( + SemanticdbSymbols.symbolName(symbol), + buildTargetIdentifier, + kind = completionItemDataKind ) def importSymbol: Symbol = symbol @@ -106,12 +123,16 @@ object CompletionValue: label: String, symbol: Symbol, override val snippetSuffix: CompletionSuffix - ) extends Symbolic + ) extends Symbolic { + override def completionItemDataKind: Integer = CompletionSource.CompilerKind.ordinal + } case class Scope( label: String, symbol: Symbol, override val snippetSuffix: CompletionSuffix, - ) extends Symbolic + ) extends Symbolic { + override def completionItemDataKind: Integer = CompletionSource.ScopeKind.ordinal + } case class Workspace( label: String, symbol: Symbol, @@ -119,6 +140,7 @@ object CompletionValue: override val importSymbol: Symbol ) extends Symbolic: override def isFromWorkspace: Boolean = true + override def completionItemDataKind: Integer = CompletionSource.WorkspaceKind.ordinal /** * CompletionValue for extension methods via SymbolSearch @@ -130,6 +152,7 @@ object CompletionValue: ) extends Symbolic: override def completionItemKind(using Context): CompletionItemKind = CompletionItemKind.Method + override def completionItemDataKind: Integer = CompletionSource.ExtensionKind.ordinal override def description(printer: ShortenedTypePrinter)(using Context): String = s"${printer.completionSymbol(symbol)} (extension)" @@ -150,8 +173,7 @@ object CompletionValue: override val range: Option[Range] ) extends Symbolic: override def insertText: Option[String] = Some(value) - override def completionItemDataKind: Integer = - CompletionItemData.OverrideKind + override def completionItemDataKind: Integer = CompletionSource.OverrideKind.ordinal override def completionItemKind(using Context): CompletionItemKind = CompletionItemKind.Method override def labelWithDescription(printer: ShortenedTypePrinter)(using Context): String = @@ -164,6 +186,7 @@ object CompletionValue: symbol: Symbol ) extends Symbolic: override def insertText: Option[String] = Some(label.replace("$", "$$").nn) + override def completionItemDataKind: Integer = CompletionSource.OverrideKind.ordinal override def completionItemKind(using Context): CompletionItemKind = CompletionItemKind.Field override def description(printer: ShortenedTypePrinter)(using Context): String = @@ -178,11 +201,13 @@ object CompletionValue: ) extends CompletionValue: override def completionItemKind(using Context): CompletionItemKind = CompletionItemKind.Enum + override def completionItemDataKind: Integer = CompletionSource.OverrideKind.ordinal override def insertText: Option[String] = Some(value) override def label: String = "Autofill with default values" case class Keyword(label: String, override val insertText: Option[String]) extends CompletionValue: + override def completionItemDataKind: Integer = CompletionSource.KeywordKind.ordinal override def completionItemKind(using Context): CompletionItemKind = CompletionItemKind.Keyword @@ -193,6 +218,7 @@ object CompletionValue: ) extends CompletionValue: override def label: String = filename override def insertText: Option[String] = Some(filename.stripSuffix(".sc")) + override def completionItemDataKind: Integer = CompletionSource.FileSystemMemberKind.ordinal override def completionItemKind(using Context): CompletionItemKind = CompletionItemKind.File @@ -202,6 +228,7 @@ object CompletionValue: override val range: Option[Range] ) extends CompletionValue: override val filterText: Option[String] = insertText + override def completionItemDataKind: Integer = CompletionSource.IvyImportKind.ordinal override def completionItemKind(using Context): CompletionItemKind = CompletionItemKind.Folder @@ -216,6 +243,7 @@ object CompletionValue: isWorkspace: Boolean = false, isExtension: Boolean = false ) extends Symbolic: + override def completionItemDataKind: Integer = CompletionSource.InterpolatorKind.ordinal override def description( printer: ShortenedTypePrinter )(using Context): String = @@ -229,6 +257,7 @@ object CompletionValue: override val additionalEdits: List[TextEdit], desc: String ) extends CompletionValue: + override def completionItemDataKind: Integer = CompletionSource.MatchCompletionKind.ordinal override def completionItemKind(using Context): CompletionItemKind = CompletionItemKind.Enum override def description(printer: ShortenedTypePrinter)(using Context): String = @@ -242,6 +271,7 @@ object CompletionValue: override val range: Option[Range] = None, override val command: Option[String] = None ) extends Symbolic: + override def completionItemDataKind: Integer = CompletionSource.CaseKeywordKind.ordinal override def completionItemKind(using Context): CompletionItemKind = CompletionItemKind.Method @@ -254,6 +284,7 @@ object CompletionValue: override def filterText: Option[String] = Some(description) override def insertText: Option[String] = Some(doc) + override def completionItemDataKind: Integer = CompletionSource.DocumentKind.ordinal override def completionItemKind(using Context): CompletionItemKind = CompletionItemKind.Snippet diff --git a/presentation-compiler/test/dotty/tools/pc/base/BaseCompletionSuite.scala b/presentation-compiler/test/dotty/tools/pc/base/BaseCompletionSuite.scala index 8314c9370fca..964f6a6894a2 100644 --- a/presentation-compiler/test/dotty/tools/pc/base/BaseCompletionSuite.scala +++ b/presentation-compiler/test/dotty/tools/pc/base/BaseCompletionSuite.scala @@ -8,6 +8,7 @@ import scala.meta.internal.metals.{CompilerOffsetParams, EmptyCancelToken} import scala.meta.pc.CancelToken import scala.language.unsafeNulls +import dotty.tools.pc.completions.CompletionSource import dotty.tools.pc.utils.MtagsEnrichments.* import dotty.tools.pc.utils.{TestCompletions, TextEdits} @@ -172,7 +173,7 @@ abstract class BaseCompletionSuite extends BasePCSuite: } .mkString("\n") - assertCompletions(expected, obtained, Some(original)) + assertWithDiff(expected, obtained, includeSources = false, Some(original)) /** * Check completions that will be shown in original param after `@@` marker @@ -245,13 +246,19 @@ abstract class BaseCompletionSuite extends BasePCSuite: .append(commitCharacter) .append("\n") } - val expectedResult = sortLines(stableOrder, expected) - val actualResult = sortLines( + val completionSources = filteredItems + .map(_.data.map(data => CompletionSource.fromOrdinal(data.kind)) + .getOrElse(CompletionSource.Empty)) + .toList + + val (expectedResult, _) = sortLines(stableOrder, expected) + val (actualResult, sources) = sortLines( stableOrder, - postProcessObtained(trimTrailingSpace(out.toString())) + postProcessObtained(trimTrailingSpace(out.toString())), + completionSources, ) - assertCompletions(expectedResult, actualResult, Some(original)) + assertWithDiff(expectedResult, actualResult, includeSources = true, Some(original), sources) if (filterText.nonEmpty) { filteredItems.foreach { item => diff --git a/presentation-compiler/test/dotty/tools/pc/base/BasePCSuite.scala b/presentation-compiler/test/dotty/tools/pc/base/BasePCSuite.scala index 7b7e7529f2f6..7d491df5c07a 100644 --- a/presentation-compiler/test/dotty/tools/pc/base/BasePCSuite.scala +++ b/presentation-compiler/test/dotty/tools/pc/base/BasePCSuite.scala @@ -14,6 +14,7 @@ import scala.meta.pc.{PresentationCompiler, PresentationCompilerConfig} import scala.language.unsafeNulls import dotty.tools.pc.* +import dotty.tools.pc.completions.CompletionSource import dotty.tools.pc.ScalaPresentationCompiler import dotty.tools.pc.tests.buildinfo.BuildInfo import dotty.tools.pc.utils._ @@ -113,10 +114,13 @@ abstract class BasePCSuite extends PcAssertions: " " + e.getRight.getValue }.trim - def sortLines(stableOrder: Boolean, string: String): String = + def sortLines(stableOrder: Boolean, string: String, completionSources: List[CompletionSource] = Nil): (String, List[CompletionSource]) = val strippedString = string.linesIterator.toList.filter(_.nonEmpty) - if (stableOrder) strippedString.mkString("\n") - else strippedString.sorted.mkString("\n") + if (stableOrder) strippedString.mkString("\n") -> completionSources + else + val paddedSources = completionSources.padTo(strippedString.size, CompletionSource.Empty) + val (sortedCompletions, sortedSources) = (strippedString zip paddedSources).sortBy(_._1).unzip + sortedCompletions.mkString("\n") -> sortedSources extension (s: String) def triplequoted: String = s.replace("'''", "\"\"\"") diff --git a/presentation-compiler/test/dotty/tools/pc/base/BaseSignatureHelpSuite.scala b/presentation-compiler/test/dotty/tools/pc/base/BaseSignatureHelpSuite.scala index f993bb49921e..0022be40a630 100644 --- a/presentation-compiler/test/dotty/tools/pc/base/BaseSignatureHelpSuite.scala +++ b/presentation-compiler/test/dotty/tools/pc/base/BaseSignatureHelpSuite.scala @@ -84,6 +84,6 @@ abstract class BaseSignatureHelpSuite extends BasePCSuite: } } - val obtainedSorted = sortLines(stableOrder, out.toString()) - val expectedSorted = sortLines(stableOrder, expected) - assertCompletions(expectedSorted, obtainedSorted, Some(original)) + val (obtainedSorted, _) = sortLines(stableOrder, out.toString()) + val (expectedSorted, _) = sortLines(stableOrder, expected) + assertWithDiff(expectedSorted, obtainedSorted, includeSources = false, Some(original)) diff --git a/presentation-compiler/test/dotty/tools/pc/utils/PcAssertions.scala b/presentation-compiler/test/dotty/tools/pc/utils/PcAssertions.scala index b77ad7f64bde..5efb0feaeb9a 100644 --- a/presentation-compiler/test/dotty/tools/pc/utils/PcAssertions.scala +++ b/presentation-compiler/test/dotty/tools/pc/utils/PcAssertions.scala @@ -2,6 +2,7 @@ package dotty.tools.pc.utils import scala.language.unsafeNulls +import dotty.tools.pc.completions.CompletionSource import dotty.tools.dotc.util.DiffUtil import dotty.tools.pc.utils.MtagsEnrichments.* @@ -10,20 +11,22 @@ import org.hamcrest.* trait PcAssertions: - def assertCompletions( + def assertWithDiff( expected: String, actual: String, - snippet: Option[String] = None + includeSources: Boolean, + snippet: Option[String] = None, + completionSources: List[CompletionSource] = Nil, ): Unit = val longestExpeceted = - expected.linesIterator.maxByOption(_.length).map(_.length).getOrElse(0) + expected.linesIterator.map(_.length).maxOption.getOrElse(0) val longestActual = - actual.linesIterator.maxByOption(_.length).map(_.length).getOrElse(0) + actual.linesIterator.map(_.length).maxOption.getOrElse(0) val actualMatcher = if longestActual >= 40 || longestExpeceted >= 40 then - lineByLineDiffMatcher(expected) - else sideBySideDiffMatcher(expected) + lineByLineDiffMatcher(expected, completionSources, includeSources) + else sideBySideDiffMatcher(expected, completionSources, includeSources) assertThat(actual, actualMatcher, snippet) @@ -115,27 +118,51 @@ trait PcAssertions: error.setStackTrace(Array.empty) throw error - private def lineByLineDiffMatcher(expected: String): TypeSafeMatcher[String] = + + private def lineByLineDiffMatcher( + expected: String, + completionSources: List[CompletionSource] = Nil, + isCompletion: Boolean = false + ): TypeSafeMatcher[String] = + def getDetailedMessage(diff: String): String = + val lines = diff.linesIterator.toList + val sources = completionSources.padTo(lines.size, CompletionSource.Empty) + val maxLength = lines.map(_.length).maxOption.getOrElse(0) + var redLineIndex = 0 + lines.map: line => + if line.startsWith(Console.BOLD + Console.RED) then + redLineIndex = redLineIndex + 1 + s"$line | [${sources(redLineIndex - 1)}]" + else + line + .mkString("\n") + new TypeSafeMatcher[String]: override def describeMismatchSafely( item: String, mismatchDescription: org.hamcrest.Description ): Unit = + val diff = DiffUtil.mkColoredHorizontalLineDiff(unifyNewlines(expected), unifyNewlines(item)) + val maybeEnhancedDiff = if isCompletion then getDetailedMessage(diff) else diff mismatchDescription.appendText(System.lineSeparator) - mismatchDescription.appendText( - DiffUtil.mkColoredHorizontalLineDiff( - unifyNewlines(expected), - unifyNewlines(item) - ) - ) + mismatchDescription.appendText(maybeEnhancedDiff) mismatchDescription.appendText(System.lineSeparator) override def describeTo(description: org.hamcrest.Description): Unit = () override def matchesSafely(item: String): Boolean = unifyNewlines(expected) == unifyNewlines(item) - private def sideBySideDiffMatcher(expected: String): TypeSafeMatcher[String] = + private def sideBySideDiffMatcher( + expected: String, + completionSources: List[CompletionSource] = Nil, + isCompletion: Boolean = false + ): TypeSafeMatcher[String] = + def getDetailedMessage(diff: String): String = + val lines = diff.linesIterator.toList + val sources = completionSources.padTo(lines.size, CompletionSource.Empty) + (lines zip sources).map((line, source) => s"$line | [$source]").mkString("\n") + new TypeSafeMatcher[String]: override def describeMismatchSafely( @@ -147,10 +174,10 @@ trait PcAssertions: val expectedLines = cleanedExpected.linesIterator.toSeq val actualLines = cleanedActual.linesIterator.toSeq + val diff = DiffUtil.mkColoredLineDiff(expectedLines, actualLines) + val maybeEnhancedDiff = if isCompletion then getDetailedMessage(diff) else diff - mismatchDescription.appendText( - DiffUtil.mkColoredLineDiff(expectedLines, actualLines) - ) + mismatchDescription.appendText(maybeEnhancedDiff) mismatchDescription.appendText(System.lineSeparator) override def describeTo(description: org.hamcrest.Description): Unit = () diff --git a/presentation-compiler/test/dotty/tools/pc/utils/TestingWorkspaceSearch.scala b/presentation-compiler/test/dotty/tools/pc/utils/TestingWorkspaceSearch.scala index 3e8f6d261155..0b49bdf8bca8 100644 --- a/presentation-compiler/test/dotty/tools/pc/utils/TestingWorkspaceSearch.scala +++ b/presentation-compiler/test/dotty/tools/pc/utils/TestingWorkspaceSearch.scala @@ -45,11 +45,7 @@ class TestingWorkspaceSearch(classpath: Seq[String]): val nioPath = Paths.get(path) val uri = nioPath.toUri() - val symbols = - DefSymbolCollector( - driver, - CompilerVirtualFileParams(uri, text) - ).namedDefSymbols + val symbols = DefSymbolCollector(driver, CompilerVirtualFileParams(uri, text)).namedDefSymbols // We have to map symbol from this Context, to one in PresentationCompiler // To do it we are searching it with semanticdb symbol