Skip to content

Change 'overrides nothing' to report via Message (see #1965) #1968

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 2 commits into from
Feb 12, 2017
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
32 changes: 32 additions & 0 deletions compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1002,4 +1002,36 @@ object messages {
|${typeCode}
|"""
}

case class OverridesNothing(member: Symbol)(implicit ctx: Context)
extends Message(37) {
val kind = "Reference"
val msg = hl"""${member} overrides nothing"""

val explanation =
hl"""|There must be a field or method with the name `${member.name}` in a super
|class of `${member.owner}` to override it. Did you misspell it?
|Are you extending the right classes?
|"""
}

case class OverridesNothingButNameExists(member: Symbol, existing: List[Denotations.SingleDenotation])(implicit ctx: Context)
extends Message(38) {
val kind = "Reference"
val msg = hl"""${member} has a different signature than the overridden declaration"""

val existingDecl = existing.map(_.showDcl).mkString(" \n")

val explanation =
hl"""|There must be a non-final field or method with the name `${member.name}` and the
|same parameter list in a super class of `${member.owner}` to override it.
|
| ${member.showDcl}
|
|The super classes of `${member.owner}` contain the following members
|named `${member.name}`:
| ${existingDecl}
|"""
}

}
12 changes: 6 additions & 6 deletions compiler/src/dotty/tools/dotc/typer/RefChecks.scala
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ import DenotTransformers._

object RefChecks {
import tpd._
import reporting.diagnostic.Message
import reporting.diagnostic.messages._


private def isDefaultGetter(name: Name): Boolean =
name.isTermName && name.asTermName.defaultGetterIndex >= 0
Expand Down Expand Up @@ -622,15 +625,12 @@ object RefChecks {
if (member.isAnyOverride && !(clazz.thisType.baseClasses exists (hasMatchingSym(_, member)))) {
// for (bc <- clazz.info.baseClasses.tail) Console.println("" + bc + " has " + bc.info.decl(member.name) + ":" + bc.info.decl(member.name).tpe);//DEBUG

val nonMatching = clazz.info.member(member.name).altsWith(alt => alt.owner != clazz && !alt.is(Final))
def issueError(suffix: String) =
ctx.error(i"$member overrides nothing$suffix", member.pos)
val nonMatching = clazz.info.member(member.name).altsWith(alt => alt.owner != clazz)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just looking at the compressed diff here - so I can't see the reason to cut !alt.is(Final)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The former error message listed only non-final methods with the same name, but I think it is useful to have even final methods in the "not eligible" list of methods so that one can easily see it as the modifiers are included.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, makes sense :)

nonMatching match {
case Nil =>
issueError("")
ctx.error(OverridesNothing(member), member.pos)
case ms =>
val superSigs = ms.map(_.showDcl).mkString("\n")
issueError(s".\nNote: the super classes of ${member.owner} contain the following, non final members named ${member.name}:\n${superSigs}")
ctx.error(OverridesNothingButNameExists(member, ms), member.pos)
}
member.resetFlag(Override)
member.resetFlag(AbsOverride)
Expand Down
43 changes: 43 additions & 0 deletions compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,47 @@ class ErrorMessagesTests extends ErrorMessagesTest {
// is a type reference to `java.lang.String`
assert(expected.dealias =:= defn.StringType, s"expected was: $expected")
}

@Test def overridesNothing =
checkMessagesAfter("refchecks") {
"""
|object Foo {
| override def bar: Unit = {}
|}
""".stripMargin
}
.expect { (ictx, messages) =>
implicit val ctx: Context = ictx
val defn = ictx.definitions

assertMessageCount(1, messages)
val OverridesNothing(member) :: Nil = messages
assertEquals("bar", member.name.show)
}

@Test def overridesNothingDifferentSignature =
checkMessagesAfter("refchecks") {
"""
|class Bar {
| def bar(s: String): Unit = {}
| def bar(s: Int): Unit = {}
| final def bar(s: Long): Unit = {}
|}
|object Foo extends Bar {
| override def bar: Unit = {}
|}
""".stripMargin
}
.expect { (ictx, messages) =>
implicit val ctx: Context = ictx
val defn = ictx.definitions

assertMessageCount(1, messages)
val OverridesNothingButNameExists(member, sameName) :: Nil = messages
// check expected context data
assertEquals("bar", member.name.show)
assertEquals(3, sameName.size)
assert(sameName.forall(_.symbol.name.show == "bar"),
"at least one method had an unexpected name")
}
}
17 changes: 17 additions & 0 deletions tests/repl/overrides.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
scala> class B { override def foo(i: Int): Unit = {}; }
-- [E037] Reference Error: <console> -------------------------------------------
4 |class B { override def foo(i: Int): Unit = {}; }
| ^
| method foo overrides nothing

longer explanation available when compiling with `-explain`
scala> class A { def foo: Unit = {}; }
defined class A
scala> class B extends A { override def foo(i: Int): Unit = {}; }
-- [E038] Reference Error: <console> -------------------------------------------
5 |class B extends A { override def foo(i: Int): Unit = {}; }
| ^
| method foo has a different signature than the overridden declaration

longer explanation available when compiling with `-explain`
scala> :quit