From dc21706d9256807977910c1e5c5cb2280a9755ef Mon Sep 17 00:00:00 2001 From: Krzysztof Bochenek Date: Tue, 18 Feb 2020 21:42:04 +0100 Subject: [PATCH] Review changes update --- .../ImplementationProvider.scala | 188 ++++++++++-------- .../implementation/MethodImplementation.scala | 5 +- .../internal/metals/ReferenceProvider.scala | 85 ++++---- .../meta/internal/rename/RenameProvider.scala | 6 +- .../src/test/scala/tests/RenameLspSuite.scala | 3 +- 5 files changed, 168 insertions(+), 119 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/implementation/ImplementationProvider.scala b/metals/src/main/scala/scala/meta/internal/implementation/ImplementationProvider.scala index c1d5b238d89..ce26732fa18 100644 --- a/metals/src/main/scala/scala/meta/internal/implementation/ImplementationProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/implementation/ImplementationProvider.scala @@ -85,71 +85,87 @@ final class ImplementationProvider( def implementations(position: TextDocumentPositionParams): List[Location] = { implementations( - FilePosition( - position.getTextDocument.getUri.toAbsolutePath, - position.getPosition - ) + Left( + FilePosition( + position.getTextDocument.getUri.toAbsolutePath, + position.getPosition + ) + ), + position.getTextDocument.getUri.toAbsolutePath ) } def implementations( - filePosition: FilePosition + filePositionOrSymbol: Either[FilePosition, SymbolInformation], + source: AbsolutePath ): List[Location] = { - lazy val global = globalTable.globalSymbolTableFor(filePosition.filePath) - val locations = for { - (symbolOccurrence, currentDocument) <- definitionProvider - .symbolOccurrence(filePosition) - .toIterable - } yield { - // 1. Search locally for symbol - // 2. Search inside workspace - // 3. Search classpath via GlobalSymbolTable - def symbolSearch(symbol: String): Option[SymbolInformation] = { - findSymbol(currentDocument, symbol) - .orElse(findSymbolDef(symbol)) - .orElse(global.flatMap(_.safeInfo(symbol))) - } - val sym = symbolOccurrence.symbol - val dealiased = - if (sym.desc.isType) dealiasClass(sym, symbolSearch _) else sym - - val definitionDocument = - if (currentDocument.definesSymbol(dealiased)) { - Some(currentDocument) - } else { - findSemanticDbForSymbol(dealiased) + filePositionOrSymbol match { + case Left(filePosition) => + definitionProvider.symbolOccurrence(filePosition) match { + case Some((so, doc)) => + implementations(Some(doc), so.symbol, source) + case None => + List.empty } + case Right(symbolInformation) => + implementations(None, symbolInformation.symbol, source) + } + } + + private def implementations( + currentDocument: Option[TextDocument], + symbol: String, + source: AbsolutePath + ): List[Location] = { + lazy val global = globalTable.globalSymbolTableFor(source) + + // 1. Search locally for symbol + // 2. Search inside workspace + // 3. Search classpath via GlobalSymbolTable + def symbolSearch(symbol: String): Option[SymbolInformation] = { + currentDocument + .flatMap(doc => findSymbol(doc, symbol)) + .orElse(findSymbolDef(symbol)) + .orElse(global.flatMap(_.safeInfo(symbol))) + } - val inheritanceContext = definitionDocument match { - // symbol is not in workspace, we only search classpath for it - case None => - globalTable.globalContextFor( - filePosition.filePath, + val dealiased = + if (symbol.desc.isType) dealiasClass(symbol, symbolSearch _) else symbol + + val definitionDocument = + currentDocument.flatMap(doc => + if (doc.definesSymbol(dealiased)) currentDocument + else findSemanticDbForSymbol(dealiased) + ) + + val inheritanceContext = definitionDocument match { + // symbol is not in workspace, we only search classpath for it + case None => + globalTable.globalContextFor( + source, + implementationsInPath.asScala.toMap + ) + // symbol is in workspace, + // we might need to search different places for related symbols + case Some(textDocument) => + Some( + InheritanceContext.fromDefinitions( + symbolSearch, implementationsInPath.asScala.toMap ) - // symbol is in workspace, - // we might need to search different places for related symbols - case Some(textDocument) => - Some( - InheritanceContext.fromDefinitions( - symbolSearch, - implementationsInPath.asScala.toMap - ) - ) - } - symbolLocationsFromContext( - dealiased, - filePosition.filePath, - inheritanceContext - ) + ) } - locations.flatten.toList + symbolLocationsFromContext( + dealiased, + source, + inheritanceContext + ).toList } def topMethodParents( doc: TextDocument, symbol: String - ): Seq[Location] = { + ): Seq[Either[Location, SymbolInformation]] = { // location in semanticDB for symbol might not be present when symbol is local then it must be in current document val textDocument = findSemanticDbForSymbol(symbol).getOrElse(doc) @@ -173,9 +189,16 @@ final class ImplementationProvider( classInfo <- findClassInfo(symbol.owner) } yield { classInfo.signature match { - case sig: ClassSignature => - methodInParentSignature(sig, currentInfo) - case _ => Nil + case classSignature: ClassSignature => + val globalSymbolTable = globalTable.globalSymbolTableFor( + workspace.resolve(textDocument.uri) + ) + methodInParentSignature( + classSignature, + currentInfo, + globalSymbolTable + ) + case _ => Seq.empty } } results.getOrElse(Seq.empty) @@ -184,13 +207,18 @@ final class ImplementationProvider( private def methodInParentSignature( sig: ClassSignature, childInfo: SymbolInformation, + globalSymbolTable: Option[GlobalSymbolTable], childASF: Map[String, String] = Map.empty - ): Seq[Location] = { + ): Seq[Either[Location, SymbolInformation]] = { sig.parents.flatMap { - case parentSym: TypeRef => + case parentSym: TypeRef if parentSym.symbol != "scala/AnyRef#" => val parentTextDocument = findSemanticDbForSymbol(parentSym.symbol) - def search(symbol: String) = - parentTextDocument.flatMap(findSymbol(_, symbol)) + + def search(symbol: String): Option[SymbolInformation] = + parentTextDocument + .flatMap(findSymbol(_, symbol)) + .orElse(globalSymbolTable.flatMap(_.safeInfo(symbol))) + val parentASF = AsSeenFrom.calculateAsSeenFrom(parentSym, sig.typeParameters) val asSeenFrom = AsSeenFrom.translateAsSeenFrom(childASF, parentASF) @@ -199,6 +227,7 @@ final class ImplementationProvider( val fromParent = methodInParentSignature( parenClassSig, childInfo, + globalSymbolTable, asSeenFrom ) if (fromParent.isEmpty) { @@ -213,10 +242,10 @@ final class ImplementationProvider( } else { fromParent } - case _ => Nil + case _ => Seq.empty } - case _ => Nil + case _ => Seq.empty } } @@ -227,31 +256,34 @@ final class ImplementationProvider( asSeenFrom: Map[String, String], search: String => Option[SymbolInformation], parentTextDocument: Option[TextDocument] - ): Option[Location] = { + ): Option[Either[Location, SymbolInformation]] = { val matchingSymbol = MethodImplementation.findParentSymbol( childInfo, - sig, parenClassSig, asSeenFrom, search ) - for { - symbol <- matchingSymbol - parentDoc <- parentTextDocument - source = workspace.resolve(parentDoc.uri) - implOccurrence <- findDefOccurrence( - parentDoc, - symbol, - source - ) - range <- implOccurrence.range - distance = TokenEditDistance.fromBuffer( - source, - parentDoc.text, - buffer - ) - revised <- distance.toRevised(range.toLSP) - } yield new Location(source.toNIO.toUri.toString, revised) + if (matchingSymbol.isDefined && parentTextDocument.isEmpty) { + Some(Right(matchingSymbol.get)) + } else { + for { + symbol <- matchingSymbol + parentDoc <- parentTextDocument + source = workspace.resolve(parentDoc.uri) + implOccurrence <- findDefOccurrence( + parentDoc, + symbol.symbol, + source + ) + range <- implOccurrence.range + distance = TokenEditDistance.fromBuffer( + source, + parentDoc.text, + buffer + ) + revised <- distance.toRevised(range.toLSP) + } yield Left(new Location(source.toNIO.toUri.toString, revised)) + } } private def symbolLocationsFromContext( diff --git a/metals/src/main/scala/scala/meta/internal/implementation/MethodImplementation.scala b/metals/src/main/scala/scala/meta/internal/implementation/MethodImplementation.scala index 4898a9101c9..042d6190509 100644 --- a/metals/src/main/scala/scala/meta/internal/implementation/MethodImplementation.scala +++ b/metals/src/main/scala/scala/meta/internal/implementation/MethodImplementation.scala @@ -30,11 +30,10 @@ object MethodImplementation { def findParentSymbol( childSymbol: SymbolInformation, - childClassSig: ClassSignature, parentClassSig: ClassSignature, asSeenFromMap: Map[String, String], findSymbol: String => Option[SymbolInformation] - ): Option[String] = { + ): Option[SymbolInformation] = { val validMethods = for { declarations <- parentClassSig.declarations.toIterable methodSymbol <- declarations.symlinks @@ -52,7 +51,7 @@ object MethodImplementation { if isOverridenMethod(methodSymbolInfo, childSymbol, findParent = true)( context ) - } yield methodSymbol + } yield methodSymbolInfo validMethods.headOption } diff --git a/metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala index 08688ab7d33..80add4718c3 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala @@ -94,10 +94,12 @@ final class ReferenceProvider( maybeOccurrence match { case Some(occurrence) => val symbolName = occurrence.symbol.desc.name.value - val shouldIncludeInheritance = - ReferenceProvider.methodsSearchedWithoutInheritance.contains(symbolName) + val shouldNotIncludeInheritance = + ReferenceProvider.methodsSearchedWithoutInheritance.contains( + symbolName + ) - if (shouldIncludeInheritance) { + if (shouldNotIncludeInheritance) { currentSymbolReferences( filePosition, includeDeclaration @@ -109,7 +111,8 @@ final class ReferenceProvider( occurrence, doc, filePosition, - _ => true + failWhenReachingDependencySymbol = false, + fnIncludeSynthetics = _ => true ) ) } @@ -126,51 +129,65 @@ final class ReferenceProvider( doc: TextDocument, filePosition: FilePosition, fnIncludeSynthetics: Synthetic => Boolean, + failWhenReachingDependencySymbol: Boolean, canSkipExactMatchCheck: Boolean = true ): Seq[Location] = { val parentSymbols = implementation .topMethodParents(doc, symbolOccurrence.symbol) - val parentPositions: Seq[FilePosition] = { - if (parentSymbols.isEmpty) List(filePosition) - else parentSymbols.map(locationToFilePosition) - } - val isLocal = symbolOccurrence.symbol.isLocal - val currentReferences = parentPositions - .flatMap( - currentSymbolReferences( + + if (failWhenReachingDependencySymbol && parentSymbols.exists(_.isRight)) { + Seq.empty + } else { + + val mainDefinitions: Seq[Either[FilePosition, SymbolInformation]] = { + if (parentSymbols.isEmpty) List(Left(filePosition)) + else parentSymbols.map(ps => ps.left.map(locationToFilePosition)) + } + + val topParentWorkspaceLocations = + mainDefinitions.flatMap(_.swap.toOption) + + val isLocal = symbolOccurrence.symbol.isLocal + val currentReferences = topParentWorkspaceLocations + .flatMap( + currentSymbolReferences( + _, + includeDeclaration = isLocal, + canSkipExactMatchCheck = canSkipExactMatchCheck, + includeSynthetics = fnIncludeSynthetics + ).locations + ) + val definitionLocation = { + val parentSymbolLocs = parentSymbols.flatMap(_.left.toOption) + if (parentSymbolLocs.isEmpty) + definition + .fromSymbol(symbolOccurrence.symbol) + .asScala + .filter(_.getUri.isScalaFilename) + else parentSymbolLocs + } + val implReferences = mainDefinitions.flatMap( + implementations( _, - includeDeclaration = isLocal, - canSkipExactMatchCheck = canSkipExactMatchCheck, - includeSynthetics = fnIncludeSynthetics - ).locations - ) - val definitionLocation = { - if (parentSymbols.isEmpty) - definition - .fromSymbol(symbolOccurrence.symbol) - .asScala - .filter(_.getUri.isScalaFilename) - else parentSymbols - } - val implReferences = parentPositions.flatMap( - implementations( - _, - !symbolOccurrence.symbol.desc.isType, - canSkipExactMatchCheck + filePosition.filePath, + !symbolOccurrence.symbol.desc.isType, + canSkipExactMatchCheck + ) ) - ) - currentReferences ++ implReferences ++ definitionLocation + currentReferences ++ implReferences ++ definitionLocation + } } private def implementations( - filePosition: FilePosition, + filePosition: Either[FilePosition, SymbolInformation], + source: AbsolutePath, shouldCheckImplementation: Boolean, canSkipExactMatchCheck: Boolean ): Seq[Location] = { if (shouldCheckImplementation) { for { - implLoc <- implementation.implementations(filePosition) + implLoc <- implementation.implementations(filePosition, source) loc <- currentSymbolReferences( locationToFilePosition(implLoc), includeDeclaration = true, diff --git a/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala b/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala index ddc40a741d3..181e4b0c232 100644 --- a/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala @@ -96,7 +96,8 @@ final class RenameProvider( doc, filePosition, includeSynthetic, - canSkipExactMatchCheck = false + canSkipExactMatchCheck = false, + failWhenReachingDependencySymbol = true ) ++ companionReferences( occurrence.symbol ) @@ -205,7 +206,8 @@ final class RenameProvider( companionLocs <- referenceProvider .currentSymbolReferences( locationToFilePosition(loc), - includeDeclaration = false + includeDeclaration = false, + canSkipExactMatchCheck = true ) .locations :+ loc } yield companionLocs diff --git a/tests/unit/src/test/scala/tests/RenameLspSuite.scala b/tests/unit/src/test/scala/tests/RenameLspSuite.scala index e8b6811bdc4..d3c50703b21 100644 --- a/tests/unit/src/test/scala/tests/RenameLspSuite.scala +++ b/tests/unit/src/test/scala/tests/RenameLspSuite.scala @@ -640,8 +640,7 @@ class RenameLspSuite extends BaseLspSuite("rename") { ) } - val openedFiles = files.keySet - .filterNot(file => nonOpened.contains(file)) + val openedFiles = files.keySet.diff(nonOpened) val fullInput = input.replaceAll(allMarkersRegex, "") for { _ <- server.initialize(