Skip to content

Commit

Permalink
Lint non-matching overrides of nullary/nilary methods
Browse files Browse the repository at this point in the history
Aka "promote -Xlint:override-nullary".

This means warn by default & error under -Xsource:3.
  • Loading branch information
dwijnand authored and SethTisue committed Jun 2, 2020
1 parent beed9c8 commit f491020
Show file tree
Hide file tree
Showing 34 changed files with 192 additions and 61 deletions.
8 changes: 4 additions & 4 deletions src/compiler/scala/tools/nsc/Reporting.scala
Expand Up @@ -318,7 +318,7 @@ object Reporting {
object WarningCategory {
private val insertDash = "(?=[A-Z][a-z])".r

var all: mutable.Map[String, WarningCategory] = mutable.Map.empty
val all: mutable.Map[String, WarningCategory] = mutable.Map.empty
private def add(c: WarningCategory): Unit = all += ((c.name, c))

object Deprecation extends WarningCategory; add(Deprecation)
Expand All @@ -336,8 +336,9 @@ object Reporting {
object OtherShadowing extends Other; add(OtherShadowing)
object OtherPureStatement extends Other; add(OtherPureStatement)
object OtherMigration extends Other; add(OtherMigration)
object OtherMatchAnalysis extends WarningCategory; add(OtherMatchAnalysis)
object OtherDebug extends WarningCategory; add(OtherDebug)
object OtherMatchAnalysis extends Other; add(OtherMatchAnalysis)
object OtherDebug extends Other; add(OtherDebug)
object OtherNullaryOverride extends Other; add(OtherNullaryOverride)

sealed trait WFlag extends WarningCategory { override def summaryCategory: WarningCategory = WFlag }
object WFlag extends WFlag { override def includes(o: WarningCategory): Boolean = o.isInstanceOf[WFlag] }; add(WFlag)
Expand All @@ -361,7 +362,6 @@ object Reporting {
object LintAdaptedArgs extends Lint; add(LintAdaptedArgs)
object LintNullaryUnit extends Lint; add(LintNullaryUnit)
object LintInaccessible extends Lint; add(LintInaccessible)
object LintNullaryOverride extends Lint; add(LintNullaryOverride)
object LintInferAny extends Lint; add(LintInferAny)
object LintMissingInterpolator extends Lint; add(LintMissingInterpolator)
object LintDocDetached extends Lint; add(LintDocDetached)
Expand Down
2 changes: 0 additions & 2 deletions src/compiler/scala/tools/nsc/settings/Warnings.scala
Expand Up @@ -169,7 +169,6 @@ trait Warnings {
val AdaptedArgs = LintWarning("adapted-args", "An argument list was modified to match the receiver.")
val NullaryUnit = LintWarning("nullary-unit", "`def f: Unit` looks like an accessor; add parens to look side-effecting.")
val Inaccessible = LintWarning("inaccessible", "Warn about inaccessible types in method signatures.")
val NullaryOverride = LintWarning("nullary-override", "Non-nullary `def f()` overrides nullary `def f`.")
val InferAny = LintWarning("infer-any", "A type argument was inferred as Any.")
val MissingInterpolator = LintWarning("missing-interpolator", "A string literal appears to be missing an interpolator id.")
val DocDetached = LintWarning("doc-detached", "When running scaladoc, warn if a doc comment is discarded.")
Expand Down Expand Up @@ -201,7 +200,6 @@ trait Warnings {
def warnAdaptedArgs = lint contains AdaptedArgs
def warnNullaryUnit = lint contains NullaryUnit
def warnInaccessible = lint contains Inaccessible
def warnNullaryOverride = lint contains NullaryOverride
def warnInferAny = lint contains InferAny
def warnMissingInterpolator = lint contains MissingInterpolator
def warnDocDetached = lint contains DocDetached
Expand Down
17 changes: 11 additions & 6 deletions src/compiler/scala/tools/nsc/typechecker/Namers.scala
Expand Up @@ -1461,12 +1461,17 @@ trait Namers extends MethodSynthesis {
// Note that matching MethodType of NullaryMethodType must be nilary not nelary.
def overriddenNilary(sym: Symbol) = sym.info.isInstanceOf[MethodType]
if (overridden != NoSymbol && vparamSymss.isEmpty && overridden.alternatives.exists(overriddenNilary)) {
if (settings.warnNullaryOverride)
context.unit.toCheck += {() =>
def javaDetermined(sym: Symbol) = sym.isJavaDefined || isUniversalMember(sym)
if (!meth.overrides.exists(javaDetermined))
context.warning(ddef.pos, "nullary method assumes an empty parameter list from the overridden definition", WarningCategory.LintNullaryOverride)
}
def exempt() = meth.overrides.exists(sym => sym.isJavaDefined || isUniversalMember(sym))
val msg = "method without a parameter list overrides a method with a single empty one"
def error(): Unit = if (!exempt()) {
ErrorUtils.issueNormalTypeError(ddef, msg)
ddef.tpt.defineType(ErrorType)
}
def warn(): Unit = if (!exempt()) {
context.warning(ddef.pos, msg, WarningCategory.OtherNullaryOverride)
meth.updateAttachment(NullaryOverrideAdapted)
}
context.unit.toCheck += (if (currentRun.isScala3) error _ else warn _)
ListOfNil
} else vparamSymss
}
Expand Down
14 changes: 10 additions & 4 deletions src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
Expand Up @@ -440,10 +440,16 @@ abstract class RefChecks extends Transform {
// Don't bother users with deprecations caused by classes they inherit.
// Only warn for the pair that has one leg in `clazz`.
if (clazz == memberClass) checkOverrideDeprecated()
if (settings.warnNullaryOverride) {
def javaDetermined(sym: Symbol) = sym.isJavaDefined || isUniversalMember(sym)
if (other.paramss.isEmpty && !member.paramss.isEmpty && !javaDetermined(member) && !member.overrides.exists(javaDetermined))
refchecksWarning(member.pos, "non-nullary method overrides nullary method", WarningCategory.LintNullaryOverride)
def javaDetermined(sym: Symbol) = sym.isJavaDefined || isUniversalMember(sym)
if (!member.hasAttachment[NullaryOverrideAdapted.type]
&& other.paramss.isEmpty && !member.paramss.isEmpty
&& !javaDetermined(member) && !member.overrides.exists(javaDetermined)
) {
val msg = "method with a single empty parameter list overrides method without any parameter list"
if (currentRun.isScala3)
overrideErrorWithMemberInfo(msg)
else
refchecksWarning(member.pos, msg, WarningCategory.OtherNullaryOverride)
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/reflect/scala/reflect/internal/StdAttachments.scala
Expand Up @@ -122,4 +122,6 @@ trait StdAttachments {

/** Mark the syntax for linting purposes. */
case object MultiargInfixAttachment extends PlainAttachment

case object NullaryOverrideAdapted extends PlainAttachment
}
1 change: 1 addition & 0 deletions src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
Expand Up @@ -65,6 +65,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse =>
this.KnownDirectSubclassesCalled
this.ConstructorNeedsFence
this.MultiargInfixAttachment
this.NullaryOverrideAdapted
this.noPrint
this.typeDebug
// inaccessible: this.posAssigner
Expand Down
2 changes: 1 addition & 1 deletion test/files/jvm/named-args-in-order/Test.scala
@@ -1,7 +1,7 @@
import scala.tools.partest.BytecodeTest

object Test extends BytecodeTest {
def show: Unit = {
def show(): Unit = {
val classNode = loadClassNode("SameBytecode")
def sameAsA(meth: String) =
sameBytecode(getMethod(classNode, "a"), getMethod(classNode, meth))
Expand Down
9 changes: 0 additions & 9 deletions test/files/neg/nilary-override.check

This file was deleted.

13 changes: 0 additions & 13 deletions test/files/neg/nilary-override.scala

This file was deleted.

13 changes: 13 additions & 0 deletions test/files/neg/nullary-override-3a.check
@@ -0,0 +1,13 @@
nullary-override-3a.scala:4: error: method with a single empty parameter list overrides method without any parameter list
def x: Int (defined in class A)
class B extends A { override def x(): Int = 4 }
^
nullary-override-3a.scala:16: error: method with a single empty parameter list overrides method without any parameter list
def x: String (defined in trait T1)
class Mix12b extends T1 with T2 { override def x() = "12b" }
^
nullary-override-3a.scala:19: error: method with a single empty parameter list overrides method without any parameter list
def x: String (defined in trait T1)
class Mix21b extends T2 with T1 { override def x() = "21b" }
^
3 errors
40 changes: 40 additions & 0 deletions test/files/neg/nullary-override-3a.scala
@@ -0,0 +1,40 @@
// scalac: -Werror -Wunused:nowarn -Xsource:3
//
class A { def x: Int = 3 }
class B extends A { override def x(): Int = 4 }


trait T1 { def x: String = "1" }
trait T2 { def x(): String = "2" }

// without overrides you just get: error: class X inherits conflicting members:
// def x: String (defined in trait T1) and
// def x(): String (defined in trait T2)
// (note: this can be resolved by declaring an `override` in class X.)

// Mix12a in nullary-override-3b
class Mix12b extends T1 with T2 { override def x() = "12b" }

class Mix21a extends T2 with T1 { override def x = "21a" }
class Mix21b extends T2 with T1 { override def x() = "21b" }

import java.util.concurrent.atomic.{ AtomicMarkableReference => AMR }
trait Ref1 { def getReference: String = "1" }
trait Ref2 { def getReference(): String = "2" }

// without overrides you just get: error: class X inherits conflicting members:
// def getReference(): String (defined in class AtomicMarkableReference) and
// def getReference: String (defined in trait Ref1)
// (note: this can be resolved by declaring an `override` in class X.)

class Mark1a extends AMR[String]("", false) with Ref1 { override def getReference = "1a" }
class Mark1b extends AMR[String]("", false) with Ref1 { override def getReference() = "1b" }

class Mark2a extends AMR[String]("", false) with Ref2 { override def getReference = "2a" }
class Mark2b extends AMR[String]("", false) with Ref2 { override def getReference() = "2b" }

class Mark12a extends AMR[String]("", false) with Ref1 with Ref2 { override def getReference = "12a" }
class Mark12b extends AMR[String]("", false) with Ref1 with Ref2 { override def getReference() = "12b" }

class Mark21a extends AMR[String]("", false) with Ref2 with Ref1 { override def getReference = "21a" }
class Mark21c extends AMR[String]("", false) with Ref2 with Ref1 { override def getReference() = "21b" }
7 changes: 7 additions & 0 deletions test/files/neg/nullary-override-3b.check
@@ -0,0 +1,7 @@
nullary-override-3b.scala:6: error: method without a parameter list overrides a method with a single empty one
class Q extends P { override def x: Int = 4 }
^
nullary-override-3b.scala:11: error: method without a parameter list overrides a method with a single empty one
class Mix12a extends T1 with T2 { override def x = "12a" }
^
2 errors
12 changes: 12 additions & 0 deletions test/files/neg/nullary-override-3b.scala
@@ -0,0 +1,12 @@
// scalac: -Werror -Wunused:nowarn -Xsource:3
//
// P has parens
class P { def x(): Int = 3 }
// Q is questionable
class Q extends P { override def x: Int = 4 }

trait T1 { def x: String = "1" }
trait T2 { def x(): String = "2" }

class Mix12a extends T1 with T2 { override def x = "12a" }
// the rest in nullary-override-3a
15 changes: 12 additions & 3 deletions test/files/neg/nullary-override.check
@@ -1,9 +1,18 @@
nullary-override.scala:15: warning: nullary method assumes an empty parameter list from the overridden definition
nullary-override.scala:15: warning: method without a parameter list overrides a method with a single empty one
class Q extends P { override def x: Int = 4 }
^
nullary-override.scala:4: warning: non-nullary method overrides nullary method
nullary-override.scala:36: warning: method without a parameter list overrides a method with a single empty one
class Mix12a extends T1 with T2 { override def x = "12a" }
^
nullary-override.scala:4: warning: method with a single empty parameter list overrides method without any parameter list
class B extends A { override def x(): Int = 4 }
^
nullary-override.scala:37: warning: method with a single empty parameter list overrides method without any parameter list
class Mix12b extends T1 with T2 { override def x() = "12b" }
^
nullary-override.scala:40: warning: method with a single empty parameter list overrides method without any parameter list
class Mix21b extends T2 with T1 { override def x() = "21b" }
^
error: No warnings can be incurred under -Werror.
2 warnings
5 warnings
1 error
45 changes: 44 additions & 1 deletion test/files/neg/nullary-override.scala
@@ -1,4 +1,4 @@
// scalac: -Werror -Xlint:nullary-override
// scalac: -Werror -Wunused:nowarn
//
class A { def x: Int = 3 }
class B extends A { override def x(): Int = 4 }
Expand All @@ -16,3 +16,46 @@ class Q extends P { override def x: Int = 4 }

// Welcome to the Happy J
class J { override def toString = "Happy J" }

import annotation._
class E { def x(): Int = 3 }
class F extends E { @nowarn override def x: Int = 4 }

class G { def x: Int = 5 }
class H extends G { @nowarn override def x(): Int = 6 }


trait T1 { def x: String = "1" }
trait T2 { def x(): String = "2" }

// without overrides you just get: error: class X inherits conflicting members:
// def x: String (defined in trait T1) and
// def x(): String (defined in trait T2)
// (note: this can be resolved by declaring an `override` in class X.)

class Mix12a extends T1 with T2 { override def x = "12a" }
class Mix12b extends T1 with T2 { override def x() = "12b" }

class Mix21a extends T2 with T1 { override def x = "21a" }
class Mix21b extends T2 with T1 { override def x() = "21b" }

import java.util.concurrent.atomic.{ AtomicMarkableReference => AMR }
trait Ref1 { def getReference: String = "1" }
trait Ref2 { def getReference(): String = "2" }

// without overrides you just get: error: class X inherits conflicting members:
// def getReference(): String (defined in class AtomicMarkableReference) and
// def getReference: String (defined in trait Ref1)
// (note: this can be resolved by declaring an `override` in class X.)

class Mark1a extends AMR[String]("", false) with Ref1 { override def getReference = "1a" }
class Mark1b extends AMR[String]("", false) with Ref1 { override def getReference() = "1b" }

class Mark2a extends AMR[String]("", false) with Ref2 { override def getReference = "2a" }
class Mark2b extends AMR[String]("", false) with Ref2 { override def getReference() = "2b" }

class Mark12a extends AMR[String]("", false) with Ref1 with Ref2 { override def getReference = "12a" }
class Mark12b extends AMR[String]("", false) with Ref1 with Ref2 { override def getReference() = "12b" }

class Mark21a extends AMR[String]("", false) with Ref2 with Ref1 { override def getReference = "21a" }
class Mark21c extends AMR[String]("", false) with Ref2 with Ref1 { override def getReference() = "21b" }
7 changes: 7 additions & 0 deletions test/files/neg/t5429.check
@@ -1,3 +1,9 @@
t5429.scala:68: warning: method without a parameter list overrides a method with a single empty one
def emptyArg = 10 // fail
^
t5429.scala:75: warning: method without a parameter list overrides a method with a single empty one
override def emptyArg = 10 // override
^
t5429.scala:20: error: `override` modifier required to override concrete member:
val value: Int (defined in class A)
object value // fail
Expand Down Expand Up @@ -145,4 +151,5 @@ Note: the super classes of class F0 contain the following, non final members nam
def oneArg(x: String): Any
override lazy val oneArg = 15 // fail
^
2 warnings
34 errors
2 changes: 1 addition & 1 deletion test/files/neg/t8244b.scala
Expand Up @@ -5,7 +5,7 @@ class Raw_1[T]{


class X extends Raw_1[X] {
override def t = this
override def t() = this
def exxx = 0
}

Expand Down
2 changes: 1 addition & 1 deletion test/files/neg/t8244c.scala
Expand Up @@ -5,7 +5,7 @@ class Raw_1[T]{


class X extends Raw_1[X] {
override def t = this
override def t() = this
def exxx = 0
}

Expand Down
10 changes: 10 additions & 0 deletions test/files/pos/nullary-override-3.scala
@@ -0,0 +1,10 @@
// scalac: -Werror -Wunused:nowarn -Xsource:3
//
class C extends java.lang.CharSequence {
def charAt(x$1: Int): Char = ???
def length: Int = ???
def subSequence(x$1: Int, x$2: Int): CharSequence = ???
}

// Welcome to the Happy J
class J { override def toString = "Happy J" }
4 changes: 2 additions & 2 deletions test/files/run/Course-2002-10.scala
Expand Up @@ -100,13 +100,13 @@ object M2 {
class IntIterator(start: Int) extends Iterator[Int] {
var current: Int = start;
def hasNext = true;
def next = { current = current + 1; current - 1 };
def next() = { current = current + 1; current - 1 };
}

class PrimeIterator() extends Iterator[Int] {
var current: Iterator[Int] = new IntIterator(2);
def hasNext = true;
def next = {
def next() = {
val p = current.next;
current = current filter { x => !((x % p) == 0) };
p
Expand Down
2 changes: 1 addition & 1 deletion test/files/run/Course-2002-13.scala
Expand Up @@ -17,7 +17,7 @@ class Tokenizer(s: String, delimiters: String) extends Iterator[String] {
i < s.length()
}

def next: String =
def next(): String =
if (hasNext) {
val start = i;
var ch = s.charAt(i); i = i + 1;
Expand Down
2 changes: 1 addition & 1 deletion test/files/run/bcodeInlinerMixed/Test_2.scala
Expand Up @@ -12,7 +12,7 @@ class D {
}

object Test extends BytecodeTest {
def show: Unit = {
def show(): Unit = {
val gIns = instructionsFromMethod(getMethod(loadClassNode("B"), "g"))
val hIns = instructionsFromMethod(getMethod(loadClassNode("C"), "h"))
for (i <- List(gIns, hIns)) {
Expand Down
2 changes: 1 addition & 1 deletion test/files/run/bugs.scala
Expand Up @@ -381,7 +381,7 @@ object Bug266Test {

class Bug316MyIterator extends Iterator[Int] {
def hasNext = false
def next = 42
def next() = 42
}

object Bug316Test {
Expand Down
2 changes: 1 addition & 1 deletion test/files/run/iterables.scala
Expand Up @@ -4,7 +4,7 @@ object Test extends App {
private var i = 0
def iterator = new Iterator[Int] {
def hasNext = i < n
def next =
def next() =
if (hasNext) { val v = i; i += 1; v }
else throw new IndexOutOfBoundsException("empty iterator")
}
Expand Down
2 changes: 1 addition & 1 deletion test/files/run/lisp.scala
Expand Up @@ -12,7 +12,7 @@ class LispTokenizer(s: String) extends Iterator[String] {
while (i < s.length() && s.charAt(i) <= ' ') i += 1
i < s.length()
}
def next: String =
def next(): String =
if (hasNext) {
val start = i
if (isDelimiter(s charAt i)) i += 1
Expand Down

0 comments on commit f491020

Please sign in to comment.