Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: extract member with associated comments #5639

Merged
merged 1 commit into from
Sep 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ final class CodeActionProvider(
)(implicit ec: ExecutionContext) {

private val extractMemberAction =
new ExtractRenameMember(trees, languageClient)
new ExtractRenameMember(trees, languageClient, buffers)

private val allActions: List[CodeAction] = List(
new ImplementAbstractMembers(compilers),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import scala.meta.Template
import scala.meta.Term
import scala.meta.Tree
import scala.meta.Type
import scala.meta.internal.metals.Buffers
import scala.meta.internal.metals.ClientCommands
import scala.meta.internal.metals.MetalsEnrichments._
import scala.meta.internal.metals.ServerCommands
Expand All @@ -24,6 +25,7 @@ import scala.meta.internal.metals.codeactions.ExtractRenameMember.getMemberType
import scala.meta.internal.parsing.Trees
import scala.meta.io.AbsolutePath
import scala.meta.pc.CancelToken
import scala.meta.tokens.Token
import scala.meta.transversers.SimpleTraverser

import org.eclipse.lsp4j.ApplyWorkspaceEditParams
Expand All @@ -34,6 +36,7 @@ import org.eclipse.{lsp4j => l}
class ExtractRenameMember(
trees: Trees,
languageClient: MetalsLanguageClient,
buffers: Buffers,
)(implicit ec: ExecutionContext)
extends CodeAction {

Expand Down Expand Up @@ -156,10 +159,22 @@ class ExtractRenameMember(
nodes.toList
}

case class Comments(text: String, startPos: l.Position)

case class EndableMember(
member: Member,
maybeEndMarker: Option[Term.EndMarker],
)
commentsAbove: Option[Comments] = None,
) {
def withComments(comments: Comments): EndableMember =
this.copy(commentsAbove = Some(comments))
def memberPos: l.Range = {
val pos = member.pos.toLsp
commentsAbove.foreach(comments => pos.setStart(comments.startPos))
pos
}
def endMarkerPos: Option[l.Range] = maybeEndMarker.map(_.pos.toLsp)
}

private def isSealed(t: Tree): Boolean = t match {
case node: Defn.Trait => node.mods.exists(_.isInstanceOf[Mod.Sealed])
Expand Down Expand Up @@ -262,7 +277,12 @@ class ExtractRenameMember(

val structure = pkg.toList.mkString("\n") ::
imports.mkString("\n") ::
endableMember.commentsAbove.map(_.text).getOrElse("") +
endableMember.member.toString + marker(endableMember) ::
maybeCompanionEndableMember
.flatMap(_.commentsAbove)
.map(_.text)
.getOrElse("") +
maybeCompanionEndableMember
.map(_.member.toString)
.getOrElse("") + maybeCompanionEndableMember
Expand Down Expand Up @@ -391,13 +411,23 @@ class ExtractRenameMember(
val range = new l.Range(pos, pos)
val path = uri.toAbsolutePath

def withComment(member: EndableMember) =
findCommentsAbove(path, member.member) match {
case Some(comments) => member.withComments(comments)
case None => member
}

val opt = for {
tree <- trees.get(path)
definitions = membersDefinitions(tree)
memberDefn <- definitions.find(
_.member.name.pos.toLsp.overlapsWith(range)
)
companion = definitions.find(isCompanion(memberDefn.member))
memberDefn <- definitions
.find(
_.member.name.pos.toLsp.overlapsWith(range)
)
.map(withComment)
companion = definitions
.find(isCompanion(memberDefn.member))
.map(withComment)
(fileContent, defnLine) = newFileContent(
tree,
range,
Expand Down Expand Up @@ -451,8 +481,8 @@ class ExtractRenameMember(

newPath.writeText(content)

def removeTreeEdits(t: Tree): List[l.TextEdit] =
List(new l.TextEdit(t.pos.toLsp, ""))
def removeEdits(range: l.Range): List[l.TextEdit] =
List(new l.TextEdit(range, ""))

val packageEdit = endableMember.member.parent
.flatMap {
Expand All @@ -465,19 +495,39 @@ class ExtractRenameMember(
Some(p)
case _ => None
}
.map(removeTreeEdits)
.map(tree => removeEdits(tree.pos.toLsp))

packageEdit.getOrElse(
removeTreeEdits(endableMember.member) ++
removeEdits(endableMember.memberPos) ++
(maybeEndableMemberCompanion
.map(_.member)
++ endableMember.maybeEndMarker
.map(_.memberPos)
++ endableMember.endMarkerPos
++ maybeEndableMemberCompanion
.flatMap(_.maybeEndMarker)).flatMap(removeTreeEdits)
.flatMap(_.endMarkerPos)).flatMap(removeEdits)
)

}

private def findCommentsAbove(path: AbsolutePath, member: Member) = {
for {
text <- buffers.get(path)
(part, _) = text.splitAt(member.pos.start)
tokenized <- Trees.defaultTokenizerDialect(part).tokenize.toOption
collectComments = tokenized.tokens.reverse.takeWhile {
case _: Token.EOF => true
case _: Token.Comment => true
case _: Token.Whitespace => true
case _ => false
}
commentPos <- collectComments
.findLast(_.isInstanceOf[Token.Comment])
.map(_.pos)
} yield Comments(
part.splitAt(commentPos.start)._2,
commentPos.toLsp.getStart(),
)
}

}

object ExtractRenameMember {
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/src/main/scala/tests/FileLayout.scala
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ object FileLayout {
if (!layout.trim.isEmpty) {
val lines = layout.replace("\r\n", "\n")
lines
.split("(?=\n/[^/])")
.split("(?=\n/[^/*])")
.map { row =>
row.stripPrefix("\n").split("\n", 2).toList match {
case path :: contents :: Nil =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,34 @@ class ExtractRenameMemberLspSuite
),
)

checkExtractedMember(
"extract-class-with-scaladoc",
"""|package a
|
|case class A()
|/**
| * some scala doc
| */
|class <<B>>()
|""".stripMargin,
expectedActions = ExtractRenameMember.title("class", "B"),
"""|package a
|
|case class A()
|""".stripMargin,
fileName = "A.scala",
newFile = (
"B.scala",
s"""|package a
|
|/**
| * some scala doc
| */
|class B()
|""".stripMargin,
),
)

checkExtractedMember(
"extract-class-without-non-in-scope-imports",
"""|package a
Expand Down Expand Up @@ -454,7 +482,7 @@ class ExtractRenameMemberLspSuite
val pos = scala.meta.Position
.Range(input, startOffset, endOffset - "<<".length())
.toLsp
val extractRenameMember = new ExtractRenameMember(trees, client)
val extractRenameMember = new ExtractRenameMember(trees, client, buffers)
buffers.put(path, sourceText)
val textDocumentIdentifier = path.toTextDocumentIdentifier
val codeActionParams = new CodeActionParams(
Expand Down
Loading