Skip to content

feat: Add actionable diagnostic for missing members #23572

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
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 @@ -232,6 +232,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
case UnnecessaryNN // errorNumber: 216
case ErasedNotPureID // errorNumber: 217
case IllegalErasedDefID // errorNumber: 218
case ConcreteClassHasUnimplementedMethodsID // errorNumer: 219

def errorNumber = ordinal - 1

Expand Down
31 changes: 31 additions & 0 deletions compiler/src/dotty/tools/dotc/reporting/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3652,3 +3652,34 @@ final class IllegalErasedDef(sym: Symbol)(using Context) extends TypeMsg(Illegal
override protected def explain(using Context): String =
"Only non-lazy immutable values can be `erased`"
end IllegalErasedDef

class ConcreteClassHasUnimplementedMethods(clazz: ClassSymbol, missingMethods: List[dotty.tools.dotc.core.Symbols.Symbol], val actions: List[CodeAction])(using Context)
extends Message(ConcreteClassHasUnimplementedMethodsID):

def kind = MessageKind.Declaration

def renderMissingMethods: List[String] = {
// Grouping missing methods by the declaring class
val regrouped = missingMethods.groupBy(_.owner).toList
def membersStrings(members: List[Symbol]) =
members.sortBy(_.name.toString).map(_.asSeenFrom(clazz.thisType).showDcl).map(m => s"- $m")

(regrouped.sortBy(_._1.name.toString()) map {
case (owner, members) =>
s"""Members declared in ${owner.fullName}:
|${membersStrings(members).mkString("\n")}"""
})
}

def msg(using Context) = missingMethods match
case single :: Nil =>
def showDclAndLocation(sym: Symbol) = s"${sym.showDcl} in ${sym.owner.showLocated}"
s"$clazz needs to be abstract, since ${showDclAndLocation(single)} is not defined"
case _ =>
s"""$clazz needs to be abstract, since it has ${missingMethods.size} unimplemented members.
|
|${renderMissingMethods.mkString("\n")}
|""".stripMargin

def explain(using Context) = ""
override def actions(using Context) = this.actions
105 changes: 83 additions & 22 deletions compiler/src/dotty/tools/dotc/typer/RefChecks.scala
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import config.Printers.refcheck
import reporting.*
import Constants.Constant
import cc.{stripCapturing, isUpdateMethod, CCState}
import dotty.tools.dotc.util.Chars.{isLineBreakChar, isWhitespace}

object RefChecks {
import tpd.*
Expand Down Expand Up @@ -629,15 +630,15 @@ object RefChecks {
// Verifying a concrete class has nothing unimplemented.
if (!clazz.isOneOf(AbstractOrTrait)) {
val abstractErrors = new mutable.ListBuffer[String]
val concreteClassUnimplementedMethodError = new mutable.ListBuffer[Message]
def abstractErrorMessage =
// a little formatting polish
if (abstractErrors.size <= 2) abstractErrors.mkString(" ")
else abstractErrors.tail.mkString(abstractErrors.head + ":\n", "\n", "")

def abstractClassError(mustBeMixin: Boolean, msg: String): Unit = {
def abstractClassError(msg: String): Unit = {
def prelude = (
if (clazz.isAnonymousClass || clazz.is(Module)) "object creation impossible"
else if (mustBeMixin) s"$clazz needs to be a mixin"
else if clazz.is(Synthetic) then "instance cannot be created"
else s"$clazz needs to be abstract"
) + ", since"
Expand Down Expand Up @@ -717,37 +718,22 @@ object RefChecks {
}

def stubImplementations: List[String] = {
// Grouping missing methods by the declaring class
val regrouped = missingMethods.groupBy(_.owner).toList
def membersStrings(members: List[Symbol]) =
members.sortBy(_.name.toString).map(_.asSeenFrom(clazz.thisType).showDcl + " = ???")

if (regrouped.tail.isEmpty)
membersStrings(regrouped.head._2)
else (regrouped.sortBy(_._1.name.toString()) flatMap {
case (owner, members) =>
("// Members declared in " + owner.fullName) +: membersStrings(members) :+ ""
}).init
membersStrings(missingMethods)
}

// If there are numerous missing methods, we presume they are aware of it and
// give them a nicely formatted set of method signatures for implementing.
if (missingMethods.size > 1) {
abstractClassError(false, "it has " + missingMethods.size + " unimplemented members.")
val preface =
"""|/** As seen from %s, the missing signatures are as follows.
| * For convenience, these are usable as stub implementations.
| */
|""".stripMargin.format(clazz)
abstractErrors += stubImplementations.map(" " + _ + "\n").mkString(preface, "", "")
if (missingMethods.size >= 1) {
concreteClassUnimplementedMethodError += ConcreteClassHasUnimplementedMethods(clazz, missingMethods, createAddMissingMethodsAction(clazz, stubImplementations))
return
}

for (member <- missingMethods) {
def showDclAndLocation(sym: Symbol) =
s"${sym.showDcl} in ${sym.owner.showLocated}"
def undefined(msg: String) =
abstractClassError(false, s"${showDclAndLocation(member)} is not defined $msg")
abstractClassError(s"${showDclAndLocation(member)} is not defined $msg")
val underlying = member.underlyingSymbol

// Give a specific error message for abstract vars based on why it fails:
Expand Down Expand Up @@ -840,7 +826,7 @@ object RefChecks {
val impl1 = clazz.thisType.nonPrivateMember(decl.name) // DEBUG
report.log(i"${impl1}: ${impl1.info}") // DEBUG
report.log(i"${clazz.thisType.memberInfo(decl)}") // DEBUG
abstractClassError(false, "there is a deferred declaration of " + infoString(decl) +
abstractClassError("there is a deferred declaration of " + infoString(decl) +
" which is not implemented in a subclass" + err.abstractVarMessage(decl))
}
if (bc.asClass.superClass.is(Abstract))
Expand Down Expand Up @@ -916,6 +902,8 @@ object RefChecks {
if (abstractErrors.nonEmpty)
report.error(abstractErrorMessage, clazz.srcPos)

concreteClassUnimplementedMethodError.foreach(report.error(_, clazz.srcPos))

checkMemberTypesOK()
checkCaseClassInheritanceInvariant()
}
Expand Down Expand Up @@ -1272,6 +1260,79 @@ object RefChecks {
case tp: NamedType if tp.prefix.typeSymbol != ctx.owner.enclosingClass =>
report.warning(UnqualifiedCallToAnyRefMethod(tree, tree.symbol), tree)
case _ => ()

private def createAddMissingMethodsAction(clazz: ClassSymbol, methods: List[String])(using Context): List[CodeAction] = {
import dotty.tools.dotc.rewrites.Rewrites.ActionPatch
import dotty.tools.dotc.util.Spans
import dotty.tools.dotc.ast.untpd

val untypedTree = NavigateAST
.untypedPath(clazz.span)
.head
.asInstanceOf[untpd.Tree]

val classSrcPos = clazz.srcPos
val content = classSrcPos.sourcePos.source.content()
val span = classSrcPos.endPos.span

val classText = new String(content.slice(untypedTree.span.start, untypedTree.span.end))
val classHasBraces = classText.contains("{") && classText.contains("}")

// Indentation for inserted methods
val lineStart = content.lastIndexWhere(isLineBreakChar, end = span.end - 1) + 1
val baseIndent = new String(content.slice(lineStart, span.end).takeWhile(c => c == ' ' || c == '\t'))
val indent = baseIndent + " "

val formattedMethods = methods.map(m => s"$indent$m").mkString(System.lineSeparator())

val isBracelessSyntax = untypedTree match
case untpd.TypeDef(_, tmpl: untpd.Template) =>
!classText.contains("{") && tmpl.body.nonEmpty
case _ => false

if (classHasBraces) {
val insertBeforeBrace = untypedTree.sourcePos.withSpan(Span(untypedTree.span.end - 1))
val braceStart = classText.indexOf('{')
val braceEnd = classText.lastIndexOf('}')
val bodyBetweenBraces = classText.slice(braceStart + 1, braceEnd)
val bodyIsEmpty = bodyBetweenBraces.forall(_.isWhitespace)
val bodyContainsNewLine = bodyBetweenBraces.exists(isLineBreakChar)

val prefix = if (bodyContainsNewLine) "" else System.lineSeparator()
val patchText =
prefix +
formattedMethods +
System.lineSeparator()

val patch = ActionPatch(insertBeforeBrace, patchText)
List(CodeAction("Add missing methods", None, List(patch)))
} else if (isBracelessSyntax) {
val insertAfterLastDef = untypedTree match
case untpd.TypeDef(_, tmpl: untpd.Template) if tmpl.body.nonEmpty =>
val lastDef = tmpl.body.last
lastDef.sourcePos.withSpan(Span(lastDef.span.end))
case _ =>
untypedTree.sourcePos.withSpan(Span(untypedTree.span.end))

val patchText = System.lineSeparator() + formattedMethods

val patch = ActionPatch(insertAfterLastDef, patchText)
List(CodeAction("Add missing methods", None, List(patch)))
} else {
// Class has no body – add whole `{ ... }` after class header, same line
val insertAfterHeader = untypedTree.sourcePos.withSpan(Span(untypedTree.span.end))

val patchText =
" {" + System.lineSeparator() +
formattedMethods + System.lineSeparator() +
"}"

val patch = ActionPatch(insertAfterHeader, patchText)
List(CodeAction("Add missing methods", None, List(patch)))
}
}


}
import RefChecks.*

Expand Down
142 changes: 142 additions & 0 deletions compiler/test/dotty/tools/dotc/reporting/CodeActionTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,148 @@ class CodeActionTest extends DottyTest:
ctxx = ctxx
)

@Test def insertMissingSingleMethodToNonEmptyClass =
checkCodeAction(
code =
"""trait Animal {
| def name: String
|}
|class Dog extends Animal {
| def bbb = 2
|}
|""".stripMargin,
title = "Add missing methods",
expected =
"""trait Animal {
| def name: String
|}
|class Dog extends Animal {
| def bbb = 2
| def name: String = ???
|}
|""".stripMargin,
afterPhase= "erasure"
)

@Test def insertMissingMethodsFromMultipleSources =
checkCodeAction(
code =
"""trait Animal {
| def name: String
|}
|trait Car {
| def wheels: Int
|}
|class Dog extends Animal with Car {
|}
|""".stripMargin,
title = "Add missing methods",
expected =
"""trait Animal {
| def name: String
|}
|trait Car {
| def wheels: Int
|}
|class Dog extends Animal with Car {
| def name: String = ???
| def wheels: Int = ???
|}
|""".stripMargin,
afterPhase= "erasure"
)

@Test def insertMissingMethodIntoEmptyClassWithBrackets =
checkCodeAction(
code =
"""trait Animal {
| def name: String
|}
|class Dog extends Animal {}
|""".stripMargin,
title = "Add missing methods",
expected =
"""trait Animal {
| def name: String
|}
|class Dog extends Animal {
| def name: String = ???
|}
|""".stripMargin,
afterPhase= "erasure"
)

@Test def insertMissingMethodIntoEmptyClassWithoutBrackets =
checkCodeAction(
code =
"""trait Animal {
| def name: String
|}
|class Dog extends Animal
|""".stripMargin,
title = "Add missing methods",
expected =
"""trait Animal {
| def name: String
|}
|class Dog extends Animal {
| def name: String = ???
|}
|""".stripMargin,
afterPhase= "erasure"
)

@Test def insertMissingMethodIntoNonEmptyClassWithBraclessSyntax =
checkCodeAction(
code =
"""trait Animal {
| def name: String
|}
|class Dog extends Animal:
| def bbb = 2
|""".stripMargin,
title = "Add missing methods",
expected =
"""trait Animal {
| def name: String
|}
|class Dog extends Animal:
| def bbb = 2
| def name: String = ???
|""".stripMargin,
afterPhase= "erasure"
)

@Test def insertMissingMethodsIntoMultilineClassDefinition =
checkCodeAction(
code =
"""trait Animal {
| def name: String
|}
|trait Car {
| def wheels: Int
|}
|class Dog extends Animal
| with Car {
|}
|""".stripMargin,
title = "Add missing methods",
expected =
"""trait Animal {
| def name: String
|}
|trait Car {
| def wheels: Int
|}
|class Dog extends Animal
| with Car {
| def name: String = ???
| def wheels: Int = ???
|}
|""".stripMargin,
afterPhase= "erasure"
)

// Make sure we're not using the default reporter, which is the ConsoleReporter,
// meaning they will get reported in the test run and that's it.
private def newContext =
Expand Down
Loading