From 7ee485566c4ec22355b0e488930b88ad948a18cd Mon Sep 17 00:00:00 2001 From: Martin Duhem Date: Mon, 27 Aug 2018 10:47:45 +0200 Subject: [PATCH 1/2] Fix #4995: Exclude constructors when finding refs When doing a rename on a class, we used to rename the class and its constructor, which yield to incorrect code as shown in #4995. Similarly, finding the references of a class also shows the constructor. To fix this issue, we omit constructors when finding the references. --- .../languageserver/DottyLanguageServer.scala | 9 ++-- .../tools/languageserver/ReferencesTest.scala | 29 +++++++++++ .../tools/languageserver/RenameTest.scala | 49 ++++++++++++++++++- 3 files changed, 82 insertions(+), 5 deletions(-) diff --git a/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala b/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala index b8aa84aec9fe..29b66e0c627b 100644 --- a/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala +++ b/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala @@ -283,9 +283,11 @@ class DottyLanguageServer extends LanguageServer // only need to look for trees in the target directory if the symbol is defined in the // current project val trees = driver.allTreesContaining(sym.name.sourceModuleName.toString) - val refs = Interactive.namedTrees(trees, includeReferences = true, (tree: tpd.NameTree) => - (includeDeclaration || !Interactive.isDefinition(tree)) - && Interactive.matchSymbol(tree, sym, Include.overriding)) + val refs = Interactive.namedTrees(trees, includeReferences = true, tree => + tree.pos.isSourceDerived + && (includeDeclaration || !Interactive.isDefinition(tree)) + && !tree.symbol.isConstructor + && (Interactive.matchSymbol(tree, sym, Include.overriding))) refs.map(ref => location(ref.namePos)).asJava } @@ -307,6 +309,7 @@ class DottyLanguageServer extends LanguageServer val refs = Interactive.namedTrees(trees, includeReferences = true, tree => tree.pos.isSourceDerived + && !tree.symbol.isConstructor && (Interactive.matchSymbol(tree, sym, Include.overriding) || (linkedSym != NoSymbol && Interactive.matchSymbol(tree, linkedSym, Include.overriding)))) diff --git a/language-server/test/dotty/tools/languageserver/ReferencesTest.scala b/language-server/test/dotty/tools/languageserver/ReferencesTest.scala index 427b2d244d5f..53ddc8c7227e 100644 --- a/language-server/test/dotty/tools/languageserver/ReferencesTest.scala +++ b/language-server/test/dotty/tools/languageserver/ReferencesTest.scala @@ -19,4 +19,33 @@ class ReferencesTest { .references(m1 to m2, List(m1 to m2, m3 to m4, m5 to m6), withDecl = true) } + @Test def classReference0: Unit = { + code"class ${m1}Foo${m2} { val a = new ${m3}Foo${m4} }".withSource + .references(m1 to m2, List(m1 to m2, m3 to m4), withDecl = true) + .references(m1 to m2, List(m3 to m4), withDecl = false) + .references(m3 to m4, List(m1 to m2, m3 to m4), withDecl = true) + .references(m3 to m4, List(m3 to m4), withDecl = false) + } + + @Test def classReference1: Unit = { + code"class ${m1}Foo${m2}(x: Int) { val a = new ${m3}Foo${m4}(1) }".withSource + .references(m1 to m2, List(m1 to m2, m3 to m4), withDecl = true) + .references(m1 to m2, List(m3 to m4), withDecl = false) + .references(m3 to m4, List(m1 to m2, m3 to m4), withDecl = true) + .references(m3 to m4, List(m3 to m4), withDecl = false) + } + + @Test def classReferenceCompanion: Unit = { + code"""class ${m1}Foo${m2}(x: Any) + object ${m3}Foo${m4} { val bar = new ${m5}Foo${m6}(${m7}Foo${m8}) }""".withSource + .references(m1 to m2, List(m1 to m2, m5 to m6), withDecl = true) + .references(m1 to m2, List(m5 to m6), withDecl = false) + .references(m3 to m4, List(m3 to m4, m7 to m8), withDecl = true) + .references(m3 to m4, List(m7 to m8), withDecl = false) + .references(m5 to m6, List(m1 to m2, m5 to m6), withDecl = true) + .references(m5 to m6, List(m5 to m6), withDecl = false) + .references(m7 to m8, List(m3 to m4, m7 to m8), withDecl = true) + .references(m7 to m8, List(m7 to m8), withDecl = false) + } + } diff --git a/language-server/test/dotty/tools/languageserver/RenameTest.scala b/language-server/test/dotty/tools/languageserver/RenameTest.scala index a349e417bdca..199778b7352a 100644 --- a/language-server/test/dotty/tools/languageserver/RenameTest.scala +++ b/language-server/test/dotty/tools/languageserver/RenameTest.scala @@ -19,12 +19,57 @@ class RenameTest { def testRenameFrom(m: CodeMarker) = withSources( code"class ${m1}Foo$m2 { new ${m3}Foo$m4 }", - code"class Bar { new ${m5}Foo$m6 }" - ).rename(m, "Bar", Set(m1 to m2, m3 to m4, m5 to m6)) + code"class Bar { new ${m5}Foo$m6 }", + code"class Baz extends ${m7}Foo${m8}" + ).rename(m, "Bar", Set(m1 to m2, m3 to m4, m5 to m6, m7 to m8)) testRenameFrom(m1) testRenameFrom(m3) testRenameFrom(m5) } + @Test def renameObject: Unit = { + def testRenameFrom(m: CodeMarker) = + withSources( + code"object ${m1}Foo${m2}", + code"class Bar { val x = ${m3}Foo${m4} }" + ).rename(m, "NewName", Set(m1 to m2, m3 to m4)) + + testRenameFrom(m1) + testRenameFrom(m2) + } + + @Test def renameDef: Unit = { + def testRenameFrom(m: CodeMarker) = + withSources( + code"object Foo { def ${m1}bar${m2} = 0 }", + code"object Buzz { Foo.${m3}bar${m4} }" + ).rename(m, "newName", Set(m1 to m2, m3 to m4)) + + testRenameFrom(m1) + testRenameFrom(m3) + } + + @Test def renameClass: Unit = { + def testRenameFrom(m: CodeMarker) = + withSources( + code"class ${m1}Foo${m2}(x: Int)", + code"class Bar extends ${m3}Foo${m4}(1)" + ).rename(m, "NewName", Set(m1 to m2, m3 to m4)) + + testRenameFrom(m1) + testRenameFrom(m3) + } + + @Test def renameCaseClass: Unit = { + def testRenameFrom(m: CodeMarker) = + withSources( + code"case class ${m1}Foo${m2}(x: Int)", + code"class Bar extends ${m3}Foo${m4}(1)" + ).rename(m, "NewName", Set(m1 to m2, m3 to m4)) + + testRenameFrom(m1) + testRenameFrom(m2) + } + } From 31935f92262ddba2f3c72113e83e6bef4e79aae1 Mon Sep 17 00:00:00 2001 From: Martin Duhem Date: Mon, 27 Aug 2018 13:35:29 +0200 Subject: [PATCH 2/2] IDE: Factor out logic to find references --- .../tools/dotc/interactive/Interactive.scala | 33 ++++++++++++++++++- .../languageserver/DottyLanguageServer.scala | 20 ++++------- 2 files changed, 39 insertions(+), 14 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/interactive/Interactive.scala b/compiler/src/dotty/tools/dotc/interactive/Interactive.scala index e051a144b1b3..cd176d9acd5e 100644 --- a/compiler/src/dotty/tools/dotc/interactive/Interactive.scala +++ b/compiler/src/dotty/tools/dotc/interactive/Interactive.scala @@ -25,7 +25,9 @@ object Interactive { type Set = Int val overridden = 1 // include trees whose symbol is overridden by `sym` val overriding = 2 // include trees whose symbol overrides `sym` (but for performance only in same source file) - val references = 4 // include references and not just definitions + val references = 4 // include references + val definitions = 8 // include definitions + val linkedClass = 16 // include `symbol.linkedClass` } /** Does this tree define a symbol ? */ @@ -299,6 +301,35 @@ object Interactive { buf.toList } + /** + * Find trees that match `symbol` in `trees`. + * + * @param trees The trees to inspect. + * @param includes Whether to include references, definitions, etc. + * @param symbol The symbol for which we want to find references. + */ + def findTreesMatching(trees: List[SourceTree], + includes: Include.Set, + symbol: Symbol)(implicit ctx: Context): List[SourceTree] = { + val linkedSym = symbol.linkedClass + val includeReferences = (includes & Include.references) != 0 + val includeDeclaration = (includes & Include.definitions) != 0 + val includeLinkedClass = (includes & Include.linkedClass) != 0 + val predicate: NameTree => Boolean = tree => + ( tree.pos.isSourceDerived + && !tree.symbol.isConstructor + && (includeDeclaration || !Interactive.isDefinition(tree)) + && ( Interactive.matchSymbol(tree, symbol, includes) + || ( includeDeclaration + && includeLinkedClass + && linkedSym.exists + && Interactive.matchSymbol(tree, linkedSym, includes) + ) + ) + ) + namedTrees(trees, includeReferences, predicate) + } + /** The reverse path to the node that closest encloses position `pos`, * or `Nil` if no such path exists. If a non-empty path is returned it starts with * the tree closest enclosing `pos` and ends with an element of `trees`. diff --git a/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala b/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala index 29b66e0c627b..a84c7501f44a 100644 --- a/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala +++ b/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala @@ -273,7 +273,6 @@ class DottyLanguageServer extends LanguageServer val driver = driverFor(uri) implicit val ctx = driver.currentCtx - val includeDeclaration = params.getContext.isIncludeDeclaration val pos = sourcePosition(driver, uri, params.getPosition) val sym = Interactive.enclosingSourceSymbol(driver.openedTrees(uri), pos) @@ -283,11 +282,10 @@ class DottyLanguageServer extends LanguageServer // only need to look for trees in the target directory if the symbol is defined in the // current project val trees = driver.allTreesContaining(sym.name.sourceModuleName.toString) - val refs = Interactive.namedTrees(trees, includeReferences = true, tree => - tree.pos.isSourceDerived - && (includeDeclaration || !Interactive.isDefinition(tree)) - && !tree.symbol.isConstructor - && (Interactive.matchSymbol(tree, sym, Include.overriding))) + val includeDeclaration = params.getContext.isIncludeDeclaration + val includes = + Include.references | Include.overriding | (if (includeDeclaration) Include.definitions else 0) + val refs = Interactive.findTreesMatching(trees, includes, sym) refs.map(ref => location(ref.namePos)).asJava } @@ -304,14 +302,10 @@ class DottyLanguageServer extends LanguageServer if (sym == NoSymbol) new WorkspaceEdit() else { val trees = driver.allTreesContaining(sym.name.sourceModuleName.toString) - val linkedSym = sym.linkedClass val newName = params.getNewName - - val refs = Interactive.namedTrees(trees, includeReferences = true, tree => - tree.pos.isSourceDerived - && !tree.symbol.isConstructor - && (Interactive.matchSymbol(tree, sym, Include.overriding) - || (linkedSym != NoSymbol && Interactive.matchSymbol(tree, linkedSym, Include.overriding)))) + val includes = + Include.references | Include.definitions | Include.linkedClass | Include.overriding + val refs = Interactive.findTreesMatching(trees, includes, sym) val changes = refs.groupBy(ref => toUri(ref.source).toString).mapValues(_.map(ref => new TextEdit(range(ref.namePos), newName)).asJava)