Skip to content

Closes #1731 by fixing error message for overloaded method without re… #2823

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

Merged
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
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/core/Contexts.scala
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ object Contexts {
protected def sstate_=(sstate: SettingsState) = _sstate = sstate
def sstate: SettingsState = _sstate

/** The current tree */
/** The current compilation unit */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

private[this] var _compilationUnit: CompilationUnit = _
protected def compilationUnit_=(compilationUnit: CompilationUnit) = _compilationUnit = compilationUnit
def compilationUnit: CompilationUnit = _compilationUnit
Expand Down
24 changes: 17 additions & 7 deletions compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1209,20 +1209,29 @@ object messages {
|""".stripMargin
}

case class OverloadedOrRecursiveMethodNeedsResultType(tree: Names.TermName)(implicit ctx: Context)
case class OverloadedOrRecursiveMethodNeedsResultType private (termName: String)(implicit ctx: Context)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make the name private? If you're an IDE, wouldn't you want to be able to extract the name from the message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was to prevent accidental use that might happen by passing in a string directly; I may not understand all the implications, but even with private it seems I can still extract the string in ErrorMessagesTests.scala:

      val OverloadedOrRecursiveMethodNeedsResultType(treeName) :: Nil = messages
      assertEquals("i", treeName)

Seems to work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I did correct this before having coffee. Ignore this comment, it's just the constructor marked private, not the members ☕️

extends Message(OverloadedOrRecursiveMethodNeedsResultTypeID) {
val kind = "Syntax"
val msg = hl"""overloaded or recursive method ${tree} needs return type"""
val msg = hl"""overloaded or recursive method $termName needs return type"""
val explanation =
hl"""Case 1: ${tree} is overloaded
|If there are multiple methods named `${tree.name}` and at least one definition of
hl"""Case 1: $termName is overloaded
|If there are multiple methods named `$termName` and at least one definition of
|it calls another, you need to specify the calling method's return type.
|
|Case 2: ${tree} is recursive
|If `${tree.name}` calls itself on any path, you need to specify its return type.
|Case 2: $termName is recursive
|If `$termName` calls itself on any path, you need to specify its return type.
|""".stripMargin
}

object OverloadedOrRecursiveMethodNeedsResultType {
def apply[T >: Trees.Untyped](tree: NameTree[T])(implicit ctx: Context)
: OverloadedOrRecursiveMethodNeedsResultType =
OverloadedOrRecursiveMethodNeedsResultType(tree.name.show)(ctx)
def apply(symbol: Symbol)(implicit ctx: Context)
: OverloadedOrRecursiveMethodNeedsResultType =
OverloadedOrRecursiveMethodNeedsResultType(symbol.name.show)(ctx)
}

case class RecursiveValueNeedsResultType(tree: Names.TermName)(implicit ctx: Context)
extends Message(RecursiveValueNeedsResultTypeID) {
val kind = "Syntax"
Expand Down Expand Up @@ -1320,7 +1329,8 @@ object messages {
|"""
}

case class MethodDoesNotTakeParameters(tree: tpd.Tree, methPartType: Types.Type)(err: typer.ErrorReporting.Errors)(implicit ctx: Context)
case class MethodDoesNotTakeParameters(tree: tpd.Tree, methPartType: Types.Type)
(err: typer.ErrorReporting.Errors)(implicit ctx: Context)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

extends Message(MethodDoesNotTakeParametersId) {
private val more = tree match {
case Apply(_, _) => " more"
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/ErrorReporting.scala
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ object ErrorReporting {
if (cx.mode is Mode.InferringReturnType) {
cx.tree match {
case tree: untpd.DefDef if !tree.tpt.typeOpt.exists =>
OverloadedOrRecursiveMethodNeedsResultType(tree.name)
OverloadedOrRecursiveMethodNeedsResultType(tree)
case tree: untpd.ValDef if !tree.tpt.typeOpt.exists =>
RecursiveValueNeedsResultType(tree.name)
case _ =>
Expand Down
8 changes: 7 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1929,7 +1929,13 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit
else
tree
case _ => tryInsertApplyOrImplicit(tree, pt) {
errorTree(tree, MethodDoesNotTakeParameters(tree, methPart(tree).tpe)(err))
pt.resType match {
case IgnoredProto(WildcardType(optBounds))
if (optBounds == NoType) && (pt.args.size == tree.productArity) =>
errorTree(tree, OverloadedOrRecursiveMethodNeedsResultType(tree.symbol))
case resType =>
errorTree(tree, MethodDoesNotTakeParameters(tree, methPart(tree).tpe)(err))
}
}
}

Expand Down
25 changes: 20 additions & 5 deletions compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ class ErrorMessagesTests extends ErrorMessagesTest {
assertTrue("expected trait", isTrait)
}

@Test def overloadedMethodNeedsReturnType =
@Test def overloadedMethodNeedsReturnType = {
checkMessagesAfter("frontend") {
"""
|class Scope() {
Expand All @@ -214,10 +214,25 @@ class ErrorMessagesTests extends ErrorMessagesTest {
val defn = ictx.definitions

assertMessageCount(1, messages)
val OverloadedOrRecursiveMethodNeedsResultType(tree) :: Nil = messages
assertEquals("foo", tree.show)
val OverloadedOrRecursiveMethodNeedsResultType(treeName) :: Nil = messages
assertEquals("foo", treeName)
}


checkMessagesAfter("frontend") {
"""
|case class Foo[T](x: T)
|object Foo { def apply[T]() = Foo(null.asInstanceOf[T]) }
""".stripMargin
}.expect { (ictx, messages) =>
implicit val ctx: Context = ictx

assertMessageCount(1, messages)
val OverloadedOrRecursiveMethodNeedsResultType(treeName2) :: Nil = messages
assertEquals("Foo", treeName2)
}
}

@Test def recursiveMethodNeedsReturnType =
checkMessagesAfter("frontend") {
"""
Expand All @@ -231,8 +246,8 @@ class ErrorMessagesTests extends ErrorMessagesTest {
val defn = ictx.definitions

assertMessageCount(1, messages)
val OverloadedOrRecursiveMethodNeedsResultType(tree) :: Nil = messages
assertEquals("i", tree.show)
val OverloadedOrRecursiveMethodNeedsResultType(treeName) :: Nil = messages
assertEquals("i", treeName)
}

@Test def recursiveValueNeedsReturnType =
Expand Down